From 4a0e2f115ad46d34c19d200c6a71fa79d264092d Mon Sep 17 00:00:00 2001 From: David Brailovsky Date: Fri, 7 Aug 2026 13:47:01 +0200 Subject: [PATCH] fix(agent): complete JSONL crash and corruption handling --- packages/agent/docs/harness-v2-test-matrix.md | 16 +-- packages/agent/docs/harness-v2.md | 4 +- .../agent/src/harness/session/jsonl/repo.ts | 11 +- .../agent/test/harness/session/jsonl.test.ts | 113 +++++++++++++++++- 4 files changed, 125 insertions(+), 19 deletions(-) diff --git a/packages/agent/docs/harness-v2-test-matrix.md b/packages/agent/docs/harness-v2-test-matrix.md index 383d6eb1c..8df60635a 100644 --- a/packages/agent/docs/harness-v2-test-matrix.md +++ b/packages/agent/docs/harness-v2-test-matrix.md @@ -16,10 +16,10 @@ No production or test changes are part of QA1. | Area | Removed cases | Status | |---|---:|---| | Harness runtime and stream behavior | 37 | Mostly uncovered by design while `AgentHarness` is scaffolded; assigned to H/L/I/C/N packages. Scaffold-safe configuration is covered by F0. | -| Branch query and corruption behavior | 6 | Core query semantics are covered; bounded SQLite validation gaps were ported by QA2, and remaining JSONL corruption gaps are assigned to J3. | +| Branch query and corruption behavior | 6 | Core query semantics are covered; bounded SQLite validation gaps were ported by QA2, and JSONL corruption behavior is covered by J3. | | Compaction helper behavior | 2 | Covered by current compaction/context tests. | | Memory/SQLite v4 conformance entrypoints | 3 | Ported to `packages/agent/test/harness/session/*` and `packages/session-backends/sqlite-node/test/conformance.test.ts`. | -| Repository/backend lifecycle and JSONL behavior | 38 | Most covered by v4 conformance or J0–J2; QA2 lifecycle/query audits are resolved, with remaining crash/corruption/v3 gaps assigned to J3–J5. | +| Repository/backend lifecycle and JSONL behavior | 38 | Format-4 lifecycle and crash/corruption behavior are covered by v4 conformance and J0–J3; format-3 normalization and conversion remain assigned to J4–J5. | | Session aggregate/context behavior | 17 | Covered by v4 conformance plus current context tests. | | SQLite search | 1 | Ported to SQLite package search tests; old scanning backend is inapplicable. | @@ -122,7 +122,7 @@ Removed files: | builds context from the branch storage without loading complete history | Inapplicable / Covered | Old branch-storage optimization was deleted; v4 context behavior is covered by `session/context.test.ts`. | | rejects repository operations and session writes after disposal | Covered / Inapplicable | The v4 core `SessionRepo` contract has no disposable state, and the in-memory/JSONL repos do not implement permanent disposal. SQLite disposal is resource release rather than repo poisoning; `packages/session-backends/sqlite-node/test/repository.test.ts` covers the remaining applicable behavior in `closes active sessions when the repository is disposed`, proving active session writes reject after repository disposal. | | supports lexical ownership with await using | Inapplicable | The old test covered permanent disposal on the deleted in-memory repository. The v4 core `SessionRepo` contract has no disposable surface, and memory/JSONL repos do not implement lexical ownership. SQLite `await using` is resource cleanup rather than repo poisoning; active-session closure is covered by `closes active sessions when the repository is disposed` in `packages/session-backends/sqlite-node/test/repository.test.ts`. | -| serializes conflicting create and fork destinations | Uncovered / J3 | The old test covered JSONL backend-wide serialization for concurrent create/create and create/fork operations targeting the same id. V4 intentionally removed global repository serialization, but the remaining format-4 lifecycle/concurrency question is whether conflicting destination creation can duplicate files or silently overwrite; assign to J3 lifecycle/concurrency edge cases. | +| serializes conflicting create and fork destinations | Covered | JSONL tests cover concurrent create/create, create/fork, and fork/fork operations targeting the same destination, plus reservation release after failed operations. | | encodes custom session IDs used in filenames | Covered | J2 JSONL repository lifecycle validates file-safe ids; `jsonl.test.ts` rejects invalid coding-agent filenames. | | allows appends to different sessions to run concurrently | Covered | J2/v4 repository conformance and JSONL concurrent write tests cover accepted concurrent writes without the old keyed queue. | | caps concurrent operations across JSONL sessions at four by default | Inapplicable | Old JSONL keyed-operation-queue implementation detail was deleted. | @@ -135,8 +135,8 @@ Removed files: | waits for accepted appends before disposal and rejects later writes | Inapplicable | The old test covered deleted JSONL repository disposal: drain accepted appends, enter a permanent disposed state, then reject later writes through existing sessions. V4 JSONL repos are not disposable, do not retain opened storages, and have no repo-level closed state. Per-session append serialization remains covered by backend conformance `linearizes concurrent writes across two lanes` and JSONL-specific `persists concurrent cross-lane writes in shared sequence order`; close/drain/reject-after-close semantics belong to harness H5/O3, not `SessionRepo` disposal. | | parses once when opened and retains state across appends | Inapplicable | Old JSONL in-memory aggregate implementation detail; v4 correctness is covered by reopen/shared-sequence tests. | | collects sessions below encoded cwd directories and lists by cwd | Covered | J2 metadata lifecycle and listing tests cover v4 JSONL metadata and cwd filtering. | -| fails loudly when listing a malformed session file | Uncovered | J3 owns JSONL crash/corruption behavior for malformed files. | -| rejects a missing active leaf when opened | Uncovered | J3 owns JSONL missing-reference rejection. SQLite equivalent is covered in `repository.test.ts`. | +| fails loudly when listing a malformed session file | Inapplicable / Covered | Section 13 now requires best-effort listing: malformed headers are skipped without opening or replaying the session. JSONL tests cover skipping the malformed file while retaining valid results; direct `open()` still rejects it. | +| rejects a missing active leaf when opened | Covered | JSONL and SQLite tests cover missing-reference rejection. | | opens, deletes, and forks by metadata (JSONL) | Covered | J2 JSONL repo conformance. | | persists header metadata through create, list, and fork | Covered | J0 codec and J2 repository metadata tests. | | repository disposal closes its owned storage | Covered / Inapplicable | Old in-memory repo disposal is inapplicable because v4 memory/JSONL repos are not disposable and do not own returned session storage lifetimes. SQLite is the only disposable repository because it owns DB/lease resources; active-session closure is covered by `closes active sessions when the repository is disposed`, and DB close behavior is covered by existing SQLite connection lifecycle tests. | @@ -145,10 +145,10 @@ Removed files: | includes assistant and summary usage in statistics | Covered | v4 conformance `keeps latest-value facts and computes ledger statistics across lanes`, JSONL storage, and SQLite repository statistics tests. | | stops branch traversal at retained-tail compaction | Covered / Inapplicable | Branch-query stop semantics are still required outside context projection and are covered explicitly by backend conformance `supports bounded filtered and cursor-based queries` via `findEntriesOnBranch({ stopAtType: "compaction" })` across memory, JSONL, and SQLite. Retained-tail materialization is covered by context test `starts at the latest compaction and materializes its retained tail`. The old implicit `getBranch()` auto-stop-at-retained-tail-compaction behavior is inapplicable because v4 uses explicit branch bounds plus context projection. | | writes headers and entries and reopens the aggregate | Covered | J1/J2 JSONL storage/repository tests. | -| fails loudly for malformed headers and entries | Covered / J3 | J3 owns malformed physical file behavior; current JSONL tests already cover malformed tail/middle lines. | +| fails loudly for malformed headers and entries | Covered | Direct JSONL opens reject malformed headers and complete invalid mutations without modifying the file; only a torn final JSON fragment is repaired. Listing separately skips malformed headers as required by section 13. | | enforces entry uniqueness and does not recreate deleted files | Covered | v4 conformance rejects duplicate ids; J2 lifecycle covers delete/reopen behavior. | | scopes entry uniqueness to the session path | Covered | v4 repository/session isolation conformance. | -| rejects non-object header metadata | Uncovered | J3/J4 should cover malformed JSONL header metadata for format-4 and v3 normalization. | +| rejects non-object header metadata | Covered | Format-4 open rejects non-object header metadata, while listing skips that malformed file. Format-3 normalization remains assigned to J4. | ## Session aggregate and context tests @@ -187,7 +187,7 @@ Removed case from `packages/agent/test/harness/sqlite-node.test.ts`. These packages must land before QA3 can re-evaluate the uncovered rows above. They do not use this matrix as their test plan. - **QA2**: completed storage/query audit and ports for bounded-query corruption/validation behavior, repository/session disposal lifecycle, listing/disposal barriers, and branch-query retained-tail semantics outside context projection. -- **J3**: JSONL malformed file, torn-tail, missing-reference, and lifecycle/concurrency edge cases. +- **J3**: completed JSONL malformed-file, torn-tail, missing-reference, and lifecycle/concurrency coverage. - **J4/J5**: v3 read-only normalization and first-write conversion; include malformed v3/header metadata cases. - **I1/I2/I3/I4/L1-L3**: hook/event/mutation/effects/loop primitive coverage required before runtime harness tests can return. - **H1-H8**: durable run, queue, configuration, wait/abort, tool, recovery, and deferred-provider runtime behavior formerly covered by legacy `agent-harness*.test.ts`. diff --git a/packages/agent/docs/harness-v2.md b/packages/agent/docs/harness-v2.md index f12f4647a..b88e5678b 100644 --- a/packages/agent/docs/harness-v2.md +++ b/packages/agent/docs/harness-v2.md @@ -1660,7 +1660,7 @@ interface JsonlSessionListOptions { cwd?: string; } A v3 `parentSession` path resolves to the parent header's id when that file is available. If it is unavailable, metadata retains `legacyParentSessionPath`; first-write conversion preserves that optional header field rather than silently dropping the relationship. Format-4 code uses `parentSessionId` for repository relationships. `modifiedAt` is read from the filesystem and is not a sequenced session mutation. -The repository layout matches coding-agent v3. Under `sessionsRoot`, each resolved cwd uses a directory named `--${resolvedCwd.replace(/^[/\\]/, "").replace(/[/\\:]/g, "-")}--`. New files are named `${createdAtIso.replace(/[:.]/g, "-")}_${sessionId}.jsonl`. `list({ cwd })` scans that cwd's directory; `list()` scans every direct child directory. First-write v3 conversion replaces the original file in place and never changes its directory or filename. +The repository layout matches coding-agent v3. Under `sessionsRoot`, each resolved cwd uses a directory named `--${resolvedCwd.replace(/^[/\\]/, "").replace(/[/\\:]/g, "-")}--`. New files are named `${createdAtIso.replace(/[:.]/g, "-")}_${sessionId}.jsonl`. `list({ cwd })` scans that cwd's directory; `list()` scans every direct child directory. Listing reads only each file's header and filesystem metadata; it does not open or replay the session. A file with a missing or malformed header is omitted from the result. First-write v3 conversion replaces the original file in place and never changes its directory or filename. One file per session: a header line, then one JSON object per line, in `seq` order. Every logical mutation is exactly one line; a line is the atomic unit. @@ -3265,7 +3265,7 @@ These packages own `packages/agent/src/harness/session/jsonl/**`, the concrete ` - [x] **J2 — format-4 repository lifecycle and forks.** Dependencies: J1. - Add create/open/list/delete, one writer queue per session, metadata ordering/filtering, branch/tree forks, and the concrete public `JsonlSessionRepo` export. - Acceptance: the complete backend-neutral conformance suite passes against JSONL, including concurrent lane writes and forks. -- [ ] **J3 — format-4 crash and corruption behavior.** Dependencies: J2. +- [x] **J3 — format-4 crash and corruption behavior.** Dependencies: J2. - Add torn-tail truncation, malformed-interior rejection, missing-reference rejection, and lifecycle/concurrency edge cases. - Acceptance: acknowledged writes survive reopen and malformed non-tail data is never silently repaired. - [ ] **J4 — read-only v3 normalization.** Dependencies: J3. diff --git a/packages/agent/src/harness/session/jsonl/repo.ts b/packages/agent/src/harness/session/jsonl/repo.ts index b683a1bab..46e7f006f 100644 --- a/packages/agent/src/harness/session/jsonl/repo.ts +++ b/packages/agent/src/harness/session/jsonl/repo.ts @@ -2,7 +2,7 @@ import { uuidv7 } from "@earendil-works/pi-ai"; import { assertJsonSerializable, Session } from "../session.ts"; import { type ForkOptions, SessionError, type SessionRepo } from "../types.ts"; import { metadataFromHeader, parseHeader } from "./codec.ts"; -import { fileResult, invalidFile } from "./errors.ts"; +import { fileResult } from "./errors.ts"; import { JsonlSessionStorage } from "./storage.ts"; import type { JsonlSessionCreateOptions, @@ -167,8 +167,13 @@ export class JsonlSessionRepo await this.fs.readTextLines(file.path, { maxLines: 1 }), `Failed to read session header ${file.path}`, ); - if (!firstLine) throw invalidFile(file.path, 1, "is missing a header"); - metadata.push(metadataFromHeader(parseHeader(firstLine, file.path), file.path, file.mtimeMs)); + if (!firstLine) continue; + try { + metadata.push(metadataFromHeader(parseHeader(firstLine, file.path), file.path, file.mtimeMs)); + } catch (error) { + if (error instanceof SessionError && error.code === "invalid_entry") continue; + throw error; + } } } return metadata.sort((left, right) => right.modifiedAt - left.modifiedAt); diff --git a/packages/agent/test/harness/session/jsonl.test.ts b/packages/agent/test/harness/session/jsonl.test.ts index abbdb8f96..878cebcd6 100644 --- a/packages/agent/test/harness/session/jsonl.test.ts +++ b/packages/agent/test/harness/session/jsonl.test.ts @@ -122,6 +122,41 @@ describe("JSONL v4 persistence", () => { expect(await repository.list({ cwd: join(root, "other", "project") })).toEqual([]); }); + it("rejects a malformed JSON header on open and skips it when listing", async () => { + const root = createTempDir(); + const repository = createRepository(root); + await repository.create({ id: "valid", cwd: root }); + const session = await repository.create({ id: "malformed-header", cwd: root }); + const metadata = await session.getMetadata(); + const malformed = "not json\n"; + writeFileSync(metadata.path, malformed); + + await expect(repository.open(metadata)).rejects.toMatchObject({ code: "invalid_entry" }); + expect((await repository.list({ cwd: root })).map((listed) => listed.id)).toEqual(["valid"]); + expect(readFileSync(metadata.path, "utf8")).toBe(malformed); + }); + + it("rejects non-object header metadata on open and skips it when listing", async () => { + const root = createTempDir(); + const repository = createRepository(root); + await repository.create({ id: "valid", cwd: root }); + const session = await repository.create({ id: "invalid-header-metadata", cwd: root }); + const metadata = await session.getMetadata(); + const malformed = `${JSON.stringify({ + kind: "header", + version: 4, + id: metadata.id, + createdAt: metadata.createdAt, + cwd: metadata.cwd, + metadata: "invalid", + })}\n`; + writeFileSync(metadata.path, malformed); + + await expect(repository.open(metadata)).rejects.toMatchObject({ code: "invalid_entry" }); + expect((await repository.list({ cwd: root })).map((listed) => listed.id)).toEqual(["valid"]); + expect(readFileSync(metadata.path, "utf8")).toBe(malformed); + }); + it("rejects session ids that cannot be used in coding-agent filenames", async () => { const root = createTempDir(); const repository = createRepository(root); @@ -145,30 +180,66 @@ describe("JSONL v4 persistence", () => { expect((await repository.list()).map((metadata) => metadata.id)).toEqual(["shared", "shared"]); }); - it("rejects concurrent create calls for the same destination", async () => { + it.each([ + ["create", "create"], + ["create", "fork"], + ["fork", "fork"], + ] as const)("rejects concurrent %s and %s calls for the same destination", async (firstKind, secondKind) => { vi.useFakeTimers({ toFake: ["Date"] }); vi.setSystemTime(new Date("2026-01-01T00:00:00.000Z")); try { const root = createTempDir(); const repository = createRepository(root); const cwd = join(root, "workspace"); + const source = await repository.create({ id: "source", cwd }); + const sourceMetadata = await source.getMetadata(); + const run = (kind: "create" | "fork") => + kind === "create" + ? repository.create({ id: "same", cwd }) + : repository.fork(sourceMetadata, { id: "same", cwd }); - const results = await Promise.allSettled([ - repository.create({ id: "same", cwd }), - repository.create({ id: "same", cwd }), - ]); + const results = await Promise.allSettled([run(firstKind), run(secondKind)]); const successes = results.flatMap((result) => (result.status === "fulfilled" ? [result.value] : [])); const failures = results.flatMap((result) => (result.status === "rejected" ? [result.reason] : [])); expect(successes).toHaveLength(1); expect(failures).toHaveLength(1); expect(failures[0]).toMatchObject({ code: "already_exists" }); - expect((await repository.list({ cwd })).map((listed) => listed.id)).toEqual(["same"]); + expect((await repository.list({ cwd })).filter((listed) => listed.id === "same")).toHaveLength(1); } finally { vi.useRealTimers(); } }); + it.each(["create", "fork"] as const)("releases a destination reservation after a failed %s", async (kind) => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + const repository = new JsonlSessionRepo({ fs: env, sessionsRoot: root }); + const cwd = join(root, "workspace"); + const source = await repository.create({ id: "source", cwd }); + const sourceMetadata = await source.getMetadata(); + const run = () => + kind === "create" + ? repository.create({ id: "retry", cwd }) + : repository.fork(sourceMetadata, { id: "retry", cwd }); + + if (kind === "create") { + vi.spyOn(env, "writeFile").mockResolvedValueOnce({ + ok: false, + error: new FileError("unknown", "injected creation failure"), + }); + } else { + vi.spyOn(env, "renameFile").mockResolvedValueOnce({ + ok: false, + error: new FileError("unknown", "injected fork failure"), + }); + } + + await expect(run()).rejects.toMatchObject({ code: "storage" }); + await expect(run()).resolves.toBeDefined(); + expect((await repository.list({ cwd })).filter((listed) => listed.id === "retry")).toHaveLength(1); + }); + it("sorts listed sessions by current filesystem modification time", async () => { const root = createTempDir(); const repository = createRepository(root); @@ -415,6 +486,15 @@ describe("JSONL v4 persistence", () => { expect((await reopened.getEntry(appendedId))?.seq).toBe(2); }); + it("rejects a complete invalid final mutation without modifying the file", async () => { + const root = createTempDir(); + const metadata = writeRawSession(root, "invalid-final-mutation", [{ kind: "unknown", seq: 1 }]); + const corrupted = readFileSync(metadata.path, "utf8"); + + await expect(createRepository(root).open(metadata)).rejects.toMatchObject({ code: "invalid_entry" }); + expect(readFileSync(metadata.path, "utf8")).toBe(corrupted); + }); + it("rejects a malformed middle line without modifying the file", async () => { const root = createTempDir(); const repository = createRepository(root); @@ -662,4 +742,25 @@ describe("JSONL v4 persistence", () => { expect(readFileSync(metadata.path, "utf8")).toBe(original); expect(existsSync(`${metadata.path}.tmp`)).toBe(false); }); + + it("preserves the session when torn-tail repair cannot be published", async () => { + const root = createTempDir(); + const repository = createRepository(root); + const session = await repository.create({ id: "repair-rename-failure", cwd: root }); + const metadata = await session.getMetadata(); + await session.appendCustomEntry("kept"); + appendFileSync(metadata.path, '{"kind":"entry"'); + const original = readFileSync(metadata.path, "utf8"); + + const env = new NodeExecutionEnv({ cwd: root }); + vi.spyOn(env, "renameFile").mockResolvedValueOnce({ + ok: false, + error: new FileError("unknown", "injected repair rename failure"), + }); + const failingRepository = new JsonlSessionRepo({ fs: env, sessionsRoot: root }); + + await expect(failingRepository.open(metadata)).rejects.toMatchObject({ code: "storage" }); + expect(readFileSync(metadata.path, "utf8")).toBe(original); + expect(existsSync(`${metadata.path}.tmp`)).toBe(false); + }); });