fix(session-persistence-jsonl): make loadLive cwd-scope-exact (Codex review)
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).
This commit is contained in:
@@ -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<StoredPrefix<number> | 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<StoredPrefix<number> | 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<StoredPrefix<number> | 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<StoredPrefix<number> | 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<StoredPrefix<number>> {
|
||||
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<void> {
|
||||
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}`
|
||||
|
||||
@@ -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<Session, Promise<void>> }
|
||||
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/<id>.jsonl` under it then fails ENOTDIR.
|
||||
const cwd = '/x'
|
||||
const ctx2 = new Context()
|
||||
|
||||
@@ -35,7 +35,7 @@ The `PersistenceBackend<TornMarker>` 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. |
|
||||
|
||||
|
||||
@@ -287,13 +287,15 @@ export class PersistenceCoordinator<TornMarker = unknown> {
|
||||
async has(id: SessionId): Promise<boolean> {
|
||||
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. */
|
||||
|
||||
Reference in New Issue
Block a user