diff --git a/docs/design/perf-cache-fix.md b/docs/design/perf-cache-fix.md index 4afd29e5..9a2b00f9 100644 --- a/docs/design/perf-cache-fix.md +++ b/docs/design/perf-cache-fix.md @@ -141,9 +141,11 @@ needed to expire correctly: - `src/session-cache.ts`: add `loadStatusSnapshot(corpusFingerprint, newestMtimeMs, queryKey)` / `saveStatusSnapshot(corpusFingerprint, newestMtimeMs, queryKey, payload)` and the `statusSnapshotSettleMs()` knob - (new `status-snapshot.json` file in the cache dir, atomic temp+rename - write, best-effort — a failed read/write just falls back to a full - recompute). `reconcileFile` is unchanged. + (one `status-snapshot..json` file per distinct query in the + cache dir — so concurrent callers with different queryKeys never evict + each other — atomic temp+rename write with a CAS re-read guard against a + slower/older write clobbering a newer one, best-effort — a failed read/ + write just falls back to a full recompute). `reconcileFile` is unchanged. - `src/main.ts`: in the `status` command's `--format menubar-json` branch, build a query key from the resolved period/day/days range, provider, project/exclude filters, optimize/timeline flags, and Claude config source; diff --git a/src/parser.ts b/src/parser.ts index e7a60f72..6e82f2fd 100644 --- a/src/parser.ts +++ b/src/parser.ts @@ -4777,13 +4777,35 @@ export type CorpusFingerprint = { // stays with each provider's own (narrower, extension-aware) file layout // knowledge. Being over-inclusive here is safe: an extra file in the hash // can only cause an extra cache miss, never a missed update. -async function collectFilesRecursive(dirPath: string): Promise { +async function collectFilesRecursive(dirPath: string, visitedDirs: Set = new Set()): Promise { const entries = await readdir(dirPath, { withFileTypes: true }).catch(() => []) const files: string[] = [] for (const entry of entries) { const p = join(dirPath, entry.name) - if (entry.isDirectory()) files.push(...await collectFilesRecursive(p)) - else files.push(p) + if (entry.isDirectory()) { + files.push(...await collectFilesRecursive(p, visitedDirs)) + continue + } + if (entry.isSymbolicLink()) { + // `Dirent.isDirectory()` is false for a symlink even when its target + // IS a directory — without this check a symlinked subdirectory falls + // into the `else` below and gets pushed as a leaf "file"; a later + // `stat()` (which follows symlinks) then returns the DIRECTORY + // inode's own mtime/size as a bogus stand-in for its contents, with no + // recursion into it at all (review finding C-G2). Resolve it and + // recurse for real. A visited-inode guard bounds the walk against a + // symlink cycle (impossible for real directories, which is why the + // `isDirectory()` branch above needs no such guard). + const target = await stat(p).catch(() => null) + if (target?.isDirectory()) { + const key = `${target.dev}:${target.ino}` + if (visitedDirs.has(key)) continue + visitedDirs.add(key) + files.push(...await collectFilesRecursive(p, visitedDirs)) + continue + } + } + files.push(p) } return files } @@ -4834,7 +4856,21 @@ export async function computeCorpusFingerprint(providerFilter?: string): Promise if (!providerByName.has(name)) providerByName.set(name, await getProvider(name)) return providerByName.get(name) } + // Non-discovery provider env vars (e.g. CODEBURN_CURSOR_MAX_BUBBLES, + // KIMI_MODEL_NAME — src/doctor.ts's NON_DISCOVERY_ENV_VARS) and a + // provider's parse-version change PARSED OUTPUT without touching any + // source's path/mtime/size, so the stat-only loop below would otherwise + // never see them move. `computeEnvFingerprint` (session-cache.ts) already + // hashes exactly this per provider for the parse-level cache; fold it in + // here too, once per distinct provider actually discovered, so this + // higher-layer snapshot fingerprint can't bypass that same guard (review + // finding A-G1). + const envFingerprinted = new Set() for (const source of sources) { + if (!envFingerprinted.has(source.provider)) { + envFingerprinted.add(source.provider) + entries.push(`env:${source.provider}|${computeEnvFingerprint(source.provider)}`) + } if (source.provider === 'claude') { for (const filePath of await collectJsonlFiles(source.path)) await record(filePath) continue diff --git a/src/providers/cline-cli.ts b/src/providers/cline-cli.ts index f4db02bc..568d0dfa 100644 --- a/src/providers/cline-cli.ts +++ b/src/providers/cline-cli.ts @@ -242,10 +242,14 @@ async function readJson(path: string): Promise { function createParser(source: SessionSource, seenKeys: Set): SessionParser { return { async *parse(): AsyncGenerator { - const meta = await readJson(source.path) + // `source.path` is the growing `.messages.json` file (see + // `discoverSessions` / review finding C-G1) — the static per-session + // metadata lives in the sibling `.json`, read below. + const metaPath = source.path.replace(/\.messages\.json$/, '.json') + const meta = await readJson(metaPath) if (!isRecord(meta)) return - const sessionId = nonEmptyString(meta['session_id']) ?? basename(source.path).replace(/\.json$/, '') + const sessionId = nonEmptyString(meta['session_id']) ?? basename(metaPath).replace(/\.json$/, '') const metadata = isRecord(meta['metadata']) ? meta['metadata'] : {} const workspace = nonEmptyString(meta['workspace_root']) ?? nonEmptyString(meta['cwd']) const sessionModel = nonEmptyString(meta['model']) ?? 'unknown' @@ -254,10 +258,10 @@ function createParser(source: SessionSource, seenKeys: Set): SessionPars // display name, so there is nothing left to default to here. const project = source.project - // Prefer the co-located messages file over the recorded absolute path, - // which is stale once a session directory is copied between machines. - const sibling = join(source.path.replace(/\.json$/, '') + '.messages.json') - let doc = await readJson(sibling) + // Prefer the co-located messages file (source.path itself) over a + // recorded absolute path, which is stale once a session directory is + // copied between machines. + let doc = await readJson(source.path) if (!isRecord(doc)) { const recorded = nonEmptyString(meta['messages_path']) if (recorded) doc = await readJson(recorded) @@ -398,9 +402,16 @@ export function createClineCliProvider(overrideDir?: string): Provider { const meta = await readJson(metaPath) if (!isRecord(meta)) continue + // Fingerprint the sibling that actually accumulates new turns, not + // the metadata file written once at session start — a live session's + // corpus fingerprint must move as new turns append, or a menubar + // poll can serve the pre-activity snapshot forever (review finding + // C-G1). `computeCorpusFingerprint` degrades this to "contributes + // nothing yet" if the messages file doesn't exist (a session with no + // turns), the same as any other missing/not-yet-created file. const workspace = nonEmptyString(meta['workspace_root']) ?? nonEmptyString(meta['cwd']) sources.push({ - path: metaPath, + path: join(dir, sessionId, `${sessionId}.messages.json`), project: projectName(workspace), provider: PROVIDER_NAME, }) diff --git a/src/session-cache.ts b/src/session-cache.ts index 515ce56a..a2ed5b3d 100644 --- a/src/session-cache.ts +++ b/src/session-cache.ts @@ -1515,8 +1515,9 @@ export async function cleanupOrphanedTempFiles(): Promise { // `session-cache.v.json..tmp` from a pre-v8 binary interrupted // mid-write. Age-guarded, so an old binary's in-flight write is left alone. - // `status-snapshot.json..tmp` (see saveStatusSnapshot below) is swept - // here too: it lives in this same parent dir, orthogonal to the month-shard + // `status-snapshot..json..tmp` (see + // writeStatusSnapshotRecord below — one file per queryKey) is swept here + // too: it lives in this same parent dir, orthogonal to the month-shard // layout below, so the shard-dir sweep further down never sees it. Same // atomic temp+rename pattern, same narrow crash window between the write // and the rename as the versioned cache file above. @@ -1742,7 +1743,7 @@ function statusSnapshotSettleMs(): number { return Number.isFinite(raw) && raw >= 0 ? Math.min(raw, 60_000) : 2000 } -const STATUS_SNAPSHOT_FILE = 'status-snapshot.json' +const STATUS_SNAPSHOT_FILE = 'status-snapshot' // Bump on any incompatible change to the *shape* of the payload this snapshot // persists (i.e. whenever `buildMenubarPayloadForRange`'s return shape @@ -1754,12 +1755,21 @@ const STATUS_SNAPSHOT_FILE = 'status-snapshot.json' // `validateCache`. A version mismatch is treated as a miss (same as a // missing/corrupt file): the caller recomputes for real and persists a fresh, // current-shaped snapshot. -const STATUS_SNAPSHOT_VERSION = 1 +// +// v2 (was v1): the file is now keyed by `queryKey` in its NAME (see +// `statusSnapshotPath` below) instead of being one shared slot — two +// concurrent callers with different queryKeys (a menubar poll for "today" +// racing a manual refresh for "week") used to unconditionally evict each +// other's save. A v1 file at the old fixed path is simply never looked at +// again under v2 (harmless leftover, not actively cleaned). +const STATUS_SNAPSHOT_VERSION = 2 type StatusSnapshotRecord = { version: number corpusFingerprint: string newestMtimeMs: number + // Kept alongside the hash in the filename as a defensive re-check against + // a (vanishingly unlikely) hash collision between two different queries. queryKey: string payload: unknown // Wall-clock time (Date.now()) the FIRST mismatch against this record's @@ -1769,19 +1779,29 @@ type StatusSnapshotRecord = { mismatchFirstSeenAt?: number } -function statusSnapshotPath(): string { - return join(getCodeburnCacheDir(), STATUS_SNAPSHOT_FILE) +// Each distinct queryKey gets its OWN file (`status-snapshot..json`) +// rather than sharing one slot — see the v2 note above / review finding +// B-G1's "amplifier." This means two writers for different queryKeys never +// touch each other's file at all, so there is no read-merge-write race to +// close between them (a shared-map-file design was tried and demonstrably +// races: two concurrent writers for different keys can each read before +// either has written, then each overwrite the whole file with only their +// own key — verified by a failing test before this per-file design replaced +// it). +function statusSnapshotPath(queryKey: string): string { + const hash = createHash('sha256').update(queryKey).digest('hex').slice(0, 16) + return join(getCodeburnCacheDir(), `${STATUS_SNAPSHOT_FILE}.${hash}.json`) } -async function readStatusSnapshotRecord(): Promise { +async function readStatusSnapshotRecord(queryKey: string): Promise { try { - const raw = await readFile(statusSnapshotPath(), 'utf-8') + const raw = await readFile(statusSnapshotPath(queryKey), 'utf-8') const parsed = JSON.parse(raw) as Partial if ( parsed.version !== STATUS_SNAPSHOT_VERSION || typeof parsed.corpusFingerprint !== 'string' || typeof parsed.newestMtimeMs !== 'number' || !Number.isFinite(parsed.newestMtimeMs) || - typeof parsed.queryKey !== 'string' + parsed.queryKey !== queryKey ) return null return parsed as StatusSnapshotRecord } catch { @@ -1795,12 +1815,27 @@ async function readStatusSnapshotRecord(): Promise * the one-shot processes that call this — each CLI poll is a fresh process * with no in-memory state to carry it). A failed write just means the next * poll recomputes, or re-observes the mismatch as if it were first, instead - * of reusing/deferring — never stale or corrupt data either way. */ -async function writeStatusSnapshotRecord(record: StatusSnapshotRecord): Promise { + * of reusing/deferring — never stale or corrupt data either way. + * + * Re-reads this queryKey's file immediately before rename and asks `guard` + * whether the record it found still matches what the write assumed — + * refusing (silently, a no-op) otherwise. This closes the race in review + * finding B-G1: a slower/older recompute landing after a faster/newer one + * for the SAME queryKey (guarded by `newestMtimeMs` ordering in + * `saveStatusSnapshot`), and a delayed "first mismatch" bookkeeping write + * reintroducing a stale payload after a real recompute already published a + * fresh one (guarded by an exact prior-record match in `loadStatusSnapshot`). */ +async function writeStatusSnapshotRecord( + queryKey: string, + record: StatusSnapshotRecord, + guard: (existing: StatusSnapshotRecord | null) => boolean, +): Promise { try { const dir = getCodeburnCacheDir() if (!existsSync(dir)) await mkdir(dir, { recursive: true }) - const finalPath = statusSnapshotPath() + const finalPath = statusSnapshotPath(queryKey) + const existing = await readStatusSnapshotRecord(queryKey) + if (!guard(existing)) return const tempPath = `${finalPath}.${randomBytes(8).toString('hex')}.tmp` const handle = await open(tempPath, 'w', 0o600) try { @@ -1829,25 +1864,42 @@ async function writeStatusSnapshotRecord(record: StatusSnapshotRecord): Promise< * THIS record's fingerprint first stopped matching makes the settle window * a true bound regardless of what else in the corpus is busy. */ export async function loadStatusSnapshot(corpusFingerprint: string, newestMtimeMs: number, queryKey: string): Promise { - const stored = await readStatusSnapshotRecord() - if (!stored || stored.queryKey !== queryKey) return null + const stored = await readStatusSnapshotRecord(queryKey) + if (!stored) return null if (stored.corpusFingerprint === corpusFingerprint) return stored.payload ?? null const now = Date.now() const firstSeenAt = stored.mismatchFirstSeenAt ?? now if (now - firstSeenAt >= statusSnapshotSettleMs()) return null if (stored.mismatchFirstSeenAt === undefined) { - await writeStatusSnapshotRecord({ ...stored, newestMtimeMs, mismatchFirstSeenAt: firstSeenAt }) + // Bookkeeping-only write: only proceed if the on-disk record is exactly + // what we just read (same corpusFingerprint, still no + // mismatchFirstSeenAt). If a concurrent real recompute already replaced + // it, this stale payload must not be reintroduced under a + // freshly-stamped timestamp. + const basisFingerprint = stored.corpusFingerprint + await writeStatusSnapshotRecord( + queryKey, + { ...stored, newestMtimeMs, mismatchFirstSeenAt: firstSeenAt }, + existing => existing !== null && existing.corpusFingerprint === basisFingerprint && existing.mismatchFirstSeenAt === undefined, + ) } return stored.payload ?? null } /** Best-effort: a failed write just means the next poll recomputes instead * of reusing. Only ever called by the caller when `loadStatusSnapshot` - * missed, so a settled recompute's result always supersedes whatever was - * there before. Always writes a fresh record with no `mismatchFirstSeenAt`, - * which is exactly what clears the settle-window clock once a real - * recompute lands. */ + * missed, so a settled recompute's result should supersede whatever was + * there before — UNLESS a concurrent recompute already published one based + * on a strictly fresher corpus observation (`newestMtimeMs`), in which case + * this (slower, now-stale) write is refused rather than clobbering it. Ties + * proceed: both reflect a correct recompute of the same corpus state. Always + * writes a fresh record with no `mismatchFirstSeenAt`, which is exactly what + * clears the settle-window clock once a real recompute lands. */ export async function saveStatusSnapshot(corpusFingerprint: string, newestMtimeMs: number, queryKey: string, payload: unknown): Promise { - await writeStatusSnapshotRecord({ version: STATUS_SNAPSHOT_VERSION, corpusFingerprint, newestMtimeMs, queryKey, payload }) + await writeStatusSnapshotRecord( + queryKey, + { version: STATUS_SNAPSHOT_VERSION, corpusFingerprint, newestMtimeMs, queryKey, payload }, + existing => !existing || existing.newestMtimeMs <= newestMtimeMs, + ) } diff --git a/tests/cli-status-menubar.test.ts b/tests/cli-status-menubar.test.ts index 5a3082c7..74b04101 100644 --- a/tests/cli-status-menubar.test.ts +++ b/tests/cli-status-menubar.test.ts @@ -1,5 +1,5 @@ import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' -import { existsSync, statSync } from 'node:fs' +import { existsSync, readdirSync, statSync } from 'node:fs' import { tmpdir } from 'node:os' import { delimiter as pathDelimiter, join } from 'node:path' import { spawnSync } from 'node:child_process' @@ -7,6 +7,15 @@ import { spawnSync } from 'node:child_process' import { describe, expect, it, vi } from 'vitest' import { CACHE_SCHEMA_VERSION } from '../src/models.js' +// Each distinct query gets its own `status-snapshot..json` file +// (review finding B-G1) rather than one shared fixed path — these tests +// don't know the hash up front, so they locate whatever landed by pattern. +const SNAPSHOT_FILE_RE = /^status-snapshot\.[0-9a-f]+\.json$/ +function findSnapshotFiles(cacheDir: string): string[] { + if (!existsSync(cacheDir)) return [] + return readdirSync(cacheDir).filter(f => SNAPSHOT_FILE_RE.test(f)).map(f => join(cacheDir, f)) +} + // Every case here spawns the real CLI and does genuine multi-provider parse // work; the 5s default is fine on a dev laptop and not on a shared 2-core // runner, where individual cases have been observed needing 6-8s. @@ -677,8 +686,9 @@ describe('codeburn status --format menubar-json', () => { // The persisted snapshot carries cost/usage aggregates and project // paths, so it must land group/world-unreadable regardless of umask. - const snapshotPath = join(home, '.cache', 'codeburn', 'status-snapshot.json') - expect(statSync(snapshotPath).mode & 0o777).toBe(0o600) + const snapshotFiles = findSnapshotFiles(join(home, '.cache', 'codeburn')) + expect(snapshotFiles).toHaveLength(1) + expect(statSync(snapshotFiles[0]!).mode & 0o777).toBe(0o600) // Identical query against an unchanged corpus: served from the // snapshot, byte-identical to the first call. @@ -782,8 +792,8 @@ describe('codeburn status --format menubar-json', () => { expect(before.status, `stderr: ${before.stderr}`).toBe(0) const findingsBefore = (JSON.parse(before.stdout) as { optimize: { findingCount: number } }).optimize.findingCount - const snapshotPath = join(home, '.cache', 'codeburn', 'status-snapshot.json') - expect(existsSync(snapshotPath)).toBe(false) + const cacheDir = join(home, '.cache', 'codeburn') + expect(findSnapshotFiles(cacheDir)).toEqual([]) // Mutable, non-fingerprinted optimize input: an unused custom agent // definition. The session corpus is untouched, so a corpus-fingerprint- @@ -797,7 +807,7 @@ describe('codeburn status --format menubar-json', () => { const findingsAfter = (JSON.parse(after.stdout) as { optimize: { findingCount: number } }).optimize.findingCount expect(findingsAfter).toBe(findingsBefore + 1) - expect(existsSync(snapshotPath)).toBe(false) + expect(findSnapshotFiles(cacheDir)).toEqual([]) } finally { await rm(home, { recursive: true, force: true }) } diff --git a/tests/providers/cline-cli.test.ts b/tests/providers/cline-cli.test.ts index a32bef17..42a59aae 100644 --- a/tests/providers/cline-cli.test.ts +++ b/tests/providers/cline-cli.test.ts @@ -161,7 +161,10 @@ describe('cline-cli provider - discovery', () => { expect(sources).toHaveLength(2) expect(sources.map(s => s.provider)).toEqual(['cline-cli', 'cline-cli']) - expect(sources[0]?.path).toBe(join(tmpDir, 'sess-a', 'sess-a.json')) + // The growing messages file is fingerprinted, not the static metadata + // file written once at session start (review finding C-G1) — otherwise + // a live session's new turns are invisible to computeCorpusFingerprint. + expect(sources[0]?.path).toBe(join(tmpDir, 'sess-a', 'sess-a.messages.json')) }) it('names the project from the workspace root', async () => { diff --git a/tests/session-cache-status-snapshot.test.ts b/tests/session-cache-status-snapshot.test.ts new file mode 100644 index 00000000..6f9e4a75 --- /dev/null +++ b/tests/session-cache-status-snapshot.test.ts @@ -0,0 +1,102 @@ +// The status-snapshot file (one `status-snapshot..json` per +// distinct query) is written by the same one-shot-CLI-process-per-poll model +// as the main session cache — a menubar poll and a manual refresh are two +// independent processes that can both be mid-write against the same cache +// dir at once. `session-cache-shards.test.ts` has a dedicated +// `describe('concurrent writers', ...)` block for the main cache's +// structurally identical tmp+rename atomic-write pattern; this file is the +// analogous coverage for the snapshot file (review finding D-G9), and +// exercises the CAS fix for finding B-G1 directly: a slower, older-corpus +// write must not clobber a faster, newer one, and two distinct queryKeys +// must not evict each other. +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { mkdir, readdir, readFile, rm } from 'fs/promises' +import { existsSync } from 'fs' +import { createHash } from 'crypto' +import { tmpdir } from 'os' +import { join } from 'path' + +import { loadStatusSnapshot, saveStatusSnapshot } from '../src/session-cache.js' + +let TMP_DIR: string + +beforeEach(async () => { + TMP_DIR = join(tmpdir(), `codeburn-snapshot-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`) + process.env['CODEBURN_CACHE_DIR'] = TMP_DIR + await mkdir(TMP_DIR, { recursive: true }) +}) + +afterEach(async () => { + if (existsSync(TMP_DIR)) await rm(TMP_DIR, { recursive: true }) +}) + +async function readRawRecord(queryKey: string): Promise | null> { + const hash = createHash('sha256').update(queryKey).digest('hex').slice(0, 16) + try { + const raw = await readFile(join(TMP_DIR, `status-snapshot.${hash}.json`), 'utf-8') + return JSON.parse(raw) as Record + } catch { + return null + } +} + +describe('concurrent writers (status snapshot)', () => { + it('never lets a slower recompute against an older corpus clobber a faster, newer one', async () => { + const queryKey = 'q1' + // Baseline: both processes started from this on-disk state. + await saveStatusSnapshot('f1', 1_000, queryKey, { p: 'baseline' }) + + // Process A observed the corpus at m=2_000 and is slow to finish. + // Process B observed it LATER, at m=3_000, and finishes first. + await saveStatusSnapshot('f3', 3_000, queryKey, { p: 'B-fresh' }) + // A's write lands after B's despite being based on an older observation. + await saveStatusSnapshot('f2', 2_000, queryKey, { p: 'A-stale' }) + + const record = await readRawRecord(queryKey) + expect(record).toMatchObject({ corpusFingerprint: 'f3', newestMtimeMs: 3_000, payload: { p: 'B-fresh' } }) + + // Confirmed via the public read path too. + const served = await loadStatusSnapshot('f3', 3_000, queryKey) + expect(served).toEqual({ p: 'B-fresh' }) + }) + + it('does not let a delayed mismatch-bookkeeping write reintroduce a payload a real recompute already superseded', async () => { + const queryKey = 'q1' + await saveStatusSnapshot('f1', 1_000, queryKey, { p: 'v1' }) + + // A load observes the corpus moved to f2 (within the settle window) and + // would normally persist a bookkeeping mismatchFirstSeenAt timestamp — + // but a real recompute for f2 lands first. + const stale = await loadStatusSnapshot('f2', 1_500, queryKey) + expect(stale).toEqual({ p: 'v1' }) // served from the settle window + + await saveStatusSnapshot('f2', 1_500, queryKey, { p: 'v2-real' }) + + const record = await readRawRecord(queryKey) + // The real recompute's record must be exactly what's on disk — no + // mismatchFirstSeenAt bookkeeping should have overwritten it with the + // stale v1 payload. + expect(record).toMatchObject({ corpusFingerprint: 'f2', payload: { p: 'v2-real' } }) + expect((record as { mismatchFirstSeenAt?: number }).mismatchFirstSeenAt).toBeUndefined() + }) + + it('never publishes a torn write and always leaves a subsequent read intact, even racing two distinct queryKeys', async () => { + for (let round = 0; round < 15; round++) { + await Promise.allSettled([ + saveStatusSnapshot(`a${round}`, round, 'query-a', { round, who: 'a' }), + saveStatusSnapshot(`b${round}`, round, 'query-b', { round, who: 'b' }), + ]) + // Whichever landed, EACH queryKey's own file must be valid JSON and + // present — a shared single-slot file would have one evict the other + // every round; distinct per-queryKey files never touch each other. + expect(await readRawRecord('query-a')).toBeTruthy() + expect(await readRawRecord('query-b')).toBeTruthy() + // A subsequent read must not throw on whatever landed. + await loadStatusSnapshot(`a${round}`, round, 'query-a') + await loadStatusSnapshot(`b${round}`, round, 'query-b') + } + // No stray .tmp files left mid-directory after the last round settles. + const leftoverTemps = (await readdir(TMP_DIR)).filter(f => f.endsWith('.tmp')) + expect(leftoverTemps).toEqual([]) + }) +})