From 93351c9864eb4a882d4054ef919f76e259621030 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:08:00 -0700 Subject: [PATCH 01/21] docs(plans): C6 small unbounded growth, items verified on main (#1278) Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-27-c6-small-unbounded.md | 22 +++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 docs/plans/2026-09-27-c6-small-unbounded.md diff --git a/docs/plans/2026-09-27-c6-small-unbounded.md b/docs/plans/2026-09-27-c6-small-unbounded.md new file mode 100644 index 000000000..dd6f7c309 --- /dev/null +++ b/docs/plans/2026-09-27-c6-small-unbounded.md @@ -0,0 +1,22 @@ +# C6 small unbounded growth (#1278) + +Source: the Stage 3 C6 hunt (`temp/quality-loop/hunt-c6.md`, rows 7–14 and part of 5). Each item was verified on origin/main `5e22c7b0` by a read-only Explore pass, then re-read by hand at the fix site. Every fix has a test that fails without it (fail-first, or the fix removed as a mutation). + +**Owner rule (2026-09-27): "do not delete stuff often."** Fixes bound *memory* and repair a cleanup that was already intended. None of them adds a new deletion policy for user-visible data. + +| # | Item | Verdict on main | Change | Test | +|---|---|---|---|---| +| 1 | `pasteDebugJournal.ts` `journals` Map | Real. A paste id is a fresh UUID and `dispose()` has no caller, so there was one writer per paste forever. | Cap at 64 with oldest-first eviction, and make `flushAll` await evicted flushes. The same bound and shape as `dictationJournal` (#1276). | New `pasteDebugJournal.test.ts`. Mutant: dropping the evicted-flush drain goes red. | +| 2 | Empty proxy parent dirs | **A bug, not a missing feature.** `removeEmptyParents` called `rm(dir, { recursive: false })`, which always throws EISDIR (confirmed on Node 24.14.1), so no parent was ever removed. The author's machine has 2,978 empty dirs, walked on every prune. | `rmdir`, which removes only an empty dir and does it atomically: a concurrent new run makes it fail with ENOTEMPTY. `root + sep` keeps sibling roots out of scope. | Real-filesystem test. Red on main; the old `rm` as a mutant goes red. | +| 3a | Legacy `saved-debug-bundles.jsonl` re-parsed on every prune | Real (18.4 MB, every 5 min). Nothing appends to it any more. | Cache the parsed set by file identity (mtime + size), so any edit re-parses. | Parse count test. Mutant: dropping the cache goes red. | +| 3b | `autosaved-debug-bundles.jsonl` append-only | Real, but small per line. Its append-only design as an operator index is a stated choice. | **Residual.** Trimming a ledger is a deletion policy; left for the owner. | none | +| 4 | WorkflowBridge `runsBySession` / `latestLifecycleByRunId` | Grows with the durable workflow-mcp store (`start()` reloads every stored run). | **Residual:** bounded by the store once #1275's retention (90 days, resumable never deleted) lands. Pruning the maps alone bounds nothing. | none | +| 5 | `sessionManager` `lastActivityAt` | Intentional; WHY at `sessionManager.ts:1095-1099` (telemetry asks about exited panes). One number per session id. | None. | none | +| 6 | Conversation caches | Grow with the transcript store plus deleted transcripts, not over time. | **Codex `heads`:** entries for files the (scope-independent) walk no longer finds are swept; this is exact. **Search `promptCache`:** LRU 1024, the same bound as the prompt folder's, via a new shared `LruMap` that now also backs `promptFolder.ts` (its private LRU functions are gone). **Claude `summaries`, codex `rolloutPaths`:** residual; a scoped discovery cannot tell a deleted transcript from an unwalked one, and a count cap below the store size would re-read the store on every full discovery. | Codex heads sweep test (mutant red); `lruMap.test.ts`; `promptFolder` suite 13/13 unchanged. | +| 7 | `cli-update-logs/` | Real but tiny. This machine has 13 files and 52 KB since 2026-09-03. `cliUpdateOrchestrator.ts:57-61` keeps them on purpose ("the diagnostic value of a two-year-old failed-update log is nonzero"). | **None.** Adding age deletion here would contradict both the WHY and the owner's rule. | none | +| 8 | `windowRegistry` `retiredWebContentsIds` | Real. The comment's "cleared with the registry" only ever happened in the test reset. | Cap at 256 by insertion order. Only a save dequeued moments after `closed` needs a tombstone, and webContents ids are never reused. | Red on main (id 1 still resolved after 300 closes). | +| 9 | `worktree-activity-index.json` rewritten whole | Derived cache, rebuilt from current candidates, so it grows with the corpus and not over time. | **Out of scope:** #767 item 3 (save on an all-cache-hit refresh) is the tracked remainder. | none | + +## Overlap + +#1411 (C5 rows) also edits `codex.ts`, `codex.system.test.ts`, `debugRetention.ts` and `debugRetention.test.ts`, in different hunks. Both PRs append tests at the same anchors, so whichever merges second resolves a trivial test-file conflict. From dfa0c7370fd7f52726a009e76a52614838f31908 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:08:01 -0700 Subject: [PATCH 02/21] fix(storage): empty proxy parent dirs are removed (rmdir, not rm), and the legacy ledger parse is cached (#1278) Co-Authored-By: Claude Opus 5.5 --- src/main/storage/debugRetention.test.ts | 50 +++++++++++++++++++++++-- src/main/storage/debugRetention.ts | 49 ++++++++++++++++++++---- 2 files changed, 88 insertions(+), 11 deletions(-) diff --git a/src/main/storage/debugRetention.test.ts b/src/main/storage/debugRetention.test.ts index e4c0683f7..da5d8f004 100644 --- a/src/main/storage/debugRetention.test.ts +++ b/src/main/storage/debugRetention.test.ts @@ -1,10 +1,10 @@ -import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs' +import { existsSync, mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { collectSessionRecordingDirs, runPrunePasses } from './debugRetention.js' +import { cachedManualLegacyBundlePaths, collectSessionRecordingDirs, removeEmptyProxyParents, runPrunePasses } from './debugRetention.js' import type { DebugStorageArtifact, DebugStorageBucket, @@ -226,3 +226,47 @@ describe('runPrunePasses', () => { expect(result).toEqual({ removed: 0, bytesFreed: 0, remainingBytes: 500 }) }) }) + +describe('removeEmptyProxyParents (#1278)', () => { + // Proxy runs live at proxy////. Pruning removed + // only the leaf, and the parent sweep called rm() without `recursive` on a + // directory, which always throws EISDIR, so no parent was ever removed: + // the author's machine held 2,978 empty session/project dirs, walked on + // every prune. This drives the real filesystem because the in-memory prune + // tests never reached the sweep. + it('removes the emptied session and project dirs, stops at a non-empty one, and never removes the root', async () => { + const proxyRoot = join(root, 'proxy') + const lonelyRun = join(proxyRoot, 'project-a', 'session-1', '2026-09-01T00-00-00') + const keptRun = join(proxyRoot, 'project-b', 'session-2', '2026-09-01T00-00-00') + const prunedSibling = join(proxyRoot, 'project-b', 'session-3', '2026-09-01T00-00-00') + for (const dir of [lonelyRun, keptRun, prunedSibling]) mkdirSync(dir, { recursive: true }) + rmSync(lonelyRun, { recursive: true }) + rmSync(prunedSibling, { recursive: true }) + + await removeEmptyProxyParents(lonelyRun, proxyRoot) + await removeEmptyProxyParents(prunedSibling, proxyRoot) + + expect(existsSync(join(proxyRoot, 'project-a'))).toBe(false) + expect(existsSync(join(proxyRoot, 'project-b', 'session-3'))).toBe(false) + expect(existsSync(keptRun)).toBe(true) + expect(existsSync(proxyRoot)).toBe(true) + }) +}) + +describe('cachedManualLegacyBundlePaths (#1278)', () => { + // The legacy mixed ledger no longer grows, but it was re-parsed on every + // five-minute prune. It is now parsed again only when the file changes. + it('parses once while the ledger is unchanged and again after it changes', async () => { + const ledger = join(root, 'saved-debug-bundles.jsonl') + writeFileSync(ledger, '{"event":"saved","reason":"manual","bundlePath":"/b/1"}\n') + const load = vi.fn(async () => new Set(['/b/1'])) + + await cachedManualLegacyBundlePaths(ledger, load) + await cachedManualLegacyBundlePaths(ledger, load) + expect(load).toHaveBeenCalledTimes(1) + + writeFileSync(ledger, '{"event":"saved","reason":"manual","bundlePath":"/b/1"}\n{"event":"saved","reason":"manual","bundlePath":"/b/2"}\n') + await cachedManualLegacyBundlePaths(ledger, load) + expect(load).toHaveBeenCalledTimes(2) + }) +}) diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 8bdaace51..0c81c1c1f 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -1,5 +1,5 @@ -import { mkdir, readFile, readdir, rm, stat, statfs } from 'node:fs/promises' -import { dirname, join, resolve } from 'node:path' +import { mkdir, readFile, readdir, rm, rmdir, stat, statfs } from 'node:fs/promises' +import { dirname, join, resolve, sep } from 'node:path' import { AUTOSAVE_DEBUG_BUNDLE_DIR, @@ -454,8 +454,30 @@ function bucketCaps(totalBudget: number): Record { } } +// WHY the legacy ledger parse is cached by file identity (#1278): nothing has +// appended to the pre-split mixed ledger since manual and autosave bundles got +// their own folders (debugBundleLog.ts), yet it was re-read and re-parsed on +// every prune, every five minutes for the life of the process (18.4 MB on the +// author's machine). Keying on mtime + size rather than caching forever keeps +// it correct if an operator hand-edits or deletes the file: any change re-parses. +// A missing file caches too (as an empty set), because readFile failing is +// what loadManualLegacyBundlePaths already treats as "no manual bundles". +let legacyLedgerCache: { key: string; paths: Set } | null = null + +export async function cachedManualLegacyBundlePaths( + file: string = DEBUG_BUNDLE_LOG_FILE, + load: () => Promise> = loadManualLegacyBundlePaths, +): Promise> { + const identity = await stat(file).then(info => `${info.mtimeMs}:${info.size}`, () => 'missing') + const key = `${file}\0${identity}` + if (legacyLedgerCache?.key === key) return legacyLedgerCache.paths + const paths = await load() + legacyLedgerCache = { key, paths } + return paths +} + async function collectArtifacts(): Promise { - const manualLegacyBundlePaths = await loadManualLegacyBundlePaths() + const manualLegacyBundlePaths = await cachedManualLegacyBundlePaths() const [feed, manualBundles, autosaveBundles, legacyBundles, proxy, performance, incidents, heapSnapshots, sessionRecordings] = await Promise.all([ collectFiles(FEED_DEBUG_DIR, 'feed-debug', name => name.endsWith('.jsonl')), collectImmediateDirs(MANUAL_DEBUG_BUNDLE_DIR, 'debug-bundles-manual'), @@ -732,16 +754,27 @@ async function removeArtifact(artifact: Artifact): Promise { async function removeEmptyParents(path: string, bucket: DebugStorageBucket): Promise { if (bucket !== 'proxy') return + await removeEmptyProxyParents(path, PROXY_EVENTS_DIR) +} + +// WHY rmdir and not rm (#1278): this used to readdir() and then call +// rm(dir, { recursive: false }), which ALWAYS throws EISDIR on a directory, so +// the catch returned on the first parent and no emptied session or project +// dir was ever removed (2,978 of them on the author's machine, each walked by +// collectProxyRunDirs on every prune). rmdir removes a directory only while it +// is empty, and it does so atomically: a session that creates a new run dir +// between our check and the removal makes rmdir fail with ENOTEMPTY instead of +// deleting its fresh run, which the old readdir-then-remove shape could not +// promise. `root + sep` keeps a sibling like `proxy-old/` out of scope. +export async function removeEmptyProxyParents(path: string, root: string): Promise { let current = dirname(path) - while (current.startsWith(PROXY_EVENTS_DIR) && current !== PROXY_EVENTS_DIR) { + while (current.startsWith(root + sep)) { try { - const entries = await readdir(current) - if (entries.length > 0) return - await rm(current, { recursive: false, force: true }) - current = dirname(current) + await rmdir(current) } catch { return } + current = dirname(current) } } From 2f7d40c5026f5c14f674748b71907118507b961f Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:08:01 -0700 Subject: [PATCH 03/21] fix(paste-debug): cap open journal writers at 64 like dictation's (#1278) Co-Authored-By: Claude Opus 5.5 --- src/main/pasteDebugJournal.test.ts | 27 +++++++++++++++++++++++++ src/main/pasteDebugJournal.ts | 32 ++++++++++++++++++++++++++++-- 2 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 src/main/pasteDebugJournal.test.ts diff --git a/src/main/pasteDebugJournal.test.ts b/src/main/pasteDebugJournal.test.ts new file mode 100644 index 000000000..5d9861c6e --- /dev/null +++ b/src/main/pasteDebugJournal.test.ts @@ -0,0 +1,27 @@ +import { mkdtemp, readFile, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, expect, it, vi } from 'vitest' + +const userData = { dir: tmpdir() } +vi.mock('electron', () => ({ app: { getPath: () => userData.dir } })) +const { PasteDebugJournalRegistry, pasteDebugLogPath } = await import('./pasteDebugJournal.js') + +const dirs: string[] = [] +afterEach(async () => { await Promise.all(dirs.splice(0).map(dir => rm(dir, { recursive: true, force: true }))) }) + +// #1278: a paste id is a fresh UUID per Enter and dispose() had no caller, so +// the registry kept one writer per paste for the life of the process. +it('keeps at most 64 writers, never evicts the one it hands out, and still writes an evicted paste on shutdown', async () => { + userData.dir = await mkdtemp(join(tmpdir(), 'ac-paste-user-')) + dirs.push(userData.dir) + const registry = new PasteDebugJournalRegistry() + registry.get('paste-0').append({ layer: 'RENDER', event: 'enter:observed' }) + for (let i = 1; i <= 200; i++) registry.get(`paste-${i}`) + expect(registry.size).toBe(64) + const newest = registry.get('paste-200') + expect(registry.get('paste-200')).toBe(newest) + + await registry.flushAll() + expect(await readFile(pasteDebugLogPath('paste-0'), 'utf8')).toContain('enter:observed') +}) diff --git a/src/main/pasteDebugJournal.ts b/src/main/pasteDebugJournal.ts index f41d13a33..5af2bc340 100644 --- a/src/main/pasteDebugJournal.ts +++ b/src/main/pasteDebugJournal.ts @@ -115,14 +115,38 @@ export class PasteDebugJournal { } } +/** + * How many paste writers the registry keeps (#1278), the same bound and + * eviction as dictationJournal's MAX_OPEN_JOURNALS (#1276). A paste id is a + * fresh renderer UUID that is never reused, and dispose() has no caller, so a + * long-running app kept one writer (and its queue) per paste forever. A paste + * logs for a second or two around one Enter, so evicting the oldest of 64 never + * touches a live one in practice; if it ever did, get() just opens a new writer + * that appends to the same file. + */ +const MAX_OPEN_JOURNALS = 64 + export class PasteDebugJournalRegistry { private journals = new Map() + /** Flushes started by dispose(), until they settle; see flushAll. */ + private readonly disposing = new Set>() + + get size(): number { + return this.journals.size + } get(pasteId: string): PasteDebugJournal { let j = this.journals.get(pasteId) if (!j) { j = new PasteDebugJournal(pasteDebugLogPath(pasteId)) this.journals.set(pasteId, j) + // Insertion order is age: evict the oldest paste (flushing it first), + // never the one just asked for. + while (this.journals.size > MAX_OPEN_JOURNALS) { + const oldest = this.journals.keys().next().value + if (oldest === undefined || oldest === pasteId) break + this.dispose(oldest) + } } return j } @@ -133,15 +157,19 @@ export class PasteDebugJournalRegistry { console.warn('[pasteDebugJournal] flush error:', err) }), ) - await Promise.all(drains) + // An evicted writer is no longer in the map, but its final flush belongs + // to the shutdown drain too, or its queued events are lost on quit. + await Promise.all([...drains, ...this.disposing]) } dispose(pasteId: string): void { const j = this.journals.get(pasteId) if (!j) return - void j.flush().catch(err => { + const flushing = j.flush().catch(err => { console.warn('[pasteDebugJournal] dispose flush error:', err) }) + this.disposing.add(flushing) + void flushing.finally(() => this.disposing.delete(flushing)) this.journals.delete(pasteId) } } From d70e0723413bef339eafeb17b1c61e0b06b9dbf3 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:08:01 -0700 Subject: [PATCH 04/21] fix(window): bound the closed-window tombstones at 256 (#1278) Co-Authored-By: Claude Opus 5.5 --- src/main/window/windowRegistry.test.ts | 15 +++++++++++++++ src/main/window/windowRegistry.ts | 16 +++++++++++++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/main/window/windowRegistry.test.ts b/src/main/window/windowRegistry.test.ts index ea0e757ba..0d724da2e 100644 --- a/src/main/window/windowRegistry.test.ts +++ b/src/main/window/windowRegistry.test.ts @@ -241,6 +241,21 @@ describe('window registry routing', () => { expect(registry.windowIdForWebContentsId(webContentsId)).toBe(window) }) + it('keeps a bounded tombstone: the newest closed windows still resolve, the oldest are forgotten (#1278)', () => { + // One tombstone per closed window was kept for the life of the process. + // Only a save dequeued moments after `closed` needs one, so the oldest + // can go once 256 newer windows have closed. + for (let i = 0; i < 300; i++) { + registry.createAppWindow() + built[i]?.hooks.onClosed() + } + // The fake assigns webContents.id from creation order, starting at 1. + expect(registry.windowIdForWebContentsId(300)).not.toBeNull() + expect(registry.windowIdForWebContentsId(300 - 255)).not.toBeNull() + expect(registry.windowIdForWebContentsId(300 - 256)).toBeNull() + expect(registry.windowIdForWebContentsId(1)).toBeNull() + }) + it('broadcasts app-wide state to every live window', () => { registry.createAppWindow() registry.createAppWindow() diff --git a/src/main/window/windowRegistry.ts b/src/main/window/windowRegistry.ts index 219a5a982..6d96b6648 100644 --- a/src/main/window/windowRegistry.ts +++ b/src/main/window/windowRegistry.ts @@ -113,10 +113,15 @@ function deliverSessionLease(lease: SessionWindowLease, channel: string, args: u * admitting exactly that save. A sender that was NEVER registered is still * rejected; this only remembers senders that were. * - * Bounded because window ids are minted per window and a session has a - * realistic ceiling on how many it opens; entries are tiny and the map is - * cleared with the registry. + * Bounded by insertion order at RETIRED_WEB_CONTENTS_LIMIT (#1278). The old + * comment said the map was "cleared with the registry", but only the test-only + * reset ever cleared it, so a long-running app kept one tombstone per window it + * ever closed. The late sender this exists for is a save dequeued moments after + * `closed`, so forgetting a window only after 256 newer ones have closed can + * never drop it. webContents ids are never reused within a process, so a + * forgotten id cannot be confused with a live window's. */ +const RETIRED_WEB_CONTENTS_LIMIT = 256 const retiredWebContentsIds = new Map() /** @@ -425,6 +430,11 @@ export function createAppWindow(options?: { const closing = windows.get(id) if (closing && !closing.window.isDestroyed()) { retiredWebContentsIds.set(closing.window.webContents.id, id) + while (retiredWebContentsIds.size > RETIRED_WEB_CONTENTS_LIMIT) { + const oldest = retiredWebContentsIds.keys().next().value + if (oldest === undefined) break + retiredWebContentsIds.delete(oldest) + } } windows.delete(id) const index = focusOrder.indexOf(id) From d5101fa59c3dc1eea2f781b3bd6e011a1586e211 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:08:01 -0700 Subject: [PATCH 05/21] fix(conversations): sweep Codex heads of gone rollouts; LRU-bound the search prompt cache via a shared LruMap (#1278) Co-Authored-By: Claude Opus 5.5 --- .../conversations/prompts/promptFolder.ts | 28 ++--------- src/main/conversations/service.ts | 9 +++- .../sources/codex.system.test.ts | 21 +++++++- src/main/conversations/sources/codex.ts | 7 +++ src/shared/lib/lruMap.test.ts | 29 +++++++++++ src/shared/lib/lruMap.ts | 48 +++++++++++++++++++ 6 files changed, 117 insertions(+), 25 deletions(-) create mode 100644 src/shared/lib/lruMap.test.ts create mode 100644 src/shared/lib/lruMap.ts diff --git a/src/main/conversations/prompts/promptFolder.ts b/src/main/conversations/prompts/promptFolder.ts index 20e75ba59..de449f063 100644 --- a/src/main/conversations/prompts/promptFolder.ts +++ b/src/main/conversations/prompts/promptFolder.ts @@ -3,6 +3,7 @@ import { open, stat } from 'fs/promises' import { performanceService } from '@main/performance/PerformanceService.js' import { asRecord, parseJsonRecord } from '@shared/lib/asRecord.js' +import { LruMap } from '@shared/lib/lruMap.js' // Incremental user-prompt reader over an append-only provider transcript. // @@ -82,26 +83,7 @@ const HEAD_CWD_WINDOW_BYTES = 64 * 1024 * across providers in practice, but we prefix to be safe. Bounded: * every transcript ever listed or searched used to stay here forever * with its full prompt list. */ -const promptCache = new Map() - -function cacheGet(key: string): CacheEntry | undefined { - const entry = promptCache.get(key) - if (entry === undefined) return undefined - // Re-insert so Map iteration order doubles as LRU order. - promptCache.delete(key) - promptCache.set(key, entry) - return entry -} - -function cacheSet(key: string, entry: CacheEntry): void { - if (promptCache.has(key)) promptCache.delete(key) - promptCache.set(key, entry) - while (promptCache.size > PROMPT_CACHE_MAX_ENTRIES) { - const oldest = promptCache.keys().next().value as string | undefined - if (oldest === undefined) break - promptCache.delete(oldest) - } -} +const promptCache = new LruMap(PROMPT_CACHE_MAX_ENTRIES) /** Test-only hooks: the cache is module state by design (one per process). */ export function __resetPromptFolderCacheForTests(): void { @@ -111,7 +93,7 @@ export function __promptFolderCacheEntryForTests( kind: AgentProviderKind, sessionId: string, ): { parsedFrom: number; parsedTo: number; prompts: number; cwd: string; lastBytesRead: number } | null { - const entry = promptCache.get(cacheKey(kind, sessionId)) + const entry = promptCache.peek(cacheKey(kind, sessionId)) if (!entry) return null return { parsedFrom: entry.parsedFrom, @@ -195,7 +177,7 @@ async function extractPromptsUnlocked( need: need === 'all' ? -1 : need, }) const key = cacheKey(kind, sessionId) - let entry = cacheGet(key) + let entry = promptCache.get(key) let size: number let mtime: number try { @@ -265,7 +247,7 @@ async function extractPromptsUnlocked( entry.mtime = mtime entry.size = size entry.lastBytesRead = bytesRead - cacheSet(key, entry) + promptCache.set(key, entry) span.end({ result: 'parsed', bytes: bytesRead, diff --git a/src/main/conversations/service.ts b/src/main/conversations/service.ts index 374a096a4..158f3691d 100644 --- a/src/main/conversations/service.ts +++ b/src/main/conversations/service.ts @@ -21,6 +21,7 @@ import { defaultOpencodeDataDir, OpencodeConversationSource } from './sources/op import { GrokConversationSource } from './sources/grok.js' import { PiConversationSource } from './sources/pi.js' import type { ConversationSource, SourceConversation } from './sources/types.js' +import { LruMap } from '@shared/lib/lruMap.js' // The single consumer of the catalog and the single owner of caches. // @@ -51,6 +52,7 @@ const DISCOVERY_FRESH_MS = 3_000 // half the bytes keeps the first search near the budget, and everything the // tail window misses is still searchable by label, first prompt and title. const SEARCH_PROMPT_ROWS = 150 +const SEARCH_PROMPT_CACHE_MAX_ENTRIES = 1024 const SEARCH_PROMPTS_PER_ROW = 40 const SEARCH_BYTES_PER_ROW = 128 * 1024 @@ -62,7 +64,12 @@ export class ConversationService { private discovery: Discovery | null = null private inflight: { key: string; promise: Promise } | null = null private discoveries = 0 - private readonly promptCache = new Map() + // Search prompts per row, LRU-bounded (#1278): every row ever searched stayed + // here for the life of the process. Same bound and eviction as the prompt + // folder's cache (prompts/promptFolder.ts), and for the same reason it is + // far above SEARCH_PROMPT_ROWS: every row one search folds must still be + // cached when the next keystroke arrives, or search thrashes and re-reads. + private readonly promptCache = new LruMap(SEARCH_PROMPT_CACHE_MAX_ENTRIES) constructor(private readonly deps: { sources: ConversationSource[] diff --git a/src/main/conversations/sources/codex.system.test.ts b/src/main/conversations/sources/codex.system.test.ts index 8b6fd394c..205be77e0 100644 --- a/src/main/conversations/sources/codex.system.test.ts +++ b/src/main/conversations/sources/codex.system.test.ts @@ -1,4 +1,4 @@ -import { rename } from 'node:fs/promises' +import { rename, rm } from 'node:fs/promises' import { join } from 'node:path' import { DatabaseSync } from 'node:sqlite' import { afterEach, describe, expect, it } from 'vitest' @@ -90,6 +90,25 @@ describe('Codex conversation source', () => { expect(everywhere.filter(r => r.origin === 'scan')).toHaveLength(counts.codex.unindexedSampled) }) + it('forgets the parsed head of a rollout that is gone (#1278)', async () => { + // `heads` is keyed by file and was never pruned: every rollout Codex ever + // wrote and then deleted or archived kept its head for the process's life. + const { corpus, listWorktrees } = await setup() + await rename(join(corpus.codexHome, 'state_5.sqlite'), join(corpus.codexHome, 'state_5.sqlite.away')) + const source = new CodexConversationSource({ codexHome: corpus.codexHome, walkTtlMs: 0 }) + const heads = (source as unknown as { heads: Map }).heads + const family = await resolveFamily('/fixture/repo', 'everywhere', { listWorktrees }) + const first = await source.discover({ scope: 'everywhere', family }) + const gone = first[0]!.file! + expect(heads.has(gone)).toBe(true) + + await rm(gone) + const second = await source.discover({ scope: 'everywhere', family }) + expect(second.map(r => r.file)).not.toContain(gone) + expect(heads.has(gone)).toBe(false) + expect(heads.size).toBe(second.length) + }) + it('falls back to the rollout scan when the index is missing and reports why', async () => { const { corpus, source, listWorktrees } = await setup() await rename(join(corpus.codexHome, 'state_5.sqlite'), join(corpus.codexHome, 'state_5.sqlite.away')) diff --git a/src/main/conversations/sources/codex.ts b/src/main/conversations/sources/codex.ts index 5a25536ca..50e7b530e 100644 --- a/src/main/conversations/sources/codex.ts +++ b/src/main/conversations/sources/codex.ts @@ -155,6 +155,13 @@ export class CodexConversationSource implements ConversationSource { } } await visit(join(this.deps.codexHome, 'sessions'), 0) + // Forget heads of rollouts that are gone (#1278). `heads` is keyed by + // file and was never pruned, so every rollout Codex ever wrote and later + // deleted or archived kept its parsed head for the life of the process. + // The walk covers the whole sessions tree whatever the scope, so it is + // the exact set of files a head can still belong to. No count cap: a cap + // below the store's size would make every full discovery re-read it. + for (const file of this.heads.keys()) if (!files.has(file)) this.heads.delete(file) this.walk = { at: Date.now(), files } return files } diff --git a/src/shared/lib/lruMap.test.ts b/src/shared/lib/lruMap.test.ts new file mode 100644 index 000000000..4268174ee --- /dev/null +++ b/src/shared/lib/lruMap.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it } from 'vitest' + +import { LruMap } from './lruMap.js' + +describe('LruMap', () => { + it('evicts the least recently used entry, where get() counts as use and peek() does not', () => { + const lru = new LruMap(3) + lru.set('a', 1) + lru.set('b', 2) + lru.set('c', 3) + expect(lru.get('a')).toBe(1) + expect(lru.peek('b')).toBe(2) + lru.set('d', 4) + expect(lru.size).toBe(3) + expect(lru.peek('b')).toBeUndefined() + expect([lru.peek('a'), lru.peek('c'), lru.peek('d')]).toEqual([1, 3, 4]) + }) + + it('re-setting a key refreshes it instead of growing', () => { + const lru = new LruMap(2) + lru.set('a', 1) + lru.set('b', 2) + lru.set('a', 10) + lru.set('c', 3) + expect(lru.size).toBe(2) + expect(lru.peek('a')).toBe(10) + expect(lru.peek('b')).toBeUndefined() + }) +}) diff --git a/src/shared/lib/lruMap.ts b/src/shared/lib/lruMap.ts new file mode 100644 index 000000000..4053015d3 --- /dev/null +++ b/src/shared/lib/lruMap.ts @@ -0,0 +1,48 @@ +/** + * A Map bounded by least-recent use: `get` and `set` move a key to the newest + * end, and `set` evicts from the oldest end past `maxEntries`. + * + * WHY one shared helper (#1278): the prompt folder's cache and the + * conversation service's search cache each needed exactly this, and the + * second one had been left unbounded because the first one's LRU lived in two + * private functions nobody could reuse. Map iteration order is insertion + * order, so delete-then-set is the whole mechanism; there is no linked list to + * get wrong. + */ +export class LruMap { + private readonly entries = new Map() + + constructor(private readonly maxEntries: number) {} + + get size(): number { + return this.entries.size + } + + /** Reads and marks as most recently used. */ + get(key: K): V | undefined { + if (!this.entries.has(key)) return undefined + const value = this.entries.get(key) as V + this.entries.delete(key) + this.entries.set(key, value) + return value + } + + /** Reads WITHOUT touching recency (inspection, tests). */ + peek(key: K): V | undefined { + return this.entries.get(key) + } + + set(key: K, value: V): void { + this.entries.delete(key) + this.entries.set(key, value) + while (this.entries.size > this.maxEntries) { + const oldest = this.entries.keys().next() + if (oldest.done) break + this.entries.delete(oldest.value) + } + } + + clear(): void { + this.entries.clear() + } +} From 83c3a4e52362eef6ae4f897d3d050a0a4c981cb3 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:13:44 -0700 Subject: [PATCH 06/21] fix(storage): an unreadable legacy ledger protects every legacy bundle and is never cached (steering q109) A transient read failure (EACCES, EMFILE) returned an empty manual set, which put every hand-saved legacy bundle in the deletable bucket; the identity cache then kept that empty set after access recovered. Now a failed stat/read is 'unknown': not cached, and every legacy bundle is protected for that prune. Only ENOENT means no ledger. The identity key adds inode and ctime, so a rename-replace or a same-size edit with the mtime set back re-parses. Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-27-c6-small-unbounded.md | 2 +- src/main/storage/debugRetention.test.ts | 59 +++++++++++++++++++- src/main/storage/debugRetention.ts | 60 ++++++++++++++++----- 3 files changed, 104 insertions(+), 17 deletions(-) diff --git a/docs/plans/2026-09-27-c6-small-unbounded.md b/docs/plans/2026-09-27-c6-small-unbounded.md index dd6f7c309..dd688d984 100644 --- a/docs/plans/2026-09-27-c6-small-unbounded.md +++ b/docs/plans/2026-09-27-c6-small-unbounded.md @@ -8,7 +8,7 @@ Source: the Stage 3 C6 hunt (`temp/quality-loop/hunt-c6.md`, rows 7–14 and par |---|---|---|---|---| | 1 | `pasteDebugJournal.ts` `journals` Map | Real. A paste id is a fresh UUID and `dispose()` has no caller, so there was one writer per paste forever. | Cap at 64 with oldest-first eviction, and make `flushAll` await evicted flushes. The same bound and shape as `dictationJournal` (#1276). | New `pasteDebugJournal.test.ts`. Mutant: dropping the evicted-flush drain goes red. | | 2 | Empty proxy parent dirs | **A bug, not a missing feature.** `removeEmptyParents` called `rm(dir, { recursive: false })`, which always throws EISDIR (confirmed on Node 24.14.1), so no parent was ever removed. The author's machine has 2,978 empty dirs, walked on every prune. | `rmdir`, which removes only an empty dir and does it atomically: a concurrent new run makes it fail with ENOTEMPTY. `root + sep` keeps sibling roots out of scope. | Real-filesystem test. Red on main; the old `rm` as a mutant goes red. | -| 3a | Legacy `saved-debug-bundles.jsonl` re-parsed on every prune | Real (18.4 MB, every 5 min). Nothing appends to it any more. | Cache the parsed set by file identity (mtime + size), so any edit re-parses. | Parse count test. Mutant: dropping the cache goes red. | +| 3a | Legacy `saved-debug-bundles.jsonl` re-parsed on every prune | Real (18.4 MB, every 5 min). Nothing appends to it any more. | Cache the parsed set by file identity: inode + ctime + mtime + size, so a rename-replace or a same-size edit with the mtime set back still re-parses. **Steering q109:** a failed read is `'unknown'`, never cached, and protects every legacy bundle for that prune; only ENOENT means "no ledger". This also closes main's one-prune window, where a transient read failure classified hand-saved bundles as deletable. | Parse-count test; a real temp ledger that is unreadable and then readable; a failure with unchanged identity that must not be cached; a same-size edit with the mtime set back. All three q109 tests are red at `d5101fa5`. Mutants red: caching `'unknown'`, an mtime+size key, `'unknown'` → legacy. | | 3b | `autosaved-debug-bundles.jsonl` append-only | Real, but small per line. Its append-only design as an operator index is a stated choice. | **Residual.** Trimming a ledger is a deletion policy; left for the owner. | none | | 4 | WorkflowBridge `runsBySession` / `latestLifecycleByRunId` | Grows with the durable workflow-mcp store (`start()` reloads every stored run). | **Residual:** bounded by the store once #1275's retention (90 days, resumable never deleted) lands. Pruning the maps alone bounds nothing. | none | | 5 | `sessionManager` `lastActivityAt` | Intentional; WHY at `sessionManager.ts:1095-1099` (telemetry asks about exited panes). One number per session id. | None. | none | diff --git a/src/main/storage/debugRetention.test.ts b/src/main/storage/debugRetention.test.ts index da5d8f004..16dd3d934 100644 --- a/src/main/storage/debugRetention.test.ts +++ b/src/main/storage/debugRetention.test.ts @@ -1,10 +1,10 @@ -import { existsSync, mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs' +import { chmodSync, existsSync, mkdtempSync, mkdirSync, statSync, utimesSync, writeFileSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { cachedManualLegacyBundlePaths, collectSessionRecordingDirs, removeEmptyProxyParents, runPrunePasses } from './debugRetention.js' +import { cachedManualLegacyBundlePaths, collectSessionRecordingDirs, legacyDebugBundleBucketForPath, removeEmptyProxyParents, runPrunePasses } from './debugRetention.js' import type { DebugStorageArtifact, DebugStorageBucket, @@ -270,3 +270,58 @@ describe('cachedManualLegacyBundlePaths (#1278)', () => { expect(load).toHaveBeenCalledTimes(2) }) }) + +describe('legacy ledger classification fails closed (steering q109)', () => { + const manualRow = (bundlePath: string) => `${JSON.stringify({ event: 'saved', reason: 'manual', bundlePath })}\n` + + // The blocker: a read failure returned an empty set, which classified every + // hand-saved legacy bundle as deletable, and the identity cache then kept + // that empty set after access recovered. + it('protects every legacy bundle while the ledger is unreadable, and classifies correctly once it is readable', async () => { + const ledger = join(root, 'saved-debug-bundles.jsonl') + const manualBundle = join(root, '2026-01-01T00-00-00') + const otherBundle = join(root, '2026-01-02T00-00-00') + writeFileSync(ledger, manualRow(manualBundle)) + chmodSync(ledger, 0o000) + try { + const unreadable = await cachedManualLegacyBundlePaths(ledger) + expect(unreadable).toBe('unknown') + expect(legacyDebugBundleBucketForPath(manualBundle, unreadable)).toBe('debug-bundles-manual') + expect(legacyDebugBundleBucketForPath(otherBundle, unreadable)).toBe('debug-bundles-manual') + } finally { + chmodSync(ledger, 0o600) + } + const readable = await cachedManualLegacyBundlePaths(ledger) + expect(legacyDebugBundleBucketForPath(manualBundle, readable)).toBe('debug-bundles-manual') + expect(legacyDebugBundleBucketForPath(otherBundle, readable)).toBe('debug-bundles-legacy') + }) + + // chmod moves ctime, so the sequence above re-parses through the identity + // key alone. This pins the other half on its own: a failed load is never + // cached, even when the file's identity has not changed at all. + it('retries a failed load on the next call even with an unchanged file identity', async () => { + const ledger = join(root, 'saved-debug-bundles.jsonl') + writeFileSync(ledger, manualRow('/b/1')) + const results: Array | 'unknown'> = ['unknown', new Set(['/b/1'])] + const load = vi.fn(async () => results.shift()!) + expect(await cachedManualLegacyBundlePaths(ledger, load)).toBe('unknown') + expect(await cachedManualLegacyBundlePaths(ledger, load)).toEqual(new Set(['/b/1'])) + expect(load).toHaveBeenCalledTimes(2) + }) + + // An operator edit with the same size that also restores the old mtime. + it('re-parses a same-size edit whose mtime was set back', async () => { + const ledger = join(root, 'saved-debug-bundles.jsonl') + writeFileSync(ledger, manualRow('/bundles/2026-01-01T00-00-01')) + // A whole-second mtime, so setting it back later reproduces it exactly. + const pinned = new Date('2026-01-01T00:00:00Z') + utimesSync(ledger, pinned, pinned) + const before = statSync(ledger) + expect(await cachedManualLegacyBundlePaths(ledger)).toEqual(new Set(['/bundles/2026-01-01T00-00-01'])) + writeFileSync(ledger, manualRow('/bundles/2026-01-01T00-00-02')) + utimesSync(ledger, pinned, pinned) + expect(statSync(ledger).size).toBe(before.size) + expect(statSync(ledger).mtimeMs).toBe(before.mtimeMs) + expect(await cachedManualLegacyBundlePaths(ledger)).toEqual(new Set(['/bundles/2026-01-01T00-00-02'])) + }) +}) diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 0c81c1c1f..a8b212e8b 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -454,24 +454,52 @@ function bucketCaps(totalBudget: number): Record { } } +/** + * Which legacy root-level bundles were saved by hand, or 'unknown' when the + * ledger exists but could not be read (steering q109). + * + * WHY 'unknown' instead of an empty set: the manual/legacy split is what keeps + * a hand-saved bundle out of the deletable `debug-bundles-legacy` bucket. An + * empty set on a transient read failure (EACCES, EMFILE) classified every + * manual bundle as deletable for that prune; with the cache below it would + * have stayed that way until the file changed. 'unknown' makes every legacy + * bundle protected for that prune instead. Only ENOENT is a real "no ledger". + */ +export type ManualLegacyBundlePaths = Set | 'unknown' + // WHY the legacy ledger parse is cached by file identity (#1278): nothing has // appended to the pre-split mixed ledger since manual and autosave bundles got // their own folders (debugBundleLog.ts), yet it was re-read and re-parsed on // every prune, every five minutes for the life of the process (18.4 MB on the -// author's machine). Keying on mtime + size rather than caching forever keeps -// it correct if an operator hand-edits or deletes the file: any change re-parses. -// A missing file caches too (as an empty set), because readFile failing is -// what loadManualLegacyBundlePaths already treats as "no manual bundles". +// author's machine). +// +// The identity is inode + ctime + mtime + size, not mtime + size alone +// (steering q109): a replacement by rename gets a new inode, and ctime moves on +// ANY content or metadata change and, unlike mtime, cannot be set back with +// utimes, so a same-size edit that restores the old mtime still re-parses. +// A failed stat or read ('unknown') is never cached: the next prune retries, +// exactly as it did before the cache existed. let legacyLedgerCache: { key: string; paths: Set } | null = null export async function cachedManualLegacyBundlePaths( file: string = DEBUG_BUNDLE_LOG_FILE, - load: () => Promise> = loadManualLegacyBundlePaths, -): Promise> { - const identity = await stat(file).then(info => `${info.mtimeMs}:${info.size}`, () => 'missing') + load: (file: string) => Promise = loadManualLegacyBundlePaths, +): Promise { + let identity: string + try { + const info = await stat(file) + identity = `${info.ino}:${info.ctimeMs}:${info.mtimeMs}:${info.size}` + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') return 'unknown' + identity = 'missing' + } const key = `${file}\0${identity}` if (legacyLedgerCache?.key === key) return legacyLedgerCache.paths - const paths = await load() + const paths = await load(file) + if (paths === 'unknown') { + legacyLedgerCache = null + return paths + } legacyLedgerCache = { key, paths } return paths } @@ -594,7 +622,7 @@ async function collectIncidentRunDirs(): Promise { async function collectLegacyDebugBundleDirs( dir: string, - manualLegacyBundlePaths: Set, + manualLegacyBundlePaths: ManualLegacyBundlePaths, ): Promise { try { const entries = await readdir(dir, { withFileTypes: true }) @@ -625,8 +653,10 @@ async function collectLegacyDebugBundleDirs( export function legacyDebugBundleBucketForPath( bundlePath: string, - manualLegacyBundlePaths: Set, + manualLegacyBundlePaths: ManualLegacyBundlePaths, ): DebugStorageBucket { + // An unreadable ledger protects every legacy bundle; see ManualLegacyBundlePaths. + if (manualLegacyBundlePaths === 'unknown') return 'debug-bundles-manual' return manualLegacyBundlePaths.has(resolve(bundlePath)) ? 'debug-bundles-manual' : 'debug-bundles-legacy' @@ -646,13 +676,15 @@ function isProtectedFromDebugPrune(artifact: Artifact): boolean { artifact.bucket === 'debug-bundles-manual' } -async function loadManualLegacyBundlePaths(): Promise> { +async function loadManualLegacyBundlePaths(file: string = DEBUG_BUNDLE_LOG_FILE): Promise { const manual = new Set() let raw: string try { - raw = await readFile(DEBUG_BUNDLE_LOG_FILE, 'utf8') - } catch { - return manual + raw = await readFile(file, 'utf8') + } catch (error) { + // Only a missing ledger means "no manual bundles"; any other failure is + // unknown and fails closed (steering q109). + return (error as NodeJS.ErrnoException).code === 'ENOENT' ? manual : 'unknown' } for (const line of raw.split('\n')) { From 00a5ba96819f80dada591ba471d8780aeca8a3de Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 04:27:53 -0700 Subject: [PATCH 07/21] fix(storage): one malformed legacy ledger row no longer stops debug pruning (#1251 row 13) Moved here from #1411 (manager q109: coordinate the overlap). Row 13 and steering q109 both change loadManualLegacyBundlePaths, so they ship together. A JSON-valid non-entry line (null, a number, a non-string bundlePath) threw, which rejected collectArtifacts and stopped every prune pass. Rows are now shape-checked; a non-string reason counts as manual, so the bundle is kept. Adds a test through the real loader and cache: #1411 review (b) found that replacing the loader's parse with an empty set survived the parser-only test. Co-Authored-By: Claude Opus 5.5 --- src/main/storage/debugRetention.test.ts | 43 ++++++++++++++++++++++++- src/main/storage/debugRetention.ts | 23 +++++++++---- 2 files changed, 59 insertions(+), 7 deletions(-) diff --git a/src/main/storage/debugRetention.test.ts b/src/main/storage/debugRetention.test.ts index 16dd3d934..c699cdf16 100644 --- a/src/main/storage/debugRetention.test.ts +++ b/src/main/storage/debugRetention.test.ts @@ -4,7 +4,7 @@ import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { cachedManualLegacyBundlePaths, collectSessionRecordingDirs, legacyDebugBundleBucketForPath, removeEmptyProxyParents, runPrunePasses } from './debugRetention.js' +import { cachedManualLegacyBundlePaths, collectSessionRecordingDirs, legacyDebugBundleBucketForPath, parseManualLegacyBundlePaths, removeEmptyProxyParents, runPrunePasses } from './debugRetention.js' import type { DebugStorageArtifact, DebugStorageBucket, @@ -325,3 +325,44 @@ describe('legacy ledger classification fails closed (steering q109)', () => { expect(await cachedManualLegacyBundlePaths(ledger)).toEqual(new Set(['/bundles/2026-01-01T00-00-02'])) }) }) + +describe('parseManualLegacyBundlePaths (#1251 row 13)', () => { + // The legacy ledger is append-only JSONL written across many app versions. + // A row that parses as JSON but is not a saved-entry object (a bare `null`, + // a number, an entry without a string bundlePath) used to throw out of the + // loop (`null.event`, `resolve(undefined)`), which rejected collectArtifacts + // and so stopped EVERY prune pass, for every bucket, on every trigger. + it('keeps every readable manual row and skips rows that are not saved-entry objects', () => { + const raw = [ + JSON.stringify({ event: 'saved', reason: 'manual', bundlePath: '/bundles/2026-01-01T00-00-00' }), + 'null', + '42', + '"saved"', + JSON.stringify({ event: 'saved', reason: 'manual' }), + JSON.stringify({ event: 'saved', reason: 'manual', bundlePath: 42 }), + JSON.stringify({ event: 'saved', reason: 7, bundlePath: '/bundles/2026-01-03T00-00-00' }), + '{not json', + JSON.stringify({ event: 'saved', reason: 'autosave-crash', bundlePath: '/bundles/2026-01-02T00-00-00' }), + JSON.stringify({ event: 'saved', reason: 'manual', bundlePath: '/bundles/2026-01-04T00-00-00' }), + ].join('\n') + expect([...parseManualLegacyBundlePaths(raw)]).toEqual([ + '/bundles/2026-01-01T00-00-00', + // A non-string reason is not an autosave label, and an unlabelled save + // was user-triggered in the versions that wrote this ledger, so it stays + // protected: when in doubt, retention keeps the bundle. + '/bundles/2026-01-03T00-00-00', + '/bundles/2026-01-04T00-00-00', + ]) + }) + + // Review of #1411 (b), a surviving mutation: the parser test alone could not + // see the loader stop using it. This goes through the real loader and cache. + it('classifies through the real loader, past rows that are not entries', async () => { + const ledger = join(root, 'saved-debug-bundles.jsonl') + const manualBundle = join(root, '2026-01-01T00-00-00') + writeFileSync(ledger, ['null', JSON.stringify({ event: 'saved', reason: 'manual', bundlePath: manualBundle }), ''].join('\n')) + const paths = await cachedManualLegacyBundlePaths(ledger) + expect(legacyDebugBundleBucketForPath(manualBundle, paths)).toBe('debug-bundles-manual') + expect(legacyDebugBundleBucketForPath(join(root, '2026-01-02T00-00-00'), paths)).toBe('debug-bundles-legacy') + }) +}) diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index a8b212e8b..9acba5793 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -677,27 +677,38 @@ function isProtectedFromDebugPrune(artifact: Artifact): boolean { } async function loadManualLegacyBundlePaths(file: string = DEBUG_BUNDLE_LOG_FILE): Promise { - const manual = new Set() let raw: string try { raw = await readFile(file, 'utf8') } catch (error) { // Only a missing ledger means "no manual bundles"; any other failure is // unknown and fails closed (steering q109). - return (error as NodeJS.ErrnoException).code === 'ENOENT' ? manual : 'unknown' + return (error as NodeJS.ErrnoException).code === 'ENOENT' ? new Set() : 'unknown' } + return parseManualLegacyBundlePaths(raw) +} +export function parseManualLegacyBundlePaths(raw: string): Set { + const manual = new Set() for (const line of raw.split('\n')) { const trimmed = line.trim() if (!trimmed) continue - let entry: DebugBundleLogEntry + let parsed: unknown try { - entry = JSON.parse(trimmed) as DebugBundleLogEntry + parsed = JSON.parse(trimmed) } catch { continue } - if (entry.event !== 'saved') continue - if (isAutosaveDebugBundleReason(entry.reason)) continue + // WHY a shape check and not only the JSON.parse guard (#1251 row 13): a + // line can be valid JSON and still not an entry (`null`, a number, a row + // from a build that wrote bundlePath differently). Such a row threw here, + // which rejected collectArtifacts and stopped every prune pass for every + // bucket. Skipping it can only fail to protect a bundle the row does not + // name, so it never exposes a manual bundle to deletion. + if (typeof parsed !== 'object' || parsed === null) continue + const entry = parsed as Partial & { bundlePath?: unknown; reason?: unknown } + if (entry.event !== 'saved' || typeof entry.bundlePath !== 'string') continue + if (isAutosaveDebugBundleReason(typeof entry.reason === 'string' ? entry.reason : null)) continue // WHY manual legacy classification comes from the old mixed ledger instead // of folder contents: every bundle contains a manifest, but reading // thousands of manifests during retention would turn a cheap directory From 6150238ad368de1e4ca7e231ded1f33a94314744 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:18:49 -0700 Subject: [PATCH 08/21] docs(plans): row 13 moved into #1417; overlap with #1411 narrowed to codex Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-27-c6-small-unbounded.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/plans/2026-09-27-c6-small-unbounded.md b/docs/plans/2026-09-27-c6-small-unbounded.md index dd688d984..d976e9615 100644 --- a/docs/plans/2026-09-27-c6-small-unbounded.md +++ b/docs/plans/2026-09-27-c6-small-unbounded.md @@ -9,6 +9,7 @@ Source: the Stage 3 C6 hunt (`temp/quality-loop/hunt-c6.md`, rows 7–14 and par | 1 | `pasteDebugJournal.ts` `journals` Map | Real. A paste id is a fresh UUID and `dispose()` has no caller, so there was one writer per paste forever. | Cap at 64 with oldest-first eviction, and make `flushAll` await evicted flushes. The same bound and shape as `dictationJournal` (#1276). | New `pasteDebugJournal.test.ts`. Mutant: dropping the evicted-flush drain goes red. | | 2 | Empty proxy parent dirs | **A bug, not a missing feature.** `removeEmptyParents` called `rm(dir, { recursive: false })`, which always throws EISDIR (confirmed on Node 24.14.1), so no parent was ever removed. The author's machine has 2,978 empty dirs, walked on every prune. | `rmdir`, which removes only an empty dir and does it atomically: a concurrent new run makes it fail with ENOTEMPTY. `root + sep` keeps sibling roots out of scope. | Real-filesystem test. Red on main; the old `rm` as a mutant goes red. | | 3a | Legacy `saved-debug-bundles.jsonl` re-parsed on every prune | Real (18.4 MB, every 5 min). Nothing appends to it any more. | Cache the parsed set by file identity: inode + ctime + mtime + size, so a rename-replace or a same-size edit with the mtime set back still re-parses. **Steering q109:** a failed read is `'unknown'`, never cached, and protects every legacy bundle for that prune; only ENOENT means "no ledger". This also closes main's one-prune window, where a transient read failure classified hand-saved bundles as deletable. | Parse-count test; a real temp ledger that is unreadable and then readable; a failure with unchanged identity that must not be cached; a same-size edit with the mtime set back. All three q109 tests are red at `d5101fa5`. Mutants red: caching `'unknown'`, an mtime+size key, `'unknown'` → legacy. | +| 3a′ | #1251 row 13 (moved here from #1411): one malformed legacy-ledger row stopped all pruning | Real: a JSON-valid non-entry (`null`, a non-string `bundlePath`) threw, which rejected `collectArtifacts`. | Rows are shape-checked (`parseManualLegacyBundlePaths`); a non-string reason counts as manual (kept). It ships here because row 13 and steering q109 change the same loader. | A parser test plus a test through the real loader and cache (the #1411 review's surviving 'loader returns empty' mutant is now red). | | 3b | `autosaved-debug-bundles.jsonl` append-only | Real, but small per line. Its append-only design as an operator index is a stated choice. | **Residual.** Trimming a ledger is a deletion policy; left for the owner. | none | | 4 | WorkflowBridge `runsBySession` / `latestLifecycleByRunId` | Grows with the durable workflow-mcp store (`start()` reloads every stored run). | **Residual:** bounded by the store once #1275's retention (90 days, resumable never deleted) lands. Pruning the maps alone bounds nothing. | none | | 5 | `sessionManager` `lastActivityAt` | Intentional; WHY at `sessionManager.ts:1095-1099` (telemetry asks about exited panes). One number per session id. | None. | none | @@ -17,6 +18,6 @@ Source: the Stage 3 C6 hunt (`temp/quality-loop/hunt-c6.md`, rows 7–14 and par | 8 | `windowRegistry` `retiredWebContentsIds` | Real. The comment's "cleared with the registry" only ever happened in the test reset. | Cap at 256 by insertion order. Only a save dequeued moments after `closed` needs a tombstone, and webContents ids are never reused. | Red on main (id 1 still resolved after 300 closes). | | 9 | `worktree-activity-index.json` rewritten whole | Derived cache, rebuilt from current candidates, so it grows with the corpus and not over time. | **Out of scope:** #767 item 3 (save on an all-cache-hit refresh) is the tracked remainder. | none | -## Overlap +## Overlap with #1411 (manager q109) -#1411 (C5 rows) also edits `codex.ts`, `codex.system.test.ts`, `debugRetention.ts` and `debugRetention.test.ts`, in different hunks. Both PRs append tests at the same anchors, so whichever merges second resolves a trivial test-file conflict. +Row 13 of #1251 moved here, so `debugRetention.ts` and its test now change only in this PR. #1411 still shares `codex.ts` (different hunks: #1411 guards `fromHead`, this PR sweeps `heads` in `walkRollouts`) and `codex.system.test.ts`. Both insert a test before the same anchor, so the second PR to merge resolves a trivial conflict there. From 380434a70a8b7cb3d89ea693f5b7cc416d6ee0e3 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:48:49 -0700 Subject: [PATCH 09/21] fix(storage): trust a ledger parse only if the file is unchanged across the read (#1417 review a) Co-Authored-By: Claude Opus 5.5 --- src/main/storage/debugRetention.test.ts | 31 ++++++++++++++++++++++++- src/main/storage/debugRetention.ts | 30 ++++++++++++++++-------- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/src/main/storage/debugRetention.test.ts b/src/main/storage/debugRetention.test.ts index c699cdf16..a42c1e40a 100644 --- a/src/main/storage/debugRetention.test.ts +++ b/src/main/storage/debugRetention.test.ts @@ -1,4 +1,4 @@ -import { chmodSync, existsSync, mkdtempSync, mkdirSync, statSync, utimesSync, writeFileSync, rmSync } from 'node:fs' +import { chmodSync, existsSync, mkdtempSync, mkdirSync, renameSync, statSync, utimesSync, writeFileSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -247,6 +247,12 @@ describe('removeEmptyProxyParents (#1278)', () => { await removeEmptyProxyParents(prunedSibling, proxyRoot) expect(existsSync(join(proxyRoot, 'project-a'))).toBe(false) + // Review of #1417 (a), a surviving mutation: `startsWith(root)` without the + // separator. A sibling root sharing the prefix must never be walked into. + const sibling = join(root, 'proxy-old', 'empty-project', 'session') + mkdirSync(sibling, { recursive: true }) + await removeEmptyProxyParents(join(sibling, 'gone-run'), proxyRoot) + expect(existsSync(sibling)).toBe(true) expect(existsSync(join(proxyRoot, 'project-b', 'session-3'))).toBe(false) expect(existsSync(keptRun)).toBe(true) expect(existsSync(proxyRoot)).toBe(true) @@ -309,6 +315,29 @@ describe('legacy ledger classification fails closed (steering q109)', () => { expect(load).toHaveBeenCalledTimes(2) }) + // Review of #1417 (a): the ledger renamed away between the stat and the + // read. The loader sees ENOENT and says "no ledger"; that empty answer must + // not classify the manual bundle as deletable, nor be cached, whether the + // file is still away or already back. + for (const back of [false, true]) { + it(`does not trust a load that raced a rename of the ledger (${back ? 'renamed back' : 'still away'})`, async () => { + const ledger = join(root, 'saved-debug-bundles.jsonl') + const manualBundle = join(root, '2026-01-01T00-00-00') + writeFileSync(ledger, manualRow(manualBundle)) + const raced = vi.fn(async (file: string) => { + renameSync(file, `${file}.away`) + if (back) renameSync(`${file}.away`, file) + return new Set() + }) + const first = await cachedManualLegacyBundlePaths(ledger, raced) + expect(first).toBe('unknown') + expect(legacyDebugBundleBucketForPath(manualBundle, first)).toBe('debug-bundles-manual') + if (!back) renameSync(`${ledger}.away`, ledger) + const settled = await cachedManualLegacyBundlePaths(ledger) + expect(legacyDebugBundleBucketForPath(manualBundle, settled)).toBe('debug-bundles-manual') + }) + } + // An operator edit with the same size that also restores the old mtime. it('re-parses a same-size edit whose mtime was set back', async () => { const ledger = join(root, 'saved-debug-bundles.jsonl') diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 9acba5793..09d49a57b 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -485,25 +485,35 @@ export async function cachedManualLegacyBundlePaths( file: string = DEBUG_BUNDLE_LOG_FILE, load: (file: string) => Promise = loadManualLegacyBundlePaths, ): Promise { - let identity: string - try { - const info = await stat(file) - identity = `${info.ino}:${info.ctimeMs}:${info.mtimeMs}:${info.size}` - } catch (error) { - if ((error as NodeJS.ErrnoException).code !== 'ENOENT') return 'unknown' - identity = 'missing' - } + const identity = await ledgerIdentity(file) + if (identity === 'unknown') return 'unknown' const key = `${file}\0${identity}` if (legacyLedgerCache?.key === key) return legacyLedgerCache.paths const paths = await load(file) - if (paths === 'unknown') { + // WHY a second stat (review of #1417, a): the load is a separate operation. + // A ledger renamed away between the stat and the read made readFile hit + // ENOENT, which the loader rightly calls "no ledger", and that empty set was + // then cached under the identity of the file that WAS there, so a manual + // bundle became deletable. The parse is trusted only if the file it read is + // the file the stat saw, before and after; any change (gone, replaced, + // edited mid-read) is 'unknown': protective for this prune, never cached. + if (paths === 'unknown' || (await ledgerIdentity(file)) !== identity) { legacyLedgerCache = null - return paths + return 'unknown' } legacyLedgerCache = { key, paths } return paths } +async function ledgerIdentity(file: string): Promise { + try { + const info = await stat(file) + return `${info.ino}:${info.ctimeMs}:${info.mtimeMs}:${info.size}` + } catch (error) { + return (error as NodeJS.ErrnoException).code === 'ENOENT' ? 'missing' : 'unknown' + } +} + async function collectArtifacts(): Promise { const manualLegacyBundlePaths = await cachedManualLegacyBundlePaths() const [feed, manualBundles, autosaveBundles, legacyBundles, proxy, performance, incidents, heapSnapshots, sessionRecordings] = await Promise.all([ From 44d557a86c683b3ebbcd0d517c38db8fa54fd968 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:48:49 -0700 Subject: [PATCH 10/21] fix(paste-debug): flush joins an in-flight append; a re-created writer appends after the evicted one (#1417 review b, c) Co-Authored-By: Claude Opus 5.5 --- src/main/pasteDebugJournal.test.ts | 35 +++++++++++++++ src/main/pasteDebugJournal.ts | 69 ++++++++++++++++++++++-------- 2 files changed, 86 insertions(+), 18 deletions(-) diff --git a/src/main/pasteDebugJournal.test.ts b/src/main/pasteDebugJournal.test.ts index 5d9861c6e..7339e3faa 100644 --- a/src/main/pasteDebugJournal.test.ts +++ b/src/main/pasteDebugJournal.test.ts @@ -25,3 +25,38 @@ it('keeps at most 64 writers, never evicts the one it hands out, and still write await registry.flushAll() expect(await readFile(pasteDebugLogPath('paste-0'), 'utf8')).toContain('enter:observed') }) + +// Review of #1417 (b, c): a writer evicted while its timer drain is mid-append +// used to be "flushed" by a drain that returned at once, so flushAll resolved +// before that append landed. A re-created writer for the same paste could +// also append first, reversing the file's order. +it('drains an evicted writer whose append is already in flight, and keeps the file in event order', async () => { + userData.dir = await mkdtemp(join(tmpdir(), 'ac-paste-user-')) + dirs.push(userData.dir) + const { appendFile: realAppend } = await import('node:fs/promises') + let release: () => void = () => {} + const gate = new Promise(resolve => { release = resolve }) + let firstHeld = false + const appendFile = (async (...args: Parameters) => { + if (!firstHeld && String(args[0]).endsWith('paste-0.paste.jsonl')) { + firstHeld = true + await gate + } + return realAppend(...args) + }) as typeof realAppend + const registry = new PasteDebugJournalRegistry({ appendFile }) + registry.get('paste-0').append({ layer: 'RENDER', event: 'first' }) + await vi.waitFor(() => expect(firstHeld).toBe(true)) + + for (let i = 1; i <= 64; i++) registry.get(`paste-${i}`) + registry.get('paste-0').append({ layer: 'RENDER', event: 'second' }) + let shutdownDone = false + const shutdown = registry.flushAll().then(() => { shutdownDone = true }) + await new Promise(resolve => setTimeout(resolve, 50)) + expect(shutdownDone).toBe(false) + + release() + await shutdown + const lines = (await readFile(pasteDebugLogPath('paste-0'), 'utf8')).trim().split('\n').map(line => JSON.parse(line).event) + expect(lines).toEqual(['first', 'second']) +}) diff --git a/src/main/pasteDebugJournal.ts b/src/main/pasteDebugJournal.ts index 5af2bc340..f4978cfed 100644 --- a/src/main/pasteDebugJournal.ts +++ b/src/main/pasteDebugJournal.ts @@ -54,10 +54,25 @@ export class PasteDebugJournal { private queue: string[] = [] private timer: NodeJS.Timeout | null = null private ensuredDir = false - private draining = false + // The append currently writing, if any (review of #1417, b and c). The old + // `draining` boolean made a second drain return at once, so flush() on a + // writer whose timer drain was mid-append resolved before that append + // landed: an evicted writer then escaped the shutdown drain and a quit could + // lose its events. flush() now joins this promise, then writes the rest. + private inFlight: Promise | null = null private sessionStartedAtMs: number | null = null - constructor(private readonly filePath: string) {} + constructor( + private readonly filePath: string, + private readonly options: { + appendFile?: typeof appendFile + // A previous writer for the same file whose final flush is still + // landing (an evicted paste that is written to again). This writer's + // first append waits for it, so the file keeps event order: the reader + // takes a session's start from its first line. + after?: Promise + } = {}, + ) {} append(input: PasteDebugEventInput): void { const now = Date.now() @@ -76,7 +91,16 @@ export class PasteDebugJournal { clearTimeout(this.timer) this.timer = null } - await this.drain() + for (;;) { + if (this.inFlight) { + // Another drain's failure is reported where it started; here we only + // need it settled before writing what follows it. + await this.inFlight.catch(() => {}) + continue + } + if (this.queue.length === 0) return + await this.drain() + } } private scheduleDrain(): void { @@ -87,27 +111,28 @@ export class PasteDebugJournal { }, FLUSH_INTERVAL_MS) } - private async drain(): Promise { - if (this.draining) return - if (this.queue.length === 0) return - this.draining = true - try { - const batch = this.queue.splice(0).join('') - await this.appendRaw(batch) - } finally { - this.draining = false - } - if (this.queue.length > 0 && !this.timer) this.scheduleDrain() + private drain(): Promise { + if (this.inFlight) return this.inFlight + if (this.queue.length === 0) return Promise.resolve() + const batch = this.queue.splice(0).join('') + const writing = this.appendRaw(batch).finally(() => { + this.inFlight = null + if (this.queue.length > 0 && !this.timer) this.scheduleDrain() + }) + this.inFlight = writing + return writing } private async appendRaw(content: string): Promise { + if (this.options.after) await this.options.after.catch(() => {}) + const append = this.options.appendFile ?? appendFile try { - await appendFile(this.filePath, content, { mode: 0o600 }) + await append(this.filePath, content, { mode: 0o600 }) } catch { if (!this.ensuredDir) { await mkdir(dirname(this.filePath), { recursive: true, mode: 0o700 }) this.ensuredDir = true - await appendFile(this.filePath, content, { mode: 0o600 }) + await append(this.filePath, content, { mode: 0o600 }) } else { throw new Error(`paste-debug append failed for ${this.filePath}`) } @@ -130,6 +155,10 @@ export class PasteDebugJournalRegistry { private journals = new Map() /** Flushes started by dispose(), until they settle; see flushAll. */ private readonly disposing = new Set>() + /** The same flushes by paste id, so a re-created writer appends after them. */ + private readonly disposingById = new Map>() + + constructor(private readonly options: { appendFile?: typeof appendFile } = {}) {} get size(): number { return this.journals.size @@ -138,7 +167,7 @@ export class PasteDebugJournalRegistry { get(pasteId: string): PasteDebugJournal { let j = this.journals.get(pasteId) if (!j) { - j = new PasteDebugJournal(pasteDebugLogPath(pasteId)) + j = new PasteDebugJournal(pasteDebugLogPath(pasteId), { ...this.options, after: this.disposingById.get(pasteId) }) this.journals.set(pasteId, j) // Insertion order is age: evict the oldest paste (flushing it first), // never the one just asked for. @@ -169,7 +198,11 @@ export class PasteDebugJournalRegistry { console.warn('[pasteDebugJournal] dispose flush error:', err) }) this.disposing.add(flushing) - void flushing.finally(() => this.disposing.delete(flushing)) + this.disposingById.set(pasteId, flushing) + void flushing.finally(() => { + this.disposing.delete(flushing) + if (this.disposingById.get(pasteId) === flushing) this.disposingById.delete(pasteId) + }) this.journals.delete(pasteId) } } From 3635167a2ee3957be4154bdcf0118f2ad6fe491f Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:48:49 -0700 Subject: [PATCH 11/21] fix(window): bound tombstones by age (10 min), not count, so a mass close keeps queued saves (#1417 review b) Co-Authored-By: Claude Opus 5.5 --- src/main/window/windowRegistry.test.ts | 32 +++++++++++++++-------- src/main/window/windowRegistry.ts | 35 +++++++++++++++----------- 2 files changed, 42 insertions(+), 25 deletions(-) diff --git a/src/main/window/windowRegistry.test.ts b/src/main/window/windowRegistry.test.ts index 0d724da2e..79c631040 100644 --- a/src/main/window/windowRegistry.test.ts +++ b/src/main/window/windowRegistry.test.ts @@ -241,19 +241,31 @@ describe('window registry routing', () => { expect(registry.windowIdForWebContentsId(webContentsId)).toBe(window) }) - it('keeps a bounded tombstone: the newest closed windows still resolve, the oldest are forgotten (#1278)', () => { + it('keeps a tombstone while a late save can still arrive, however many windows close, and forgets it after ten minutes (#1278)', () => { // One tombstone per closed window was kept for the life of the process. - // Only a save dequeued moments after `closed` needs one, so the oldest - // can go once 256 newer windows have closed. - for (let i = 0; i < 300; i++) { + // A count cap (review of #1417, b) could drop a still-queued final save + // during a mass close, so the bound is age: a mass close keeps them all, + // and a later close sweeps the ones past ten minutes. + vi.useFakeTimers({ toFake: ['Date'] }) + try { + vi.setSystemTime(0) + for (let i = 0; i < 300; i++) { + registry.createAppWindow() + built[i]?.hooks.onClosed() + } + // The fake assigns webContents.id from creation order, starting at 1. + expect(registry.windowIdForWebContentsId(1)).not.toBeNull() + expect(registry.windowIdForWebContentsId(300)).not.toBeNull() + + vi.setSystemTime(10 * 60_000) registry.createAppWindow() - built[i]?.hooks.onClosed() + built[300]?.hooks.onClosed() + expect(registry.windowIdForWebContentsId(1)).toBeNull() + expect(registry.windowIdForWebContentsId(300)).toBeNull() + expect(registry.windowIdForWebContentsId(301)).not.toBeNull() + } finally { + vi.useRealTimers() } - // The fake assigns webContents.id from creation order, starting at 1. - expect(registry.windowIdForWebContentsId(300)).not.toBeNull() - expect(registry.windowIdForWebContentsId(300 - 255)).not.toBeNull() - expect(registry.windowIdForWebContentsId(300 - 256)).toBeNull() - expect(registry.windowIdForWebContentsId(1)).toBeNull() }) it('broadcasts app-wide state to every live window', () => { diff --git a/src/main/window/windowRegistry.ts b/src/main/window/windowRegistry.ts index 6d96b6648..42b06a521 100644 --- a/src/main/window/windowRegistry.ts +++ b/src/main/window/windowRegistry.ts @@ -113,16 +113,20 @@ function deliverSessionLease(lease: SessionWindowLease, channel: string, args: u * admitting exactly that save. A sender that was NEVER registered is still * rejected; this only remembers senders that were. * - * Bounded by insertion order at RETIRED_WEB_CONTENTS_LIMIT (#1278). The old - * comment said the map was "cleared with the registry", but only the test-only - * reset ever cleared it, so a long-running app kept one tombstone per window it - * ever closed. The late sender this exists for is a save dequeued moments after - * `closed`, so forgetting a window only after 256 newer ones have closed can - * never drop it. webContents ids are never reused within a process, so a - * forgotten id cannot be confused with a live window's. + * Bounded by AGE, not count (#1278). The old comment said the map was + * "cleared with the registry", but only the test-only reset ever cleared it, + * so a long-running app kept one tombstone per window it ever closed. The + * first fix capped it at 256 by insertion order, and review of #1417 (b) + * showed that can still drop the save it exists for: a queued final save from + * a window, followed by 256 more closes before main handles that IPC (a mass + * close, a stalled main thread). Age is what makes a tombstone useless: the + * late sender is a save dequeued moments after `closed`, so one older than + * RETIRED_WEB_CONTENTS_TTL_MS is dropped whenever another window closes. + * webContents ids are never reused within a process, so a forgotten id + * cannot be confused with a live window's. */ -const RETIRED_WEB_CONTENTS_LIMIT = 256 -const retiredWebContentsIds = new Map() +const RETIRED_WEB_CONTENTS_TTL_MS = 10 * 60_000 +const retiredWebContentsIds = new Map() /** * Notified (debounced) when a window is moved, resized, or full-screened, so @@ -429,12 +433,13 @@ export function createAppWindow(options?: { onClosed: () => { const closing = windows.get(id) if (closing && !closing.window.isDestroyed()) { - retiredWebContentsIds.set(closing.window.webContents.id, id) - while (retiredWebContentsIds.size > RETIRED_WEB_CONTENTS_LIMIT) { - const oldest = retiredWebContentsIds.keys().next().value - if (oldest === undefined) break - retiredWebContentsIds.delete(oldest) + const now = Date.now() + // Insertion order is close order, so the expired ones are a prefix. + for (const [webContentsId, tombstone] of retiredWebContentsIds) { + if (now - tombstone.closedAt < RETIRED_WEB_CONTENTS_TTL_MS) break + retiredWebContentsIds.delete(webContentsId) } + retiredWebContentsIds.set(closing.window.webContents.id, { windowId: id, closedAt: now }) } windows.delete(id) const index = focusOrder.indexOf(id) @@ -499,7 +504,7 @@ export function windowIdForWebContentsId(webContentsId: number): WindowId | null if (entry.window.isDestroyed()) continue if (entry.window.webContents.id === webContentsId) return entry.id } - return retiredWebContentsIds.get(webContentsId) ?? null + return retiredWebContentsIds.get(webContentsId)?.windowId ?? null } /** The focused window, else the most recently focused one that still exists. */ From 5734da87fff81aedbd8187ee4ef71130bfd18342 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:48:49 -0700 Subject: [PATCH 12/21] fix(conversations): sweep Claude summaries per listed directory; LRU-bound Codex rolloutPaths (#1417 review c) Co-Authored-By: Claude Opus 5.5 --- .../sources/claude.system.test.ts | 21 ++++++++++++++++++- src/main/conversations/sources/claude.ts | 13 +++++++++++- src/main/conversations/sources/codex.ts | 9 ++++++-- 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/src/main/conversations/sources/claude.system.test.ts b/src/main/conversations/sources/claude.system.test.ts index ac90f054b..5ad5d362b 100644 --- a/src/main/conversations/sources/claude.system.test.ts +++ b/src/main/conversations/sources/claude.system.test.ts @@ -1,4 +1,4 @@ -import { mkdir, readdir, writeFile } from 'node:fs/promises' +import { mkdir, readdir, rm, writeFile } from 'node:fs/promises' import { join } from 'node:path' import { afterAll, beforeAll, describe, expect, it } from 'vitest' @@ -107,4 +107,23 @@ describe('Claude conversation source', () => { expect(prompts[i - 1]!.timestamp ?? 0).toBeGreaterThanOrEqual(prompts[i]!.timestamp ?? 0) } }) + + // Review of #1417 (c): `summaries` is keyed by file and was never pruned, so + // a transcript deleted from a directory that discovery keeps listing kept + // its parsed head and user texts for the life of the process. Its own corpus: + // this test deletes a file, and the shared one is read-only by convention. + it('forgets the summary of a transcript gone from a directory it listed (#1278)', async () => { + const own = await setup() + const summaries = (own.source as unknown as { summaries: Map }).summaries + const family = await resolveFamily('/fixture/repo', 'repository', { listWorktrees: own.listWorktrees }) + const first = await own.source.discover({ scope: 'repository', family }) + const gone = first.find(row => row.file && summaries.has(row.file))!.file! + const unrelated = [...summaries.keys()].length + + await rm(gone) + const second = await own.source.discover({ scope: 'repository', family }) + expect(second.map(row => row.file)).not.toContain(gone) + expect(summaries.has(gone)).toBe(false) + expect(summaries.size).toBe(unrelated - 1) + }) }) diff --git a/src/main/conversations/sources/claude.ts b/src/main/conversations/sources/claude.ts index e48be53d2..be77faae0 100644 --- a/src/main/conversations/sources/claude.ts +++ b/src/main/conversations/sources/claude.ts @@ -1,5 +1,5 @@ import { open, readdir, stat } from 'node:fs/promises' -import { isAbsolute, join } from 'node:path' +import { dirname, isAbsolute, join } from 'node:path' import type { ConversationPrompt } from '@shared/conversations/types.js' import { asRecord, parseJsonRecord } from '@shared/lib/asRecord.js' @@ -279,6 +279,7 @@ export class ClaudeConversationSource implements ConversationSource { // the redirect stub left where it started, or a plain copy. The largest // file is the conversation; a stub is a few hundred bytes. const candidates: Array<{ file: string; nativeId: string; exact: boolean }> = [] + const enumerated = new Set() for (const { dir, exact } of dirs) { let names: string[] try { @@ -286,6 +287,7 @@ export class ClaudeConversationSource implements ConversationSource { } catch { continue } + enumerated.add(join(this.deps.projectsDir, dir)) for (const name of names) { if (!name.endsWith('.jsonl')) continue const nativeId = name.slice(0, -6) @@ -293,6 +295,15 @@ export class ClaudeConversationSource implements ConversationSource { candidates.push({ file: join(this.deps.projectsDir, dir, name), nativeId, exact }) } } + // Forget summaries of transcripts gone from a directory this discovery + // just listed (review of #1417, c). `summaries` is keyed by file and was + // never pruned. A scoped discovery cannot judge directories it did not + // walk, but for every directory it did list, the listing is the exact set + // of files a summary can still belong to. Memory only; nothing on disk. + const listed = new Set(candidates.map(candidate => candidate.file)) + for (const file of this.summaries.keys()) { + if (enumerated.has(dirname(file)) && !listed.has(file)) this.summaries.delete(file) + } const summarized = await mapWithConcurrency(candidates, SUMMARY_CONCURRENCY, async candidate => { try { const s = await stat(candidate.file) diff --git a/src/main/conversations/sources/codex.ts b/src/main/conversations/sources/codex.ts index 50e7b530e..1869b5648 100644 --- a/src/main/conversations/sources/codex.ts +++ b/src/main/conversations/sources/codex.ts @@ -10,6 +10,7 @@ import { extractPromptsFromFile } from '@main/conversations/prompts/promptFolder import { findCodexRolloutPathByThreadId } from 'codex-headless' import { newestCodexStateDb, openReadOnlySqlite } from './sqlite.js' import type { ConversationSource, SourceConversation, SourceScope, PromptReadOptions } from './types.js' +import { LruMap } from '@shared/lib/lruMap.js' // Codex keeps its own index at ~/.codex/state_N.sqlite (`threads`, // `thread_spawn_edges`), maintained by the CLI and backfilled from rollouts. @@ -121,8 +122,12 @@ export class CodexConversationSource implements ConversationSource { private walk: { at: number; files: Map } | null = null private readonly heads = new Map() // Rollout paths learnt at discovery, so a search that reads prompts for a - // hundred and fifty rows does not open the index once per row. - private readonly rolloutPaths = new Map() + // hundred and fifty rows does not open the index once per row. LRU-bounded + // (review of #1417, c): archived or removed threads stayed here forever. A + // miss only costs the existing SQLite/fallback lookup in prompts(), never a + // wrong path, and 4096 is twice the recorded store's 2,023 indexed threads, + // far above one search's 150 rows. + private readonly rolloutPaths = new LruMap(4096) constructor(private readonly deps: { codexHome: string; walkTtlMs?: number }) {} From 620914937ab0eb4e24e377c3efb4e175efb6d952 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 05:48:49 -0700 Subject: [PATCH 13/21] docs(plans): #1417 round-1 review disposition Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-27-c6-small-unbounded.md | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/docs/plans/2026-09-27-c6-small-unbounded.md b/docs/plans/2026-09-27-c6-small-unbounded.md index d976e9615..e94452bfe 100644 --- a/docs/plans/2026-09-27-c6-small-unbounded.md +++ b/docs/plans/2026-09-27-c6-small-unbounded.md @@ -10,7 +10,7 @@ Source: the Stage 3 C6 hunt (`temp/quality-loop/hunt-c6.md`, rows 7–14 and par | 2 | Empty proxy parent dirs | **A bug, not a missing feature.** `removeEmptyParents` called `rm(dir, { recursive: false })`, which always throws EISDIR (confirmed on Node 24.14.1), so no parent was ever removed. The author's machine has 2,978 empty dirs, walked on every prune. | `rmdir`, which removes only an empty dir and does it atomically: a concurrent new run makes it fail with ENOTEMPTY. `root + sep` keeps sibling roots out of scope. | Real-filesystem test. Red on main; the old `rm` as a mutant goes red. | | 3a | Legacy `saved-debug-bundles.jsonl` re-parsed on every prune | Real (18.4 MB, every 5 min). Nothing appends to it any more. | Cache the parsed set by file identity: inode + ctime + mtime + size, so a rename-replace or a same-size edit with the mtime set back still re-parses. **Steering q109:** a failed read is `'unknown'`, never cached, and protects every legacy bundle for that prune; only ENOENT means "no ledger". This also closes main's one-prune window, where a transient read failure classified hand-saved bundles as deletable. | Parse-count test; a real temp ledger that is unreadable and then readable; a failure with unchanged identity that must not be cached; a same-size edit with the mtime set back. All three q109 tests are red at `d5101fa5`. Mutants red: caching `'unknown'`, an mtime+size key, `'unknown'` → legacy. | | 3a′ | #1251 row 13 (moved here from #1411): one malformed legacy-ledger row stopped all pruning | Real: a JSON-valid non-entry (`null`, a non-string `bundlePath`) threw, which rejected `collectArtifacts`. | Rows are shape-checked (`parseManualLegacyBundlePaths`); a non-string reason counts as manual (kept). It ships here because row 13 and steering q109 change the same loader. | A parser test plus a test through the real loader and cache (the #1411 review's surviving 'loader returns empty' mutant is now red). | -| 3b | `autosaved-debug-bundles.jsonl` append-only | Real, but small per line. Its append-only design as an operator index is a stated choice. | **Residual.** Trimming a ledger is a deletion policy; left for the owner. | none | +| 3b | `autosaved-debug-bundles.jsonl` append-only | **Mostly moot** (corrected by review of #1417, c): normal autosaves return before `appendDebugBundleSaved` (`debugBundle.ts:226-232`), and this machine has no autosave ledger at all. | **Residual.** Nothing to trim in practice, and trimming a ledger would be a deletion policy. | none | | 4 | WorkflowBridge `runsBySession` / `latestLifecycleByRunId` | Grows with the durable workflow-mcp store (`start()` reloads every stored run). | **Residual:** bounded by the store once #1275's retention (90 days, resumable never deleted) lands. Pruning the maps alone bounds nothing. | none | | 5 | `sessionManager` `lastActivityAt` | Intentional; WHY at `sessionManager.ts:1095-1099` (telemetry asks about exited panes). One number per session id. | None. | none | | 6 | Conversation caches | Grow with the transcript store plus deleted transcripts, not over time. | **Codex `heads`:** entries for files the (scope-independent) walk no longer finds are swept; this is exact. **Search `promptCache`:** LRU 1024, the same bound as the prompt folder's, via a new shared `LruMap` that now also backs `promptFolder.ts` (its private LRU functions are gone). **Claude `summaries`, codex `rolloutPaths`:** residual; a scoped discovery cannot tell a deleted transcript from an unwalked one, and a count cap below the store size would re-read the store on every full discovery. | Codex heads sweep test (mutant red); `lruMap.test.ts`; `promptFolder` suite 13/13 unchanged. | @@ -21,3 +21,17 @@ Source: the Stage 3 C6 hunt (`temp/quality-loop/hunt-c6.md`, rows 7–14 and par ## Overlap with #1411 (manager q109) Row 13 of #1251 moved here, so `debugRetention.ts` and its test now change only in this PR. #1411 still shares `codex.ts` (different hunks: #1411 guards `fromHead`, this PR sweeps `heads` in `walkRollouts`) and `codex.system.test.ts`. Both insert a test before the same anchor, so the second PR to merge resolves a trivial conflict there. + +## Review round 1 (a, b, c codex at `6150238a`): all FIX-BEFORE-MERGE + +| Finding | Verdict | Change | +|---|---|---| +| **a, major:** the ledger renamed away between the identity `stat` and the read. The loader's ENOENT "no ledger" was cached under the identity of the file that WAS there, so a manual bundle became deletable (this machine's ledger holds 14 manual saves). | valid | A second `stat` after the load. The parse is trusted only if the identity is unchanged; otherwise `'unknown'` (protective, not cached). Fail-first for "still away" and "renamed back". | +| **a survivor:** `startsWith(root)` without `sep` | valid | A sibling `proxy-old/` fixture; the mutant is red. | +| **a survivor:** `ino` dropped from the identity | declined | A portable test cannot change only the inode: a rename-replace also moves ctime. `ino` stays as defence in depth. | +| **b + c, major:** an evicted paste writer with an append IN FLIGHT escaped the shutdown drain (`draining` made the second drain return at once), and a re-created writer could append first. | valid | `flush()` joins the in-flight append, then writes the rest. A re-created writer's first append waits for the evicted writer's flush (the reader takes a session's start from the first line). A gated-append test; both mutants (no join, no ordering) red. | +| **b, major:** the 256 count cap on tombstones could drop a queued final save during a mass close | valid | The bound is now age: a tombstone older than 10 minutes is swept when another window closes. A mass close keeps them all. Red under the count cap. | +| **c, major:** scoped Claude discovery kept summaries of files gone from directories it had just listed | valid | Each discovery sweeps summaries under the directories it enumerated that are not in the listing; unwalked directories are untouched. Own-corpus test. | +| **c, minor:** Codex `rolloutPaths` unbounded | valid | `LruMap(4096)`, twice the recorded 2,023 indexed threads. A miss only costs the existing SQLite lookup. | +| **c:** plan 3b stale | valid | Corrected above. | +| **b survivors:** promptFolder's test hook using `get` instead of `peek`; a search-cache cap of 1 | declined | Neither is a behaviour of the app. The hook is test-only inspection, and the cap is a performance contract no test times; both are shared `LruMap` semantics, pinned in `lruMap.test.ts`. | From c79cc0c77602e163892e51c22718e7d9ae1a5772 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 07:19:23 -0700 Subject: [PATCH 14/21] fix(storage): an absent legacy ledger is unknown and protects every legacy bundle (#1417 review round 2) Co-Authored-By: Claude Opus 5.5 --- src/main/storage/debugRetention.test.ts | 16 +++++++++++++++ src/main/storage/debugRetention.ts | 27 +++++++++++++++++-------- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/src/main/storage/debugRetention.test.ts b/src/main/storage/debugRetention.test.ts index a42c1e40a..b39c68c6c 100644 --- a/src/main/storage/debugRetention.test.ts +++ b/src/main/storage/debugRetention.test.ts @@ -338,6 +338,22 @@ describe('legacy ledger classification fails closed (steering q109)', () => { }) } + // Review of #1417, round 2 (a): a ledger moved aside BEFORE the prune and + // back after it. Both stats see ENOENT, so no identity check can notice; + // an absent ledger must itself protect every legacy bundle. + it('protects every legacy bundle while the ledger is absent, and classifies again once it is back', async () => { + const ledger = join(root, 'saved-debug-bundles.jsonl') + const manualBundle = join(root, '2026-01-01T00-00-00') + writeFileSync(`${ledger}.away`, manualRow(manualBundle)) + const absent = await cachedManualLegacyBundlePaths(ledger) + expect(absent).toBe('unknown') + expect(legacyDebugBundleBucketForPath(manualBundle, absent)).toBe('debug-bundles-manual') + renameSync(`${ledger}.away`, ledger) + const back = await cachedManualLegacyBundlePaths(ledger) + expect(legacyDebugBundleBucketForPath(manualBundle, back)).toBe('debug-bundles-manual') + expect(legacyDebugBundleBucketForPath(join(root, '2026-01-02T00-00-00'), back)).toBe('debug-bundles-legacy') + }) + // An operator edit with the same size that also restores the old mtime. it('re-parses a same-size edit whose mtime was set back', async () => { const ledger = join(root, 'saved-debug-bundles.jsonl') diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 09d49a57b..9b90a3743 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -456,14 +456,23 @@ function bucketCaps(totalBudget: number): Record { /** * Which legacy root-level bundles were saved by hand, or 'unknown' when the - * ledger exists but could not be read (steering q109). + * ledger cannot be read OR is absent (steering q109; review of #1417 round 2). * * WHY 'unknown' instead of an empty set: the manual/legacy split is what keeps * a hand-saved bundle out of the deletable `debug-bundles-legacy` bucket. An * empty set on a transient read failure (EACCES, EMFILE) classified every * manual bundle as deletable for that prune; with the cache below it would * have stayed that way until the file changed. 'unknown' makes every legacy - * bundle protected for that prune instead. Only ENOENT is a real "no ledger". + * bundle protected for that prune instead. + * + * WHY an ABSENT ledger is 'unknown' too (review of #1417, round 2, a): a + * ledger moved aside before a prune and back after it is indistinguishable, + * from inside the prune, from one that never existed, and treating absence as + * "no manual bundles" made every hand-saved legacy bundle deletable for that + * prune. The ledger is the only record of which root-level bundles were + * manual, so without it none can be proven disposable. Stated cost: with no + * ledger at all, pre-split legacy bundles are never aged out; that is the + * owner's "do not delete stuff often" applied to evidence we cannot classify. */ export type ManualLegacyBundlePaths = Set | 'unknown' @@ -509,8 +518,10 @@ async function ledgerIdentity(file: string): Promise { try { const info = await stat(file) return `${info.ino}:${info.ctimeMs}:${info.mtimeMs}:${info.size}` - } catch (error) { - return (error as NodeJS.ErrnoException).code === 'ENOENT' ? 'missing' : 'unknown' + } catch { + // ENOENT included: an absent ledger classifies nothing (see + // ManualLegacyBundlePaths), so it is never cached as an answer. + return 'unknown' } } @@ -690,10 +701,10 @@ async function loadManualLegacyBundlePaths(file: string = DEBUG_BUNDLE_LOG_FILE) let raw: string try { raw = await readFile(file, 'utf8') - } catch (error) { - // Only a missing ledger means "no manual bundles"; any other failure is - // unknown and fails closed (steering q109). - return (error as NodeJS.ErrnoException).code === 'ENOENT' ? new Set() : 'unknown' + } catch { + // Every failure, including ENOENT, is unknown and fails closed (steering + // q109, review of #1417 round 2); see ManualLegacyBundlePaths. + return 'unknown' } return parseManualLegacyBundlePaths(raw) } From 349e72b247f748d45021e2244d489b0868f225e0 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 07:19:23 -0700 Subject: [PATCH 15/21] fix(conversations): sweep summaries of Claude project directories that are gone (#1417 review round 2) Co-Authored-By: Claude Opus 5.5 --- .../sources/claude.system.test.ts | 21 ++++++++++++++- src/main/conversations/sources/claude.ts | 27 ++++++++++++++++--- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/main/conversations/sources/claude.system.test.ts b/src/main/conversations/sources/claude.system.test.ts index 5ad5d362b..8fd8b03e0 100644 --- a/src/main/conversations/sources/claude.system.test.ts +++ b/src/main/conversations/sources/claude.system.test.ts @@ -1,5 +1,5 @@ import { mkdir, readdir, rm, writeFile } from 'node:fs/promises' -import { join } from 'node:path' +import { dirname, join } from 'node:path' import { afterAll, beforeAll, describe, expect, it } from 'vitest' import { resolveFamily } from '../family.js' @@ -126,4 +126,23 @@ describe('Claude conversation source', () => { expect(summaries.has(gone)).toBe(false) expect(summaries.size).toBe(unrelated - 1) }) + + // Review of #1417, round 2 (a): a whole project directory removed (a pruned + // worktree's transcripts) never reached the per-directory sweep. + it('forgets the summaries of a project directory that is gone (#1278)', async () => { + const own = await setup() + const summaries = (own.source as unknown as { summaries: Map }).summaries + const family = await resolveFamily('/fixture/repo', 'repository', { listWorktrees: own.listWorktrees }) + await own.source.discover({ scope: 'repository', family }) + const counts = new Map() + for (const file of summaries.keys()) counts.set(dirname(file), (counts.get(dirname(file)) ?? 0) + 1) + const [goneDir, goneCount] = [...counts.entries()].sort((a, b) => b[1] - a[1])[0]! + expect(counts.size).toBeGreaterThan(1) + + await rm(goneDir, { recursive: true }) + const before = summaries.size + await own.source.discover({ scope: 'repository', family }) + expect([...summaries.keys()].some(file => dirname(file) === goneDir)).toBe(false) + expect(summaries.size).toBe(before - goneCount) + }) }) diff --git a/src/main/conversations/sources/claude.ts b/src/main/conversations/sources/claude.ts index be77faae0..76f12c368 100644 --- a/src/main/conversations/sources/claude.ts +++ b/src/main/conversations/sources/claude.ts @@ -1,5 +1,5 @@ import { open, readdir, stat } from 'node:fs/promises' -import { dirname, isAbsolute, join } from 'node:path' +import { basename, dirname, isAbsolute, join } from 'node:path' import type { ConversationPrompt } from '@shared/conversations/types.js' import { asRecord, parseJsonRecord } from '@shared/lib/asRecord.js' @@ -219,13 +219,19 @@ export class ClaudeConversationSource implements ConversationSource { constructor(private readonly deps: { projectsDir: string; history: ClaudeHistoryIndex }) {} + // Every project directory the last root listing saw, or null when that + // listing failed (unknown); see the summaries sweep in discover(). + private projectNames: Set | null = null + private async candidateDirs(scope: SourceScope): Promise> { let names: string[] try { names = (await readdir(this.deps.projectsDir, { withFileTypes: true })).filter(d => d.isDirectory()).map(d => d.name) } catch { + this.projectNames = null return [] } + this.projectNames = new Set(names) if (scope.scope === 'everywhere') return names.map(dir => ({ dir, exact: false })) // Folded like the family folds cwds: darwin and win32 name a directory // for a lowercased cwd that the canonical cwd would not match otherwise. @@ -280,11 +286,15 @@ export class ClaudeConversationSource implements ConversationSource { // file is the conversation; a stub is a few hundred bytes. const candidates: Array<{ file: string; nativeId: string; exact: boolean }> = [] const enumerated = new Set() + const vanished = new Set() for (const { dir, exact } of dirs) { let names: string[] try { names = await readdir(join(this.deps.projectsDir, dir)) - } catch { + } catch (error) { + // Gone between the root listing and this one: as good as unlisted. + // Any other failure is unknown, and its summaries are kept. + if ((error as NodeJS.ErrnoException).code === 'ENOENT') vanished.add(join(this.deps.projectsDir, dir)) continue } enumerated.add(join(this.deps.projectsDir, dir)) @@ -300,9 +310,20 @@ export class ClaudeConversationSource implements ConversationSource { // never pruned. A scoped discovery cannot judge directories it did not // walk, but for every directory it did list, the listing is the exact set // of files a summary can still belong to. Memory only; nothing on disk. + // + // A whole project directory can disappear too (review of #1417, round 2, + // a): it then never reaches `enumerated`, so its summaries stayed forever. + // The root listing (projectNames) names every project directory whatever + // the scope, so a summary whose directory is not in it, or vanished while + // being listed, belongs to nothing. A failed root listing is unknown and + // sweeps nothing on that account. const listed = new Set(candidates.map(candidate => candidate.file)) + const projectNames = this.projectNames for (const file of this.summaries.keys()) { - if (enumerated.has(dirname(file)) && !listed.has(file)) this.summaries.delete(file) + const projectDir = dirname(file) + const dirGone = vanished.has(projectDir) + || (projectNames !== null && dirname(projectDir) === this.deps.projectsDir && !projectNames.has(basename(projectDir))) + if (dirGone || (enumerated.has(projectDir) && !listed.has(file))) this.summaries.delete(file) } const summarized = await mapWithConcurrency(candidates, SUMMARY_CONCURRENCY, async candidate => { try { From 67cdaba3f6140285fb74f1b3dd93183b8eeb55f2 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 07:19:23 -0700 Subject: [PATCH 16/21] fix(window): age tombstones on a monotonic clock (#1417 review round 2) Co-Authored-By: Claude Opus 5.5 --- src/main/window/windowRegistry.test.ts | 25 ++++++++++++++++++++++--- src/main/window/windowRegistry.ts | 11 +++++++++-- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/main/window/windowRegistry.test.ts b/src/main/window/windowRegistry.test.ts index 79c631040..ceac6644c 100644 --- a/src/main/window/windowRegistry.test.ts +++ b/src/main/window/windowRegistry.test.ts @@ -246,9 +246,8 @@ describe('window registry routing', () => { // A count cap (review of #1417, b) could drop a still-queued final save // during a mass close, so the bound is age: a mass close keeps them all, // and a later close sweeps the ones past ten minutes. - vi.useFakeTimers({ toFake: ['Date'] }) + vi.useFakeTimers({ toFake: ['performance'] }) try { - vi.setSystemTime(0) for (let i = 0; i < 300; i++) { registry.createAppWindow() built[i]?.hooks.onClosed() @@ -257,7 +256,7 @@ describe('window registry routing', () => { expect(registry.windowIdForWebContentsId(1)).not.toBeNull() expect(registry.windowIdForWebContentsId(300)).not.toBeNull() - vi.setSystemTime(10 * 60_000) + vi.advanceTimersByTime(10 * 60_000) registry.createAppWindow() built[300]?.hooks.onClosed() expect(registry.windowIdForWebContentsId(1)).toBeNull() @@ -268,6 +267,26 @@ describe('window registry routing', () => { } }) + it('ages tombstones by elapsed time, not the wall clock (#1278)', () => { + // Review of #1417, round 2 (b): the wall clock stepped back and then + // corrected made a second-old tombstone look twenty minutes old, so a + // queued final save from that window was refused. + vi.useFakeTimers({ toFake: ['Date', 'performance'] }) + try { + vi.setSystemTime(20 * 60_000) + vi.setSystemTime(0) + registry.createAppWindow() + built[0]?.hooks.onClosed() + vi.advanceTimersByTime(1_000) + vi.setSystemTime(20 * 60_000 + 1_000) + registry.createAppWindow() + built[1]?.hooks.onClosed() + expect(registry.windowIdForWebContentsId(1)).not.toBeNull() + } finally { + vi.useRealTimers() + } + }) + it('broadcasts app-wide state to every live window', () => { registry.createAppWindow() registry.createAppWindow() diff --git a/src/main/window/windowRegistry.ts b/src/main/window/windowRegistry.ts index 42b06a521..e55aeceb7 100644 --- a/src/main/window/windowRegistry.ts +++ b/src/main/window/windowRegistry.ts @@ -124,6 +124,12 @@ function deliverSessionLease(lease: SessionWindowLease, channel: string, args: u * RETIRED_WEB_CONTENTS_TTL_MS is dropped whenever another window closes. * webContents ids are never reused within a process, so a forgotten id * cannot be confused with a live window's. + * + * WHY a monotonic clock (review of #1417, round 2): with Date.now(), a wall + * clock stepped back and then corrected made a one-second-old tombstone look + * twenty minutes old, dropping a queued final save, and a future-dated + * tombstone at the front stopped the sweep from reaching expired ones behind + * it. performance.now() only moves forward, so insertion order is age order. */ const RETIRED_WEB_CONTENTS_TTL_MS = 10 * 60_000 const retiredWebContentsIds = new Map() @@ -433,8 +439,9 @@ export function createAppWindow(options?: { onClosed: () => { const closing = windows.get(id) if (closing && !closing.window.isDestroyed()) { - const now = Date.now() - // Insertion order is close order, so the expired ones are a prefix. + const now = performance.now() + // Insertion order is close order and the clock is monotonic, so the + // expired ones are a prefix. for (const [webContentsId, tombstone] of retiredWebContentsIds) { if (now - tombstone.closedAt < RETIRED_WEB_CONTENTS_TTL_MS) break retiredWebContentsIds.delete(webContentsId) From d003f0e474f9a665be61bb523073c6ef66331d26 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 07:19:23 -0700 Subject: [PATCH 17/21] fix(paste-debug): a failed append keeps its batch (bounded), never rejects unhandled, never flushes as success (#1417 review round 2) Co-Authored-By: Claude Opus 5.5 --- src/main/pasteDebugJournal.test.ts | 70 ++++++++++++++++++++++++++++++ src/main/pasteDebugJournal.ts | 60 ++++++++++++++++++++++--- 2 files changed, 124 insertions(+), 6 deletions(-) diff --git a/src/main/pasteDebugJournal.test.ts b/src/main/pasteDebugJournal.test.ts index 7339e3faa..f081cf9ba 100644 --- a/src/main/pasteDebugJournal.test.ts +++ b/src/main/pasteDebugJournal.test.ts @@ -60,3 +60,73 @@ it('drains an evicted writer whose append is already in flight, and keeps the fi const lines = (await readFile(pasteDebugLogPath('paste-0'), 'utf8')).trim().split('\n').map(line => JSON.parse(line).event) expect(lines).toEqual(['first', 'second']) }) + +// Review of #1417, round 2 (a, b, c): a failed append dropped its batch for +// good, the timer's failure was an unhandled rejection, and a flush that +// joined the failed write resolved as if it had landed. +it('keeps a failed batch, retries it in order, and never resolves a flush over a lost write', async () => { + userData.dir = await mkdtemp(join(tmpdir(), 'ac-paste-user-')) + dirs.push(userData.dir) + const { appendFile: realAppend } = await import('node:fs/promises') + let failures = 2 + const appendFile = (async (...args: Parameters) => { + if (failures > 0) { + failures-- + throw Object.assign(new Error('injected EIO'), { code: 'EIO' }) + } + return realAppend(...args) + }) as typeof realAppend + const unhandled: unknown[] = [] + const onUnhandled = (reason: unknown) => { unhandled.push(reason) } + process.on('unhandledRejection', onUnhandled) + try { + const registry = new PasteDebugJournalRegistry({ appendFile }) + const journal = registry.get('paste-retry') + journal.append({ layer: 'RENDER', event: 'first' }) + // The timer's drain fails twice (the append and its mkdir retry). + await vi.waitFor(() => expect(failures).toBe(0)) + await new Promise(resolve => setTimeout(resolve, 20)) + journal.append({ layer: 'RENDER', event: 'second' }) + await registry.flushAll() + + const events = (await readFile(pasteDebugLogPath('paste-retry'), 'utf8')).trim().split('\n').map(line => JSON.parse(line).event) + expect(events).toEqual(['first', 'second']) + expect(unhandled).toEqual([]) + } finally { + process.off('unhandledRejection', onUnhandled) + } +}) + +it('a flush that cannot write rejects instead of reporting success', async () => { + userData.dir = await mkdtemp(join(tmpdir(), 'ac-paste-user-')) + dirs.push(userData.dir) + const appendFile = (async () => { throw Object.assign(new Error('injected EIO'), { code: 'EIO' }) }) as unknown as typeof import('node:fs/promises').appendFile + const journal = new PasteDebugJournalRegistry({ appendFile }).get('paste-dead-disk') + journal.append({ layer: 'RENDER', event: 'lost?' }) + // Both callers, including the one that joined the other's write (round 2, + // c: it used to settle as fulfilled). + const settled = await Promise.allSettled([journal.flush(), journal.flush()]) + expect(settled.map(result => result.status)).toEqual(['rejected', 'rejected']) +}) + +// The retry queue is itself bounded: a disk that keeps failing must not turn +// this writer into the unbounded growth #1278 is about. +it('holds at most 1000 lines while writes fail, and reports what it dropped once they work', async () => { + userData.dir = await mkdtemp(join(tmpdir(), 'ac-paste-user-')) + dirs.push(userData.dir) + const { appendFile: realAppend } = await import('node:fs/promises') + let broken = true + const appendFile = (async (...args: Parameters) => { + if (broken) throw Object.assign(new Error('injected EIO'), { code: 'EIO' }) + return realAppend(...args) + }) as typeof realAppend + const journal = new PasteDebugJournalRegistry({ appendFile }).get('paste-bounded') + for (let i = 0; i < 1500; i++) journal.append({ layer: 'RENDER', event: `e${i}` }) + await expect(journal.flush()).rejects.toThrow() + + broken = false + await journal.flush() + const events = (await readFile(pasteDebugLogPath('paste-bounded'), 'utf8')).trim().split('\n').map(line => JSON.parse(line)) + expect(events[0]).toMatchObject({ layer: 'ERROR', event: 'journal:dropped-lines', data: { lines: 500 } }) + expect(events.slice(1).map(event => event.event)).toEqual(Array.from({ length: 1000 }, (_, i) => `e${i + 500}`)) +}) diff --git a/src/main/pasteDebugJournal.ts b/src/main/pasteDebugJournal.ts index f4978cfed..36873b787 100644 --- a/src/main/pasteDebugJournal.ts +++ b/src/main/pasteDebugJournal.ts @@ -48,6 +48,10 @@ import type { const FLUSH_INTERVAL_MS = 100 +// Bound on lines held while appends keep failing (review of #1417, round 2). +// A paste logs tens of lines; a thousand is many pastes' worth of retries. +const MAX_QUEUED_LINES = 1000 + const PRUNE_AFTER_MS = 14 * 24 * 60 * 60 * 1000 export class PasteDebugJournal { @@ -60,6 +64,9 @@ export class PasteDebugJournal { // landed: an evicted writer then escaped the shutdown drain and a quit could // lose its events. flush() now joins this promise, then writes the rest. private inFlight: Promise | null = null + // Lines dropped because the queue hit MAX_QUEUED_LINES while appends kept + // failing; reported as one ERROR line once a write succeeds. + private dropped = 0 private sessionStartedAtMs: number | null = null constructor( @@ -93,8 +100,10 @@ export class PasteDebugJournal { } for (;;) { if (this.inFlight) { - // Another drain's failure is reported where it started; here we only - // need it settled before writing what follows it. + // A joined drain that failed put its batch back in the queue (see + // drain), so the next pass retries it and THIS flush reports the + // outcome of that retry. It never resolves over a lost batch (review + // of #1417, round 2, c). await this.inFlight.catch(() => {}) continue } @@ -107,22 +116,61 @@ export class PasteDebugJournal { if (this.timer) return this.timer = setTimeout(() => { this.timer = null - void this.drain() + // No caller awaits the timer: without this catch a failed append was an + // unhandled rejection in the main process (review of #1417, round 2, c). + // The batch is already back in the queue; the next append or flush + // retries it. + this.drain().catch(error => { console.warn('[pasteDebugJournal] append failed; will retry:', error) }) }, FLUSH_INTERVAL_MS) } private drain(): Promise { if (this.inFlight) return this.inFlight if (this.queue.length === 0) return Promise.resolve() - const batch = this.queue.splice(0).join('') - const writing = this.appendRaw(batch).finally(() => { + const lines = this.queue.splice(0) + const dropped = this.dropped + const batch = (dropped ? this.droppedLine(dropped) : '') + lines.join('') + const writing = this.appendRaw(batch).then( + () => { + this.dropped -= dropped + // Lines appended while this write was in flight: their timer found the + // write busy and joined it, so drain them now. Only after SUCCESS: after + // a failure the next append or flush retries, instead of a dead disk + // being hammered every 100 ms. + if (this.queue.length > 0 && !this.timer) this.scheduleDrain() + }, + (error: unknown) => { + // A failed append used to drop its batch for good (review of #1417, + // round 2, a, b, c). Put it back IN FRONT, so order holds and the next + // drain retries it, but bounded: a disk that keeps failing must not + // turn this into unbounded memory (#1278 is about exactly that). + this.queue.unshift(...lines) + const excess = this.queue.length - MAX_QUEUED_LINES + if (excess > 0) { + this.queue.splice(0, excess) + this.dropped += excess + } + throw error + }, + ).finally(() => { this.inFlight = null - if (this.queue.length > 0 && !this.timer) this.scheduleDrain() }) this.inFlight = writing return writing } + private droppedLine(lines: number): string { + const now = Date.now() + const event: PasteDebugEvent = { + ts: now, + tMs: this.sessionStartedAtMs === null ? 0 : now - this.sessionStartedAtMs, + layer: 'ERROR', + event: 'journal:dropped-lines', + data: { lines }, + } + return JSON.stringify(event) + '\n' + } + private async appendRaw(content: string): Promise { if (this.options.after) await this.options.after.catch(() => {}) const append = this.options.appendFile ?? appendFile From 02c7c91855f061065f7eefeded2a76f037bb6c3b Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 07:19:23 -0700 Subject: [PATCH 18/21] docs(plans): #1417 round-2 disposition Co-Authored-By: Claude Opus 5.5 --- docs/plans/2026-09-27-c6-small-unbounded.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/plans/2026-09-27-c6-small-unbounded.md b/docs/plans/2026-09-27-c6-small-unbounded.md index e94452bfe..374207d2c 100644 --- a/docs/plans/2026-09-27-c6-small-unbounded.md +++ b/docs/plans/2026-09-27-c6-small-unbounded.md @@ -35,3 +35,14 @@ Row 13 of #1251 moved here, so `debugRetention.ts` and its test now change only | **c, minor:** Codex `rolloutPaths` unbounded | valid | `LruMap(4096)`, twice the recorded 2,023 indexed threads. A miss only costs the existing SQLite lookup. | | **c:** plan 3b stale | valid | Corrected above. | | **b survivors:** promptFolder's test hook using `get` instead of `peek`; a search-cache cap of 1 | declined | Neither is a behaviour of the app. The hook is test-only inspection, and the cap is a performance contract no test times; both are shared `LruMap` semantics, pinned in `lruMap.test.ts`. | + +## Review round 2 (a, b, c codex at `62091493`): all FIX-BEFORE-MERGE (final round) + +| Finding | Verdict | Change | +|---|---|---| +| **a, major:** a ledger absent for the whole prune (moved aside before it) read as "no ledger", so manual bundles became deletable | valid | **An absent ledger is `'unknown'` too:** it protects every legacy bundle and is never cached. Stated cost: with no ledger at all, pre-split legacy bundles are never aged out. Fail-first. | +| **a, major:** a whole Claude project directory removed kept its summaries forever | valid | The sweep also drops summaries whose project directory is missing from the scope-independent root listing, or vanished (ENOENT) while being listed. A failed root listing, or EACCES on a directory, stays unknown and keeps them. Fail-first. | +| **b, major; a and c, minor:** tombstone age used the wall clock, so a backward step plus correction dropped a fresh tombstone, and a future-dated one blocked the sweep | valid | `performance.now()` (monotonic). Tests fake only `performance`, plus a wall-clock step test; the `Date.now` mutant is red. | +| **c, major; a and b, minor:** a failed paste append dropped its batch, the timer's failure was an unhandled rejection, and a joining flush resolved as success | valid | A failed batch goes back in front of the queue (order holds), and the timer path catches and logs. A flush retries after the write it joined, so both callers reject on a dead disk. The retry queue is bounded at 1000 lines, and dropped lines are reported as one `ERROR journal:dropped-lines` line once writes work. The failed-batch and false-success tests are red at the previous head; a new test pins the bound. | +| **a / b / c survivors:** the `rolloutPaths` bound (Map, or the cap size) | declined | Pinning it needs over 4096 indexed rows or reaching into the private map; the mechanism is the shared `LruMap`, pinned in `lruMap.test.ts`. A miss only costs the existing SQLite lookup. | +| **b survivors (from round 1):** the promptFolder hook, a search-cache cap of 1 | declined | Unchanged reasons: a test-only hook and an untimed performance contract. | From 26ee43a56458fa52e05e50902d9de33a7b24823e Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 09:35:27 -0700 Subject: [PATCH 19/21] test(storage): a ledger replacement only the inode distinguishes is re-parsed (#1417 manager verification) Removing the inode from the cache key passed every test: a real rename moves ctime, so no portable filesystem sequence isolates it. stat() is controlled for the ledger path only (size, mtime, ctime equal; the inode differs); the content is real. The no-inode mutant is red. Co-Authored-By: Claude Opus 5.5 --- .../debugRetention.ledgerIdentity.test.ts | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 src/main/storage/debugRetention.ledgerIdentity.test.ts diff --git a/src/main/storage/debugRetention.ledgerIdentity.test.ts b/src/main/storage/debugRetention.ledgerIdentity.test.ts new file mode 100644 index 000000000..865563c8d --- /dev/null +++ b/src/main/storage/debugRetention.ledgerIdentity.test.ts @@ -0,0 +1,48 @@ +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, expect, it, vi } from 'vitest' + +// The ledger's stat() fields are controlled for ONE path so a replacement can +// match size, mtime AND ctime and differ only in the inode: a real rename +// always moves ctime, so no portable filesystem sequence isolates the inode. +// Everything else, including the ledger's content, is the real filesystem. +// Its own file because this mock covers the module. +const identity = vi.hoisted(() => ({ path: '', ino: 1 })) +vi.mock('node:fs/promises', async importOriginal => { + const real = await importOriginal() + return { + ...real, + stat: (async (...args: Parameters) => { + const info = await real.stat(...args) + if (String(args[0]) !== identity.path) return info + return Object.assign(Object.create(Object.getPrototypeOf(info)), info, { + ino: identity.ino, ctimeMs: 1_000, mtimeMs: 1_000, size: info.size, + }) + }) as typeof real.stat, + } +}) +const { cachedManualLegacyBundlePaths } = await import('./debugRetention.js') + +const dirs: string[] = [] +afterEach(() => { for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }) }) + +// Manager verification of #1417: removing the inode from the cache key passed +// every test. A ledger replaced by another file with the same size, mtime and +// ctime (a restore from backup, a rename-over within the timestamp resolution) +// is a DIFFERENT file, and only the inode says so; a stale cached parse would +// keep classifying bundles from the old ledger. +it('re-parses a replacement that only a new inode distinguishes', async () => { + const root = mkdtempSync(join(tmpdir(), 'ledger-identity-')) + dirs.push(root) + const ledger = join(root, 'saved-debug-bundles.jsonl') + identity.path = ledger + const row = (bundlePath: string) => `${JSON.stringify({ event: 'saved', reason: 'manual', bundlePath })}\n` + writeFileSync(ledger, row('/bundles/2026-01-01T00-00-01')) + expect(await cachedManualLegacyBundlePaths(ledger)).toEqual(new Set(['/bundles/2026-01-01T00-00-01'])) + + // Same size, and the mocked stat keeps mtime and ctime equal too. + writeFileSync(ledger, row('/bundles/2026-01-01T00-00-02')) + identity.ino = 2 + expect(await cachedManualLegacyBundlePaths(ledger)).toEqual(new Set(['/bundles/2026-01-01T00-00-02'])) +}) From 00a6ccbb37fd5615027de1ecaca27f946cf3f999 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 09:35:27 -0700 Subject: [PATCH 20/21] test(paste-debug): a failed batch is retried ahead of lines appended while it wrote (#1417 manager verification) Re-queueing a failed batch at the back passed every test. A held write that fails, with a line appended during it, must land first/second in order; the back-requeue mutant is red. Co-Authored-By: Claude Opus 5.5 --- src/main/pasteDebugJournal.test.ts | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/main/pasteDebugJournal.test.ts b/src/main/pasteDebugJournal.test.ts index f081cf9ba..c47c20a08 100644 --- a/src/main/pasteDebugJournal.test.ts +++ b/src/main/pasteDebugJournal.test.ts @@ -130,3 +130,33 @@ it('holds at most 1000 lines while writes fail, and reports what it dropped once expect(events[0]).toMatchObject({ layer: 'ERROR', event: 'journal:dropped-lines', data: { lines: 500 } }) expect(events.slice(1).map(event => event.event)).toEqual(Array.from({ length: 1000 }, (_, i) => `e${i + 500}`)) }) + +// Manager verification of #1417: re-queueing a failed batch at the BACK passed +// every test. A line appended while the failing write was in flight must land +// after the retried batch; the reader takes a session's start from line one. +it('retries a failed batch ahead of lines appended while it was writing', async () => { + userData.dir = await mkdtemp(join(tmpdir(), 'ac-paste-user-')) + dirs.push(userData.dir) + const { appendFile: realAppend } = await import('node:fs/promises') + let release: () => void = () => {} + const gate = new Promise(resolve => { release = resolve }) + let calls = 0 + const appendFile = (async (...args: Parameters) => { + calls++ + if (calls <= 2) { + // The first write and its mkdir retry: held, then both fail. + await gate + throw Object.assign(new Error('injected EIO'), { code: 'EIO' }) + } + return realAppend(...args) + }) as typeof realAppend + const journal = new PasteDebugJournalRegistry({ appendFile }).get('paste-order') + journal.append({ layer: 'RENDER', event: 'first' }) + await vi.waitFor(() => expect(calls).toBe(1)) + journal.append({ layer: 'RENDER', event: 'second' }) + release() + await journal.flush() + + const events = (await readFile(pasteDebugLogPath('paste-order'), 'utf8')).trim().split('\n').map(line => JSON.parse(line).event) + expect(events).toEqual(['first', 'second']) +}) From 8b3928d79ae301b8b5b8f75623f33a73c5d874df Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 27 Sep 2026 12:56:21 -0700 Subject: [PATCH 21/21] docs(debug-retention): the second-stat comment describes the round-2 loader (#1417 manager verify) Comment-only. The comment still said the loader "rightly" answers ENOENT with "no ledger"; since round 2 an absent ledger is 'unknown', and the second stat now guards a ledger replaced or edited mid-read. Co-Authored-By: Claude Opus 5.5 --- src/main/storage/debugRetention.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/main/storage/debugRetention.ts b/src/main/storage/debugRetention.ts index 7caab3d35..e80c41e7a 100644 --- a/src/main/storage/debugRetention.ts +++ b/src/main/storage/debugRetention.ts @@ -499,13 +499,17 @@ export async function cachedManualLegacyBundlePaths( const key = `${file}\0${identity}` if (legacyLedgerCache?.key === key) return legacyLedgerCache.paths const paths = await load(file) - // WHY a second stat (review of #1417, a): the load is a separate operation. - // A ledger renamed away between the stat and the read made readFile hit - // ENOENT, which the loader rightly calls "no ledger", and that empty set was - // then cached under the identity of the file that WAS there, so a manual - // bundle became deletable. The parse is trusted only if the file it read is - // the file the stat saw, before and after; any change (gone, replaced, - // edited mid-read) is 'unknown': protective for this prune, never cached. + // WHY a second stat (review of #1417, round 1, a): the load is a separate + // operation, so the file it reads need not be the file the first stat saw. + // In round 1 the loader still answered ENOENT with an empty set, and a + // ledger renamed away between the stat and the read cached that empty set + // under the identity of the file that WAS there, so a manual bundle became + // deletable. The loader now answers ENOENT with 'unknown' (round 2), which + // closes that exact case, but a ledger REPLACED mid-read (renamed over, or + // edited) still parses successfully as some other content. So the parse is + // trusted only if the identity is the same before and after the read; any + // change (gone, replaced, edited mid-read) is 'unknown': protective for this + // prune, never cached. if (paths === 'unknown' || (await ledgerIdentity(file)) !== identity) { legacyLedgerCache = null return 'unknown'