From 3d67a982919bb18f1a8b860d1dc4291cfa36a728 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 20 Jun 2026 04:13:37 +0800 Subject: [PATCH] fix(session-persistence-jsonl): make loadLive cwd-scope-exact (Codex review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's converge pass on PR B found a cross-cwd adoption hole: the coordinator calls loadLive(id, session.header.cwd) for HMR live-adoption, but JSONL's loadLive delegated to findLog(id, cwd) which, for cwd === undefined, scanned ALL cwd buckets. So a live NO-CWD session could adopt a same-id log from a real cwd bucket, ending with a live cwd: undefined but a persisted meta.cwd: '/w'. loadLive must treat `undefined` as the DEFINITE no-cwd bucket, not "unknown": it now goes straight to logPath(cwd, id) (which maps undefined -> _no-cwd), never the all-buckets scan. loadStored/deleteStored keep the any-cwd scan (resume/removal identify by id alone), so findLog is now a pure scan-all and loses its dead cwd-direct branch. The coordinator's has() relied on loadLive(id, undefined) meaning "any scope" for an untracked id — fixed to use loadStored for the untracked (unknown-cwd) case and loadLive only for a tracked session's known cwd. Adds a regression test: a no-cwd live session reusing an id persisted in a real cwd bucket no longer cross-cwd-adopts — it falls through to createCore's any-cwd collision probe and REJECTS, leaving the original log untouched. Also fixes the README to say `tornMarker !== undefined` (a marker may be falsy, 0). --- .../session-persistence-jsonl/src/index.ts | 46 +++++++++++-------- .../tests/jsonl.spec.ts | 37 ++++++++++++++- packages/session-persistence/README.md | 2 +- .../session-persistence/src/coordinator.ts | 16 ++++--- 4 files changed, 73 insertions(+), 28 deletions(-) diff --git a/packages/session-persistence-jsonl/src/index.ts b/packages/session-persistence-jsonl/src/index.ts index a469095c5d..76df3f3ccb 100644 --- a/packages/session-persistence-jsonl/src/index.ts +++ b/packages/session-persistence-jsonl/src/index.ts @@ -127,24 +127,32 @@ export class SessionPersistenceJsonl extends SessionPersistence implements Persi /** Read a stored prefix by id across ALL cwd buckets (cwd unknown). */ async loadStored(id: SessionId): Promise | undefined> { - return this.readPrefix(id, undefined) - } - - /** Read a stored prefix SCOPED to `cwd` (HMR live-adoption must not cross cwd). */ - async loadLive(id: SessionId, cwd: string | undefined): Promise | undefined> { - return this.readPrefix(id, cwd) + const file = await this.findLog(id) + if (file === undefined) return undefined + return this.readPrefix(file.path) } /** - * Read and scan a session's log into a {@link StoredPrefix}. Folds the + * Read a stored prefix SCOPED to `cwd` (HMR live-adoption must not cross cwd). + * `undefined` is the DEFINITE "no-cwd" bucket, NOT "unknown" — a live session + * with no cwd may only adopt a persisted no-cwd log, never a same-id log that + * lives in some other cwd bucket. So this looks at exactly `logPath(cwd)` + * (which maps `undefined` → the `_no-cwd` bucket), never the all-buckets scan. + */ + async loadLive(id: SessionId, cwd: string | undefined): Promise | undefined> { + const path = logPath(this.root, cwd, id) + if (!await this.exists(path)) return undefined + return this.readPrefix(path) + } + + /** + * Read and scan a session's log file into a {@link StoredPrefix}. Folds the * torn-tail comparison HERE so the `tornMarker` is the byte offset to truncate * to (or `undefined` when nothing is torn) — the coordinator never sees the * raw byteLength. */ - private async readPrefix(id: SessionId, cwd: string | undefined): Promise | undefined> { - const file = await this.findLog(id, cwd) - if (file === undefined) return undefined - const buffer = await readFile(file.path) + private async readPrefix(path: string): Promise> { + const buffer = await readFile(path) const { meta, events, committedBytes } = scanLog(buffer) return { meta, @@ -174,7 +182,7 @@ export class SessionPersistenceJsonl extends SessionPersistence implements Persi /** Remove a session's log file (the coordinator clears its in-memory state). */ async deleteStored(id: SessionId): Promise { - const file = await this.findLog(id, undefined) + const file = await this.findLog(id) if (file) await rm(file.path, { force: true }) } @@ -331,13 +339,13 @@ export class SessionPersistenceJsonl extends SessionPersistence implements Persi } } - /** Find a session's log file across cwd buckets (when cwd is unknown). */ - private async findLog(id: SessionId, cwd: string | undefined): Promise<{ path: string; cwd: string | undefined } | undefined> { - if (cwd !== undefined) { - const path = logPath(this.root, cwd, id) - return (await this.exists(path)) ? { path, cwd } : undefined - } - // Unknown cwd: scan buckets for a matching file name. + /** + * Find a session's log file by id across ALL cwd buckets — the any-cwd scan + * for `loadStored`/`deleteStored` (resume and removal identify a session by id + * alone). The cwd-scoped lookup (`loadLive`) does NOT use this; it goes + * straight to `logPath(cwd)` so a no-cwd session can't match a real-cwd bucket. + */ + private async findLog(id: SessionId): Promise<{ path: string; cwd: string | undefined } | undefined> { const target = encodeSegment(id) + '.jsonl' for (const dir of await this.listCwdDirs()) { const path = `${dir}/${target}` diff --git a/packages/session-persistence-jsonl/tests/jsonl.spec.ts b/packages/session-persistence-jsonl/tests/jsonl.spec.ts index b48bb249d1..d36723f396 100644 --- a/packages/session-persistence-jsonl/tests/jsonl.spec.ts +++ b/packages/session-persistence-jsonl/tests/jsonl.spec.ts @@ -484,6 +484,41 @@ describe('SessionPersistenceJsonl: edge cases', () => { await expect(backend.inits.get(b)).rejects.toThrow(/already bound to a different live session|already has a persisted log on disk/) }) + it('a NO-CWD live session does NOT cross-cwd-adopt a same-id log from a real cwd bucket (loadLive is scope-exact)', async () => { + // Backend 1: materialize a log under id "x" in the cwd "/w" bucket, then + // dispose the WHOLE backend (so backend 2 mounts with an EMPTY states map — + // the HMR/reload path where onCreated goes through loadLive, not a tracked + // collision). + await ctx.sessionPersistence.create(meta('x', '/w')) + await ctx.sessionPersistence.append(SessionId('x'), oneTurnLog()) + await ctx.fiber.dispose() + + // Backend 2 over the SAME root. A live no-cwd session reuses id "x". Because + // loadLive(id, undefined) is the DEFINITE no-cwd bucket (NOT an all-buckets + // scan), case-2 adoption does NOT match the "/w" log — so it would NOT + // silently graft the no-cwd events onto the "/w" log with a mismatched cwd + // (the bug a non-scope-exact loadLive caused). It falls through to the + // new-session path, where createCore's any-cwd collision probe (loadStored) + // catches the duplicate id and REJECTS — the id is taken in another bucket. + const ctx2 = new Context() + await ctx2.plugin(SessionStore) + await ctx2.plugin(SessionPersistenceJsonl, { root }) + const backend = ctx2.sessionPersistence as unknown as { inits: Map> } + let b!: Session + await ctx2.plugin(Object.assign((inner: Context) => { + b = inner.sessions.create('x') // no cwd + }, { inject: ['sessions'] })) + await expect(backend.inits.get(b)).rejects.toThrow(/already has a persisted log on disk/) + + // The "/w" log is untouched — no no-cwd events were grafted onto it, and no + // `_no-cwd` log for "x" was created. + const inW = scanLog(await readFile(logPath(root, '/w', SessionId('x')))) + expect(inW.meta.cwd).toBe('/w') + expect(inW.events).toHaveLength(6) + await expect(stat(logPath(root, undefined, SessionId('x')))).rejects.toThrow() + await ctx2.fiber.dispose() + }) + it('a seed with matching seq/type/time but DIFFERENT data is rejected (deep prefix compare)', async () => { // Materialize and load (ownerless, cursor = 6). await ctx.sessionPersistence.create(meta('divergent', '/a')) @@ -549,7 +584,7 @@ describe('SessionPersistenceJsonl: edge cases', () => { // open() must surface, not be collapsed to "not found" (which would let a // collision check proceed under a false absence assumption). A LAZY session // (created, never appended) keeps its cwd in state, so has() reaches - // findLog(id, cwd) → exists(logPath). Make that cwd's bucket DIRECTORY a + // loadLive(id, cwd) → exists(logPath). Make that cwd's bucket DIRECTORY a // regular file: open()ing `bucket/.jsonl` under it then fails ENOTDIR. const cwd = '/x' const ctx2 = new Context() diff --git a/packages/session-persistence/README.md b/packages/session-persistence/README.md index a63ea6284a..3ec9f7860b 100644 --- a/packages/session-persistence/README.md +++ b/packages/session-persistence/README.md @@ -35,7 +35,7 @@ The `PersistenceBackend` hooks (the only seam between the coordinato | `loadStored(id)` | Read a stored prefix by id, scanning ANY storage scope. Used by resume/load and, via `!== undefined`, the create-collision probe. Returns an opaque `tornMarker` iff a torn tail must be truncated. | | `loadLive(id, cwd)` | Read a stored prefix SCOPED to `cwd` (HMR live-adoption must only adopt a log at the SAME cwd; a same-id log elsewhere is a collision, not a resume). A globally-unique-id backend ignores `cwd`. | | `appendBatch(meta, events, isMaterialized)` | Durably append a contiguous batch, lazily materializing ATOMICALLY when not yet materialized. | -| `commitRepair(meta, tornMarker, closers)` | Make a crash repair durable: truncate the torn tail (iff `tornMarker`) and append `closers`. NOT required to be atomic. Used by load (truncate + closers) and live-adoption (truncate only). | +| `commitRepair(meta, tornMarker, closers)` | Make a crash repair durable: truncate the torn tail (iff `tornMarker !== undefined` — a marker may be falsy, e.g. seq/offset `0`) and append `closers`. NOT required to be atomic. Used by load (truncate + closers) and live-adoption (truncate only). | | `deleteStored(id)` / `list()` | Remove a stored artifact / list all stored metadata. | | `close?()` | Optional lifecycle teardown (e.g. close a db handle), awaited after the dispose drain. | diff --git a/packages/session-persistence/src/coordinator.ts b/packages/session-persistence/src/coordinator.ts index 09bc6eb943..f35685c677 100644 --- a/packages/session-persistence/src/coordinator.ts +++ b/packages/session-persistence/src/coordinator.ts @@ -287,13 +287,15 @@ export class PersistenceCoordinator { async has(id: SessionId): Promise { const state = this.states.get(id) if (state?.materialized) return true - // Probe storage scoped to the tracked cwd if known, else any scope. A tracked - // lazy session has a known cwd, so loadLive(id, cwd) hits the exact artifact - // path — a storage fault there (e.g. a non-ENOENT lookup error) must surface, - // not be masked by an any-scope scan that filters a non-directory bucket out. - // For an untracked id `cwd` is undefined, where loadLive scans any scope (= - // loadStored), so this single call covers both. - return (await this.backend.loadLive(id, state?.meta.cwd)) !== undefined + // A TRACKED lazy session has a known cwd: probe that exact bucket via + // loadLive(id, cwd) — including the no-cwd bucket when its cwd is undefined. + // An UNTRACKED id has a genuinely UNKNOWN cwd, so it must scan ANY scope via + // loadStored — loadLive(id, undefined) would (correctly) look ONLY in the + // no-cwd bucket and miss a materialized session that lives in a real cwd. + const probe = state !== undefined + ? await this.backend.loadLive(id, state.meta.cwd) + : await this.backend.loadStored(id) + return probe !== undefined } /** Remove a session and all its persisted artifacts. */