diff --git a/docs/plans/2026-09-27-lsp-open-containment.md b/docs/plans/2026-09-27-lsp-open-containment.md new file mode 100644 index 000000000..f92dbf3cd --- /dev/null +++ b/docs/plans/2026-09-27-lsp-open-containment.md @@ -0,0 +1,34 @@ +# LSP open: re-check physical containment at the moment of use (#1268) + +**Gap.** `authorizeContext` (`src/main/ipc/lsp.ts`) validates the physical target, then `LspManager.openDocumentNow` awaits server startup, which can take seconds on a cold spawn. Only after that does it build a lexical `file://` URI and send `didOpen`. If a directory on the path is swapped for a symlink to an outside directory during startup, the server is handed a URI that resolves outside the root. + +**Fix.** +- **Manager:** `OpenDocumentParams.assertPhysicalTarget`, a callback from the authorizing caller. The manager awaits it inside the per-document queue, after server startup and immediately before a NEW server document's `didOpen`. A refusal fails open (returns false: no LSP for this document) and names nothing to the server. +- **IPC:** both open paths (`lsp:open-document`, `lsp:reopen-document`) pass `lspPhysicalTargetAssertion(context)`. It re-runs the same rule: `resolveInsideRoot` + `validateExistingTarget` (no symlink, canonical inside the root) + regular file + an unchanged relative location. +- **Why a callback and not a filesystem check in the manager:** the IPC layer owns authorization for both editor roots and AI Workspace entries, and the manager's unit tests run on fake roots. + +**Tests.** +- **Real filesystem:** the reviewer's probe. Authorize `src/a.ts`, then swap `src` for a symlink to an outside directory: refused. A leaf that became a symlink: refused. An untouched file: passes. A virtual document: nothing to check. +- **Manager:** + - the re-check runs after `initialized` and before `didOpen`; + - a refusal returns false with no notification; + - a pass opens normally. +- **Mutations killed:** no re-check in the manager; no physical validation in the assertion. + +## After review a of #1412 +- **Shared joins:** the re-check runs at the top of the queued step for EVERY open, including one that joins an existing shared document (another alias of the same file). Before, only a new document's `didOpen` was guarded, so a join after a swap sent `didChange` for the escaped URI. +- **Virtual documents:** they are named under `root/.agent-code-lsp`, so that directory must not be a symlink out of the root. They now get a re-check too. +- **No exact relative-path comparison:** a case-only rename on a case-insensitive filesystem still resolves inside the root, and refusing it only lost LSP. Containment plus a regular file is the property. + +**Residuals.** +- **One await:** the window between the re-check and the notification is one await, inherent to any path-based open. +- **A swap after a document is already open** (a later `didChange`, or a document request, for a URI the server already holds) is not guarded. The server already has that URI, and no per-change path check stops it from reading the path later. This issue is the authorization-to-use window of an OPEN. +- **The IPC wiring** of the callback is not covered by a test, because the handlers need Electron. + +## After review b (1cb8b3cf) +Every assertion first checks that the canonical root still resolves to itself (`assertRootUnchanged`). A pathless document's check used to return early when the virtual directory was missing, without checking the root. Review b's other findings are the path-based LSP limit; B6 (owner proxy) accepted this PR as narrowing the window, `Refs #1268`. + +## After review c +- The virtual branch now checks the LEAF `didOpen` names (`virtual-.`, from `lspVirtualDocumentName`, shared with the manager). It must be absent, or a regular file inside the root. A leaf symlink created in advance escaped with no timing window. A pathless open without its leaf name is refused. +- The regular-file re-check is pinned: an authorized file that became a directory is refused. +- The IPC wiring of the callback still has no committed test. A wiring test is feasible with the existing Electron mock; it is left out under the freeze and stated as a residual. diff --git a/docs/plans/2026-09-27-monitor-unexamined-run.md b/docs/plans/2026-09-27-monitor-unexamined-run.md new file mode 100644 index 000000000..8c090fad6 --- /dev/null +++ b/docs/plans/2026-09-27-monitor-unexamined-run.md @@ -0,0 +1,56 @@ +# A run the monitor store never examined is not empty (#1453) + +## Evidence +- Found while verifying #1411 (q115), with a probe: store B indexes the monitor folder at startup. A second store under another run id then creates `runs/run-a` and writes `incidents.json` + `operations.json`. B's next `maintain()` deletes `run-a` (ENOENT afterwards). +- Cause, `MonitorHistoryStore.maintain()`: the expired-run pass deletes every run folder not in `index` / `incidentRuns` / `unindexedRuns`. Those maps are filled only by startup indexing, so a run that appeared later is "not known", which the pass reads as "empty". +- Two app processes can share one data folder: `--packaging-smoke` skips the single-instance lock. +- It is the same "never seen means empty" shape the worker rule forbids (q109, q115). + +## Change +- `examinedRuns`: the runs startup indexing actually looked at. +- Retention deletes a run as empty only if it was examined. An unexamined run is unknown and waits for the next start to index it. +- `clear()` resets the set with the rest. + +## Not changed (residual) +- The capacity budget (`pruneRuns`) may still remove an unexamined run, as it already may for `unindexedRuns`. That is the documented policy: the 128 MiB ceiling wins over unknown runs. It orders an unexamined run as oldest, since it has no indexed points. +- ~~Two live stores can still examine each other's run while it is empty at startup.~~ Closed by review b's `touchedSince` (see below). + +## Test (real files) +`MonitorHistoryStore.test.ts`: run-a appears after store B indexed. +- It survives two of B's maintenance passes. This is red before the fix (ENOENT on the first pass). +- A restarted store examines it and keeps its in-retention incident. +- Past retention, its incident file is deleted, and its (then empty, aged) folder on a later pass, so the protection is not permanent. + +## Review a (round 1), fixed +- **An unreadable or untrusted incident file:** at indexing, an `incidents.json` that exists but cannot be read, parsed or trusted now makes the run UNKNOWN (`unindexedRuns`). It used to read as `[]`, so an examined run was deleted. +- **`examinedRuns` is forgotten when the run is deleted** (retention or capacity), so a name another store recreates with fresh data is unexamined again and kept. +- **Tests:** real files. An incident file at mode 000 during indexing survives maintenance once readable; a run recreated after its retention deletion survives. Both were red before. (Their mutation gates were shadowed by review b's `touchedSince` until review c's real-clock rewrite; see below.) + +## Review b (round 1), fixed +- **A tier `stat` failure other than ENOENT** now marks the run unknown instead of skipping the tier as absent. +- **A tier file with unparseable lines** marks the run unknown. It was indexed with no points, deleted as fully expired, and then its run was deleted. +- **Retention keeps any run with a file touched within the retention window** (`touchedSince`; any list or stat failure counts as touched). A run examined while empty can be filled later by the other store, which is residual 2 above, now closed. One side effect: once an expired run's last file is removed, its fresh folder mtime keeps the empty folder for one more window. +- **Tests:** real files, one per finding (ELOOP tier link, unparseable tier line, filled after examination). Each was red before. Mutations: "unparsed ignored" and "no touched check" fail on their own; the stat and touched guards back each other up on the ELOOP case (removing both fails). + +## Review a round 2: residual (manager decision) +A second store's write can land between retention's final `touchedSince()` check and its recursive `rm()`: a check-then-act race between two uncoordinated processes. It needs two app processes sharing one data folder (possible only under `--packaging-smoke`, which skips the single-instance lock) and a write inside that sub-millisecond window. Closing it needs a cross-process lock on the monitor folder. A rename-to-tombstone-then-recheck scheme narrows it but brings restore-collision cases of its own. That is left out under the PR freeze; B6 decides whether to accept this residual or require the lock. + +## Review c (round 1), fixed (tests and docs) +- **The problem:** every new test used a 1970-scale fake clock, so `touchedSince` saw every fixture as freshly touched and shadowed the other guards. Removing the `examinedRuns` guard itself (#1453's fix), the unknown-incidents guard, or the forget-on-delete survived the suite. +- **The fix:** the tests run on the real clock, and each fixture's files and folder are aged past retention with `utimes`, so only the guard a test names can keep the run. A cleanup-direction assertion is added: an examined run whose data expired loses its folder on a later pass. +- **Mutations, each killed on its own:** the `examinedRuns` guard dropped (2 red); `examinedRuns` never populated (2 red); examined kept after delete; unknown incidents unprotected; unparsed lines ignored; no touched check (2 red). Only the ELOOP case has two guards (indexing and `touchedSince`), as noted in its test. +- **Docs:** residual 2 is closed; "deleted as before" is corrected (the incident file goes first, the emptied folder on a later pass). + + +## Review b round 2, fixed +- **The problem:** the index is only a snapshot of another live store's run. Expiring or compacting a foreign tier file, or rewriting its incidents, from that snapshot deleted what the other store wrote afterwards, including content appended after indexing. +- **The fix:** each foreign tier and incident file's size and mtime are recorded at indexing (`foreignFiles`). Every expiry, compaction and incident rewrite of a foreign file first checks the file is unchanged. A changed or vanished file makes the run unknown (`unindexedRuns`) instead. After the store's own rewrite of a foreign file, the new fingerprint is recorded. The store's own run needs no check, since only it writes there. +- **Tests (two live stores on one folder, real files and clock):** + - a foreign tier expired from a stale index; + - content appended to a foreign tier after indexing (compaction); + - a fresh incident added after indexing (incident rewrite). + With the check disabled, the first two fail; removing only the incident-loop check fails the third. +- **Residual unchanged:** the sub-millisecond window between the check and the act (the review a round 2 cross-process residual). + +## Review b round 3, fixed +The global incident limit also rewrote a foreign incident file from the cached rows. The unchanged-file check now lives inside `writeRunIncidents`, so retention and the limit both pass it. Pinned by "never enforces the incident limit on another live store's changed file". Removing the check fails that test and the incident-retention test. diff --git a/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md new file mode 100644 index 000000000..ca5ccf319 --- /dev/null +++ b/docs/plans/2026-09-27-retention-collects-keylog-run-dirs.md @@ -0,0 +1,51 @@ +# Debug retention collects key-log-only proxy run dirs (#1385) + +## Problem +`collectProxyRunDirs` recognised a run dir only by `proxy-events.jsonl`. A run dir holding just `session-meta.json` + `sslkeylog.log` was walked into and never collected. #1380 review c recounted names and sizes only (contents never read): 23 such dirs on the owner's machine, 5.18 MB of plaintext TLS session secrets, May–September 2026. + +## Fix (narrowed to FUTURE runs; B6's oldest-first list) +- A run dir is recognised by either evidence file: `proxy-events.jsonl` or `sslkeylog.log`. It matches the key log itself (review c). +- A dir with `proxy-events.jsonl` is collected as before. +- A key-log-only dir is collected only when it is NOT in the BASELINE: the set of key-log-only dirs that existed when a build containing this code first started (`keyLogBaseline()`, captured at run start in `holdDebugStoragePruneUntilRecovered`, written once with an exclusive create to `STATE_DIR/debug-retention-keylog-baseline.json`). Capture is strict: any directory it cannot list means no baseline, nothing is written, and a later start retries. With no baseline, NO key-log-only dir is collected. +- **WHY a captured set (#1388 review a, two rounds):** + - a date constant excluded runs made on the merge day forever; + - a first-prune marker was written minutes after start (the boot gate delays the first prune), so runs made in between were excluded forever; + - any timestamp comparison admits a pre-upgrade run whose name sorts later after a clock step back. + Membership in "what already existed" needs no clock. +- Every key-log-only dir in the baseline, including the owner's 23, is left untouched and never walked into, whatever its name. Names play no part: a NEW key-log-only dir is collected even if its name is not a timestamp. The decision on the existing ones is tracked in #1460 (q91). +- `session-meta.json` alone stays uncollected, and `_shared-conf` is still skipped. + +## Owner decision kept (q91) +- Deleting the EXISTING key logs is still the owner's decision. This PR no longer makes it: the first prune after merge does not touch any of the 23 dirs. +- New key-log-only runs fall under the normal TTL pass (48 h, `AGENT_CODE_DEBUG_TTL_HOURS`) and the proxy budget. + +## Also (q115, "unknown is never empty") +`dirStats` no longer skips a child it cannot read. Only ENOENT means absent; any other error leaves the whole dir uncollected that pass. The manual-bundle ledger loader is NOT changed here, because W4's #1417 owns it (q118). + +## Tests +`debugRetention.keylog.test.ts`, on the real directory shapes (`proxy////`): +- a NEW key-log-only dir is collected beside a normal run dir; +- baseline members (including one whose name sorts after a new run), `_shared-conf` and a metadata-only dir are not; +- the unreadable-child test: fail once, recover, maintain, and the bytes survive. +- Mutations killed: removing the cutoff, and removing the name check. + +## Review a (round 1) +- **Fixed:** the date constant replaced by the first-run marker (above). +- **Tests:** a same-day run after the marker is collected; one before it is not; a null cutoff collects no key-log-only dir; the marker is written once and kept, and fails closed on an unknown shape or an unreadable path. +- **Mutations killed:** no cutoff; null collecting everything; any marker shape accepted. Removing the up-front marker read ALONE survives, because the exclusive create then hits EEXIST and reads the stored marker. Removing both guards fails. +- **Not changed (finding 1):** a run dir with an events file AND a key log is collected whole, key log included. That is main's existing behaviour for event-bearing runs, which this PR does not touch. The owner decision (q91) is about the key-log-only dirs, which stay untouched. + +## Review a round 2 + b (fixed at the next head) +- The marker is replaced by the baseline set, captured at run start. +- **Tests:** a baseline dir named after a new run (a clock step back) stays excluded; a run made after capture is collected; capture over an unreadable subtree yields no baseline and writes nothing; a baseline that cannot be written is not established (review b); the file is reused and a malformed one fails closed. +- **Mutations killed:** membership ignored; null collecting everything; lenient capture; capture recording nothing; returning an unsaved baseline. +- **Residuals:** + - The early-capture wiring in `holdDebugStoragePruneUntilRecovered` is not separately pinned; the boot-gate suite exercises it against a scratch state dir. + - `dirStats`' EIO/ELOOP branches (review b) are not reproducible on a real filesystem: symlink entries are skipped, and EIO cannot be produced on demand. EACCES is pinned. + + +## Review a round 3 (last pass) +- **Fixed, unsafe direction:** a proxy root missing at capture saved an empty baseline, so old key logs that reappeared became collectable. Capture now has no ENOENT exception, even for the root: no baseline, nothing written, retried at a later start. +- **Not fixed, conservative direction (decided with review b round 3):** a run created WHILE the startup scan runs is baselined and kept forever. A birthtime filter was tried and REVERTED: after a clock step back, a pre-existing dir's birthtime can look later than the capture start, which would exclude an old key log from the baseline (the unsafe direction). Capture starts at run start, before any session exists, so the window is the few milliseconds of the scan. +- **Residual, conservative direction:** after a failed capture (for example an unwritable state dir), runs made before a later successful capture are baselined and never collected. That is a retention gap, never a deletion. The same holds on a fresh install that has no proxy folder yet: the first capture happens at the start after the folder appears. +- **Mutation killed:** the root ENOENT exception. diff --git a/src/main/agentActivity/AgentActivityStore.test.ts b/src/main/agentActivity/AgentActivityStore.test.ts index a4c1eab99..d12ea96c9 100644 --- a/src/main/agentActivity/AgentActivityStore.test.ts +++ b/src/main/agentActivity/AgentActivityStore.test.ts @@ -1,4 +1,4 @@ -import { appendFile, mkdtemp, readFile, rm } from 'node:fs/promises' +import { appendFile, chmod, mkdir, mkdtemp, readFile, readdir, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -124,3 +124,224 @@ describe('AgentActivityStore', () => { expect(keys.size).toBe(1) }) }) + +// #1303: the context id was cached BEFORE its context line was written. One +// failed append (ENOSPC, EIO) then left every later interval for that agent +// this month pointing at a context line that never reached disk, and +// readIntervals dropped each one silently. +describe('a failed context write', () => { + it('does not orphan the agent\'s later intervals', async () => { + const store = new AgentActivityStore(dir) + const internal = store as unknown as { appendLines: (file: string, lines: string[]) => Promise } + const realAppend = internal.appendLines.bind(store) + let failNext = true + internal.appendLines = async (file, lines) => { + if (failNext) { failNext = false; throw Object.assign(new Error('no space left'), { code: 'ENOSPC' }) } + return realAppend(file, lines) + } + const start = Date.parse('2026-09-01T09:00:00Z') + await expect(store.appendInterval({ context, startedAt: start, endedAt: start + HOUR })).rejects.toThrow('no space left') + await store.appendInterval({ context, startedAt: start + 2 * HOUR, endedAt: start + 3 * HOUR }) + const read = await new AgentActivityStore(dir).readIntervals(start, start + 4 * HOUR) + expect(read.map(interval => interval.startedAt)).toEqual([start + 2 * HOUR]) + }) +}) + +// #1414 review a+b: a PARTIAL write (A's context line lands, then the append +// fails before its interval line) left id 1 on disk for A. A was not cached, so +// B's next context also took id 1; after a restart a later A interval reused +// id 1 and read back as B's time. +describe('a partially written context', () => { + it('never lets another agent reuse its id', async () => { + const store = new AgentActivityStore(dir) + const internal = store as unknown as { appendLines: (file: string, lines: string[]) => Promise } + const realAppend = internal.appendLines.bind(store) + let partial = true + internal.appendLines = async (file, lines) => { + if (partial) { + partial = false + await appendFile(file, lines[0] + '\n') + throw Object.assign(new Error('no space left'), { code: 'ENOSPC' }) + } + return realAppend(file, lines) + } + const start = Date.parse('2026-09-01T09:00:00Z') + const a = { ...context, agentKey: 'A', label: 'A' } + const b = { ...context, agentKey: 'B', label: 'B' } + await expect(store.appendInterval({ context: a, startedAt: start, endedAt: start + HOUR })).rejects.toThrow('no space left') + await store.appendInterval({ context: b, startedAt: start + HOUR, endedAt: start + 2 * HOUR }) + const restarted = new AgentActivityStore(dir) + await restarted.appendInterval({ context: a, startedAt: start + 2 * HOUR, endedAt: start + 3 * HOUR }) + const read = await new AgentActivityStore(dir).readIntervals(start, start + 4 * HOUR) + expect(read.map(interval => interval.context.agentKey)).toEqual(['B', 'A']) + }) +}) + +// #1414 review a round 2 (q115, "unknown is never empty"): an existing month +// file that cannot be READ was treated as absent, so a restarted store started +// ids at 1 and gave a second agent the id the first agent's lines already use. +// Once readable again, the first agent's later hours read back as the second +// agent's. Only ENOENT means "no file yet"; any other failure refuses the +// append, and the bytes stay as they were. +describe('an unreadable month file', () => { + it('refuses the append instead of restarting ids, and ids continue once it is readable', async () => { + const start = Date.parse('2026-09-01T09:00:00Z') + const a = { ...context, agentKey: 'A', label: 'A' } + const b = { ...context, agentKey: 'B', label: 'B' } + await new AgentActivityStore(dir).appendInterval({ context: a, startedAt: start, endedAt: start + HOUR }) + const file = join(dir, '2026-09.jsonl') + const before = await readFile(file) + await chmod(file, 0o200) + try { + await expect(new AgentActivityStore(dir).appendInterval({ context: b, startedAt: start + HOUR, endedAt: start + 2 * HOUR })).rejects.toThrow() + } finally { + await chmod(file, 0o600) + } + expect(await readFile(file)).toEqual(before) + const restarted = new AgentActivityStore(dir) + await restarted.appendInterval({ context: b, startedAt: start + HOUR, endedAt: start + 2 * HOUR }) + await restarted.appendInterval({ context: a, startedAt: start + 2 * HOUR, endedAt: start + 3 * HOUR }) + const read = await new AgentActivityStore(dir).readIntervals(start, start + 4 * HOUR) + expect(read.map(interval => interval.context.agentKey)).toEqual(['A', 'B', 'A']) + }) +}) + +// #1414 review b round 2 (test gap): a failed write that wrote NOTHING still +// consumed its id, and a restart must continue from the highest id on disk, +// not from the count of contexts (which would reissue a live id). +describe('an id gap left by a failed write', () => { + it('is never filled by a later context after a restart', async () => { + const store = new AgentActivityStore(dir) + const internal = store as unknown as { appendLines: (file: string, lines: string[]) => Promise } + const realAppend = internal.appendLines.bind(store) + let failNext = true + internal.appendLines = async (file, lines) => { + if (failNext) { failNext = false; throw Object.assign(new Error('no space left'), { code: 'ENOSPC' }) } + return realAppend(file, lines) + } + const start = Date.parse('2026-09-01T09:00:00Z') + const a = { ...context, agentKey: 'A', label: 'A' } + const b = { ...context, agentKey: 'B', label: 'B' } + const c = { ...context, agentKey: 'C', label: 'C' } + await expect(store.appendInterval({ context: a, startedAt: start, endedAt: start + HOUR })).rejects.toThrow('no space left') + await store.appendInterval({ context: b, startedAt: start + HOUR, endedAt: start + 2 * HOUR }) + const restarted = new AgentActivityStore(dir) + await restarted.appendInterval({ context: c, startedAt: start + 2 * HOUR, endedAt: start + 3 * HOUR }) + await restarted.appendInterval({ context: b, startedAt: start + 3 * HOUR, endedAt: start + 4 * HOUR }) + const read = await new AgentActivityStore(dir).readIntervals(start, start + 5 * HOUR) + expect(read.map(interval => interval.context.agentKey)).toEqual(['B', 'C', 'B']) + }) +}) + +// #1414 review a round 3 (q115): recovery treated an UNREADABLE open.json as +// "no open file" and overwrote it with an empty snapshot, so the pending +// interval was lost for good. Now an unreadable (or corrupt) snapshot is moved +// aside, bytes intact, and a later start recovers it once it can be read. +describe('an unreadable open-interval snapshot', () => { + it('is set aside instead of overwritten, and recovered once readable', async () => { + const start = Date.parse('2026-09-01T09:00:00Z') + const lastTouch = start + 2 * HOUR + await new AgentActivityStore(dir).writeOpen([{ sessionId: 'session-1', context, startedAt: start }], lastTouch) + const file = join(dir, 'open.json') + const before = await readFile(file) + await chmod(file, 0o200) + expect(await new AgentActivityStore(dir).recoverOpenIntervals(start + 10 * HOUR)).toBe(0) + const aside = (await readdir(dir)).filter(name => name.startsWith('open.json.unrecovered-')) + expect(aside).toHaveLength(1) + await chmod(join(dir, aside[0]!), 0o600) + expect(await readFile(join(dir, aside[0]!))).toEqual(before) + // Readable again: the next start recovers it and removes the set-aside copy. + expect(await new AgentActivityStore(dir).recoverOpenIntervals(start + 11 * HOUR)).toBe(1) + expect((await readdir(dir)).filter(name => name.startsWith('open.json.unrecovered-'))).toEqual([]) + const read = await new AgentActivityStore(dir).readIntervals(start, start + 12 * HOUR) + expect(read.map(interval => [interval.startedAt, interval.endedAt])).toEqual([[start, lastTouch]]) + }) +}) + +// #1414 review c: a recovery that fails PARTWAY (the second entry's append +// fails) set the whole snapshot aside, so the next start re-appended the +// entry already recovered. Only the entries not yet recovered are set aside. +describe('a recovery that fails partway', () => { + it('sets aside only what was not recovered, so nothing is counted twice', async () => { + const start = Date.parse('2026-09-01T09:00:00Z') + const lastTouch = start + 2 * HOUR + const a = { ...context, agentKey: 'A', label: 'A' } + const b = { ...context, agentKey: 'B', label: 'B' } + await new AgentActivityStore(dir).writeOpen([ + { sessionId: 'session-a', context: a, startedAt: start }, + { sessionId: 'session-b', context: b, startedAt: start + HOUR }, + ], lastTouch) + const store = new AgentActivityStore(dir) + const internal = store as unknown as { appendLines: (file: string, lines: string[]) => Promise } + const realAppend = internal.appendLines.bind(store) + let calls = 0 + internal.appendLines = async (file, lines) => { + calls += 1 + if (calls === 2) throw Object.assign(new Error('no space left'), { code: 'ENOSPC' }) + return realAppend(file, lines) + } + expect(await store.recoverOpenIntervals(start + 10 * HOUR)).toBe(1) + expect(await new AgentActivityStore(dir).recoverOpenIntervals(start + 11 * HOUR)).toBe(1) + const read = await new AgentActivityStore(dir).readIntervals(start, start + 12 * HOUR) + expect(read.map(interval => interval.context.agentKey).sort()).toEqual(['A', 'B']) + }) + + // #1414 review c (test gap): a set-aside copy that still cannot be read is + // KEPT for a later start, never deleted. + it('keeps a set-aside copy that still cannot be read', async () => { + const aside = join(dir, 'open.json.unrecovered-1000') + await mkdir(dir, { recursive: true }) + await writeFile(aside, '{"aliveAt":1,"open":[]}') + await chmod(aside, 0o200) + try { + await new AgentActivityStore(dir).recoverOpenIntervals(Date.parse('2026-09-01T12:00:00Z')) + expect((await readdir(dir)).filter(name => name.startsWith('open.json.unrecovered-'))).toEqual(['open.json.unrecovered-1000']) + } finally { + await chmod(aside, 0o600) + } + }) +}) + +// B6 check (q115): an aliases file that exists but cannot be READ was treated +// as absent twice over. The tail repair skipped its newline check, so a new +// edge was glued onto a torn last line and lost; and the aliases were cached +// as empty. Only ENOENT is "no file": otherwise the append is refused, the +// bytes are untouched, and nothing is cached, so a later call reads again. +describe('an unreadable aliases file', () => { + it('refuses the append instead of gluing onto an unseen tail, and works once readable', async () => { + const file = join(dir, 'aliases.jsonl') + await mkdir(dir, { recursive: true }) + await writeFile(file, '{"f":"A","t":"B"}') + const before = await readFile(file) + await chmod(file, 0o200) + try { + await expect(new AgentActivityStore(dir).appendAliases([['B', 'C']])).rejects.toThrow() + } finally { + await chmod(file, 0o600) + } + expect(await readFile(file)).toEqual(before) + await new AgentActivityStore(dir).appendAliases([['B', 'C']]) + const lines = (await readFile(file, 'utf8')).trim().split('\n').map(line => JSON.parse(line) as { f: string; t: string }) + expect(lines).toEqual([{ f: 'A', t: 'B' }, { f: 'B', t: 'C' }]) + }) + + // The READ path has one guard: `loadAliases`' ENOENT-only catch (B6 check, + // 2110). The append test above cannot pin it, because the tail repair + // refuses that append on its own. Here the SAME store instance reads while + // the file is unreadable: with a catch-all, the empty map would be cached + // and A and B would stay split for the life of the store. + it('does not cache an unreadable file as empty: the same store groups once it is readable', async () => { + const file = join(dir, 'aliases.jsonl') + await mkdir(dir, { recursive: true }) + await writeFile(file, '{"f":"A","t":"B"}\n') + const store = new AgentActivityStore(dir) + await chmod(file, 0o200) + try { + await expect(store.agentKeyGrouping()).rejects.toThrow() + } finally { + await chmod(file, 0o600) + } + const group = await store.agentKeyGrouping() + expect(group('A')).toBe(group('B')) + }) +}) diff --git a/src/main/agentActivity/AgentActivityStore.ts b/src/main/agentActivity/AgentActivityStore.ts index 49d69572d..0333a737d 100644 --- a/src/main/agentActivity/AgentActivityStore.ts +++ b/src/main/agentActivity/AgentActivityStore.ts @@ -1,4 +1,4 @@ -import { appendFile, mkdir, open, readFile, readdir, rename, writeFile } from 'node:fs/promises' +import { appendFile, mkdir, open, readFile, readdir, rename, rm, writeFile } from 'node:fs/promises' import { join } from 'node:path' import type { SystemSuspension } from '@shared/types/systemSuspension.js' @@ -107,6 +107,16 @@ function parseJsonLines(text: string): Record[] { return out } +type SetAsideSnapshot = { aliveAt: number | null; open: unknown[] } + +/** An open-interval snapshot recovered only partway: `remaining` starts at + * the entry whose append failed (#1414 review c). */ +class PartialRecovery extends Error { + constructor(readonly recovered: number, readonly remaining: SetAsideSnapshot) { + super('open-interval recovery stopped partway') + } +} + export class AgentActivityStore { private tail: Promise = Promise.resolve() /** aliases.jsonl in memory, loaded on first use. */ @@ -115,6 +125,15 @@ export class AgentActivityStore { private readonly cleanTails = new Set() /** Context ids already written to each month file this process has touched. */ private readonly monthContexts = new Map>() + /** + * The next context id to mint per month (#1414 review). Minting always + * advances it, even when the write then fails, so an id is never issued + * twice: a PARTIAL write can leave a context line on disk for an id whose + * mapping was never cached, and reusing that id for another agent made the + * later reads attribute one agent's time to the other. Loaded as the + * file's highest id + 1. + */ + private readonly monthNextId = new Map() constructor(private readonly dir: string) {} @@ -132,16 +151,26 @@ export class AgentActivityStore { const known = this.monthContexts.get(month) if (known) return known const ids = new Map() + let highest = 0 try { for (const line of parseJsonLines(await readFile(join(this.dir, `${month}.jsonl`), 'utf8'))) { if (line.t !== 'c' || !isNumber(line.c)) continue + highest = Math.max(highest, line.c) const context = parseContext(line) if (context) ids.set(contextKey(context), line.c) } - } catch { - // No file yet for this month. + } catch (error) { + // Only a missing file means "no contexts yet" (#1414 review a round 2, + // q115 "unknown is never empty"). A file that exists but cannot be read + // was treated as empty: ids restarted at 1, a second agent got the id + // the first agent's lines already use, and once readable the first + // agent's later hours read back as the second's. Refuse the append + // instead (the interval is lost, as for any failed write); nothing is + // cached, so the next append reads again. + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error } this.monthContexts.set(month, ids) + this.monthNextId.set(month, highest + 1) return ids } @@ -154,15 +183,28 @@ export class AgentActivityStore { const key = contextKey(interval.context) const lines: string[] = [] let id = ids.get(key) + const isNewContext = id === undefined if (id === undefined) { - id = ids.size + 1 - ids.set(key, id) + // contextsFor always sets the month's next id. No `ids.size + 1` + // fallback: counting contexts reissues an id after any gap (#1414 + // review c; the gap test pins it). + id = this.monthNextId.get(month)! + this.monthNextId.set(month, id + 1) const contextLine: ContextLine = { t: 'c', c: id, ...interval.context } lines.push(JSON.stringify(contextLine)) } const intervalLine: IntervalLine = { t: 'i', c: id, s: interval.startedAt, e: interval.endedAt } lines.push(JSON.stringify(intervalLine)) await this.appendLines(join(this.dir, `${month}.jsonl`), lines) + // Cache the id only once its context line is on disk (#1303). Caching it + // first meant one failed append (ENOSPC, EIO) left every later interval + // for this agent this month pointing at a context line that never + // landed, and readIntervals drops an interval with no context. Not + // caching on failure means the next interval for this agent mints a + // NEW id (monthNextId already advanced) and writes its context line + // again. The failed id is burned: if its line landed partially, it + // still names this agent, and no other agent is ever given that id. + if (isNewContext) ids.set(key, id) }) } @@ -175,8 +217,12 @@ export class AgentActivityStore { aliases.set(line.f, line.t) } } - } catch { - // No aliases yet. + } catch (error) { + // Only a missing file means "no aliases yet" (B6 check, q115). An + // unreadable one was cached as empty, so every read grouped agents + // wrongly until restart. Throw instead; nothing is cached, so a later + // call reads again. + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error } this.aliases = aliases return aliases @@ -239,8 +285,12 @@ export class AgentActivityStore { } finally { await handle.close() } - } catch { - // No file yet: nothing to repair. + } catch (error) { + // Only a missing file has no tail to repair (B6 check, q115). A file + // that cannot be read (write-only, EIO) has an unseen tail: appending + // blindly glued the new line onto a torn last line and lost it. + // Refuse the append instead. + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error } } await appendFile(path, text) @@ -310,32 +360,104 @@ export class AgentActivityStore { /** Close every interval a previous run left open at that run's last touch. * An unclean shutdown therefore contributes at most one touch period of time - * that was not observed, never the hours until the next launch. */ + * that was not observed, never the hours until the next launch. + * + * "Unknown is never empty" (#1414 review a round 3, q115): recovery used to + * treat ANY failure (unreadable, corrupt, a failed append midway) as "no + * open file" and then overwrite open.json with an empty snapshot, losing the + * pending interval for good. Now only a missing file (ENOENT) is empty. + * Anything else is moved aside to `open.json.unrecovered-