mirror of
https://github.com/deepseek-ai/deepseek-harness
synced 2026-08-15 21:04:50 +00:00
fix review findings: stale seam docs + race-free doneFor test helper
Codex review of the PR2 diff caught doc/comment sites doc-sync does not gate (core-data-structures prose) and a latent test-helper race: - docs/core-data-structures/persistence.md + bash.md, sqlite README, and two source comments (coordinator.ts, jsonl.spec.ts) still listed the removed has/delete/get/list methods — updated to the surviving four-method persistence surface and the get/list-free bash seam. - doneFor(ctx, id) attached its onTaskDone listener lazily, after the task could already have closed (e.g. `true`), so it could miss the completion and hang. Replaced with trackCompletions(ctx): one eagerly-installed listener (mounted in setup() before any task starts) records every completion, and doneFor resolves immediately for an already-finished task or on completion otherwise. Race-free, and there is no get-by-id seam left to poll instead.
This commit is contained in:
@@ -120,4 +120,4 @@ interface BashTaskRead {
|
||||
|
||||
## The service
|
||||
|
||||
`BashExecutor` (`ctx.bash`, abstract — defined in [`packages/bash/bash/src/index.ts`](../../packages/bash/bash/src/index.ts)) mirrors the `LlmService`/`LlmAdapter` split: `resolve` (request → spec), `run` (foreground), `start` (background), `get`/`ownerOf`/`list`/`readOutput`/`kill`, and `onTaskDone` (a `BashTaskListener` completion callback). Spawned commands get a **scrubbed env** (dropping `*KEY*`/`*SECRET*`/`*TOKEN*`) and spill files use a private 0700 dir with random names and owner-only opens — model output never gets the ambient environment or a predictable path. The implementation that provides all this is `dsh-bash-local`; the model-facing `bash`/`bash_output`/`bash_kill` schemas that call it are in `dsh-tool-bash` (and present as terminals via the [tool-presentation vocabulary](tools.md#tool-presentation-ui-vocabulary)).
|
||||
`BashExecutor` (`ctx.bash`, abstract — defined in [`packages/bash/bash/src/index.ts`](../../packages/bash/bash/src/index.ts)) mirrors the `LlmService`/`LlmAdapter` split: `resolve` (request → spec), `run` (foreground), `start` (background), `ownerOf`/`readOutput`/`kill`, and `onTaskDone` (a `BashTaskListener` completion callback). Spawned commands get a **scrubbed env** (dropping `*KEY*`/`*SECRET*`/`*TOKEN*`) and spill files use a private 0700 dir with random names and owner-only opens — model output never gets the ambient environment or a predictable path. The implementation that provides all this is `dsh-bash-local`; the model-facing `bash`/`bash_output`/`bash_kill` schemas that call it are in `dsh-tool-bash` (and present as terminals via the [tool-presentation vocabulary](tools.md#tool-presentation-ui-vocabulary)).
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -24,6 +24,7 @@ async function setup() {
|
||||
await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000 })
|
||||
;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 }
|
||||
await ctx.plugin(ToolBash)
|
||||
trackCompletions(ctx)
|
||||
return ctx
|
||||
}
|
||||
|
||||
@@ -67,22 +68,42 @@ function text(result: { content: { type: string; text?: string }[] }): string {
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve once the background task with `id` completes. The task is started
|
||||
* indirectly (via `ctx.tools.execute`), so `start()`'s return is not accessible
|
||||
* here; the executor's `onTaskDone` listener delivers the SAME task object on
|
||||
* completion, which is the surviving seam for awaiting a task by id.
|
||||
* Per-context background-completion tracker. The task is started indirectly
|
||||
* (via `ctx.tools.execute`), so `start()`'s return is not accessible here, and
|
||||
* there is no get-by-id seam to poll current state — the only surviving way to
|
||||
* await a task by id is the executor's `onTaskDone` listener. Registering that
|
||||
* listener lazily (after the task may have already closed) would miss the
|
||||
* completion and hang; so {@link trackCompletions} installs ONE listener
|
||||
* EAGERLY (before any task starts) that records every completion, and
|
||||
* {@link doneFor} resolves from that record — immediately if the task already
|
||||
* finished, otherwise when it does. Call `trackCompletions(ctx)` right after
|
||||
* the executor is mounted (`setup()` does this for you).
|
||||
*/
|
||||
function doneFor(ctx: Context, id: string): Promise<BashTask> {
|
||||
return new Promise<BashTask>((resolve) => {
|
||||
const dispose = ctx.bash.onTaskDone((task) => {
|
||||
if (task.id === id) {
|
||||
dispose()
|
||||
resolve(task)
|
||||
}
|
||||
})
|
||||
const completions = new WeakMap<Context, { done: Map<string, BashTask>; waiters: Map<string, (task: BashTask) => void> }>()
|
||||
|
||||
function trackCompletions(ctx: Context): void {
|
||||
const state = { done: new Map<string, BashTask>(), waiters: new Map<string, (task: BashTask) => void>() }
|
||||
completions.set(ctx, state)
|
||||
ctx.bash.onTaskDone((task) => {
|
||||
const waiter = state.waiters.get(task.id)
|
||||
if (waiter) {
|
||||
state.waiters.delete(task.id)
|
||||
waiter(task)
|
||||
} else {
|
||||
state.done.set(task.id, task)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
/** Resolve (with the task object) once the background task `id` has completed. */
|
||||
function doneFor(ctx: Context, id: string): Promise<BashTask> {
|
||||
const state = completions.get(ctx)
|
||||
if (!state) throw new Error('trackCompletions(ctx) must be called before doneFor(ctx, …)')
|
||||
const already = state.done.get(id)
|
||||
if (already) return Promise.resolve(already)
|
||||
return new Promise<BashTask>(resolve => state.waiters.set(id, resolve))
|
||||
}
|
||||
|
||||
class LossyReadBashExecutor extends BashExecutor {
|
||||
private readonly task: BashTask = {
|
||||
id: 'bash-lossy',
|
||||
@@ -312,6 +333,7 @@ describe('background tools', () => {
|
||||
await ctx.plugin(LocalBashExecutor, { maxOutputBytes: 100 })
|
||||
;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 }
|
||||
await ctx.plugin(ToolBash)
|
||||
trackCompletions(ctx)
|
||||
|
||||
const started = await call(ctx, 'bash', { command: 'for i in $(seq 1 200); do printf "line-%04d\\n" $i; done', description: 'test command', run_in_background: true })
|
||||
const id = /task (bash-\d+)/.exec(text(started))![1]!
|
||||
|
||||
@@ -629,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)
|
||||
|
||||
@@ -6,7 +6,7 @@ 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).
|
||||
|
||||
|
||||
@@ -202,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`)
|
||||
|
||||
Reference in New Issue
Block a user