Merge pull request #83 from deepseek-ai/worktree-simplify-prune-seam
simplify(seams): prune dead methods from the persistence seam
This commit is contained in:
Tianyi Cui
GitHub
@@ -369,8 +369,6 @@ abstract create(meta: SessionHeader): Promise<void>
|
||||
abstract append(id: SessionId, events: readonly SessionEvent[]): Promise<void>
|
||||
abstract load(id: SessionId): Promise<{ meta: SessionHeader; events: SessionEvent[] }>
|
||||
abstract list(): Promise<SessionHeader[]>
|
||||
abstract has(id: SessionId): Promise<boolean>
|
||||
abstract delete(id: SessionId): Promise<void>
|
||||
```
|
||||
|
||||
Types: [SessionEvent](../core-data-structures/core.md)
|
||||
|
||||
@@ -2,7 +2,7 @@
|
||||
|
||||
The **durability seam** for the event log. [session.md](session.md) describes the in-memory `Session` — the append-only `SessionEvent` log that is the source of truth. This page describes how that log is made durable: the abstract `SessionPersistence` service, its backends, the flush checkpoint, crash recovery, and the metadata header that travels alongside the log.
|
||||
|
||||
The seam is a textbook [capability seam](../rfc/implemented/architecture/2026-06-13-capability-seams.md): one abstract service ([dsh-session-persistence](../../packages/session-persistence/session-persistence), `ctx.sessionPersistence`) defining create/append/load/list/has/delete over the existing `SessionEvent` — **no parallel persisted type** — and two interchangeable backends that pass the same `runPersistenceContract` suite. See the [session-persistence RFC](../rfc/implemented/architecture/2026-06-14-session-persistence.md).
|
||||
The seam is a textbook [capability seam](../rfc/implemented/architecture/2026-06-13-capability-seams.md): one abstract service ([dsh-session-persistence](../../packages/session-persistence/session-persistence), `ctx.sessionPersistence`) defining create/append/load/list over the existing `SessionEvent` — **no parallel persisted type** — and two interchangeable backends that pass the same `runPersistenceContract` suite. See the [session-persistence RFC](../rfc/implemented/architecture/2026-06-14-session-persistence.md).
|
||||
|
||||
## The flush checkpoint
|
||||
|
||||
@@ -55,7 +55,7 @@ Replay/fork is therefore `ctx.sessions.create(id, { seed: seedEvents })`; resumi
|
||||
|
||||
## The backends
|
||||
|
||||
Both implement the same abstract `SessionPersistence` (create/append/load/list/has/delete over `SessionEvent`) and pass `runPersistenceContract`, proving the seam is genuinely backend-agnostic:
|
||||
Both implement the same abstract `SessionPersistence` (create/append/load/list over `SessionEvent`) and pass `runPersistenceContract`, proving the seam is genuinely backend-agnostic:
|
||||
|
||||
- **[dsh-session-persistence-jsonl](../../packages/session-persistence/session-persistence-jsonl)** — an append-only JSONL log per session with crash-safe atomic writes, the interrupted-turn crash recovery above, and a read/replay path.
|
||||
- **[dsh-session-persistence-sqlite](../../packages/session-persistence/session-persistence-sqlite)** — `node:sqlite`, one row per `SessionEvent`. The row shape `(session_id, seq, type, time, data)` maps 1:1 onto the event, so there is no parallel persisted schema to keep in sync.
|
||||
|
||||
+1
-1
@@ -52,7 +52,6 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r
|
||||
| [Unify the agent id and the session id](proposed/simplification/2026-06-20-unify-agent-and-session-id.md) | 2026-06-20 |
|
||||
| [Stop mirroring durable boundaries as agent events](proposed/simplification/2026-06-20-remove-agent-boundary-mirror-events.md) | 2026-06-20 |
|
||||
| [Keep one public stop primitive](proposed/simplification/2026-06-20-public-agent-stop-surface.md) | 2026-06-20 |
|
||||
| [Prune dead methods from the persistence and bash seams](proposed/simplification/2026-06-20-prune-dead-seam-methods.md) | 2026-06-20 |
|
||||
| [Fold trace-only session facts into load-bearing events](proposed/simplification/2026-06-20-collapse-trace-only-session-events.md) | 2026-06-20 |
|
||||
|
||||
### Architecture
|
||||
@@ -96,6 +95,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r
|
||||
| [Drop the mutable session summary](implemented/simplification/2026-06-19-drop-mutable-session-summary.md) | 2026-06-19 |
|
||||
| [Drop unconsumed assembled LLM convenience surfaces](implemented/simplification/2026-06-20-drop-unconsumed-llm-assembled-surfaces.md) | 2026-06-20 |
|
||||
| [Drop the unconsumed `llm/adapter-change` event](implemented/simplification/2026-06-20-drop-unconsumed-llm-adapter-change-event.md) | 2026-06-20 |
|
||||
| [Prune dead methods from the persistence seam](implemented/simplification/2026-06-20-prune-dead-seam-methods.md) | 2026-06-20 |
|
||||
|
||||
### Architecture
|
||||
|
||||
|
||||
@@ -16,7 +16,7 @@ The [event-sourced model](2026-06-11-event-sourced-sessions.md) makes the append
|
||||
|
||||
Persistence is an abstract **capability seam** ([capability seams](2026-06-13-capability-seams.md), the `dsh-bash` template), not loop or core logic:
|
||||
|
||||
1. **Interface** (`dsh-session-persistence`, `ctx.sessionPersistence`) — an abstract `SessionPersistence` service: `create`/`append`/`load`/`list`/`has`/`delete`. Its persisted unit IS the existing `SessionEvent` (`{ type, seq, time, data }`), reused verbatim — no conversion type.
|
||||
1. **Interface** (`dsh-session-persistence`, `ctx.sessionPersistence`) — an abstract `SessionPersistence` service: `create`/`append`/`load`/`list`. Its persisted unit IS the existing `SessionEvent` (`{ type, seq, time, data }`), reused verbatim — no conversion type.
|
||||
2. **Implementation** (`dsh-session-persistence-jsonl`) — an append-only JSONL log per session (a `SessionHeader` line then one `SessionEvent` per line, verbatim **including `assistant/chunk`**).
|
||||
|
||||
Key choices recorded here because they are durable, contested, and surprising:
|
||||
|
||||
+6
-6
@@ -4,24 +4,24 @@ Status: implemented (proposed and accepted 2026-06-18, implemented 2026-06-20)
|
||||
|
||||
## Problem
|
||||
|
||||
`dsh-session-persistence-jsonl` and `dsh-session-persistence-sqlite` intentionally prove the same `SessionPersistence` contract over different storage media, but their write-path orchestration was duplicated: per-session state, `session/created` adoption, backend-specific prefix reads, write-behind buffers, serialized flush chains, HMR seeding, and dispose drains. The pure seed-prefix collision and serializability guards had already moved into the seam package; the remaining orchestration was still correctness-heavy and received the same fixes twice. A code-level diff showed the two backends were byte-identical — or same-algorithm — for ALL of it: the four maps (`states`/`buffers`/`chains`/`inits`), `installWritePath`, `initFor`, `onCreated`'s four cases, `flush`, `drain`, `serialize`, `adopt`, `adoptLivePrefix`, `assertVersion`, and the `create`/`append`/`load`/`has`/`delete` skeletons. Only the storage primitives (write bytes vs. INSERT rows) differed.
|
||||
`dsh-session-persistence-jsonl` and `dsh-session-persistence-sqlite` intentionally prove the same `SessionPersistence` contract over different storage media, but their write-path orchestration was duplicated: per-session state, `session/created` adoption, backend-specific prefix reads, write-behind buffers, serialized flush chains, HMR seeding, and dispose drains. The pure seed-prefix collision and serializability guards had already moved into the seam package; the remaining orchestration was still correctness-heavy and received the same fixes twice. A code-level diff showed the two backends were byte-identical — or same-algorithm — for ALL of it: the four maps (`states`/`buffers`/`chains`/`inits`), `installWritePath`, `initFor`, `onCreated`'s four cases, `flush`, `drain`, `serialize`, `adopt`, `adoptLivePrefix`, `assertVersion`, and the `create`/`append`/`load` skeletons. Only the storage primitives (write bytes vs. INSERT rows) differed.
|
||||
|
||||
## Decision
|
||||
|
||||
Extract a backend-agnostic `PersistenceCoordinator` into `dsh-session-persistence`. The coordinator owns the orchestration once; each first-party backend composes one (`new PersistenceCoordinator(ctx, this)`), implements a small `PersistenceBackend` hook interface, and delegates its six public service methods (`create`/`append`/`load`/`list`/`has`/`delete`) to it.
|
||||
Extract a backend-agnostic `PersistenceCoordinator` into `dsh-session-persistence`. The coordinator owns the orchestration once; each first-party backend composes one (`new PersistenceCoordinator(ctx, this)`), implements a small `PersistenceBackend` hook interface, and delegates its four public service methods (`create`/`append`/`load`/`list`) to it.
|
||||
|
||||
Composition, not inheritance. The coordinator is a concrete class the backend holds, not a base class the backend extends. The RFC's risk — "a coordinator must not make unusual backends fight an inheritance hierarchy" — is avoided: a backend exposes only the hooks; it cannot reach the coordinator's private orchestration state, and the public `SessionPersistence` service shape is unchanged, so a third-party backend MAY still implement the abstract service directly without the coordinator at all.
|
||||
|
||||
### The hook interface (`PersistenceBackend<TornMarker>`)
|
||||
|
||||
Seven methods (six required + an optional lifecycle hook) — the only seam between the coordinator and storage:
|
||||
Six methods (five required + an optional lifecycle hook) — the only seam between the coordinator and storage:
|
||||
|
||||
- `name` — backend label for the dispose-failure `AggregateError`.
|
||||
- `loadStored(id)` — read a stored prefix by id, scanning ANY storage scope (every JSONL cwd bucket; SQLite's id is globally unique). Used by resume/load and, via `!== undefined`, the create-collision probe and `has`.
|
||||
- `loadStored(id)` — read a stored prefix by id, scanning ANY storage scope (every JSONL cwd bucket; SQLite's id is globally unique). Used by resume/load and, via `!== undefined`, the create-collision probe.
|
||||
- `loadLive(id, cwd)` — read a stored prefix SCOPED to `cwd`. **Deliberately distinct from `loadStored`**: HMR live-adoption must only adopt a persisted log at the SAME cwd as the live session; a same-id log at a different cwd is a collision, not a resume. Collapsing the two reintroduces a cross-cwd adoption bug. SQLite ignores `cwd`.
|
||||
- `appendBatch(meta, events, isMaterialized)` — durably append a contiguous batch, lazily materializing the session ATOMICALLY when not yet materialized (the materialize-write and the first event batch must commit together — a crash between them must not leave a materialized-but-empty session; this is why there is no separate `materialize` hook).
|
||||
- `commitRepair(meta, tornMarker, closers)` — make a crash repair durable: truncate the torn tail (iff `tornMarker !== undefined`) and append `closers`. **NOT required to be atomic** — JSONL legitimately truncates-then-appends in two fsync'd steps, SQLite does DELETE+INSERT in one transaction. Used by `load` (truncate + synthetic closers) and live-adoption (truncate only, `closers = []`).
|
||||
- `deleteStored(id)` / `list()` — remove a stored artifact / list all stored metadata.
|
||||
- `list()` — list all stored metadata.
|
||||
- `close?()` — optional lifecycle teardown (SQLite closes its db handle; JSONL omits it), awaited in the dispose effect AFTER the quiescence drain so a close failure never masks a drain error.
|
||||
|
||||
### The opaque torn marker
|
||||
@@ -34,4 +34,4 @@ The shared `runPersistenceContract` (public-API contract) keeps running for ever
|
||||
|
||||
## Risks and what we gave up
|
||||
|
||||
The pre-extraction duplication was verbose but explicit — each backend read top-to-bottom. The coordinator adds one indirection (the hook seam) and one new concept (the opaque torn marker). This clears the bar because the centralized logic is the correctness-heavy part that was already being fixed twice, and the hook set is narrow (seven methods, no inheritance). The hook surface was deliberately held to the minimum: `has` and the create-collision probe are NOT separate hooks — they fold into `loadStored(id) !== undefined`; there is no separate `materialize` hook (folded into `appendBatch` for atomicity); `list()` stays a backend method with no coordinator pass-through (listing needs none of the orchestration). The net effect is a reduction: one orchestration copy instead of two, the backends shrank by ~1200 lines of duplicated churn, and a future backend implements ~7 small primitives instead of copying the entire `session/event` → buffer → flush machinery.
|
||||
The pre-extraction duplication was verbose but explicit — each backend read top-to-bottom. The coordinator adds one indirection (the hook seam) and one new concept (the opaque torn marker). This clears the bar because the centralized logic is the correctness-heavy part that was already being fixed twice, and the hook set is narrow (six methods, no inheritance). The hook surface was deliberately held to the minimum: the create-collision probe is NOT a separate hook — it folds into `loadStored(id) !== undefined`; there is no separate `materialize` hook (folded into `appendBatch` for atomicity); `list()` stays a backend method with no coordinator pass-through (listing needs none of the orchestration). The net effect is a reduction: one orchestration copy instead of two, the backends shrank by ~1200 lines of duplicated churn, and a future backend implements a handful of small primitives instead of copying the entire `session/event` → buffer → flush machinery.
|
||||
@@ -0,0 +1,42 @@
|
||||
# RFC: Prune dead methods from the persistence seam
|
||||
|
||||
Status: implemented (proposed and accepted 2026-06-20)
|
||||
|
||||
> **Decision (scope: persistence only).** The shipped change removes the two dead persistence methods `SessionPersistence.has()` and `.delete()`; the body below records that decision. The bash seam's `BashExecutor.get()`/`.list()` were **considered for the same treatment and deliberately kept**: each is a one-line accessor over the executor's already-tracked `tasks` map, and removing them would force `dsh-tool-bash`'s tests onto a ~35-line `onTaskDone`-based completion-tracking harness to replace the one-line `ctx.bash.get(id)` lookup — the migration cost dwarfs the surface removed. Per the [AGENTS.md "RFCs are proposals, not golden truth"](../../../../AGENTS.md) principle, that friction is evidence the method earns its keep: a test harness IS a consumer that programs against the seam, so `get()`/`list()` stay. (`BashTaskId`-branding those surviving methods is taken up by the [branded-ids RFC](../../proposed/architecture/2026-06-20-branded-ids.md).) The persistence removal carries no such cost: `has()`/`delete()` had only contract-test callers and no test-ergonomics consumer to migrate.
|
||||
|
||||
## Problem
|
||||
|
||||
A capability seam ([interface / implementation / consumer](../../implemented/architecture/2026-06-13-capability-seams.md)) carries abstract methods that no consumer calls. The seam exists to let implementations and consumers evolve independently — but a method no consumer programs against is not a seam, it is speculative surface every implementation must still implement and test.
|
||||
|
||||
### `SessionPersistence.has()` and `.delete()`
|
||||
|
||||
The abstract service declared its operations beyond create/append: `load`, `list`, `has`, `delete`. Production consumers of `ctx.sessionPersistence` use only two: the agent-loop resume path calls `load()` ([packages/core/agent-loop/src/index.ts:176](../../../../packages/core/agent-loop/src/index.ts)), and the ACP bridge calls `list()` for `session/list` ([packages/ui/acp/src/index.ts:494](../../../../packages/ui/acp/src/index.ts)). Grepping every `sessionPersistence.*` / `persistence.*` use across `packages/*/src` and `examples/` finds no `has(` and no `delete(` on the service. The `.has(`/`.delete(` calls in `packages/ui/acp/src/index.ts` are on the in-memory `SessionStore` and a local `Set` of loading ids, not persistence. The only callers of `has`/`delete` were the contract suites and per-backend specs.
|
||||
|
||||
`has()` was not just unused — it was the most intricate branch in the shared coordinator: a tracked-vs-untracked dual-probe (`loadLive(id, cwd)` for a live-tracked session vs `loadStored(id)` for an untracked one) with a multi-line rationale. `delete()` dragged the `deleteStored` backend hook that every backend had to implement. This is the [drop-mutable-session-summary](../../implemented/simplification/2026-06-19-drop-mutable-session-summary.md) pattern: a contract test exercised both, but no shipping code asks "is this session persisted?" or removes one.
|
||||
|
||||
## Proposal
|
||||
|
||||
Remove the methods nothing consumes, from the abstract seam, the implementation, and the contract/spec suites that exist only to exercise them:
|
||||
|
||||
- `SessionPersistence.has()` / `.delete()`: delete the abstract declarations, the coordinator's `has`/`delete`/`deleteCore`, and the `PersistenceBackend.deleteStored` hook. Remove the `has`/`delete` rows from the contract suite and the per-backend specs (jsonl + sqlite each implemented `deleteStored` only to satisfy the hook — that implementation goes too). The backends are the [dual-backend](../../implemented/architecture/2026-06-14-session-persistence.md) design and otherwise out of scope, but removing a hook they implement for no consumer is part of removing the hook, not a backend redesign.
|
||||
- Update every doc and source-comment reference to the removed methods — not only literal `has(`/`delete(`/`deleteStored` call spellings, but also `{@link has}`/`{@link delete}` JSDoc links and prose that counts the methods (removing 2 of the persistence service's 6 public methods makes any "six public methods" phrasing wrong). The implementing PR greps `has`/`delete`/`deleteStored`/`{@link `/`six ` across `docs/`, `packages/*/README.md`, and source comments, and fixes each. The known doc sites: the seam README ([packages/session-persistence/session-persistence/README.md](../../../../packages/session-persistence/session-persistence/README.md)'s `has(id)`/`delete(id)` API row and its "delegates its six public service methods" prose → four), the backend READMEs that describe `has`/`list` semantics ([packages/session-persistence/session-persistence-sqlite/README.md](../../../../packages/session-persistence/session-persistence-sqlite/README.md), [packages/session-persistence/session-persistence-jsonl/README.md](../../../../packages/session-persistence/session-persistence-jsonl/README.md) — reword "absent from `has()`/`list()`" to just `list()`), the service-map / seam docs in [docs/architecture.md](../../../architecture.md), and the persistence prose in the [session-persistence RFC](../../implemented/architecture/2026-06-14-session-persistence.md) and [shared write-coordinator RFC](../../implemented/architecture/2026-06-18-shared-persistence-write-coordinator.md). The known source-comment sites: the abstract `create()` JSDoc's `{@link has}/{@link list}` link ([packages/session-persistence/session-persistence/src/index.ts](../../../../packages/session-persistence/session-persistence/src/index.ts) — drop the `has` link), the coordinator's "six public methods"/"six public service methods" module + class JSDoc and its lazy-materialization JSDoc justifying the `materialized` flag by "the signal `has`/`list` rely on" ([packages/session-persistence/session-persistence/src/coordinator.ts](../../../../packages/session-persistence/session-persistence/src/coordinator.ts)), the JSONL backend's `loadStored`/`deleteStored` comment, and the SQLite backend's `schema.ts` and `index.ts` comments that mention "absent from `has`/`list`" — all reworded to the surviving four-method, `list()`-only contract.
|
||||
|
||||
## Why not keep them as "the seam should be complete"?
|
||||
|
||||
The instinct that a persistence seam "should" offer delete is real — and it is exactly the speculative-completeness the pre-release stance warns against ([AGENTS.md](../../../../AGENTS.md): optimize for the correct foundation, not for hypothetical callers you do not have). `delete()` is one method to re-add the day a consumer needs it: a session-management UI that deletes old sessions will want it — add it then, designed against that UI's real needs (soft-delete? cascade? confirmation?), not guessed now.
|
||||
|
||||
Re-adding a seam method with a live consumer is cheap and better-designed than the speculative version, because the consumer pins the contract. Carrying it unused means every implementation (and every future backend) must implement and test a method that does nothing.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `has`/`delete`/`deleteStored` are gone from the persistence seam, impl, and contract suites; `pnpm run knip` reports no new dead exports.
|
||||
- The remaining persistence operations (`create`/`append`/`load`/`list`) are untouched; ACP `session/list` and crash-recovery behave identically.
|
||||
- `pnpm run test:coverage` stays 100% per-file (the contract/spec rows for the removed persistence methods are deleted with them).
|
||||
- The persistence seam README and `docs/architecture.md` no longer list the removed `has`/`delete` methods.
|
||||
|
||||
## Risks
|
||||
|
||||
- **`delete()` is the kind of operation a product eventually wants.** True — but "eventually" is the point. Deleting it now and re-adding it against a real consumer is strictly better than shipping a guessed contract. The dual backends each shed a `deleteStored` impl, which is a bounded edit in otherwise-out-of-scope packages.
|
||||
- **Low coupling.** The removal is confined to the persistence seam + impl + tests; no cross-package consumer references the removed methods, so there is no ripple beyond the docs.
|
||||
|
||||
Modest size, but it converts the seam from "what an implementation must provide for nobody" back to "exactly what a consumer uses."
|
||||
@@ -1,49 +0,0 @@
|
||||
# RFC: Prune dead methods from the persistence and bash capability seams
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
Two capability seams ([interface / implementation / consumer](../../implemented/architecture/2026-06-13-capability-seams.md)) carry abstract methods that no consumer calls. The seam exists to let implementations and consumers evolve independently — but a method no consumer programs against is not a seam, it is speculative surface every implementation must still implement and test.
|
||||
|
||||
### `SessionPersistence.has()` and `.delete()`
|
||||
|
||||
The abstract service declares four operations beyond create/append: `load`, `list`, `has`, `delete` ([packages/session-persistence/session-persistence/src/index.ts:142-151](../../../../packages/session-persistence/session-persistence/src/index.ts)). Production consumers of `ctx.sessionPersistence` use only two of them: the agent-loop resume path calls `load()` ([packages/core/agent-loop/src/index.ts:176-194](../../../../packages/core/agent-loop/src/index.ts)), and the ACP bridge calls `list()` for `session/list` ([packages/ui/acp/src/index.ts](../../../../packages/ui/acp/src/index.ts)). Grepping every `sessionPersistence.*` / `persistence.*` use across `packages/*/src` and `examples/` finds no `has(` and no `delete(` on the service. The `.has(`/`.delete(` calls in `packages/ui/acp/src/index.ts` are on the in-memory `SessionStore` and a local `Set` of loading ids, not persistence. The only callers of `has`/`delete` are the contract suites and per-backend specs.
|
||||
|
||||
`has()` is not just unused — it is the most intricate branch in the shared coordinator: a tracked-vs-untracked dual-probe (`loadLive(id, cwd)` for a live-tracked session vs `loadStored(id)` for an untracked one) with a multi-line rationale ([packages/session-persistence/session-persistence/src/coordinator.ts:298-310](../../../../packages/session-persistence/session-persistence/src/coordinator.ts)). `delete()` drags the `deleteStored` backend hook ([coordinator.ts:99](../../../../packages/session-persistence/session-persistence/src/coordinator.ts), [coordinator.ts:313-319](../../../../packages/session-persistence/session-persistence/src/coordinator.ts)) that every backend must implement. This is the [drop-mutable-session-summary](../../implemented/simplification/2026-06-19-drop-mutable-session-summary.md) pattern: a contract test exercises both, but no shipping code asks "is this session persisted?" or removes one.
|
||||
|
||||
### `BashExecutor.get()` and `.list()`
|
||||
|
||||
The bash seam declares `get(id)` ("look up a background task by id") and `list()` ("all tracked background tasks") ([packages/bash/bash/src/index.ts:88-107](../../../../packages/bash/bash/src/index.ts)), both implemented by `LocalBashExecutor` ([packages/bash/bash-local/src/index.ts:179-191](../../../../packages/bash/bash-local/src/index.ts)). The sole production consumer — `dsh-tool-bash` — drives tasks via `ownerOf`, `onTaskDone`, `start`, `readOutput`, `kill`, `resolve`, `run`; it never calls `get`/`list` in shipping code, and there is no `bash_list` tool exposing a task roster to the model. So both are dead production seam surface. They are used by tests, more broadly than a single idiom: the bash seam/executor specs assert them directly ([packages/bash/bash/tests/service.spec.ts](../../../../packages/bash/bash/tests/service.spec.ts), [packages/bash/bash-local/tests/executor.spec.ts](../../../../packages/bash/bash-local/tests/executor.spec.ts) both call `get()`/`list()`), and several `dsh-tool-bash` tests reach through `ctx.bash.get(id)` to await a task's `done`, read its `status`, or inspect task fields ([packages/bash/tool-bash/tests/tools.spec.ts](../../../../packages/bash/tool-bash/tests/tools.spec.ts), [packages/bash/tool-bash/tests/integration.spec.ts](../../../../packages/bash/tool-bash/tests/integration.spec.ts)). These are test-harness conveniences, not shipping consumers — but they are real test code an implementing PR must migrate or delete.
|
||||
|
||||
## Proposal
|
||||
|
||||
Remove the methods nothing consumes, from the abstract seam, the implementation, and the contract/spec suites that exist only to exercise them:
|
||||
|
||||
- `SessionPersistence.has()` / `.delete()`: delete the abstract declarations, the coordinator's `has`/`delete`/`deleteCore`, and the `PersistenceBackend.deleteStored` hook. Remove the `has`/`delete` rows from the contract suite and the per-backend specs (jsonl + sqlite each implement `deleteStored` only to satisfy the hook — that implementation goes too). The backends are the [dual-backend](../../implemented/architecture/2026-06-14-session-persistence.md) design and otherwise out of scope, but removing a hook they implement for no consumer is part of removing the hook, not a backend redesign.
|
||||
- `BashExecutor.get()` / `.list()`: delete the abstract declarations and the `LocalBashExecutor` impls. The seam/executor specs that assert `get()`/`list()` directly (`bash/tests/service.spec.ts`, `bash-local/tests/executor.spec.ts`) lose those assertions (the behavior is being removed). The `dsh-tool-bash` tests that reach through `ctx.bash.get(id)` to await `done`, read `status`, or inspect task fields switch to the public completion/status seam they should use — `onTaskDone` (or the `done` promise and status the `start()` return already exposes) — keeping their coverage without the removed lookup method.
|
||||
- Update every doc and source-comment reference to the removed methods — not only literal `has(`/`delete(`/`get(`/`list(`/`deleteStored` call spellings, but also `{@link has}`/`{@link delete}` JSDoc links and prose that counts the methods (removing 2 of the persistence service's 6 public methods makes any "six public methods" phrasing wrong). The implementing PR greps `has`/`delete`/`get`/`list`/`deleteStored`/`{@link `/`six ` across `docs/`, `packages/*/README.md`, and source comments, and fixes each. The known doc sites: the seam READMEs ([packages/session-persistence/session-persistence/README.md](../../../../packages/session-persistence/session-persistence/README.md)'s `has(id)`/`delete(id)` API row and its "delegates its six public service methods" prose → four, [packages/bash/bash/README.md](../../../../packages/bash/bash/README.md)'s `get(id)`/`list()` row), the backend READMEs that describe `has`/`list` semantics ([packages/session-persistence/session-persistence-sqlite/README.md](../../../../packages/session-persistence/session-persistence-sqlite/README.md), [packages/session-persistence/session-persistence-jsonl/README.md](../../../../packages/session-persistence/session-persistence-jsonl/README.md) — reword "absent from `has()`/`list()`" to just `list()`), the service-map / seam docs in [docs/architecture.md](../../../architecture.md), and the persistence prose in the [session-persistence RFC](../../implemented/architecture/2026-06-14-session-persistence.md) and [shared write-coordinator RFC](../../implemented/architecture/2026-06-18-shared-persistence-write-coordinator.md). The known source-comment sites: the abstract `create()` JSDoc's `{@link has}/{@link list}` link ([packages/session-persistence/session-persistence/src/index.ts](../../../../packages/session-persistence/session-persistence/src/index.ts) — drop the `has` link), the coordinator's "six public methods"/"six public service methods" module + class JSDoc and its lazy-materialization JSDoc justifying the `materialized` flag by "the signal `has`/`list` rely on" ([packages/session-persistence/session-persistence/src/coordinator.ts](../../../../packages/session-persistence/session-persistence/src/coordinator.ts)), the JSONL backend's `loadStored`/`deleteStored` comment, and the SQLite backend's `schema.ts` and `index.ts` comments that mention "absent from `has`/`list`" — all reworded to the surviving four-method, `list()`-only contract.
|
||||
|
||||
## Why not keep them as "the seam should be complete"?
|
||||
|
||||
The instinct that a persistence seam "should" offer delete, or a task executor "should" offer enumeration, is real — and it is exactly the speculative-completeness the pre-release stance warns against ([AGENTS.md](../../../../AGENTS.md): optimize for the correct foundation, not for hypothetical callers you do not have). Each of these is one method to re-add the day a consumer needs it:
|
||||
|
||||
- A session-management UI that deletes old sessions will want `delete()` — add it then, designed against that UI's real needs (soft-delete? cascade? confirmation?), not guessed now.
|
||||
- A `bash_list` tool that shows the model its running tasks will want `list()` — add it with the tool.
|
||||
|
||||
Re-adding a seam method with a live consumer is cheap and better-designed than the speculative version, because the consumer pins the contract. Carrying it unused means every implementation (and every future backend) must implement and test a method that does nothing.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- `has`/`delete`/`deleteStored` and `get`/`list` are gone from their seams, impls, and contract suites; `pnpm run knip` reports no new dead exports.
|
||||
- The remaining seam operations (`create`/`append`/`load`/`list` for persistence; `run`/`start`/`ownerOf`/`onTaskDone`/`readOutput`/`kill`/`resolve` for bash) are untouched; ACP `session/list`, bash tool flows, and crash-recovery behave identically.
|
||||
- `pnpm run test:coverage` stays 100% per-file (the contract/spec rows for the removed methods are deleted with them).
|
||||
- Seam READMEs and `docs/architecture.md` no longer list the removed methods.
|
||||
|
||||
## Risks
|
||||
|
||||
- **`delete()` is the kind of operation a product eventually wants.** True — but "eventually" is the point. Deleting it now and re-adding it against a real consumer is strictly better than shipping a guessed contract. The dual backends each shed a `deleteStored` impl, which is a bounded edit in otherwise-out-of-scope packages.
|
||||
- **`list()` on the bash seam is the natural seed for a future `bash_list`.** Acknowledged in the [pre-release foundation stance](../../../../AGENTS.md): add the seed when the tool lands. The executor still tracks tasks internally (the `tasks` map backs `ownerOf`/`readOutput`/`kill`); exposing an enumeration is a one-line re-add.
|
||||
- **Low coupling.** Both removals are confined to their seam + impl + tests; no cross-package consumer references the removed methods, so there is no ripple beyond the docs.
|
||||
|
||||
Modest size, but it converts two seams from "what an implementation must provide for nobody" back to "exactly what a consumer uses."
|
||||
@@ -21,7 +21,7 @@ The JSONL durable session-persistence backend — a concrete `SessionPersistence
|
||||
|
||||
## Durability and crash semantics
|
||||
|
||||
- **Lazy materialization.** `create(meta)` writes nothing; the `.jsonl` (header + first batch) is written atomically (temp-write + `fsync` + rename) on the first `append`. A created-but-never-appended session leaves nothing on disk and is absent from `has`/`list`.
|
||||
- **Lazy materialization.** `create(meta)` writes nothing; the `.jsonl` (header + first batch) is written atomically (temp-write + `fsync` + rename) on the first `append`. A created-but-never-appended session leaves nothing on disk and is absent from `list`.
|
||||
- **Append-only.** Committed events (at or below a flushed `turn/end`) are never rewritten. Subsequent appends are line appends at EOF + `fsync`.
|
||||
- **Crash recovery — close, don't truncate.** A crash can leave a log whose final turn never closed (real events after the last `turn/end`). `load` PRESERVES those events (a turn can be huge — they are real work) and closes the orphaned turn by durably appending synthetic boundary events: an error `tool/result` for every `tool-call` the crash left unanswered (the loop logs the assistant message before running the tools, so a mid-tool crash leaves dangling calls — and `deriveMessages()` would replay an assistant tool-call with no result, which providers reject), then a `step/end` if a step was open, then `turn/end {kind:'interrupted'}`, returning a balanced log. Only a never-fully-written **torn tail fragment** (a final line with no newline / unparseable) is `ftruncate`d away before the closers are written. See [session persistence](../../../docs/rfc/implemented/architecture/2026-06-14-session-persistence.md).
|
||||
- **Contiguous-seq.** `load` rejects a mid-log parse error or `seq` gap (unloadable); `append` rejects a batch whose first `seq` does not continue the stored log, and rejects non-JSON-serializable `event.data` naming the offending event type.
|
||||
|
||||
@@ -11,7 +11,7 @@
|
||||
* (the `session/event` → buffer → `session/flush` drain, per-session
|
||||
* serialization, write cursors, fork-seed persistence, HMR live-adoption,
|
||||
* crash-repair sequencing, dispose quiescence) lives in the backend-agnostic
|
||||
* {@link PersistenceCoordinator} this class composes. The six public
|
||||
* {@link PersistenceCoordinator} this class composes. The four public
|
||||
* {@link SessionPersistence} methods delegate to the coordinator.
|
||||
*
|
||||
* @module @deepseek-ai/dsh-session-persistence-jsonl
|
||||
@@ -101,14 +101,6 @@ export class SessionPersistenceJsonl extends SessionPersistence implements Persi
|
||||
return this.coordinator.load(id)
|
||||
}
|
||||
|
||||
has(id: SessionId): Promise<boolean> {
|
||||
return this.coordinator.has(id)
|
||||
}
|
||||
|
||||
delete(id: SessionId): Promise<void> {
|
||||
return this.coordinator.delete(id)
|
||||
}
|
||||
|
||||
// `list` is BOTH the public service method and the PersistenceBackend hook —
|
||||
// one method, the bucket walk below. The coordinator adds no orchestration for
|
||||
// listing (no per-id serialization, no cursor), so it would just call back into
|
||||
@@ -180,12 +172,6 @@ export class SessionPersistenceJsonl extends SessionPersistence implements Persi
|
||||
if (closers.length > 0) await this.appendLines(meta, closers)
|
||||
}
|
||||
|
||||
/** 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)
|
||||
if (file) await rm(file.path, { force: true })
|
||||
}
|
||||
|
||||
/** List all stored sessions' metadata (header line only — no full-log parse). */
|
||||
async list(): Promise<SessionHeader[]> {
|
||||
const metas: SessionHeader[] = []
|
||||
@@ -341,9 +327,9 @@ export class SessionPersistenceJsonl extends SessionPersistence implements Persi
|
||||
|
||||
/**
|
||||
* 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.
|
||||
* for `loadStored` (resume identifies 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'
|
||||
|
||||
@@ -105,12 +105,12 @@ describe('SessionPersistenceJsonl: durability and crash semantics', () => {
|
||||
// nothing on disk yet
|
||||
const dir = sessionDir(root, '/work')
|
||||
await expect(stat(logPath(root, '/work', m.id))).rejects.toThrow()
|
||||
expect(await ctx.sessionPersistence.has(m.id)).toBe(false)
|
||||
expect((await ctx.sessionPersistence.list()).map(h => h.id)).not.toContain(m.id)
|
||||
|
||||
await ctx.sessionPersistence.append(m.id, oneTurnLog())
|
||||
// now materialized
|
||||
expect((await stat(logPath(root, '/work', m.id))).isFile()).toBe(true)
|
||||
expect(await ctx.sessionPersistence.has(m.id)).toBe(true)
|
||||
expect((await ctx.sessionPersistence.list()).map(h => h.id)).toContain(m.id)
|
||||
void dir
|
||||
})
|
||||
|
||||
@@ -448,19 +448,6 @@ describe('SessionPersistenceJsonl: edge cases', () => {
|
||||
expect(ids).toContain('big')
|
||||
})
|
||||
|
||||
it('has() finds a session on disk under an unknown cwd (cross-bucket scan)', async () => {
|
||||
const m = meta('scan-me', '/somewhere')
|
||||
await ctx.sessionPersistence.create(m)
|
||||
await ctx.sessionPersistence.append(m.id, oneTurnLog())
|
||||
// A fresh backend with no in-memory state → has() must scan disk buckets.
|
||||
const ctx2 = new Context()
|
||||
await ctx2.plugin(SessionStore)
|
||||
await ctx2.plugin(SessionPersistenceJsonl, { root })
|
||||
expect(await ctx2.sessionPersistence.has(m.id)).toBe(true)
|
||||
expect(await ctx2.sessionPersistence.has(SessionId('absent'))).toBe(false)
|
||||
await ctx2.fiber.dispose()
|
||||
})
|
||||
|
||||
it('a DIFFERENT live session object reusing a disposed id gets its own init (no stale cache)', async () => {
|
||||
// Session A materializes a log under id "reuse".
|
||||
const sessFiberA = await ctx.plugin(Object.assign((inner: Context) => {
|
||||
@@ -579,20 +566,23 @@ describe('SessionPersistenceJsonl: edge cases', () => {
|
||||
await ctx2.fiber.dispose()
|
||||
})
|
||||
|
||||
it('exists() surfaces a non-ENOENT lookup error (ENOTDIR) instead of reporting absent', async () => {
|
||||
// Same contract on the existence path: a non-ENOENT error from the per-id
|
||||
// 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
|
||||
// loadLive(id, cwd) → exists(logPath). Make that cwd's bucket DIRECTORY a
|
||||
// regular file: open()ing `bucket/<id>.jsonl` under it then fails ENOTDIR.
|
||||
it('loadLive surfaces a non-ENOENT lookup error (ENOTDIR) instead of reporting absent', async () => {
|
||||
// A non-ENOENT error from the per-id open() must surface, not be collapsed to
|
||||
// "not found" (which would let live-adoption proceed under a false absence
|
||||
// assumption). A live session's onCreated reaches 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()
|
||||
await ctx2.plugin(SessionStore)
|
||||
await ctx2.plugin(SessionPersistenceJsonl, { root })
|
||||
await ctx2.sessionPersistence.create(meta('exists-fault', cwd)) // lazy: no bucket yet
|
||||
await writeFile(sessionDir(root, cwd), 'x') // bucket path is now a FILE
|
||||
await expect(ctx2.sessionPersistence.has(SessionId('exists-fault'))).rejects.toThrow(/ENOTDIR/)
|
||||
const backend = ctx2.sessionPersistence as unknown as { inits: Map<Session, Promise<void>> }
|
||||
let s!: Session
|
||||
await ctx2.plugin(Object.assign((inner: Context) => {
|
||||
s = inner.sessions.create('exists-fault', { meta: { cwd } })
|
||||
}, { inject: ['sessions'] }))
|
||||
await expect(backend.inits.get(s)).rejects.toThrow(/ENOTDIR/)
|
||||
await ctx2.fiber.dispose()
|
||||
})
|
||||
|
||||
@@ -639,8 +629,8 @@ describe('SessionPersistenceJsonl: edge cases', () => {
|
||||
const a = meta('dup-id', '/projA')
|
||||
await ctx.sessionPersistence.create(a)
|
||||
await ctx.sessionPersistence.append(a.id, oneTurnLog())
|
||||
// A fresh backend creating the SAME id under cwd B must still refuse: load/
|
||||
// has identify by id across all buckets, so a second log would make resume
|
||||
// A fresh backend creating the SAME id under cwd B must still refuse: load
|
||||
// identifies by id across all buckets, so a second log would make resume
|
||||
// nondeterministic. create scans every bucket, not just meta.cwd's.
|
||||
const ctx2 = new Context()
|
||||
await ctx2.plugin(SessionStore)
|
||||
@@ -687,7 +677,7 @@ describe('SessionPersistenceJsonl: edge cases', () => {
|
||||
circ.self = circ
|
||||
await expect(ctx.sessionPersistence.append(m.id, bad(circ))).rejects.toThrow(/non-JSON-serializable/)
|
||||
// The session was never materialized by any of the rejected appends.
|
||||
expect(await ctx.sessionPersistence.has(m.id)).toBe(false)
|
||||
expect((await ctx.sessionPersistence.list()).map(h => h.id)).not.toContain(m.id)
|
||||
})
|
||||
|
||||
it('accepts well-formed JSON values (null, booleans, nested arrays/objects)', async () => {
|
||||
@@ -695,7 +685,7 @@ describe('SessionPersistenceJsonl: edge cases', () => {
|
||||
await ctx.sessionPersistence.create(m)
|
||||
const ev = [{ type: 'user/message', seq: 0, time: 1, data: { content: [{ type: 'text', text: 'x' }], source: { kind: 'user' }, extra: { a: null, b: true, c: [1, 2, { d: 'nested' }] } } }] as unknown as SessionEvent[]
|
||||
await ctx.sessionPersistence.append(m.id, ev)
|
||||
expect(await ctx.sessionPersistence.has(m.id)).toBe(true)
|
||||
expect((await ctx.sessionPersistence.list()).map(h => h.id)).toContain(m.id)
|
||||
})
|
||||
|
||||
it('Session.append rejects a non-serializable event at the source (never enters the log)', () => {
|
||||
|
||||
@@ -6,15 +6,15 @@ A SQLite durable session-persistence backend — a second `SessionPersistence` i
|
||||
|
||||
## Storage model
|
||||
|
||||
Each `SessionEvent` maps 1:1 onto a row in an `events` table `(session_id, seq, type, time, data)` — `data` is the event payload as JSON text, so the row shape is the event verbatim (including `assistant/chunk`, keeping `seq` contiguous). Out-of-log metadata (`SessionHeader`) lives in a `sessions` row. A `sessions` row is written only by the first `append` — its existence is the lazy-materialization signal (`has`/`list` report exactly the sessions that have a row), so no separate column is needed.
|
||||
Each `SessionEvent` maps 1:1 onto a row in an `events` table `(session_id, seq, type, time, data)` — `data` is the event payload as JSON text, so the row shape is the event verbatim (including `assistant/chunk`, keeping `seq` contiguous). Out-of-log metadata (`SessionHeader`) lives in a `sessions` row. A `sessions` row is written only by the first `append` — its existence is the lazy-materialization signal (`list` reports exactly the sessions that have a row), so no separate column is needed.
|
||||
|
||||
The repo targets Node ≥ 24 (the root `engines` field), which includes the stable `node:sqlite` module. The database opens with `foreign_keys = ON` (so `ON DELETE CASCADE` drops a session's events with its row) and `journal_mode = WAL`. The table-layout version is stored in `PRAGMA user_version` and checked on open: a fresh database is stamped with the current `SCHEMA_VERSION`; a database written by any other, incompatible build (a non-current `user_version`, older or newer) is rejected rather than opened against an unknown layout — there is no migration (unreleased software).
|
||||
|
||||
## Contract semantics over rows
|
||||
|
||||
- **Append = a transaction.** `append` runs `BEGIN`/`COMMIT` around the batch: it materializes the `sessions` row (if still lazy) and INSERTs every event, asserting the contiguous-seq contract first (the first event's `seq` must equal the stored next-seq). A mid-batch failure (a UNIQUE violation on a duplicated seq) rolls back entirely, so the stored log and the in-memory cursor stay consistent. (`load()` already balanced the stored log, so `append` never has to repair a crash tail.)
|
||||
- **Lazy materialization.** `create()` records intent in memory only — no row is written until the first `append`. A created-but-never-appended session has no `sessions` row, so it is absent from `has()`/`list()` (which report exactly the sessions that have a row).
|
||||
- **Interrupted-turn close on load.** `load()` reads every stored event ordered by `seq` and finds the longest seq-contiguous, parseable prefix — INCLUDING the real events of an interrupted final turn after the last `turn/end` (the loop only flushes at `turn/end`, so a process killed mid-turn leaves real, fully-written rows past it). A single turn can be huge in a long-horizon task, so those events are **preserved, never truncated**: `load()` CLOSES the orphaned turn by durably appending the minimal synthetic boundary events (an error `tool/result` for every assistant tool call left unanswered, a `step/end` if a step was open, then a `turn/end` carrying `{ kind: 'interrupted' }`), inside one transaction that also DELETEs any never-fully-written torn tail row. `load()` is therefore mutating — after it the stored rows are balanced and the cursor is truthful, so the next `append` continues cleanly. The boundary (last `turn/end`, torn-tail detection) is computed from the `seq`/`type` columns so a malformed `data` in a torn tail row is never parsed (discarded, not unloadable). A parse error or `seq` gap inside the committed region (at or before the last real `turn/end`) makes the session unloadable. A session whose only turn never closed keeps its metadata row and stays present in `has()`/`list()` — the same as the JSONL backend, whose file likewise survives a first append that never reached `turn/end`.
|
||||
- **Lazy materialization.** `create()` records intent in memory only — no row is written until the first `append`. A created-but-never-appended session has no `sessions` row, so it is absent from `list()` (which reports exactly the sessions that have a row).
|
||||
- **Interrupted-turn close on load.** `load()` reads every stored event ordered by `seq` and finds the longest seq-contiguous, parseable prefix — INCLUDING the real events of an interrupted final turn after the last `turn/end` (the loop only flushes at `turn/end`, so a process killed mid-turn leaves real, fully-written rows past it). A single turn can be huge in a long-horizon task, so those events are **preserved, never truncated**: `load()` CLOSES the orphaned turn by durably appending the minimal synthetic boundary events (an error `tool/result` for every assistant tool call left unanswered, a `step/end` if a step was open, then a `turn/end` carrying `{ kind: 'interrupted' }`), inside one transaction that also DELETEs any never-fully-written torn tail row. `load()` is therefore mutating — after it the stored rows are balanced and the cursor is truthful, so the next `append` continues cleanly. The boundary (last `turn/end`, torn-tail detection) is computed from the `seq`/`type` columns so a malformed `data` in a torn tail row is never parsed (discarded, not unloadable). A parse error or `seq` gap inside the committed region (at or before the last real `turn/end`) makes the session unloadable. A session whose only turn never closed keeps its metadata row and stays present in `list()` — the same as the JSONL backend, whose file likewise survives a first append that never reached `turn/end`.
|
||||
|
||||
## Configuration (schemastery)
|
||||
|
||||
|
||||
@@ -11,7 +11,7 @@
|
||||
* Like the JSONL backend it supplies ONLY the storage primitives (the
|
||||
* {@link PersistenceBackend} hooks below — INSERT/DELETE/SELECT inside
|
||||
* transactions); all the write-path orchestration lives in the backend-agnostic
|
||||
* {@link PersistenceCoordinator} this class composes. The six public
|
||||
* {@link PersistenceCoordinator} this class composes. The four public
|
||||
* {@link SessionPersistence} methods delegate to the coordinator.
|
||||
*
|
||||
* @module @deepseek-ai/dsh-session-persistence-sqlite
|
||||
@@ -99,14 +99,6 @@ export class SessionPersistenceSqlite extends SessionPersistence implements Pers
|
||||
return this.coordinator.load(id)
|
||||
}
|
||||
|
||||
has(id: SessionId): Promise<boolean> {
|
||||
return this.coordinator.has(id)
|
||||
}
|
||||
|
||||
delete(id: SessionId): Promise<void> {
|
||||
return this.coordinator.delete(id)
|
||||
}
|
||||
|
||||
// `list` is BOTH the public service method and the PersistenceBackend hook —
|
||||
// one method (the SELECT below). The coordinator adds no orchestration for
|
||||
// listing, so routing it through the coordinator would just recurse. Defined
|
||||
@@ -203,12 +195,6 @@ export class SessionPersistenceSqlite extends SessionPersistence implements Pers
|
||||
}
|
||||
}
|
||||
|
||||
/** Remove a session's row (ON DELETE CASCADE drops its events). */
|
||||
async deleteStored(id: SessionId): Promise<void> {
|
||||
await this.ready
|
||||
this.db.prepare('DELETE FROM sessions WHERE id = ?').run(id)
|
||||
}
|
||||
|
||||
/** List all materialized sessions' metadata (every row is a materialized session). */
|
||||
async list(): Promise<SessionHeader[]> {
|
||||
await this.ready
|
||||
@@ -234,7 +220,7 @@ export class SessionPersistenceSqlite extends SessionPersistence implements Pers
|
||||
/**
|
||||
* Insert-or-replace a session's metadata row. The only caller is the first
|
||||
* materializing `appendBatch`, so writing the row IS the materialization (its
|
||||
* existence is the signal `has`/`list` read).
|
||||
* existence is the signal `list` reads).
|
||||
*/
|
||||
private writeRow(meta: SessionHeader): void {
|
||||
this.db.prepare(`
|
||||
|
||||
@@ -21,7 +21,7 @@ export const SCHEMA_VERSION = 2
|
||||
* A row of the `sessions` table — the out-of-log metadata ({@link SessionHeader}).
|
||||
* The row's EXISTENCE is the materialization signal: it is written only by the
|
||||
* first `append` (lazy materialization), so a created-but-never-appended
|
||||
* session has no row and is absent from `has`/`list`, mirroring the JSONL
|
||||
* session has no row and is absent from `list`, mirroring the JSONL
|
||||
* backend's "no file until first append".
|
||||
*/
|
||||
export interface SessionRow {
|
||||
|
||||
@@ -214,17 +214,15 @@ describe('SessionPersistenceSqlite: durability and crash semantics', () => {
|
||||
{ type: 'turn/start', seq: 0, time: 1, data: { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } } },
|
||||
{ type: 'user/message', seq: 1, time: 2, data: { content: [{ type: 'text', text: 'hi' }], source: { kind: 'user' } } },
|
||||
])
|
||||
expect(await b1.ctx.sessionPersistence.has(m.id)).toBe(true) // materialized
|
||||
await b1.dispose()
|
||||
|
||||
// A fresh backend loads it: the interrupted (only) turn's real events are
|
||||
// preserved and closed with a synthetic turn/end {interrupted} — NOT
|
||||
// truncated. The session was materialized, so has()/list() report it present.
|
||||
// truncated. The session was materialized, so list() reports it present.
|
||||
const b2 = await backend(path)
|
||||
const loaded = await b2.ctx.sessionPersistence.load(m.id)
|
||||
expect(loaded.events.map(e => e.type)).toEqual(['turn/start', 'user/message', 'turn/end'])
|
||||
expect(loaded.events.at(-1)!.type === 'turn/end' && loaded.events.at(-1)!.data).toMatchObject({ reason: { kind: 'interrupted' } })
|
||||
expect(await b2.ctx.sessionPersistence.has(m.id)).toBe(true)
|
||||
expect((await b2.ctx.sessionPersistence.list()).map(x => x.id)).toContain(m.id)
|
||||
await b2.dispose()
|
||||
})
|
||||
|
||||
@@ -11,8 +11,7 @@ The persisted unit IS the existing `SessionEvent` (event-sourced model — the l
|
||||
| `create(meta): Promise<void>` | Register a new session's metadata. MAY defer the physical write until the first `append` (lazy materialization). |
|
||||
| `append(id, events): Promise<void>` | Durably persist a batch (from the `session/flush` drain). Append-only; first event `seq` == stored next-seq after any repair; rejects non-JSON-serializable data naming the offending type. |
|
||||
| `load(id): Promise<{ meta; events }>` | Reload meta + log. Preserves an interrupted (unclosed) final turn and closes it with synthetic closers — an error `tool/result` per unanswered `tool-call`, then `step/end?`+`turn/end {interrupted}` (a turn can be huge — never truncated); only a torn tail fragment is dropped. Events contiguous (`events[i].seq === i`); rejects a committed-region gap/parse error or unknown `version`. |
|
||||
| `list(): Promise<SessionHeader[]>` | Lightweight listing from metadata, no full-log parse. |
|
||||
| `has(id)` / `delete(id)` | Existence / removal. A zero-event lazily-materialized session is absent from `has`/`list`. |
|
||||
| `list(): Promise<SessionHeader[]>` | Lightweight listing from metadata, no full-log parse. A zero-event lazily-materialized session is absent from `list`. |
|
||||
|
||||
## Invariants every backend must honor
|
||||
|
||||
@@ -25,7 +24,7 @@ The persisted unit IS the existing `SessionEvent` (event-sourced model — the l
|
||||
|
||||
The two first-party backends were byte-identical (or same-algorithm) for ALL of their write-path orchestration — the in-memory bookkeeping (per-id state, write-behind buffers, per-id serialization chains, per-session init promises), the `session/event` → buffer → `session/flush` drain, lazy materialization, crash-tail repair on load, the four `session/created` adoption cases (new / HMR-adopt / collision / ownerless-claim), and dispose-time quiescence. Only the STORAGE primitives differed (write bytes vs. INSERT rows).
|
||||
|
||||
`PersistenceCoordinator` owns that orchestration once. A first-party backend composes one (`new PersistenceCoordinator(ctx, this)`), implements the small `PersistenceBackend` hook interface, and delegates its six public service methods to the coordinator. This keeps the duplicated, correctness-heavy orchestration in a single place (it used to receive the same fixes twice).
|
||||
`PersistenceCoordinator` owns that orchestration once. A first-party backend composes one (`new PersistenceCoordinator(ctx, this)`), implements the small `PersistenceBackend` hook interface, and delegates its four public service methods to the coordinator. This keeps the duplicated, correctness-heavy orchestration in a single place (it used to receive the same fixes twice).
|
||||
|
||||
The `PersistenceBackend<TornMarker>` hooks (the only seam between the coordinator and storage):
|
||||
|
||||
@@ -36,7 +35,7 @@ The `PersistenceBackend<TornMarker>` hooks (the only seam between the coordinato
|
||||
| `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 !== 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. |
|
||||
| `list()` | List all stored metadata. |
|
||||
| `close?()` | Optional lifecycle teardown (e.g. close a db handle), awaited after the dispose drain. |
|
||||
|
||||
The `tornMarker` is fully OPAQUE: the coordinator only tests `!== undefined` and round-trips it to `commitRepair`, never inspecting its value (the JSONL backend uses the byte offset to truncate to, the SQLite backend the seq to delete from). The public `SessionPersistence` service shape is unchanged, so a third-party backend MAY still implement the abstract service directly without the coordinator. See [the write-coordinator RFC](../../../docs/rfc/implemented/architecture/2026-06-18-shared-persistence-write-coordinator.md).
|
||||
|
||||
@@ -14,7 +14,7 @@
|
||||
* {@link PersistenceBackend} hook object.
|
||||
*
|
||||
* The abstract {@link SessionPersistence} service's public API is independent of
|
||||
* this: a backend IS a `SessionPersistence` (its six public methods delegate to
|
||||
* this: a backend IS a `SessionPersistence` (its four public methods delegate to
|
||||
* a coordinator it composes), so a third-party backend MAY implement the service
|
||||
* directly without using the coordinator at all.
|
||||
*
|
||||
@@ -95,9 +95,6 @@ export interface PersistenceBackend<TornMarker = unknown> {
|
||||
*/
|
||||
commitRepair(meta: SessionHeader, tornMarker: TornMarker | undefined, closers: readonly SessionEvent[]): Promise<void>
|
||||
|
||||
/** Remove the stored artifact for `id` (the coordinator clears in-memory state). */
|
||||
deleteStored(id: SessionId): Promise<void>
|
||||
|
||||
/** List all stored (materialized) sessions' metadata. */
|
||||
list(): Promise<SessionHeader[]>
|
||||
|
||||
@@ -119,13 +116,12 @@ interface SessionState {
|
||||
* SQLite row exists). `create()` registers state LAZILY — cursor 0,
|
||||
* materialized false, nothing on disk — so an empty session leaves no
|
||||
* artifact and the FIRST `appendBatch` writes the header + its events in ONE
|
||||
* transaction (the "a row exists ⇔ it has events" invariant `has`/`list`
|
||||
* rely on; a separate up-front materialize could crash leaving a row with
|
||||
* transaction (the "a row exists ⇔ it has events" invariant `list`
|
||||
* relies on; a separate up-front materialize could crash leaving a row with
|
||||
* zero events). The flag is the only signal that distinguishes a session
|
||||
* registered-but-never-written from one durably present, which two callers
|
||||
* need: `has()` (lazy-but-unwritten is not yet durable) and the reclaim path
|
||||
* (an abandoned id with no artifact AND no buffered events is free to reuse;
|
||||
* a materialized one is a real collision).
|
||||
* registered-but-never-written from one durably present, which the reclaim
|
||||
* path needs (an abandoned id with no artifact AND no buffered events is free
|
||||
* to reuse; a materialized one is a real collision).
|
||||
*/
|
||||
materialized: boolean
|
||||
/**
|
||||
@@ -150,7 +146,7 @@ async function settledErrors(promises: Iterable<Promise<unknown>>): Promise<unkn
|
||||
/**
|
||||
* Owns the backend-agnostic session write-path orchestration. A backend
|
||||
* constructs one (`new PersistenceCoordinator(ctx, this)`), implements
|
||||
* {@link PersistenceBackend}, and delegates its six public service methods to
|
||||
* {@link PersistenceBackend}, and delegates its four public service methods to
|
||||
* the matching coordinator methods.
|
||||
*
|
||||
* All per-id operations are serialized (a per-id promise chain) so concurrent
|
||||
@@ -206,7 +202,7 @@ export class PersistenceCoordinator<TornMarker = unknown> {
|
||||
throw new Error(`session "${meta.id}" already exists in this backend`)
|
||||
}
|
||||
// A persisted artifact under this id (in ANY scope) blocks creation: load/
|
||||
// has/resume identify a session by id alone, so a second artifact would make
|
||||
// resume identify a session by id alone, so a second artifact would make
|
||||
// resume nondeterministic.
|
||||
if (await this.backend.loadStored(meta.id) !== undefined) {
|
||||
throw new Error(`session "${meta.id}" already has a persisted log on disk; load/resume it instead of creating`)
|
||||
@@ -294,31 +290,6 @@ export class PersistenceCoordinator<TornMarker = unknown> {
|
||||
// through the coordinator would only forward to that same hook, so the
|
||||
// coordinator stays out of the listing path entirely.
|
||||
|
||||
/** Whether a session is durably present (materialized). */
|
||||
async has(id: SessionId): Promise<boolean> {
|
||||
const state = this.states.get(id)
|
||||
if (state?.materialized) return true
|
||||
// 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. */
|
||||
delete(id: SessionId): Promise<void> {
|
||||
return this.serialize(id, () => this.deleteCore(id))
|
||||
}
|
||||
|
||||
private async deleteCore(id: SessionId): Promise<void> {
|
||||
await this.backend.deleteStored(id)
|
||||
this.states.delete(id)
|
||||
}
|
||||
|
||||
// --- per-id serialization + adoption helpers ---
|
||||
|
||||
/**
|
||||
|
||||
@@ -103,7 +103,7 @@ export abstract class SessionPersistence extends Service {
|
||||
/**
|
||||
* Register a new session's metadata. A backend MAY defer the physical write
|
||||
* until the first {@link append} (lazy materialization), in which case a
|
||||
* created-but-never-appended session is absent from {@link has}/{@link list}
|
||||
* created-but-never-appended session is absent from {@link list}
|
||||
* — abandoned sessions leave nothing behind.
|
||||
*/
|
||||
abstract create(meta: SessionHeader): Promise<void>
|
||||
@@ -143,12 +143,6 @@ export abstract class SessionPersistence extends Service {
|
||||
|
||||
/** Lightweight listing from metadata, without a full-log parse. */
|
||||
abstract list(): Promise<SessionHeader[]>
|
||||
|
||||
/** Whether a session is durably present (materialized). */
|
||||
abstract has(id: SessionId): Promise<boolean>
|
||||
|
||||
/** Remove a session and all its persisted artifacts. */
|
||||
abstract delete(id: SessionId): Promise<void>
|
||||
}
|
||||
|
||||
export default SessionPersistence
|
||||
@@ -142,24 +142,22 @@ export function runPersistenceContract(name: string, make: () => Promise<Contrac
|
||||
}
|
||||
})
|
||||
|
||||
it('has()/list() exclude a created-but-never-appended (zero-event) session', async () => {
|
||||
it('list() excludes a created-but-never-appended (zero-event) session', async () => {
|
||||
const { persistence, dispose } = await make()
|
||||
try {
|
||||
await persistence.create(meta('empty'))
|
||||
expect(await persistence.has(SessionId('empty'))).toBe(false)
|
||||
expect((await persistence.list()).map(m => m.id)).not.toContain(SessionId('empty'))
|
||||
} finally {
|
||||
await dispose()
|
||||
}
|
||||
})
|
||||
|
||||
it('has()/list() include a session once it has events', async () => {
|
||||
it('list() includes a session once it has events', async () => {
|
||||
const { persistence, dispose } = await make()
|
||||
try {
|
||||
const m = meta('s2')
|
||||
await persistence.create(m)
|
||||
await persistence.append(m.id, oneTurnLog())
|
||||
expect(await persistence.has(m.id)).toBe(true)
|
||||
expect((await persistence.list()).map(x => x.id)).toContain(m.id)
|
||||
} finally {
|
||||
await dispose()
|
||||
@@ -227,19 +225,5 @@ export function runPersistenceContract(name: string, make: () => Promise<Contrac
|
||||
await dispose()
|
||||
}
|
||||
})
|
||||
|
||||
it('delete removes a session', async () => {
|
||||
const { persistence, dispose } = await make()
|
||||
try {
|
||||
const m = meta('s6')
|
||||
await persistence.create(m)
|
||||
await persistence.append(m.id, oneTurnLog())
|
||||
expect(await persistence.has(m.id)).toBe(true)
|
||||
await persistence.delete(m.id)
|
||||
expect(await persistence.has(m.id)).toBe(false)
|
||||
} finally {
|
||||
await dispose()
|
||||
}
|
||||
})
|
||||
})
|
||||
}
|
||||
@@ -625,7 +625,7 @@ export function runCoordinatorContract(name: string, makeFixture: () => Promise<
|
||||
const m = meta('empty-batch', WORK)
|
||||
await ctx.sessionPersistence.create(m)
|
||||
await ctx.sessionPersistence.append(m.id, [])
|
||||
expect(await ctx.sessionPersistence.has(m.id)).toBe(false)
|
||||
expect((await ctx.sessionPersistence.list()).map(h => h.id)).not.toContain(m.id)
|
||||
} finally {
|
||||
await fiber.dispose()
|
||||
await fix.cleanup()
|
||||
@@ -643,17 +643,6 @@ export function runCoordinatorContract(name: string, makeFixture: () => Promise<
|
||||
}
|
||||
})
|
||||
|
||||
it('delete of a non-existent session is a no-op', async () => {
|
||||
const fix = await makeFixture()
|
||||
const { ctx, fiber } = await freshCtx(fix)
|
||||
try {
|
||||
await expect(ctx.sessionPersistence.delete(SessionId('ghost'))).resolves.toBeUndefined()
|
||||
} finally {
|
||||
await fiber.dispose()
|
||||
await fix.cleanup()
|
||||
}
|
||||
})
|
||||
|
||||
it('create rejects a duplicate id (in memory and on a persisted log)', async () => {
|
||||
const fix = await makeFixture()
|
||||
const first = await freshCtx(fix)
|
||||
|
||||
@@ -61,14 +61,6 @@ class MemoryPersistence extends SessionPersistence implements PersistenceBackend
|
||||
return this.coordinator.load(id)
|
||||
}
|
||||
|
||||
has(id: SessionId): Promise<boolean> {
|
||||
return this.coordinator.has(id)
|
||||
}
|
||||
|
||||
delete(id: SessionId): Promise<void> {
|
||||
return this.coordinator.delete(id)
|
||||
}
|
||||
|
||||
/** White-box accessor: await a specific session's onCreated init. */
|
||||
get inits(): Map<Session, Promise<void>> {
|
||||
return this.coordinator.inits
|
||||
@@ -114,10 +106,6 @@ class MemoryPersistence extends SessionPersistence implements PersistenceBackend
|
||||
if (closers.length > 0) entry.events.push(...structuredClone(closers) as SessionEvent[])
|
||||
}
|
||||
|
||||
async deleteStored(id: SessionId): Promise<void> {
|
||||
this.store.delete(id)
|
||||
}
|
||||
|
||||
async list(): Promise<SessionHeader[]> {
|
||||
return [...this.store.values()].map(e => structuredClone(e.meta))
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user