From bdd3a4d958562fc7b54b8f5c7cdee508bcf5cc8f Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Fri, 28 Aug 2026 10:15:08 +0200 Subject: [PATCH 1/4] =?UTF-8?q?spec:=20bug=20=E2=80=94=20vault=20ui=20resu?= =?UTF-8?q?me=20races=20the=20live=20headless=20turn?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spec 040 shipped the non-interactive branch persisting claude_session_id before spawn + returning after a 10s liveness window; the Vault UI flips to Resume while the turn still runs, so resume fails (session not found, partial replay, two writers on one transcript). Inverts the non-interactive design: block until the detached child exits (bounded by sessionTurnTimeout = 30 min, never a kill), capture stdout to a temp file and validate the JSON (shared helper with the interactive branch), and only then persist the id. Interactive branch + scenarios/005 untouched. --- specs/bug-resume-races-live-headless-turn.md | 243 +++++++++++++++++++ 1 file changed, 243 insertions(+) create mode 100644 specs/bug-resume-races-live-headless-turn.md diff --git a/specs/bug-resume-races-live-headless-turn.md b/specs/bug-resume-races-live-headless-turn.md new file mode 100644 index 0000000..aff2e4c --- /dev/null +++ b/specs/bug-resume-races-live-headless-turn.md @@ -0,0 +1,243 @@ +--- +status: draft +--- +# Vault UI Resume Races the Live Headless Turn + +## Summary + +- The v0.116.3/0.116.4 fix (spec 040) made the **non-interactive** branch of `StartSession` persist `claude_session_id` **before** spawning the detached headless `claude --print` turn, then return after a 10s `livenessWindow` while the turn keeps running (2–5+ min). +- The Vault UI watcher sees the id on the task at ~10s and flips the button to **▶ Resume** while the turn is still running. Clicking Resume runs `claude --resume ` against a transcript that is **mid-write by another process**: "session not found" first, then progressively more of the initial work on each reopen, and two writers on one jsonl (a corruption risk, not just a UX nuisance). +- Fix: the non-interactive branch **blocks until the detached child exits** (its headless turn completes), captures the turn's JSON output to a temp file and validates it, and only **then** does the caller persist `claude_session_id`. The id is never on disk while a child exists, so the button shows `⏳ Starting` for the whole turn and flips to `▶ Resume` exactly when the transcript is complete and resumable. +- This **reverses spec 040's non-interactive design** (persist-before-spawn + 10s liveness + compensated-failure path + "`--output-format json` is dead weight on the detached branch"). The interactive TTY branch, `defaultCommandRunner`, and `scenarios/005` are untouched. Spec 040 stays as the historical record. + +## Goal + +After this work, a **non-TTY** `task work-on` (the Vault UI Start button) persists `claude_session_id` only once the headless turn has completed, so the button holds `⏳ Starting` for the turn and offers `▶ Resume` only against a complete, single-writer transcript. A turn that fails — exit non-zero, `is_error:true`, 0 turns, or the bound expiring — leaves **no id**, so the button reverts to `▶ Start` rather than offering a broken Resume. The **TTY** start-then-resume flow behaves exactly as it does today. + +## Problem + +The shipped fix's contract was "return a session id in ~10s". That contract is what breaks Resume: an id on the frontmatter means "a session exists", but it does **not** mean "a session is resumable". The turn writes the transcript for minutes after the id appears. The Vault UI's button keys off `claude_session_id` presence (`sessionButtonHtml`, `hasSession`), so it advertises Resume while `claude --resume` cannot succeed. + +Two consequences: +- **The operator cannot trust the button.** Resume fails, and on re-open the conversation replays the turn's tail as the writer advances — the same "live transcript" confusion the headless path was built to avoid. +- **Two writers on one transcript.** The resumed TTY and the detached child both hold the same session file concurrently. Even a "successful" resume mid-turn risks interleaved writes on the jsonl. + +The verification that shipped the previous fix marked "card flips to Resume" as success without ever running `claude --resume` — the gap that let this through. + +## Reproduction + +vault-cli `v0.116.4` installed (`v0.116.4-dirty` on the operator's PATH at the time); observed 2026-08-28 on the Brogrammers vault (task `BRO-21734 Check Alerts`). + +Setup — any task whose `/vault-cli:work-on-task` bootstrap turn takes longer than ~10s (all real tasks; a trivial task may not reproduce). + +Action — click **Start** on the task in the Vault UI Kanban, wait for the card to flip to **▶ Resume** (~10s), copy the offered command and run it: + +```bash +/Users/bborbe/Documents/workspaces/scripts/cc-brogrammers-deepseek --resume 96bc2eda-acf4-4402-bca7-9f93c5bcb02a +``` + +Observed — the resume session replays the still-running headless turn: + +``` +❯ /vault-cli:work-on-task "/Users/bborbe/Documents/Obsidian/Brogrammers/24 Tasks/BRO-21734 Check Alerts.md" +--non-interactive + Press Ctrl-C again to exit +Resume this session with: +claude --resume "BRO-21734 Check Alerts" +``` + +On the first attempt the operator reports "session not found"; re-opening a few seconds later shows the first ~10s of the turn, and each reopen shows more — the transcript is being written by the detached child concurrently. + +Root-cause evidence, `pkg/ops/claude_session.go` (v0.116.4): + +```go +// non-interactive branch +done, err := c.detachRun(args, cwd) // spawn detached +go func() { waitCh <- c.waiter.Wait(ctx, c.livenessWindow) }() // 10s +select { +case exitErr := <-done: // only consumed if child dies in-window + return errors.Errorf(ctx, "claude session exited during startup: %v", exitErr) +case err := <-waitCh: + return nil // returns after 10s, child abandoned +} +``` + +and `pkg/ops/workon.go` `handleClaudeSession`: + +```go +sessionID := w.uuidGenerator() +persistSessionAndMetrics(ctx, vaultPath, task.Name, sessionID, startedAt, w.taskStorage) // BEFORE spawn +w.starter.StartSession(ctx, sessionID, prompt, sessionDir, task.Name, isInteractive) +``` + +## Expected vs Actual + +| | Expected (this spec) | Actual (v0.116.3/0.116.4) | +|---|---|---| +| Start click | `⏳ Starting` for the whole headless turn | flips to `▶ Resume` at ~10s | +| During the turn | no `claude_session_id` on the task | id present (persist-before-spawn) | +| Resume click (when offered) | `claude --resume ` opens the **complete** bootstrap conversation | "session not found" / partial replay / two writers | +| Turn fails (exit ≠ 0, `is_error`, 0 turns, bound) | no id persisted; button back to `▶ Start` | id persisted at ~10s regardless → broken Resume offered | + +## Why this is a bug + +`work-on` exists so the Start button hands the operator a session they can actually resume. Shipping an id at 10s hands them an id whose session is still being written — the button lies, the resume fails, and the transcript can be corrupted by the second writer. The prior spec's own acceptance criterion ("the generated UUID is the id the session actually uses — `claude --resume ` opens the bootstrap conversation") was never exercised on the non-interactive path; the verification stopped at "the jsonl exists". + +## Non-goals + +- No change to the interactive TTY branch, `defaultCommandRunner`, or the 5m TTY cap — turn 2 `syscall.Exec`s `claude --resume` against turn 1's on-disk result, so the blocking wait stays correct there. +- No change to how the turn works (guide loading, plan→execute chain) — the fix is to stop advertising Resume before the turn is done, not to make the turn faster. +- No vault-ui frontend/backend changes in this spec — the button already renders `⏳ Starting` when no id is present; only the id's timing changes. The "Creating session… up to 2 minutes" modal copy is a separate cosmetic change (Open Question 2). +- No config field for `sessionTurnTimeout` — a tunable const, per Open Question 1. +- No double-Start guard — two concurrent starts are a documented residual risk (Failure Modes row 7), not fixed here. + +## Do-Nothing Option + +Doing nothing keeps the status quo: the button flips to `▶ Resume` at ~10s, clicking it fails ("session not found", partial replay) and risks a second writer on the transcript. That is the exact failure this spec exists to remove — it is not "safe current behavior", it is a shipped bug (Problem). The alternative of reverting spec 040 wholesale (back to the 5m kill) is strictly worse — it reintroduces the kill-mid-write and the "no id persisted on timeout" failure. The 30-min bound is the cost that buys "Starting until done" without either erroring on normal turns or hanging forever on a pathological one. + +- **Interactive branch behavior unchanged.** `defaultCommandRunner`, the 5m TTY cap, and `scenarios/005-work-on-resume-auto-invokes-subtask.md` are untouched. The only edit to the interactive branch is extracting its inline JSON validation into the shared `validateSessionTurn` helper — behavior-preserving, same checks, byte-identical error strings. Evidence: `git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md` (HEAD-relative, so a prompt committing a change before verifying cannot pass). +- **Detachment preserved.** `exec.Command` (NOT `CommandContext`), `Setpgid`, stdout/stderr handling that lets the child survive the parent — never SIGKILL the child on timeout; `--max-turns` is inert (`-1`), so the 30-min bound is a **wait-channel select**, not a context kill. Do NOT resurrect `"claude session start timed out"`. +- **Never offer a broken Resume.** On any failure (exit error, `is_error`, 0 turns, bound expiry, ctx cancel) `StartSession` returns an **error** so the caller persists nothing. Returning `nil` on ctx-cancel is now WRONG (it would persist an id for a still-running child) — the previous spec's cancel-returns-nil only worked because the id was already pre-persisted. +- **JSON validation.** `num_turns > 0` AND `is_error == false`. `session_id` is always present even for a dead session, so checking the id alone never catches a failure. Lowercase UUIDs; keep `-n ""` at mint so resume inherits the title. +- **Error idiom.** `errors.Wrapf(ctx, err, …)`; no `fmt.Errorf`; no bare `return err`; no `context.Background()` in `pkg/`. +- **No compensating clear.** The id is never pre-written, so `clearSessionAndMetrics` / `clearGoalSession` become dead code — delete them. On failure the task simply carries no id. +- **Persist-before-spawn → persist-after-exit is still race-free.** The child has already exited before the post-exit persist, so there is no concurrent writer; the re-read-modify-write preserves the child's frontmatter writes (the [[Fix vault-cli work-on Clobbering Task Frontmatter After Headless Turn]] invariant holds — it is the interactive branch's proven ordering). + +## Design + +### `pkg/ops/claude_session.go` + +1. **Constant rename:** `livenessWindow = 10 * libtime.Second` → `sessionTurnTimeout = 30 * libtime.Minute`. Comment: bounds the wait for the detached turn's exit — never a kill; the child is detached and survives expiry. +2. **`defaultDetachedRunner`:** signature → `func(args []string, dir string, stdout *os.File) (<-chan error, error)`; `cmd.Stdout = stdout` (caller-owned temp file), `cmd.Stderr = devNull` (stderr still discarded — a crash surfaces via exit code; keeps the `os.DevNull` reference). Do NOT close the caller-owned stdout file. Keep `exec.Command`, `Setpgid`, the buffered `done` reaper, and the spawn audit log. +3. **Non-interactive branch** (replaces lines 177–204): + - `outFile, err := os.CreateTemp("", "vault-claude-session-*.json")`; `defer` remove+close (covers spawn failure and the cancel/timeout early returns; unlink-before-close is safe on POSIX while the child may still hold the fd). + - `done, err := c.detachRun(args, cwd, outFile)` (spawn error → wrapped error, unchanged). + - Waiter goroutine: `waitCh <- c.waiter.Wait(ctx, c.sessionTurnTimeout)`. + - `select`: + - `case exitErr := <-done` → non-nil → `errors.Errorf(ctx, "claude session exited with error: %v", exitErr)`; nil → read the file, validate. + - `case err := <-waitCh` → `err != nil` means ctx cancelled → `errors.Wrap(ctx, err, "claude session wait cancelled")`; `err == nil` means bound expired → `errors.Errorf(ctx, "claude session turn did not complete within %v", c.sessionTurnTimeout)`. **Both are errors.** The child survives detached either way. + - After a clean exit: `output, err := os.ReadFile(outFile.Name())` then `validateSessionTurn(output)`. +4. **Extract `validateSessionTurn(output []byte) error`** from the interactive branch's inline block (lines 219–239) — same struct, same checks, **byte-identical error strings** ("parse claude output", "claude returned empty session_id", "claude returned 0 turns: %s", "claude reported error: %s"). Call it from both branches. `defaultCommandRunner` itself is untouched. + +### `pkg/ops/workon.go` + +`handleClaudeSession` non-interactive branch: reorder to **start → persist** (structurally identical to the interactive branch): + +```go +startedAt := libtime.DateOrDateTime(w.currentDateTime.Now().Time()) // captured BEFORE spawn — the turn's true start +if err := w.starter.StartSession(ctx, sessionID, prompt, sessionDir, task.Name, isInteractive); err != nil { + // Nothing was persisted for this id (no pre-spawn write) — nothing to compensate. + return "", nil, errors.Wrap(ctx, err, "start claude session") +} +sessionID, err := persistSessionAndMetrics(ctx, vaultPath, task.Name, sessionID, startedAt, w.taskStorage) +return sessionID, nil, err +``` + +Delete `clearSessionAndMetrics` (dead code). Cached-session path unchanged. Update the doc comments on `persistSessionAndMetrics` and `handleClaudeSession` (the re-read is now load-bearing on both branches). + +### `pkg/ops/goal_workon.go` + +Same reorder — `StartSession` first, then `persistGoalSessionID`. Delete `clearGoalSession`. Cached path unchanged. Doc comments updated. + +### `pkg/ops/export_test.go` + +`const LivenessWindow = livenessWindow` → `const SessionTurnTimeout = sessionTurnTimeout` (comment updated). + +### Interface / mocks + +`ClaudeSessionStarter.StartSession` signature is unchanged — `mocks/claude-session-starter.go` is untouched. + +## Acceptance Criteria + +1. **`StartSession` blocks until the detached child exits (non-interactive).** Unit test: with a blocking waiter, `StartSession` must not return until the test's fake `done` fires; assert the waiter receives `ops.SessionTurnTimeout` (wiring) and that equals `30 * libtime.Minute` (value). Evidence: `grep -c 'Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))' pkg/ops/claude_session_test.go` ≥ 1, `grep -c 'sessionTurnTimeout' pkg/ops/claude_session.go` ≥ 1 (lowercase — the source identifier is unexported; the capital form exists only in `export_test.go`), `grep -c '30 \* libtime.Minute' pkg/ops/claude_session_test.go` ≥ 1. +2. **A clean child exit proceeds to validation and succeeds.** Unit test: fake writes valid JSON to the stdout file, `done <- nil`; `StartSession` returns nil and the temp file is removed. Evidence: `grep -c 'validateSessionTurn' pkg/ops/claude_session.go` ≥ 2 (both branches), temp-file cleanup covered by the test. +3. **Turn JSON is validated on the non-interactive branch** (`is_error`, 0 turns, unparseable). Unit tests: `{"num_turns":0,...}` → "0 turns" error; `{"is_error":true,...}` → "reported error"; empty file → "parse claude output". Same error strings as the interactive branch (lock: existing interactive tests unchanged). +4. **Child exit with error → error, nothing persisted.** Unit test: `done <- errors.New("exit status 1")`, blocking waiter → error wrapping "exit status 1" under the NEW wording "claude session exited with error" — the existing assertion `ContainSubstring("exited during startup")` (claude_session_test.go:317) must be updated to the new string. The "nothing persisted" half is observable only through prompt 2's reworked `workon_test.go` (AC7) — state it there, not here. +5. **Bound expiry → error, nothing persisted.** Unit test: waiter returns nil immediately, `done` never fires → "did not complete within" error. +6. **Ctx cancellation mid-wait → error, child survives.** Unit test: blocking waiter, cancel ctx → error (NOT nil). Integration test: real `sleep 12; touch sentinel` script, cancel at ~1s → StartSession returns <12s with an error, sentinel still appears later (detachment invariant preserved). Evidence: the integration test asserts the sentinel appears after the cancelled return. +7. **`writeTaskAt` is AFTER `childExitAt` (post-exit persist).** `pkg/ops/workon_test.go` / `goal_workon_test.go`: reworked "persisting the session id" tests assert `writeTaskAt.After(childExitAt)` and `writtenSessionID == spawnedSessionID`. Evidence: `grep -c 'After(childExitAt)' pkg/ops/workon_test.go` ≥ 1, `grep -c 'After(childExitAt)' pkg/ops/goal_workon_test.go` ≥ 1. +8. **The writeback invariant survives.** `workon_session_writeback_test.go`: frontmatter the child wrote (phase `execution`, `session_note`) survives the post-exit persist; `ClaudeSessionID() == pinnedSessionID`; `MetricsSessions()` len 1. Assertions byte-identical to today; the fake shape changes in TWO ways: the fakes must write a **valid JSON line to the stdout `*os.File`** (else `StartSession` returns "parse claude output" and the test's `Expect(err).To(BeNil())` fails) AND exit cleanly via `done <- nil` with a blocking waiter. Evidence: the existing assertion greps (`TaskPhaseExecution` 2, `GoalPhaseExecution` 2, `session_note` 4, `MetricsSessions()` 2, `ClaudeSessionID()` 2) still return their pinned counts. +9. **No compensating clear remains.** `grep -c 'clearSessionAndMetrics' pkg/ops/workon.go` == 0, `grep -c 'clearGoalSession' pkg/ops/goal_workon.go` == 0. The old clear-failure tests are deleted. +10. **Interactive branch + scenario 005 untouched.** `git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md` empty (HEAD-relative); `grep -c 'defaultCommandRunner' pkg/ops/claude_session.go` == 3 (current count — the two constructors assign `runCmd: defaultCommandRunner` and the func is defined once; pinned so a rework cannot silently drop it); the 5m TTY cap still present (`grep -c 'context.WithTimeout' pkg/ops/claude_session.go` == 1, on the interactive branch). +11. **Docs/scenario updated.** `docs/work-on-session-lifecycle.md` rewords the liveness-window sections (intro, session-id ownership, "Pre-spawn write ordering" → "Post-exit write ordering", "`--output-format json` fate" → captured+validated, "liveness window" → "turn timeout", "Compensated failure path" → "Failure path" with no-clear). `scenarios/002-task-lifecycle.md` note: "returns within ~10s (the liveness window)" → "blocks until the headless turn completes (typically minutes)". Evidence: `grep -c 'livenessWindow' docs/work-on-session-lifecycle.md` == 0, `grep -ci 'liveness window' docs/work-on-session-lifecycle.md` == 0 (prose form too — the reword must not leave the concept behind under different casing), `grep -c '~10s' scenarios/002-task-lifecycle.md` == 0. +12. **CHANGELOG.** A `## Unreleased` bullet describes the inversion (non-interactive `task work-on`/`goal work-on` wait for the detached turn to exit before persisting `claude_session_id`, bounded by a 30-min turn timeout; TTY branch unchanged). Evidence — `## Unreleased` must exist (v0.116.6 consumed it, so the prompt creates it) and carry the bullet: `grep -c '^## Unreleased' CHANGELOG.md` ≥ 1 AND `grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume'` ≥ 1. +13. **Full gate.** `make precommit` exits 0. + +## Failure Modes + +| Mode | Expected behavior / Detection | Recovery | +|---|---|---| +| Turn exceeds 30-min bound | `StartSession` returns "did not complete within 30m0s" error; no id persisted | Child survives detached and eventually completes; button back to `▶ Start`; re-click starts a fresh turn (the orphaned turn's transcript is unreferenced) | +| Ctx cancelled mid-wait (vault-ui request dies before the turn ends) | `StartSession` returns cancel error; no id persisted | Child survives detached and completes; button shows `▶ Start`; the completed transcript is orphaned unless the user re-runs work-on (which mints a new id) — strictly safer than today's broken Resume | +| Child exits non-zero (bad flag, auth failure, skill crash) | `StartSession` returns "exited with error"; no id persisted | Error surfaces to vault-ui, which clears its own `claude_session_started` flag (vault-ui's sweep — out of this spec's scope), button back to `▶ Start` | +| Turn completes but JSON says `is_error:true` / 0 turns | `validateSessionTurn` returns the interactive branch's error strings; no id persisted | Same recovery as the previous row | +| Temp file unreadable / empty | `validateSessionTurn` → "parse claude output"; no id persisted | No false success; same recovery | +| `claude` binary missing (`ErrStarterUnavailable`) | Unchanged: `StartSession` never constructed (workon.go:119-121 / goal_workon.go:110-112) | vault-ui surfaces the existing "claude binary missing" soft-failure path; task stays `in_progress` with no id — same as today | +| Post-exit persist fails (storage error on the re-read/write) | `persistSessionAndMetrics` error after a clean turn | Turn's frontmatter survives (it wrote it); the id is not persisted → button shows `▶ Start`; user re-clicks, which mints a fresh id and a fresh turn (the completed transcript is orphaned) | +| UI request timeout < turn duration | vault-ui kills the subprocess → ctx cancel path (row 2) | Document as a vault-ui-side consideration (client timeout ≥ the 30-min bound, or treat the error as "still starting"); out of scope for vault-cli | +| Two Start clicks on one task | Second `work-on` short-circuits on the first's id if present; if the first hasn't persisted yet, both spawn | Two detached children can both write the task (last-writer-wins on frontmatter); NOT jsonl corruption — ids differ → different transcripts. No mitigation in this spec (Non-goals); the window widens from ~10s to the whole turn — accepted, documented residual risk | + +## Suggested Decomposition + +Three prompts, driven by the daemon (`autoGeneratePrompts: true`): + +| # | Prompt focus | Covers ACs | Depends on | +|---|---|---|---| +| 1 | `claude_session.go` + `export_test.go` + `claude_session_test.go` + `claude_session_detach_test.go` — constant rename, temp-file capture, `validateSessionTurn` extraction, the select rework, unit/integration test matrix | 1–6, 10 (the `defaultCommandRunner`==3 guard) | — | +| 2 | `workon.go` + `goal_workon.go` + `workon_test.go` + `goal_workon_test.go` + `workon_session_writeback_test.go` — start→persist reorder, delete compensating clears + their tests, writeback fake rework (must write valid JSON to the stdout `*os.File`) | 7–9 | 1 | +| 3 | `docs/work-on-session-lifecycle.md` + `scenarios/002-task-lifecycle.md` + `CHANGELOG.md` — reword + create `## Unreleased` + bullet | 11–12 | 1, 2 | + +AC13 (`make precommit`) is the full-gate check across all three. Spec 040 (`specs/completed/040-…`) is the historical record — do NOT edit it; reference it from this spec. + +## Workaround + +Until this ships: do not click Resume in the Vault UI until the initial work has visibly finished (the card shows `⏳ Starting` then `▶ Resume`; wait for the flip to have settled, or wait several minutes after clicking Start). Prefer the TTY `vault-cli task work-on` path for critical tasks — it blocks through the turn before handing over. + +## Verification + +### Container-executable (runs inside the YOLO container at prompt time) + +``` +make test # exit 0; the new block-until-exit + validation tests run +``` + +``` +grep -c 'sessionTurnTimeout' pkg/ops/claude_session.go # >= 1 (lowercase — source identifier) +grep -c '30 \* libtime.Minute' pkg/ops/claude_session_test.go # >= 1 (value pin) +grep -c 'validateSessionTurn' pkg/ops/claude_session.go # >= 2 (both branches) +grep -c 'Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))' pkg/ops/claude_session_test.go # >= 1 +grep -c 'clearSessionAndMetrics' pkg/ops/workon.go # == 0 +grep -c 'clearGoalSession' pkg/ops/goal_workon.go # == 0 +grep -c 'After(childExitAt)' pkg/ops/workon_test.go # >= 1 +grep -c 'After(childExitAt)' pkg/ops/goal_workon_test.go # >= 1 +# writeback invariant counts (AC8) — must hold exactly, deletion-safe: +grep -c 'TaskPhaseExecution' pkg/ops/workon_session_writeback_test.go # == 2 +grep -c 'GoalPhaseExecution' pkg/ops/workon_session_writeback_test.go # == 2 +grep -c 'session_note' pkg/ops/workon_session_writeback_test.go # == 4 +grep -c 'MetricsSessions()' pkg/ops/workon_session_writeback_test.go # == 2 +grep -c 'ClaudeSessionID()' pkg/ops/workon_session_writeback_test.go # == 2 +# AC10 guards: +grep -c 'defaultCommandRunner' pkg/ops/claude_session.go # == 3 +grep -c 'context.WithTimeout' pkg/ops/claude_session.go # == 1 (interactive branch) +# AC11 docs/scenario reword: +grep -c 'livenessWindow' docs/work-on-session-lifecycle.md # == 0 +grep -ci 'liveness window' docs/work-on-session-lifecycle.md # == 0 (prose form too) +grep -c '~10s' scenarios/002-task-lifecycle.md # == 0 +# AC12 CHANGELOG (Unreleased must exist — v0.116.6 consumed it — and carry the bullet): +grep -c '^## Unreleased' CHANGELOG.md # >= 1 +grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume' # >= 1 +git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md # empty +make precommit # exit 0 +``` + +### Operator-executable (host, after PR merge + release + `make install`; spec verification ladder) + +1. `vault-cli --version` — new version, not `-dirty`. +2. In the Vault UI, click **Start** on a real (non-trivial) task. Confirm the card shows `⏳ Starting` for the whole turn and flips to `▶ Resume` only once it finishes. +3. **This time actually run resume:** click Resume, take the offered `claude --resume ` command, run it in a terminal. Confirm it opens the **completed** bootstrap conversation — no "session not found", no partial replay. +4. Failure path: force a turn failure (e.g. a task whose work-on command errors) and confirm no `claude_session_id` lands on the task and the button reverts to `▶ Start`. + +## Open Questions + +1. Should `sessionTurnTimeout` (30 min) be configurable per-vault, or is a tunable const enough? (Recommend const for now — no second caller exists.) +2. Should the vault-ui "Creating session… up to 2 minutes" modal copy be updated in the same release (cosmetic; separate repo)? From 25b4a28d2d74adc6a2f65f6a9707206a57c7cb32 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Fri, 28 Aug 2026 10:34:03 +0200 Subject: [PATCH 2/4] spec: apply auditor fixes (AC evidence pins, Constraints section, drop H1) --- specs/bug-resume-races-live-headless-turn.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/specs/bug-resume-races-live-headless-turn.md b/specs/bug-resume-races-live-headless-turn.md index aff2e4c..06ee816 100644 --- a/specs/bug-resume-races-live-headless-turn.md +++ b/specs/bug-resume-races-live-headless-turn.md @@ -1,7 +1,6 @@ --- status: draft --- -# Vault UI Resume Races the Live Headless Turn ## Summary @@ -95,6 +94,8 @@ w.starter.StartSession(ctx, sessionID, prompt, sessionDir, task.Name, isInteract Doing nothing keeps the status quo: the button flips to `▶ Resume` at ~10s, clicking it fails ("session not found", partial replay) and risks a second writer on the transcript. That is the exact failure this spec exists to remove — it is not "safe current behavior", it is a shipped bug (Problem). The alternative of reverting spec 040 wholesale (back to the 5m kill) is strictly worse — it reintroduces the kill-mid-write and the "no id persisted on timeout" failure. The 30-min bound is the cost that buys "Starting until done" without either erroring on normal turns or hanging forever on a pathological one. +## Constraints + - **Interactive branch behavior unchanged.** `defaultCommandRunner`, the 5m TTY cap, and `scenarios/005-work-on-resume-auto-invokes-subtask.md` are untouched. The only edit to the interactive branch is extracting its inline JSON validation into the shared `validateSessionTurn` helper — behavior-preserving, same checks, byte-identical error strings. Evidence: `git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md` (HEAD-relative, so a prompt committing a change before verifying cannot pass). - **Detachment preserved.** `exec.Command` (NOT `CommandContext`), `Setpgid`, stdout/stderr handling that lets the child survive the parent — never SIGKILL the child on timeout; `--max-turns` is inert (`-1`), so the 30-min bound is a **wait-channel select**, not a context kill. Do NOT resurrect `"claude session start timed out"`. - **Never offer a broken Resume.** On any failure (exit error, `is_error`, 0 turns, bound expiry, ctx cancel) `StartSession` returns an **error** so the caller persists nothing. Returning `nil` on ctx-cancel is now WRONG (it would persist an id for a still-running child) — the previous spec's cancel-returns-nil only worked because the id was already pre-persisted. @@ -151,9 +152,9 @@ Same reorder — `StartSession` first, then `persistGoalSessionID`. Delete `clea 1. **`StartSession` blocks until the detached child exits (non-interactive).** Unit test: with a blocking waiter, `StartSession` must not return until the test's fake `done` fires; assert the waiter receives `ops.SessionTurnTimeout` (wiring) and that equals `30 * libtime.Minute` (value). Evidence: `grep -c 'Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))' pkg/ops/claude_session_test.go` ≥ 1, `grep -c 'sessionTurnTimeout' pkg/ops/claude_session.go` ≥ 1 (lowercase — the source identifier is unexported; the capital form exists only in `export_test.go`), `grep -c '30 \* libtime.Minute' pkg/ops/claude_session_test.go` ≥ 1. 2. **A clean child exit proceeds to validation and succeeds.** Unit test: fake writes valid JSON to the stdout file, `done <- nil`; `StartSession` returns nil and the temp file is removed. Evidence: `grep -c 'validateSessionTurn' pkg/ops/claude_session.go` ≥ 2 (both branches), temp-file cleanup covered by the test. -3. **Turn JSON is validated on the non-interactive branch** (`is_error`, 0 turns, unparseable). Unit tests: `{"num_turns":0,...}` → "0 turns" error; `{"is_error":true,...}` → "reported error"; empty file → "parse claude output". Same error strings as the interactive branch (lock: existing interactive tests unchanged). -4. **Child exit with error → error, nothing persisted.** Unit test: `done <- errors.New("exit status 1")`, blocking waiter → error wrapping "exit status 1" under the NEW wording "claude session exited with error" — the existing assertion `ContainSubstring("exited during startup")` (claude_session_test.go:317) must be updated to the new string. The "nothing persisted" half is observable only through prompt 2's reworked `workon_test.go` (AC7) — state it there, not here. -5. **Bound expiry → error, nothing persisted.** Unit test: waiter returns nil immediately, `done` never fires → "did not complete within" error. +3. **Turn JSON is validated on the non-interactive branch** (`is_error`, 0 turns, unparseable). Unit tests: `{"num_turns":0,...}` → "0 turns" error; `{"is_error":true,...}` → "reported error"; empty file → "parse claude output". Same error strings as the interactive branch (lock: existing interactive tests unchanged). Evidence: `grep -c '"0 turns"' pkg/ops/claude_session_test.go` ≥ 1, `grep -c 'validateSessionTurn' pkg/ops/claude_session.go` ≥ 2. +4. **Child exit with error → error, nothing persisted.** Unit test: `done <- errors.New("exit status 1")`, blocking waiter → error wrapping "exit status 1" under the NEW wording "claude session exited with error" — the existing assertion `ContainSubstring("exited during startup")` (claude_session_test.go:317) must be updated to the new string. The "nothing persisted" half is observable only through prompt 2's reworked `workon_test.go` (AC7) — state it there, not here. Evidence: `grep -c '"claude session exited with error"' pkg/ops/claude_session.go` ≥ 1, `grep -c 'exited during startup' pkg/ops/claude_session.go` == 0. +5. **Bound expiry → error, nothing persisted.** Unit test: waiter returns nil immediately, `done` never fires → "did not complete within" error. Evidence: `grep -c '"did not complete within"' pkg/ops/claude_session.go` ≥ 1. 6. **Ctx cancellation mid-wait → error, child survives.** Unit test: blocking waiter, cancel ctx → error (NOT nil). Integration test: real `sleep 12; touch sentinel` script, cancel at ~1s → StartSession returns <12s with an error, sentinel still appears later (detachment invariant preserved). Evidence: the integration test asserts the sentinel appears after the cancelled return. 7. **`writeTaskAt` is AFTER `childExitAt` (post-exit persist).** `pkg/ops/workon_test.go` / `goal_workon_test.go`: reworked "persisting the session id" tests assert `writeTaskAt.After(childExitAt)` and `writtenSessionID == spawnedSessionID`. Evidence: `grep -c 'After(childExitAt)' pkg/ops/workon_test.go` ≥ 1, `grep -c 'After(childExitAt)' pkg/ops/goal_workon_test.go` ≥ 1. 8. **The writeback invariant survives.** `workon_session_writeback_test.go`: frontmatter the child wrote (phase `execution`, `session_note`) survives the post-exit persist; `ClaudeSessionID() == pinnedSessionID`; `MetricsSessions()` len 1. Assertions byte-identical to today; the fake shape changes in TWO ways: the fakes must write a **valid JSON line to the stdout `*os.File`** (else `StartSession` returns "parse claude output" and the test's `Expect(err).To(BeNil())` fails) AND exit cleanly via `done <- nil` with a blocking waiter. Evidence: the existing assertion greps (`TaskPhaseExecution` 2, `GoalPhaseExecution` 2, `session_note` 4, `MetricsSessions()` 2, `ClaudeSessionID()` 2) still return their pinned counts. From 247a7897f4f1448816a5993a6ac7b60d4578d08d Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Fri, 28 Aug 2026 11:15:39 +0200 Subject: [PATCH 3/4] persist claude session id only after the headless turn finishes --- CHANGELOG.md | 5 + docs/work-on-session-lifecycle.md | 117 +++++++----- pkg/ops/claude_session.go | 179 ++++++++++++------ pkg/ops/claude_session_detach_test.go | 31 ++- pkg/ops/claude_session_test.go | 154 +++++++++++++-- pkg/ops/export_test.go | 12 +- pkg/ops/goal_workon.go | 84 +++----- pkg/ops/goal_workon_test.go | 109 ++++++----- pkg/ops/workon.go | 102 ++++------ pkg/ops/workon_session_writeback_test.go | 120 ++++-------- pkg/ops/workon_test.go | 130 +++++-------- scenarios/002-task-lifecycle.md | 2 +- ...41-bug-resume-races-live-headless-turn.md} | 4 +- 13 files changed, 575 insertions(+), 474 deletions(-) rename specs/{bug-resume-races-live-headless-turn.md => in-progress/041-bug-resume-races-live-headless-turn.md} (99%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 045ec94..7e3c7e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,11 @@ Please choose versions by [Semantic Versioning](http://semver.org/). * MINOR version when you add functionality in a backwards-compatible manner, and * PATCH version when you make backwards-compatible bug fixes. +## Unreleased + +- fix: non-interactive `task work-on` / `goal work-on` now wait for the detached headless turn to finish before persisting `claude_session_id`, so the Vault UI only offers Resume against a complete transcript — previously the id landed within ~10s while the turn was still writing, and `claude --resume` failed with "session not found" or replayed partial output. The wait is bounded by a 30m turn timeout (a wait bound, never a kill — the child stays detached), and the turn's JSON result is now validated on both branches, so a failed or zero-turn session persists no id at all. The interactive TTY branch is unchanged. +- fix: a failed session-id persist (re-read or write error) no longer reports the id back to the caller — nothing landed on disk, so returning it advertised a session the Vault UI could not resume. Affects `task work-on` and `goal work-on` on every branch, including the cached-session path. + ## v0.116.6 - fix: bump errcheck to v1.20.0 for Go 1.27 compatibility diff --git a/docs/work-on-session-lifecycle.md b/docs/work-on-session-lifecycle.md index 60ff47e..588d1d0 100644 --- a/docs/work-on-session-lifecycle.md +++ b/docs/work-on-session-lifecycle.md @@ -1,9 +1,16 @@ # Work-on Session Lifecycle This document records the design decisions behind the `work-on` session-start fix -(spec 040): why `task work-on` / `goal work-on` return a session id within ~10 -seconds on a non-TTY caller instead of blocking for the whole headless bootstrap -turn, and what each branch of `StartSession` does and does not guarantee. +(spec 040, revised by spec 041): why `task work-on` / `goal work-on` persist a +session id only once the headless bootstrap turn has finished on a non-TTY caller, +and what each branch of `StartSession` does and does not guarantee. + +Spec 040 originally had the non-interactive branch return within ~10 seconds while +the turn kept running. That shipped a worse bug: the Vault UI offers **Resume** as +soon as `claude_session_id` appears, so it advertised a session whose transcript was +still being written — `claude --resume` failed, and a second writer could land on the +same jsonl. Spec 041 inverted it. **An id on disk now means the session is +resumable**, not merely that one exists. It is decisions only — implementation details live in the code and its doc comments. The spec's Design section is the source; this file is its durable form. @@ -19,22 +26,25 @@ invents or substitutes an id. `claude --session-id ` is documented by the client itself as *"Use a specific session ID for the conversation (must be a valid UUID)"* (verified 2026-08-27). The -caller mints the id so it can be persisted to the task/goal file **before the child -process exists** — the ordering that makes the fix a guarantee rather than a race. +caller mints the id so it can be passed to the child and correlated with the +transcript the child writes. + +## Post-exit write ordering -## Pre-spawn write ordering +The id — and, on the task path, its `metrics_sessions` entry — are persisted only +**after the child has exited**. On the non-interactive branch `StartSession` is +called first and `persistSessionAndMetrics` (task) / `persistGoalSessionID` (goal) +run only when it returns cleanly. -The id — and, on the task path, its `metrics_sessions` entry — are persisted while -**no child exists**. `persistSessionAndMetrics` (task) and `persistGoalSessionID` -(goal) run before `StartSession` is called on the non-interactive branch, so the -session's own read-modify-write always reads a file that already contains the id. +This is the load-bearing part of the fix, and it is what makes the id trustworthy: +the id is the UI's signal that Resume will work, so it must not appear while a child +still holds the transcript. Writing after the child exits is also race-free — there +is no concurrent writer left — and the re-read before writing is load-bearing on +**every** branch, because the turn mutates the same file it is being written to. -This is the load-bearing part of the fix. Before it, the id was written *after* the -headless turn returned, and the re-read existed because the turn mutated the very -file being written. With persist-before-spawn there is no post-turn id write to -revert on the fresh-start path; the re-read remains load-bearing on the interactive -branch and the cached-session path, where the turn may still mutate the file before -the post-return persist. +On any failure — child exit error, invalid turn JSON, bound expiry, ctx cancel — +nothing is persisted. There is no compensating clear because there is nothing to +undo, and the task keeps whatever frontmatter the child wrote. ## Why stream-json was rejected @@ -42,8 +52,9 @@ the post-return persist. event within ~1 second, which looks like a faster answer. It was rejected: returning early from a stream would reintroduce the exact hazard this fix removes, from the opposite direction — the parent would stop waiting on a live child that the request -context could still kill mid-write. The liveness window waits on the child's exit, -not on a message the child emits. +context could still kill mid-write. The non-interactive branch waits on the child's +exit, not on a message the child emits — and an init event says only that a session +started, which is exactly the claim that proved untrustworthy. ## Why the TTY branch is untouched @@ -58,28 +69,48 @@ interactive` from a pipe takes the blocking path. ## The fate of --output-format json -Spec Open Question 4 is decided: `--output-format json` is **kept on both -branches**. The interactive branch still validates the JSON blob, so the flag is -required there. On the non-interactive branch it is harmless dead weight — the -detached child's stdout goes to `os.DevNull` and nothing parses the blob — and -dropping it would add risk for no gain: it would create a second argv difference -between the branches and one more way for the two paths to diverge. - -## What the liveness window does and does not cover - -On the non-interactive branch `StartSession` waits for `livenessWindow` (10s, -tunable) on a channel fed by the child's `Wait`. An exit inside the window is a real -failure and returns an error naming the child's exit status. The window covers the -failure mode that actually bites: a session that dies on startup (bad flag, auth -failure). It is deliberately **not** an inactivity watchdog — a session that hangs -*after* starting is left to the Vault UI's existing `claude_session_started` cleanup -sweep, which is out of scope here. - -## Compensated failure path - -When the spawn fails inside the liveness window, the pre-persisted state is rolled -back: the caller re-reads the task (or goal) from disk and clears only the -`claude_session_id` and, on the task path, this run's `metrics_sessions` entry. The -clear is itself a re-read-modify-write, so any frontmatter the child wrote before -dying (for example `phase: planning` at 8s) survives. A failed clear is surfaced as -a warning rather than masking the spawn error. +`--output-format json` is **kept on both branches**, and on both it is now +load-bearing. The interactive branch validates the blob from `cmd.Output()`. The +non-interactive branch redirects the detached child's stdout to a caller-owned temp +file and validates the same blob after the child exits, through the shared +`validateSessionTurn` helper. + +Validation is not optional. `claude` reports a `session_id` even for a turn that did +no work or failed outright, so an unvalidated id would be handed to the operator as +resumable when it is not — the same class of lie this fix exists to remove. A turn +whose result is `num_turns: 0`, `is_error: true`, or unparseable is an error, and no +id is persisted. + +A temp **file** rather than a pipe is deliberate: the child writes to an inherited fd +with no reader, so there is no pipe-buffer deadlock and no EPIPE if the parent goes +away, and the file is complete once `cmd.Wait()` returns. It is unlinked eagerly, so +no path — including cancel and timeout, where the child still holds the fd — leaves +anything behind. Stderr still goes to `os.DevNull`; a crash surfaces via exit code. + +## What the turn timeout does and does not cover + +On the non-interactive branch `StartSession` blocks on a channel fed by the child's +`Wait`, bounded by `sessionTurnTimeout` (30m, tunable). The bound is a **wait bound, +never a kill**: the child is detached in its own process group and survives expiry — +the parent only stops waiting. `--max-turns` is inert (`maxTurns` is -1), so a +legitimate agentic chain can run for minutes; 30m is roughly 6-10x the observed turn +length, chosen to bound a pathological hang without cutting off normal work. + +Expiry, ctx cancellation, and a non-zero child exit all return an error, so the +caller persists nothing and the UI keeps showing **Start** rather than offering a +Resume that cannot work. This is deliberately **not** an inactivity watchdog — a +session that hangs after starting is left to the Vault UI's existing +`claude_session_started` cleanup sweep, which is out of scope here. + +## Failure path + +Nothing is persisted on any failure, so there is nothing to compensate for. The +previous design pre-wrote the id and needed a re-read-modify-write to clear it after +a failed spawn; with post-exit ordering that path is gone entirely, along with its +"failed to clear" warning. Frontmatter the child wrote before failing (for example +`phase: planning`) is untouched, because the caller never writes on the failure path. + +The persist step itself can also fail — the re-read or the write. When it does, the +caller is handed an **empty** id, never the one it minted. Nothing landed on disk, so +reporting the id would advertise a session the UI cannot resume, which is the same lie +in a different place. The rule is uniform: the id is returned only when it is on disk. diff --git a/pkg/ops/claude_session.go b/pkg/ops/claude_session.go index 49ee9e0..4ed4b0e 100644 --- a/pkg/ops/claude_session.go +++ b/pkg/ops/claude_session.go @@ -27,18 +27,25 @@ type ClaudeSessionStarter interface { // (not by claude) so it can be persisted before the child process exists. // When name is non-empty, the session is created with -n so its // custom-title and agent-name are set from turn 1. - // On the interactive branch it blocks until the headless turn completes (bounded - // by a 5m timeout) and validates the JSON result. On the non-interactive branch - // it spawns the child detached from the request context and returns within the - // liveness window once the child has proven it survives startup. See + // Both branches block until the headless turn completes and validate its JSON + // result; they differ in how. The interactive branch runs the child under the + // request context (bounded by a 5m timeout). The non-interactive branch spawns + // the child detached from the request context and waits for its exit, bounded by + // sessionTurnTimeout — a wait bound, not a kill. Any outcome other than a clean, + // validated turn returns an error, so the caller persists no session id and the + // UI never offers Resume against a live or failed transcript. See // docs/work-on-session-lifecycle.md. StartSession(ctx context.Context, sessionID string, prompt string, cwd string, name string, isInteractive bool) error } -// livenessWindow is how long the non-interactive branch waits for the detached child -// to prove it survived startup (auth failure, bad flag). Tunable constant; no config -// field unless a second caller needs one. -const livenessWindow = 10 * libtime.Second +// sessionTurnTimeout bounds how long the non-interactive branch waits for the detached +// child's headless turn to finish. It is a wait bound, never a kill: the child is +// detached (own process group) and survives expiry — the parent simply stops waiting +// and reports an error so the caller persists no session id. --max-turns is inert +// (maxTurns is -1), so a legitimate agentic chain can run for minutes; 30m is ~6-10x +// the observed turn length. Tunable constant; no config field unless a second caller +// needs one. +const sessionTurnTimeout = 30 * libtime.Minute // NewClaudeSessionStarter creates a ClaudeSessionStarter using the given claude script. // Returns nil if the binary is not found. @@ -48,12 +55,12 @@ func NewClaudeSessionStarter(claudeScript string) ClaudeSessionStarter { return nil } return &claudeSessionStarter{ - claudePath: claudePath, - maxTurns: -1, - runCmd: defaultCommandRunner, - detachRun: defaultDetachedRunner, - waiter: libtime.NewWaiterDuration(), - livenessWindow: livenessWindow, + claudePath: claudePath, + maxTurns: -1, + runCmd: defaultCommandRunner, + detachRun: defaultDetachedRunner, + waiter: libtime.NewWaiterDuration(), + sessionTurnTimeout: sessionTurnTimeout, } } @@ -62,16 +69,16 @@ func NewClaudeSessionStarter(claudeScript string) ClaudeSessionStarter { func NewClaudeSessionStarterWithRunner( claudePath string, runCmd func(ctx context.Context, args []string, dir string) ([]byte, error), - detachRun func(args []string, dir string) (<-chan error, error), + detachRun func(args []string, dir string, stdout *os.File) (<-chan error, error), waiter libtime.WaiterDuration, ) ClaudeSessionStarter { return &claudeSessionStarter{ - claudePath: claudePath, - maxTurns: -1, - runCmd: runCmd, - detachRun: detachRun, - waiter: waiter, - livenessWindow: livenessWindow, + claudePath: claudePath, + maxTurns: -1, + runCmd: runCmd, + detachRun: detachRun, + waiter: waiter, + sessionTurnTimeout: sessionTurnTimeout, } } @@ -97,24 +104,29 @@ func defaultCommandRunner(ctx context.Context, args []string, dir string) ([]byt } // defaultDetachedRunner is the non-interactive runner. It spawns the child detached -// from the request context: exec.Command (not CommandContext), stdout/stderr -// redirected to os.DevNull so the child never dies on EPIPE when the parent exits, -// and Setpgid so it lives in its own process group and survives the parent. It -// returns a buffered channel that receives cmd.Wait()'s error, plus a spawn error -// when Start fails. The child may outlive this process by minutes; that is the -// point of the detachment, not an accident. -func defaultDetachedRunner(args []string, dir string) (<-chan error, error) { +// from the request context: exec.Command (not CommandContext), stderr redirected to +// os.DevNull so the child never dies on EPIPE when the parent exits, and Setpgid so +// it lives in its own process group and survives the parent. It returns a buffered +// channel that receives cmd.Wait()'s error, plus a spawn error when Start fails. The +// child may outlive this process by minutes; that is the point of the detachment, +// not an accident. +// +// stdout is the caller-owned temp file the turn's --output-format json blob lands in. +// A file (not a pipe) is deliberate: the child writes to an inherited fd with no +// reader, so there is no pipe-buffer deadlock and no EPIPE, and the file is complete +// once cmd.Wait() returns. The caller owns its lifecycle — this function never closes it. +func defaultDetachedRunner(args []string, dir string, stdout *os.File) (<-chan error, error) { cmd := exec.Command(args[0], args[1:]...) //#nosec G204 -- args[0] is the claude binary path from LookPath cmd.Dir = dir // os.DevNull is a string constant ("/dev/null"), NOT an io.Writer — assigning it - // directly to cmd.Stdout does not compile. Open it as a file. Leaving Stdout/Stderr - // nil would also route to /dev/null, but AC1 requires an explicit os.DevNull - // reference so the detachment is deliberate rather than accidental. + // directly to cmd.Stderr does not compile. Open it as a file. Leaving Stderr nil + // would also route to /dev/null, but the explicit os.DevNull reference keeps the + // detachment deliberate rather than accidental. devNull, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0) if err != nil { return nil, err // caller wraps with ctx; never context.Background() here } - cmd.Stdout = devNull + cmd.Stdout = stdout cmd.Stderr = devNull cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} if err := cmd.Start(); err != nil { @@ -148,10 +160,10 @@ type claudeSessionStarter struct { // (both constructors hardcode -1, no test sets it positive), // but dropping it silently changes the struct's contract — // out of scope for this bug fix. - runCmd func(ctx context.Context, args []string, dir string) ([]byte, error) - detachRun func(args []string, dir string) (<-chan error, error) - waiter libtime.WaiterDuration - livenessWindow libtime.Duration + runCmd func(ctx context.Context, args []string, dir string) ([]byte, error) + detachRun func(args []string, dir string, stdout *os.File) (<-chan error, error) + waiter libtime.WaiterDuration + sessionTurnTimeout libtime.Duration } func (c *claudeSessionStarter) StartSession( @@ -175,32 +187,7 @@ func (c *claudeSessionStarter) StartSession( } if !isInteractive { - // Non-interactive branch: spawn detached and return once the child has - // outlived the liveness window. The child keeps running after this process - // exits (the Vault UI Start button gets its session id back in ~10s). - done, err := c.detachRun(args, cwd) - if err != nil { - return errors.Wrap(ctx, err, "start detached claude session") - } - waitCh := make(chan error, 1) - // Raw go func is deliberate here (go-concurrency/no-raw-go-func): this adapts the - // injectable waiter into a channel so the select below can race it against the - // child's exit. Bounded by livenessWindow, buffered with capacity 1 and exactly - // one send, so it neither leaks nor blocks when the child wins the race. - go func() { - waitCh <- c.waiter.Wait(ctx, c.livenessWindow) - }() - select { - case exitErr := <-done: - return errors.Errorf(ctx, "claude session exited during startup: %v", exitErr) - case err := <-waitCh: - if err != nil { - // The request context was cancelled mid-window. The child is detached - // and survives on its own; the parent is exiting anyway. - return nil - } - return nil - } + return c.runDetachedTurn(ctx, args, cwd) } // Interactive branch, unchanged behaviour: block through the headless turn so @@ -216,6 +203,74 @@ func (c *claudeSessionStarter) StartSession( return errors.Wrap(ctx, err, "run claude") } + return validateSessionTurn(ctx, output) +} + +// runDetachedTurn spawns the child detached and blocks until its headless turn +// finishes. Returning early would hand the caller a session id whose transcript is +// still being written — the Vault UI would offer Resume against a live, +// single-writer-assumed jsonl and `claude --resume` would fail. So every exit path +// except a clean, validated turn returns an error, and the caller persists nothing. +func (c *claudeSessionStarter) runDetachedTurn( + ctx context.Context, + args []string, + cwd string, +) error { + outFile, err := os.CreateTemp("", "vault-claude-session-*.json") + if err != nil { + return errors.Wrap(ctx, err, "create claude output file") + } + // Unlink eagerly: on POSIX the child keeps its inherited fd, so the data stays + // readable to us until we close, and nothing is left behind on any return path + // (including the cancel/timeout ones, where the child still holds the fd). + defer func() { + _ = os.Remove(outFile.Name()) + _ = outFile.Close() + }() + + done, err := c.detachRun(args, cwd, outFile) + if err != nil { + return errors.Wrap(ctx, err, "start detached claude session") + } + waitCh := make(chan error, 1) + // Raw go func is deliberate here (go-concurrency/no-raw-go-func): this adapts the + // injectable waiter into a channel so the select below can race it against the + // child's exit. Bounded by sessionTurnTimeout, buffered with capacity 1 and + // exactly one send, so it neither leaks nor blocks when the child wins the race. + go func() { + waitCh <- c.waiter.Wait(ctx, c.sessionTurnTimeout) + }() + select { + case exitErr := <-done: + if exitErr != nil { + return errors.Errorf(ctx, "claude session exited with error: %v", exitErr) + } + case err := <-waitCh: + // Both outcomes are errors so the caller persists no session id. The child + // is detached and keeps running in either case; we only stop waiting on it. + if err != nil { + return errors.Wrap(ctx, err, "claude session wait cancelled") + } + return errors.Errorf( + ctx, + "claude session turn did not complete within %v", + c.sessionTurnTimeout, + ) + } + + // The child has exited, so its fd is closed and the file is complete. + output, err := os.ReadFile(outFile.Name()) + if err != nil { + return errors.Wrap(ctx, err, "read claude output") + } + return validateSessionTurn(ctx, output) +} + +// validateSessionTurn checks the --output-format json blob a finished headless turn +// emits. Shared by both branches: a session id alone proves nothing, because claude +// reports one even for a turn that did no work or failed, so an unvalidated id would +// be handed to the operator as resumable when it is not. +func validateSessionTurn(ctx context.Context, output []byte) error { var result struct { SessionID string `json:"session_id"` NumTurns int `json:"num_turns"` diff --git a/pkg/ops/claude_session_detach_test.go b/pkg/ops/claude_session_detach_test.go index 1395961..6183ec2 100644 --- a/pkg/ops/claude_session_detach_test.go +++ b/pkg/ops/claude_session_detach_test.go @@ -8,6 +8,7 @@ import ( "context" "os" "path/filepath" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -16,23 +17,37 @@ import ( ) var _ = Describe("ClaudeSessionStarter detachment (integration)", func() { - It("child outlives the parent and survives context cancellation", func() { + // The non-interactive branch waits for the child's headless turn to finish, but + // the wait is a bound, never a kill: the child runs in its own process group and + // must survive the parent giving up on it. Cancelling the context is the cheapest + // way to make the parent stop waiting while the child is still mid-turn. + It("child outlives a cancelled parent wait", func() { dir, err := os.MkdirTemp("", "vault-claude-detach-*") Expect(err).To(BeNil()) defer os.RemoveAll(dir) sentinel := filepath.Join(dir, "sentinel.txt") script := filepath.Join(dir, "worker.sh") - Expect(os.WriteFile(script, []byte("#!/bin/sh\nsleep 12\ntouch "+sentinel+"\n"), 0755)).To(Succeed()) + Expect(os.WriteFile(script, []byte("#!/bin/sh\nsleep 6\ntouch "+sentinel+"\n"), 0755)).To(Succeed()) + starter := ops.NewClaudeSessionStarter(script) Expect(starter).NotTo(BeNil()) + ctx, cancel := context.WithCancel(context.Background()) - // The child sleeps past the liveness window (12s > 10s), so StartSession - // waits out the real window and returns nil while the child still runs. + defer cancel() + go func() { + defer GinkgoRecover() + time.Sleep(500 * time.Millisecond) + cancel() + }() + + // The parent stops waiting well before the child's 6s turn ends, so it must + // report an error and persist no session id. err = starter.StartSession(ctx, "123e4567-e89b-12d3-a456-426614174000", "prompt", dir, "worker", false) - Expect(err).To(BeNil()) - cancel() - // The sentinel appears only because the detached child survived the - // parent's context cancellation and process exit. + Expect(err).To(HaveOccurred()) + Expect(os.Stat(sentinel)).Error().To(HaveOccurred()) + + // The sentinel appears only because the detached child survived the parent's + // context cancellation and ran its turn to completion. Eventually(func() bool { _, statErr := os.Stat(sentinel) return statErr == nil diff --git a/pkg/ops/claude_session_test.go b/pkg/ops/claude_session_test.go index c287e82..d9f8270 100644 --- a/pkg/ops/claude_session_test.go +++ b/pkg/ops/claude_session_test.go @@ -7,6 +7,7 @@ package ops_test import ( "context" "errors" + "os" "time" libtime "github.com/bborbe/time" @@ -249,27 +250,43 @@ var _ = Describe("ClaudeSessionStarter", func() { doneCh chan error detachErr error capturedWindow libtime.Duration + blockWaiter chan struct{} ) + // validTurnJSON is what a clean headless turn writes to the captured stdout + // file. Every success-path fake must write it, or StartSession fails + // validation with "parse claude output". + const validTurnJSON = `{"session_id":"session-abc","num_turns":3,"is_error":false,"result":"done"}` + BeforeEach(func() { detachArgs = nil detachDir = "" - doneCh = make(chan error) + doneCh = make(chan error, 1) detachErr = nil capturedWindow = 0 + // The waiter must BLOCK on the success paths. StartSession selects on the + // child-exit channel and the waiter channel; a waiter that returns + // immediately makes both ready and the select picks nondeterministically, + // flipping between success and a spurious turn-timeout error. + blockWaiter = make(chan struct{}) + DeferCleanup(func() { close(blockWaiter) }) }) JustBeforeEach(func() { starter = ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, - func(args []string, dir string) (<-chan error, error) { + func(args []string, dir string, stdout *os.File) (<-chan error, error) { detachArgs = args detachDir = dir + if stdout != nil { + _, _ = stdout.WriteString(validTurnJSON) + } return doneCh, detachErr }, libtime.WaiterDurationFunc(func(_ context.Context, d libtime.Duration) error { capturedWindow = d + <-blockWaiter return nil }), ) @@ -277,6 +294,7 @@ var _ = Describe("ClaudeSessionStarter", func() { It("passes the session id and name to the detached runner", func() { sessionID := "123e4567-e89b-12d3-a456-426614174000" + doneCh <- nil err := starter.StartSession(ctx, sessionID, "prompt", "/my/vault", "My Task", false) Expect(err).To(BeNil()) Expect(detachArgs).To(ContainElement("--session-id")) @@ -289,42 +307,146 @@ var _ = Describe("ClaudeSessionStarter", func() { Expect(err).To(BeNil()) }) - It("waits for the liveness window", func() { - err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) - Expect(err).To(BeNil()) + It("blocks until the detached child exits", func() { + returned := make(chan error, 1) + go func() { + returned <- starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) + }() + // The child has not exited, so StartSession must still be waiting. This is + // the whole point of the fix: returning here would hand the caller an id + // whose transcript is still being written. + Consistently(returned, "100ms").ShouldNot(Receive()) + doneCh <- nil + Eventually(returned).Should(Receive(BeNil())) // Locks the wiring: StartSession hands the constant, not a stray literal. - Expect(capturedWindow).To(Equal(ops.LivenessWindow)) - // Locks the value: LivenessWindow is an alias for livenessWindow, so the - // line above moves with the constant and would survive any retune. This - // line is the one that fails when the window is changed. - Expect(capturedWindow).To(Equal(10 * libtime.Second)) + Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout)) + // Locks the value: SessionTurnTimeout is an alias, so the line above moves + // with the constant and would survive any retune. This line is the one that + // fails when the bound is changed. + Expect(capturedWindow).To(Equal(30 * libtime.Minute)) + }) + + It("validates the turn and rejects a zero-turn result", func() { + starter = ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, stdout *os.File) (<-chan error, error) { + _, _ = stdout.WriteString(`{"session_id":"session-abc","num_turns":0,"is_error":false,"result":"Unknown command"}`) + done := make(chan error, 1) + done <- nil + return done, nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-blockWaiter + return nil + }), + ) + err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("0 turns")) + }) + + It("validates the turn and rejects an is_error result", func() { + starter = ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, stdout *os.File) (<-chan error, error) { + _, _ = stdout.WriteString(`{"session_id":"session-abc","num_turns":2,"is_error":true,"result":"boom"}`) + done := make(chan error, 1) + done <- nil + return done, nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-blockWaiter + return nil + }), + ) + err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("claude reported error")) + }) + + It("rejects an unparseable turn result", func() { + starter = ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, _ *os.File) (<-chan error, error) { + done := make(chan error, 1) + done <- nil + return done, nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-blockWaiter + return nil + }), + ) + err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("parse claude output")) }) - It("treats an early exit as an error", func() { + It("treats a child exit error as an error", func() { earlyDone := make(chan error, 1) earlyDone <- errors.New("exit status 1") starter = ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, - func(_ []string, _ string) (<-chan error, error) { + func(_ []string, _ string, _ *os.File) (<-chan error, error) { return earlyDone, nil }, - libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { return nil }), + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-blockWaiter + return nil + }), ) err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("exit status 1")) - Expect(err.Error()).To(ContainSubstring("exited during startup")) + Expect(err.Error()).To(ContainSubstring("exited with error")) + }) + + It("treats the turn timeout as an error so no id is persisted", func() { + starter = ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, _ *os.File) (<-chan error, error) { + // Child never exits within the bound. + return make(chan error), nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { return nil }), + ) + err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("did not complete within")) + }) + + It("treats context cancellation as an error so no id is persisted", func() { + starter = ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, _ *os.File) (<-chan error, error) { + return make(chan error), nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + return context.Canceled + }), + ) + err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("wait cancelled")) }) It("wraps a spawn failure", func() { starter = ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, - func(_ []string, _ string) (<-chan error, error) { + func(_ []string, _ string, _ *os.File) (<-chan error, error) { return nil, ErrTest }, - libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { return nil }), + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-blockWaiter + return nil + }), ) err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) Expect(err).To(HaveOccurred()) diff --git a/pkg/ops/export_test.go b/pkg/ops/export_test.go index 0587732..7a9bf8a 100644 --- a/pkg/ops/export_test.go +++ b/pkg/ops/export_test.go @@ -4,8 +4,12 @@ package ops -// LivenessWindow exposes the unexported livenessWindow constant to the external -// ops_test package so tests can assert the duration StartSession hands to its -// waiter against the real value, rather than against a copied literal. Test-only: +// SessionTurnTimeout exposes the unexported sessionTurnTimeout constant to the +// external ops_test package so tests can assert the duration StartSession hands to +// its waiter against the real value, rather than against a copied literal. Test-only: // this file is a _test.go file and is not part of the package's public API. -const LivenessWindow = livenessWindow +// +// Note this is an alias: asserting only against it locks the wiring (StartSession +// passes the constant, not a stray literal) but NOT the value — a retune moves both +// sides. Tests must also assert the literal to lock the value. +const SessionTurnTimeout = sessionTurnTimeout diff --git a/pkg/ops/goal_workon.go b/pkg/ops/goal_workon.go index 43393f4..4938245 100644 --- a/pkg/ops/goal_workon.go +++ b/pkg/ops/goal_workon.go @@ -104,8 +104,7 @@ func (g *goalWorkOnOperation) Execute( ) } - sessionID, sessionWarnings, sessionErr := g.handleClaudeSession(ctx, goal, vaultPath, sessionDir, vault, isInteractive) - warnings = append(warnings, sessionWarnings...) + sessionID, sessionErr := g.handleClaudeSession(ctx, goal, vaultPath, sessionDir, vault, isInteractive) if sessionErr != nil { if errors.Is(sessionErr, ErrStarterUnavailable) { // Soft failure — claude binary missing. Spec 014 Failure Modes table: @@ -165,11 +164,13 @@ func applyGoalAssigneeMatrix(goal *domain.Goal, assignee string) string { } // persistGoalSessionID re-reads the goal from disk and writes back only the session id. -// The re-read is load-bearing on the interactive branch: the headless turn may mutate -// the file before the post-return persist, so writing the stale in-memory copy would -// revert the session's own frontmatter changes. On the non-interactive branch the -// persist runs before the child exists, so the session's own read-modify-write reads a -// file that already contains the id. +// The re-read is load-bearing on every branch: the headless turn mutates the same file +// and always finishes before this runs, so writing the stale in-memory copy would +// revert the session's own frontmatter changes. +// +// On failure it returns an empty id, never the one it was handed. The id is the Vault +// UI's signal that Resume will work; reporting an id whose write did not land would +// advertise a session that is not on disk. func persistGoalSessionID( ctx context.Context, vaultPath string, @@ -179,23 +180,21 @@ func persistGoalSessionID( ) (string, error) { refreshed, err := goalStorage.FindGoalByName(ctx, vaultPath, goalName) if err != nil { - return sessionID, errors.Wrap(ctx, err, "re-read goal after claude session") + return "", errors.Wrap(ctx, err, "re-read goal after claude session") } refreshed.SetClaudeSessionID(sessionID) if err := goalStorage.WriteGoal(ctx, refreshed); err != nil { - return sessionID, errors.Wrap(ctx, err, "save session id to goal") + return "", errors.Wrap(ctx, err, "save session id to goal") } return sessionID, nil } // handleClaudeSession starts or returns an existing Claude session for the goal. -// On the non-interactive branch the session id is persisted BEFORE the child is -// spawned, so the session's own read-modify-write always reads a file that already -// contains the id. A spawn failure inside the liveness window triggers a -// compensating re-read-based clear that removes the id while preserving any -// frontmatter the child wrote before dying; a failed clear is surfaced as a warning -// rather than masking the spawn error. On the interactive branch the id is persisted -// after the headless turn returns so frontmatter the session itself wrote survives. +// On both branches the session id is persisted only AFTER the headless turn has +// finished cleanly, so an id on disk means the session is resumable rather than merely +// that one was started. Nothing is written on any failure path, so there is no +// compensating clear: frontmatter the child wrote before failing stays untouched, and +// the Vault UI correctly keeps offering Start. func (g *goalWorkOnOperation) handleClaudeSession( ctx context.Context, goal *domain.Goal, @@ -203,12 +202,12 @@ func (g *goalWorkOnOperation) handleClaudeSession( sessionDir string, vault *config.Vault, isInteractive bool, -) (string, []string, error) { +) (string, error) { if existing := goal.ClaudeSessionID(); existing != "" { - return existing, nil, nil + return existing, nil } if g.starter == nil { - return "", nil, ErrStarterUnavailable + return "", ErrStarterUnavailable } // The bootstrap always runs headless `claude --print`, which cannot answer // AskUserQuestion; --non-interactive tells the work-on command to take safe @@ -220,45 +219,20 @@ func (g *goalWorkOnOperation) handleClaudeSession( // TTY branch, unchanged: block through the headless turn, then re-read and // persist so frontmatter the session itself wrote survives. if err := g.starter.StartSession(ctx, sessionID, prompt, sessionDir, goal.Name, isInteractive); err != nil { - return "", nil, errors.Wrap(ctx, err, "start claude session") + return "", errors.Wrap(ctx, err, "start claude session") } sessionID, err := persistGoalSessionID(ctx, vaultPath, goal.Name, sessionID, g.goalStorage) - return sessionID, nil, err - } - // Non-interactive branch: persist the id BEFORE the child exists, so the session's - // own read-modify-write always reads a file that already contains it. - if _, err := persistGoalSessionID(ctx, vaultPath, goal.Name, sessionID, g.goalStorage); err != nil { - return "", nil, errors.Wrap(ctx, err, "persist claude session id before spawn") + return sessionID, err } + // Non-interactive branch: persist the id only AFTER the turn has finished. An id on + // disk is what makes the Vault UI offer Resume, and a resume against a still-running + // turn hits a transcript another process is mid-write on. On any failure nothing is + // persisted, so the button correctly stays on Start. if err := g.starter.StartSession(ctx, sessionID, prompt, sessionDir, goal.Name, isInteractive); err != nil { - // Compensating clear: the child may have written frontmatter before dying - // inside the window (e.g. phase: execution). Re-read and clear only the id, - // preserving every other field on disk. - if clearErr := g.clearGoalSession(ctx, vaultPath, goal.Name); clearErr != nil { - return "", []string{fmt.Sprintf("failed to clear claude session id after spawn failure: %v", clearErr)}, - errors.Wrap(ctx, err, "start claude session") - } - return "", nil, errors.Wrap(ctx, err, "start claude session") - } - return sessionID, nil, nil -} - -// clearGoalSession re-reads the goal after a spawn failure and clears only the -// claude_session_id, preserving any frontmatter the child wrote before dying. -// The re-read is load-bearing: clearing from the stale in-memory copy would -// revert the child's writes. -func (g *goalWorkOnOperation) clearGoalSession( - ctx context.Context, - vaultPath string, - goalName string, -) error { - refreshed, err := g.goalStorage.FindGoalByName(ctx, vaultPath, goalName) - if err != nil { - return errors.Wrap(ctx, err, "re-read goal after spawn failure") - } - refreshed.ClearClaudeSessionID() - if err := g.goalStorage.WriteGoal(ctx, refreshed); err != nil { - return errors.Wrap(ctx, err, "clear goal session id after spawn failure") + // No compensating clear needed: nothing was written for this id, so there is + // nothing to undo. Frontmatter the child wrote before failing stays untouched. + return "", errors.Wrap(ctx, err, "start claude session") } - return nil + sessionID, err := persistGoalSessionID(ctx, vaultPath, goal.Name, sessionID, g.goalStorage) + return sessionID, err } diff --git a/pkg/ops/goal_workon_test.go b/pkg/ops/goal_workon_test.go index 925b433..b4c3274 100644 --- a/pkg/ops/goal_workon_test.go +++ b/pkg/ops/goal_workon_test.go @@ -91,8 +91,8 @@ var _ = Describe("GoalWorkOnOperation", func() { }) It("calls FindGoalByName", func() { - // Twice: once to load the goal, once to re-read it before spawning so the - // session id lands on the freshest on-disk state. + // Twice: once to load the goal, once to re-read it after the child exits + // so the session id lands on the freshest on-disk state. Expect(mockGoalStorage.FindGoalByNameCallCount()).To(Equal(2)) actualCtx, actualVaultPath, actualGoalName := mockGoalStorage.FindGoalByNameArgsForCall( 0, @@ -102,11 +102,11 @@ var _ = Describe("GoalWorkOnOperation", func() { Expect(actualGoalName).To(Equal(goalName)) }) - It("re-reads the goal from the vault path before spawning the session", func() { - // The second FindGoalByName is the pre-spawn persist re-read: under the - // non-interactive branch the session id is persisted before the child is - // spawned, so the session's own read-modify-write always reads a file that - // already contains it. + It("re-reads the goal from the vault path after the session finishes", func() { + // The second FindGoalByName is the post-spawn persist re-read: under the + // non-interactive branch the session id is persisted only after the child + // exits cleanly, so the read-modify-write happens against whatever the + // session's own turn already wrote. Expect(mockGoalStorage.FindGoalByNameCallCount()).To(Equal(2)) _, reReadVaultPath, reReadGoalName := mockGoalStorage.FindGoalByNameArgsForCall(1) Expect(reReadVaultPath).To(Equal(vaultPath)) @@ -352,10 +352,13 @@ var _ = Describe("GoalWorkOnOperation", func() { }) }) - Context("when persisting the goal session id before spawn", func() { - var writeGoalAt, spawnAt time.Time + Context("when persisting the goal session id after the child exits", func() { + var writeGoalAt, childExitAt time.Time + var blockWaiter chan struct{} BeforeEach(func() { + blockWaiter = make(chan struct{}) + DeferCleanup(func() { close(blockWaiter) }) mockGoalStorage.WriteGoalStub = func(_ context.Context, g *domain.Goal) error { if g.ClaudeSessionID() != "" { writeGoalAt = time.Now() @@ -365,11 +368,21 @@ var _ = Describe("GoalWorkOnOperation", func() { realStarter := ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, - func(_ []string, _ string) (<-chan error, error) { - spawnAt = time.Now() - return make(chan error), nil + func(_ []string, _ string, stdout *os.File) (<-chan error, error) { + if stdout != nil { + _, _ = stdout.WriteString( + `{"session_id":"` + pinnedSessionID + `","num_turns":3,"is_error":false,"result":"done"}`, + ) + } + done := make(chan error, 1) + done <- nil + childExitAt = time.Now() + return done, nil }, - libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { return nil }), + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-blockWaiter + return nil + }), ) goalWorkOnOp = ops.NewGoalWorkOnOperation( mockGoalStorage, @@ -379,26 +392,43 @@ var _ = Describe("GoalWorkOnOperation", func() { ) }) - It("writes the goal session id to storage before the runner spawns the child", func() { + It("writes the goal session id to storage after the child exits", func() { Expect(err).To(BeNil()) Expect(writeGoalAt).NotTo(BeZero()) - Expect(spawnAt).NotTo(BeZero()) - Expect(writeGoalAt.Before(spawnAt)).To(BeTrue()) + Expect(childExitAt).NotTo(BeZero()) + Expect(writeGoalAt.After(childExitAt)).To(BeTrue()) }) }) Context("when capturing the spawned claude argv", func() { var capturedArgs []string + var blockWaiter chan struct{} BeforeEach(func() { + // The waiter must block: StartSession selects on child-exit vs the turn + // bound, so a waiter that returns immediately makes both cases ready and + // the test flips nondeterministically between success and a spurious + // "did not complete within" error. + blockWaiter = make(chan struct{}) + DeferCleanup(func() { close(blockWaiter) }) realStarter := ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, - func(args []string, _ string) (<-chan error, error) { + func(args []string, _ string, stdout *os.File) (<-chan error, error) { capturedArgs = args - return make(chan error), nil + if stdout != nil { + _, _ = stdout.WriteString( + `{"session_id":"` + pinnedSessionID + `","num_turns":3,"is_error":false,"result":"done"}`, + ) + } + done := make(chan error, 1) + done <- nil + return done, nil }, - libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { return nil }), + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-blockWaiter + return nil + }), ) goalWorkOnOp = ops.NewGoalWorkOnOperation( mockGoalStorage, @@ -442,18 +472,18 @@ body []byte(rollbackFixture), 0600, )).To(Succeed()) - // The liveness window has NOT elapsed when the child exits, so the starter - // must treat the exit as inside-the-window. A nil-returning waiter would - // race the select against the child's buffered exit; the waiter stays - // blocked until the spec is done, so only `done` is ready and the error - // path is deterministic. + // A nil-returning waiter would race the select against the child's buffered + // exit; the waiter stays blocked until the spec is done, so only `done` is + // ready and the error path is deterministic. block := make(chan struct{}) realStarter := ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, - func(_ []string, _ string) (<-chan error, error) { - // The child writes frontmatter before dying: the compensating clear - // must re-read this write and preserve it. + func(_ []string, _ string, _ *os.File) (<-chan error, error) { + // The child writes frontmatter before dying; nothing was persisted + // for this id (the non-interactive branch only persists after a + // clean, validated turn), so the child's write is the only mutation + // on disk and must survive. fresh, ferr := realGoalStore.FindGoalByName(ctx, realVaultPath, "Rollback Goal") if ferr != nil { return nil, ferr @@ -490,8 +520,9 @@ body Expect(err.Error()).To(ContainSubstring("exit status 1")) Expect(result.Success).To(BeFalse()) - // The child's write survived the compensating clear; the pre-persisted id - // is gone. + // The child's write survived; the id was never persisted in the first + // place, since the non-interactive branch only writes it after a clean, + // validated turn. written, ferr := realGoalStore.FindGoalByName(ctx, realVaultPath, "Rollback Goal") Expect(ferr).To(BeNil()) Expect(written.ClaudeSessionID()).To(Equal("")) @@ -499,24 +530,4 @@ body Expect(*written.Phase()).To(Equal(domain.GoalPhaseExecution)) }) }) - - Context("when the clear after a spawn failure cannot re-read the goal", func() { - BeforeEach(func() { - mockStarter.StartSessionReturns(ErrTest) - // call 0 = Execute load, call 1 = pre-spawn persist re-read, - // call 2 = the compensating clear's re-read. - mockGoalStorage.FindGoalByNameReturnsOnCall(2, nil, ErrTest) - }) - - It("returns the spawn error, never masked by the failed clear", func() { - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("start work-on session")) - }) - - It("surfaces the failed clear as a warning", func() { - Expect(result.Warnings).To(ContainElement( - ContainSubstring("failed to clear claude session id after spawn failure"), - )) - }) - }) }) diff --git a/pkg/ops/workon.go b/pkg/ops/workon.go index abfbea3..6a764f1 100644 --- a/pkg/ops/workon.go +++ b/pkg/ops/workon.go @@ -113,8 +113,7 @@ func (w *workOnOperation) Execute( slog.Warn("workon warning", "warning", warning) } - sessionID, sessionWarnings, sessionErr := w.handleClaudeSession(ctx, task, vaultPath, sessionDir, vault, isInteractive) - warnings = append(warnings, sessionWarnings...) + sessionID, sessionErr := w.handleClaudeSession(ctx, task, vaultPath, sessionDir, vault, isInteractive) if sessionErr != nil { if errors.Is(sessionErr, ErrStarterUnavailable) { warnings = appendSessionWarning(warnings, sessionErr) @@ -206,13 +205,15 @@ func applyAssigneeMatrix(task *domain.Task, assignee string) string { } // persistSessionAndMetrics re-reads the task from disk and writes back the session id -// and one metrics_sessions entry in a single write. The re-read is load-bearing on the -// interactive branch and the cached-session path: the headless turn may mutate the -// file before the post-return persist, so writing the stale in-memory copy would -// revert the session's own frontmatter changes. On the non-interactive branch the -// persist runs before the child exists, so the session's own read-modify-write reads a -// file that already contains the id. Used on both the fresh-start path (the session id -// is new) and the cached-session path (the id already exists and is preserved). +// and one metrics_sessions entry in a single write. The re-read is load-bearing on +// every branch: the headless turn mutates the same file, and it always finishes before +// this runs, so writing the stale in-memory copy would revert the session's own +// frontmatter changes. Used on the fresh-start path (the session id is new) and the +// cached-session path (the id already exists and is preserved). +// +// On failure it returns an empty id, never the one it was handed. The id is the Vault +// UI's signal that Resume will work; reporting an id whose write did not land would +// advertise a session that is not on disk. func persistSessionAndMetrics( ctx context.Context, vaultPath string, @@ -223,7 +224,7 @@ func persistSessionAndMetrics( ) (string, error) { refreshed, err := taskStorage.FindTaskByName(ctx, vaultPath, taskName) if err != nil { - return sessionID, errors.Wrap(ctx, err, "re-read task after claude session") + return "", errors.Wrap(ctx, err, "re-read task after claude session") } if refreshed.ClaudeSessionID() == "" { refreshed.SetClaudeSessionID(sessionID) @@ -233,18 +234,17 @@ func persistSessionAndMetrics( StartedAt: startedAt, }) if err := taskStorage.WriteTask(ctx, refreshed); err != nil { - return sessionID, errors.Wrap(ctx, err, "save session id to task") + return "", errors.Wrap(ctx, err, "save session id to task") } return sessionID, nil } // handleClaudeSession starts or returns an existing Claude session for the task. -// On the non-interactive branch the session id and its metrics entry are persisted -// BEFORE the child is spawned, so the session's own read-modify-write always reads a -// file that already contains the id. A spawn failure inside the liveness window -// triggers a compensating re-read-based clear that removes the id and this run's -// metrics entry while preserving any frontmatter the child wrote before dying; a -// failed clear is surfaced as a warning rather than masking the spawn error. +// On both branches the session id and its metrics entry are persisted only AFTER the +// headless turn has finished cleanly, so an id on disk means the session is resumable +// rather than merely that one was started. Nothing is written on any failure path, so +// there is no compensating clear: frontmatter the child wrote before failing stays +// untouched, and the Vault UI correctly keeps offering Start. func (w *workOnOperation) handleClaudeSession( ctx context.Context, task *domain.Task, @@ -252,14 +252,14 @@ func (w *workOnOperation) handleClaudeSession( sessionDir string, vault *config.Vault, isInteractive bool, -) (string, []string, error) { +) (string, error) { if existing := task.ClaudeSessionID(); existing != "" { startedAt := libtime.DateOrDateTime(w.currentDateTime.Now().Time()) sessionID, err := persistSessionAndMetrics(ctx, vaultPath, task.Name, existing, startedAt, w.taskStorage) - return sessionID, nil, err + return sessionID, err } if w.starter == nil { - return "", nil, ErrStarterUnavailable + return "", ErrStarterUnavailable } // The bootstrap always runs headless `claude --print`, which cannot answer // AskUserQuestion; --non-interactive tells the work-on command to take safe @@ -271,62 +271,26 @@ func (w *workOnOperation) handleClaudeSession( // TTY branch, unchanged: block through the headless turn, then re-read and // persist so frontmatter the session itself wrote survives. if err := w.starter.StartSession(ctx, sessionID, prompt, sessionDir, task.Name, isInteractive); err != nil { - return "", nil, errors.Wrap(ctx, err, "start claude session") + return "", errors.Wrap(ctx, err, "start claude session") } startedAt := libtime.DateOrDateTime(w.currentDateTime.Now().Time()) sessionID, err := persistSessionAndMetrics(ctx, vaultPath, task.Name, sessionID, startedAt, w.taskStorage) - return sessionID, nil, err + return sessionID, err } - // Non-interactive branch: persist id + metrics BEFORE the child exists, so the - // session's own read-modify-write always reads a file that already contains it. + // Non-interactive branch: persist id + metrics only AFTER the turn has finished. + // An id on disk is what makes the Vault UI offer Resume, and a resume against a + // still-running turn hits a transcript another process is mid-write on. So the id + // lands only once StartSession reports a clean, validated turn; on any failure + // nothing is persisted and the button correctly stays on Start. startedAt is still + // captured before the spawn — it records the turn's true start, not the write time. startedAt := libtime.DateOrDateTime(w.currentDateTime.Now().Time()) - if _, err := persistSessionAndMetrics(ctx, vaultPath, task.Name, sessionID, startedAt, w.taskStorage); err != nil { - return "", nil, errors.Wrap(ctx, err, "persist claude session before spawn") - } if err := w.starter.StartSession(ctx, sessionID, prompt, sessionDir, task.Name, isInteractive); err != nil { - // Compensating clear: the child may have written frontmatter before dying - // inside the window (e.g. phase: planning). Re-read and clear only the id and - // this run's metrics entry, preserving every other field on disk. - if clearErr := w.clearSessionAndMetrics(ctx, vaultPath, task.Name, sessionID); clearErr != nil { - return "", []string{fmt.Sprintf("failed to clear claude session id after spawn failure: %v", clearErr)}, - errors.Wrap(ctx, err, "start claude session") - } - return "", nil, errors.Wrap(ctx, err, "start claude session") + // No compensating clear needed: nothing was written for this id, so there is + // nothing to undo. Frontmatter the child wrote before failing stays untouched. + return "", errors.Wrap(ctx, err, "start claude session") } - return sessionID, nil, nil -} - -// clearSessionAndMetrics re-reads the task after a spawn failure and clears only -// the claude_session_id and the metrics_sessions entry for this run, preserving -// any frontmatter the child wrote before dying. The re-read is load-bearing: -// clearing from the stale in-memory copy would revert the child's writes. -func (w *workOnOperation) clearSessionAndMetrics( - ctx context.Context, - vaultPath string, - taskName string, - sessionID string, -) error { - refreshed, err := w.taskStorage.FindTaskByName(ctx, vaultPath, taskName) - if err != nil { - return errors.Wrap(ctx, err, "re-read task after spawn failure") - } - refreshed.ClearClaudeSessionID() - var kept []domain.MetricsSession - // No ctx.Done() check in this loop (go-functional-composition/list-checks-ctx-done): - // it filters an already-loaded in-memory slice of a handful of entries with no I/O - // and no blocking call per iteration, so there is nothing for cancellation to - // interrupt. The surrounding I/O (FindTaskByName above, WriteTask below) is - // ctx-aware. - for _, m := range refreshed.MetricsSessions() { - if m.SessionID != sessionID { - kept = append(kept, m) - } - } - refreshed.Set("metrics_sessions", kept) - if err := w.taskStorage.WriteTask(ctx, refreshed); err != nil { - return errors.Wrap(ctx, err, "clear session id after spawn failure") - } - return nil + sessionID, err := persistSessionAndMetrics(ctx, vaultPath, task.Name, sessionID, startedAt, w.taskStorage) + return sessionID, err } // updateDailyNote updates the daily note to mark the task as in-progress. diff --git a/pkg/ops/workon_session_writeback_test.go b/pkg/ops/workon_session_writeback_test.go index d8f1f46..86a8be4 100644 --- a/pkg/ops/workon_session_writeback_test.go +++ b/pkg/ops/workon_session_writeback_test.go @@ -38,19 +38,27 @@ var _ = Describe("work-on session write-back", func() { ) // newStarter builds a real starter whose detached child runs the given fake. - // The child is "still running" once detachRun returns — the parent returns - // within the liveness window while the turn continues independently. - newStarter := func(detachRun func(args []string, dir string) (<-chan error, error)) ops.ClaudeSessionStarter { + // StartSession now blocks until the child exits, so the waiter must block too + // (via blockWaiter, closed in AfterEach) — a waiter that returns immediately + // races the select against the child's buffered exit and makes the outcome + // nondeterministic. + var blockWaiter chan struct{} + newStarter := func(detachRun func(args []string, dir string, stdout *os.File) (<-chan error, error)) ops.ClaudeSessionStarter { return ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, detachRun, - libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { return nil }), + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-blockWaiter + return nil + }), ) } BeforeEach(func() { ctx = context.Background() + blockWaiter = make(chan struct{}) + DeferCleanup(func() { close(blockWaiter) }) var err error vaultPath, err = os.MkdirTemp("", "vault-workon-writeback-*") @@ -102,7 +110,7 @@ body // before the call. The write happens inside the detached child (the // detachRun fake) while the parent has already returned within the // liveness window. - detachRun := func(_ []string, _ string) (<-chan error, error) { + detachRun := func(_ []string, _ string, stdout *os.File) (<-chan error, error) { fresh, err := taskStore.FindTaskByName(ctx, vaultPath, "Repro Task") if err != nil { return nil, err @@ -114,7 +122,14 @@ body if err := taskStore.WriteTask(ctx, fresh); err != nil { return nil, err } - return make(chan error), nil + if _, err := stdout.WriteString( + `{"session_id":"` + pinnedSessionID + `","num_turns":3,"is_error":false,"result":"done"}`, + ); err != nil { + return nil, err + } + done := make(chan error, 1) + done <- nil + return done, nil } starter = newStarter(detachRun) }) @@ -178,7 +193,7 @@ body // before the call. The write happens inside the detached child (the // detachRun fake) while the parent has already returned within the // liveness window. - detachRun := func(_ []string, _ string) (<-chan error, error) { + detachRun := func(_ []string, _ string, stdout *os.File) (<-chan error, error) { fresh, err := goalStore.FindGoalByName(ctx, vaultPath, "Repro Goal") if err != nil { return nil, err @@ -190,7 +205,14 @@ body if err := goalStore.WriteGoal(ctx, fresh); err != nil { return nil, err } - return make(chan error), nil + if _, err := stdout.WriteString( + `{"session_id":"` + pinnedSessionID + `","num_turns":3,"is_error":false,"result":"done"}`, + ); err != nil { + return nil, err + } + done := make(chan error, 1) + done <- nil + return done, nil } starter = newStarter(detachRun) }) @@ -246,7 +268,7 @@ body // waiter stays blocked until the spec is done, so only `done` is ready // and the error path is deterministic. block := make(chan struct{}) - detachRun := func(_ []string, _ string) (<-chan error, error) { + detachRun := func(_ []string, _ string, _ *os.File) (<-chan error, error) { fresh, err := taskStore.FindTaskByName(ctx, vaultPath, "Repro Task") if err != nil { return nil, err @@ -271,7 +293,7 @@ body DeferCleanup(func() { close(block) }) }) - It("clears the pre-persisted id and preserves the child's frontmatter write when the child exited non-zero inside the window", func() { + It("never persists a session id and preserves the child's frontmatter write when the child exited non-zero inside the window", func() { currentDateTime := libtime.NewCurrentDateTime() currentDateTime.SetNow(libtimetest.ParseDateTime("2026-03-03T12:00:00Z")) testVault := config.Vault{ @@ -295,17 +317,18 @@ body Expect(err.Error()).To(ContainSubstring("exit status 1")) Expect(result.Success).To(BeFalse()) - // The child's write survived the compensating clear. + // The child's write survived; nothing in the new design ever touches it. written, err := taskStore.FindTaskByName(ctx, vaultPath, "Repro Task") Expect(err).To(BeNil()) Expect(written.Phase()).NotTo(BeNil()) Expect(*written.Phase()).To(Equal(domain.TaskPhasePlanning)) - // On-disk shape: the pre-persisted id and this run's metrics entry are - // gone (the AC6 grep pins keep the count of the id/metrics accessor calls - // in this file at 2, so the clearance is asserted via the raw file). The - // id itself is the shared fingerprint of both claude_session_id and the - // metrics_sessions entry, so its absence proves both were cleared. + // On-disk shape: the id and this run's metrics entry were never written + // in the first place (the AC6 grep pins keep the count of the id/metrics + // accessor calls in this file at 2, so the absence is asserted via the + // raw file). The id itself is the shared fingerprint of both + // claude_session_id and the metrics_sessions entry, so its absence proves + // neither was persisted. raw, err := os.ReadFile(filepath.Join(vaultPath, "24 Tasks", "Repro Task.md")) Expect(err).To(BeNil()) Expect(strings.Count(string(raw), "claude_session_id:")).To(Equal(0)) @@ -314,69 +337,4 @@ body }) }) - Context("when the clear after a spawn failure cannot re-read the task", func() { - const clearFixture = `--- -assignee: user@example.com -phase: planning -status: in_progress ---- -body -` - var taskStore storage.TaskStorage - - BeforeEach(func() { - taskStore = storage.NewTaskStorage(storageConfig) - Expect(os.WriteFile( - filepath.Join(vaultPath, "24 Tasks", "Repro Task.md"), - []byte(clearFixture), 0600, - )).To(Succeed()) - - block := make(chan struct{}) - detachRun := func(_ []string, _ string) (<-chan error, error) { - // The child deletes the task file mid-flight, so the compensating - // clear's re-read fails with ErrNotFound. The failed clear must - // surface as a warning, never masking the spawn error. - Expect(os.Remove(filepath.Join(vaultPath, "24 Tasks", "Repro Task.md"))).To(Succeed()) - done := make(chan error, 1) - done <- errors.New("exit status 1") - return done, nil - } - starter = ops.NewClaudeSessionStarterWithRunner( - "/usr/local/bin/claude", - nil, - detachRun, - libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { - <-block - return nil - }), - ) - DeferCleanup(func() { close(block) }) - }) - - It("surfaces the failed clear as a warning instead of masking the spawn error", func() { - currentDateTime := libtime.NewCurrentDateTime() - currentDateTime.SetNow(libtimetest.ParseDateTime("2026-03-03T12:00:00Z")) - testVault := config.Vault{ - Path: vaultPath, - Name: "test-vault", - WorkOnCommand: "/vault-cli:work-on-task", - } - workOnOp := ops.NewWorkOnOperation( - taskStore, mockDailyNote, currentDateTime, func() string { return pinnedSessionID }, starter, nil, - ) - - result, err := workOnOp.Execute( - ctx, vaultPath, "Repro Task", "user@example.com", "test-vault", - false, sessionDir, &testVault, - ) - // The spawn error is still returned, never masked by the failed clear. - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("start work-on session")) - Expect(err.Error()).To(ContainSubstring("exit status 1")) - // ...and the failed clear surfaces as a warning. - Expect(result.Warnings).To(ContainElement( - ContainSubstring("failed to clear claude session id after spawn failure"), - )) - }) - }) }) diff --git a/pkg/ops/workon_test.go b/pkg/ops/workon_test.go index 8776cc4..cc240f0 100644 --- a/pkg/ops/workon_test.go +++ b/pkg/ops/workon_test.go @@ -6,6 +6,7 @@ package ops_test import ( "context" + "os" "strings" "time" @@ -95,8 +96,8 @@ var _ = Describe("WorkOnOperation", func() { }) It("calls FindTaskByName", func() { - // Twice: once to load the task, once to re-read it before spawning so - // the session id is persisted while no child exists yet. + // Twice: once to load the task, once to re-read it after the child exits + // so the session id is persisted only once the turn is done. Expect(mockTaskStorage.FindTaskByNameCallCount()).To(Equal(2)) actualCtx, actualVaultPath, actualTaskName := mockTaskStorage.FindTaskByNameArgsForCall( 0, @@ -106,10 +107,10 @@ var _ = Describe("WorkOnOperation", func() { Expect(actualTaskName).To(Equal(taskName)) }) - It("re-reads the task from the vault path before spawning the session", func() { + It("re-reads the task from the vault path after the session finishes", func() { Expect(mockTaskStorage.FindTaskByNameCallCount()).To(Equal(2)) - // The second FindTaskByName is persistSessionAndMetrics' pre-spawn re-read: - // the session id is written to disk before the child starts. + // The second FindTaskByName is persistSessionAndMetrics' post-spawn + // re-read: the session id is written to disk only after the child exits. _, reReadVaultPath, reReadTaskName := mockTaskStorage.FindTaskByNameArgsForCall(1) Expect(reReadVaultPath).To(Equal(vaultPath)) Expect(reReadTaskName).To(Equal(taskName)) @@ -844,11 +845,11 @@ var _ = Describe("WorkOnOperation", func() { }) }) - Context("when the pre-spawn persist re-read fails", func() { + Context("when the persist re-read after the session finishes fails", func() { BeforeEach(func() { mockTaskStorage.FindTaskByNameReturnsOnCall(0, task, nil) // Under the new ordering the failing call is persistSessionAndMetrics' - // pre-spawn re-read, which runs before any child is spawned. + // re-read, which runs only after StartSession reports a clean turn. mockTaskStorage.FindTaskByNameReturnsOnCall(1, nil, ErrTest) }) @@ -858,29 +859,33 @@ var _ = Describe("WorkOnOperation", func() { Expect(result.Success).To(BeFalse()) }) - It("does not report a session id because no session was ever spawned", func() { + It("does not report a session id because the persist never completed", func() { Expect(result.SessionID).To(Equal("")) }) It("does not write a second time with the stale in-memory task", func() { - // Execute's write only — the pre-spawn persist failed before writing. + // Execute's write only — the post-spawn persist failed before writing. Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(1)) }) - It("does not append a metrics entry when the pre-spawn re-read fails", func() { + It("does not append a metrics entry when the post-spawn re-read fails", func() { _, writtenTask := mockTaskStorage.WriteTaskArgsForCall(0) Expect(writtenTask.MetricsSessions()).To(BeNil()) }) }) - Context("when persisting the session id before spawn", func() { + Context("when persisting the session id after the child exits", func() { var ( - writeTaskAt, spawnAt time.Time - writtenSessionID string - spawnedSessionID string + writeTaskAt, childExitAt time.Time + writtenSessionID string + spawnedSessionID string + blockWaiter chan struct{} ) BeforeEach(func() { + blockWaiter = make(chan struct{}) + DeferCleanup(func() { close(blockWaiter) }) + mockTaskStorage.WriteTaskStub = func(_ context.Context, t *domain.Task) error { if t.ClaudeSessionID() != "" { writtenSessionID = t.ClaudeSessionID() @@ -891,16 +896,28 @@ var _ = Describe("WorkOnOperation", func() { realStarter := ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, - func(args []string, _ string) (<-chan error, error) { + func(args []string, _ string, stdout *os.File) (<-chan error, error) { for i, a := range args { if a == "--session-id" && i+1 < len(args) { spawnedSessionID = args[i+1] } } - spawnAt = time.Now() - return make(chan error), nil + _, err := stdout.WriteString( + `{"session_id":"` + pinnedSessionID + `","num_turns":3,"is_error":false,"result":"done"}`, + ) + Expect(err).To(BeNil()) + done := make(chan error, 1) + childExitAt = time.Now() + done <- nil + return done, nil }, - libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { return nil }), + // Blocking waiter: only the child-exit channel is ever ready, so the + // select in StartSession cannot nondeterministically pick the + // timeout branch instead of the success branch. + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-blockWaiter + return nil + }), ) currentDateTime := libtime.NewCurrentDateTime() currentDateTime.SetNow(libtimetest.ParseDateTime("2026-03-03T12:00:00Z")) @@ -913,11 +930,11 @@ var _ = Describe("WorkOnOperation", func() { ) }) - It("writes the session id to storage before the runner spawns the child", func() { + It("writes the session id to storage only after the child exits", func() { Expect(err).To(BeNil()) Expect(writeTaskAt).NotTo(BeZero()) - Expect(spawnAt).NotTo(BeZero()) - Expect(writeTaskAt.Before(spawnAt)).To(BeTrue()) + Expect(childExitAt).NotTo(BeZero()) + Expect(writeTaskAt.After(childExitAt)).To(BeTrue()) // AC5's "id equals the value in task frontmatter" — capture the id written to // storage and the id handed to detachRun and assert they are the same value. // Both derive from the pinned generator today, so this holds implicitly; assert @@ -926,7 +943,7 @@ var _ = Describe("WorkOnOperation", func() { }) }) - Context("when the spawn fails after the pre-spawn persist", func() { + Context("when the spawn fails", func() { BeforeEach(func() { mockStarter.StartSessionReturns(ErrTest) }) @@ -940,69 +957,12 @@ var _ = Describe("WorkOnOperation", func() { Expect(result.Success).To(BeFalse()) }) - It("clears the pre-persisted session id on the last write", func() { - Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(3)) - _, lastWritten := mockTaskStorage.WriteTaskArgsForCall(mockTaskStorage.WriteTaskCallCount() - 1) - Expect(lastWritten.ClaudeSessionID()).To(Equal("")) - }) - }) - - Context("when the clear after a spawn failure cannot re-read the task", func() { - BeforeEach(func() { - mockStarter.StartSessionReturns(ErrTest) - // call 0 = Execute load, call 1 = pre-spawn persist re-read, - // call 2 = the compensating clear's re-read. - mockTaskStorage.FindTaskByNameReturnsOnCall(2, nil, ErrTest) - }) - - It("returns the spawn error, never masked by the failed clear", func() { - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("start work-on session")) - }) - - It("surfaces the failed clear as a warning", func() { - Expect(result.Warnings).To(ContainElement( - ContainSubstring("failed to clear claude session id after spawn failure"), - )) - }) - }) - - Context("when the clear preserves other sessions' metrics entries", func() { - BeforeEach(func() { - task.AppendMetricsSession(domain.MetricsSession{ - SessionID: "other-session", - StartedAt: libtime.DateOrDateTime( - libtimetest.ParseDateTime("2026-02-01T08:00:00Z").Time(), - ), - }) - mockStarter.StartSessionReturns(ErrTest) - }) - - It("keeps the other session's metrics entry on the last write", func() { - Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(3)) - _, lastWritten := mockTaskStorage.WriteTaskArgsForCall(2) - Expect(lastWritten.MetricsSessions()).To(HaveLen(1)) - Expect(lastWritten.MetricsSessions()[0].SessionID).To(Equal("other-session")) - }) - }) - - Context("when the clear after a spawn failure cannot write the task", func() { - BeforeEach(func() { - mockStarter.StartSessionReturns(ErrTest) - // The clear's re-read succeeds (call 2) but its WriteTask fails; the - // spawn error must still be returned, the failed clear a warning. - mockTaskStorage.WriteTaskReturnsOnCall(2, ErrTest) - }) - - It("returns the spawn error, never masked by the failed clear", func() { - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("start work-on session")) - }) - - It("surfaces the failed clear as a warning", func() { - Expect(result.Warnings).To(ContainElement( - ContainSubstring("failed to clear claude session id after spawn failure"), - )) + It("never persists a session id: WriteTask reflects only Execute's initial write", func() { + // No compensating clear exists in the new design — nothing was written + // for this id, so there is nothing to undo. + Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(1)) + _, onlyWritten := mockTaskStorage.WriteTaskArgsForCall(0) + Expect(onlyWritten.ClaudeSessionID()).To(Equal("")) }) }) }) diff --git a/scenarios/002-task-lifecycle.md b/scenarios/002-task-lifecycle.md index 5b49f73..dd9edb6 100644 --- a/scenarios/002-task-lifecycle.md +++ b/scenarios/002-task-lifecycle.md @@ -30,7 +30,7 @@ TOMORROW=$(date -v+1d +%Y-%m-%d 2>/dev/null || date -d '+1 day' +%Y-%m-%d) ### Work on a task - [ ] `$VAULT_CLI --config $CONFIG task work-on "Simple Task"` exits 0 -> Spawns a real headless `claude --print` turn. On a non-TTY caller (CI, an agent shell, a pipe) the CLI now returns within ~10s (the liveness window) with `✅ Now working on: …` and `session_id: …`; the bootstrap turn continues after the CLI exits. Run the session-lifecycle check in Expected below. TTY callers (a real terminal) still block through the turn and hand you the interactive session. +> Spawns a real headless `claude --print` turn. **Both branches block until the turn completes** — expect no output for the whole bootstrap (typically 2-5 minutes; bounded by a 30m turn timeout), then `✅ Now working on: …` and `session_id: …`. A fast return is a FAIL, not a pass: the session id is written only once the turn has finished, which is what makes `claude --resume ` work. Allow ≥300s. Run the session-lifecycle check in Expected below. ### Defer the task - [ ] `$VAULT_CLI --config $CONFIG task defer "Simple Task" +1d` exits 0 diff --git a/specs/bug-resume-races-live-headless-turn.md b/specs/in-progress/041-bug-resume-races-live-headless-turn.md similarity index 99% rename from specs/bug-resume-races-live-headless-turn.md rename to specs/in-progress/041-bug-resume-races-live-headless-turn.md index 06ee816..053a14e 100644 --- a/specs/bug-resume-races-live-headless-turn.md +++ b/specs/in-progress/041-bug-resume-races-live-headless-turn.md @@ -1,5 +1,7 @@ --- -status: draft +status: approved +approved: "2026-08-28T08:35:16Z" +branch: dark-factory/bug-resume-races-live-headless-turn --- ## Summary From b38e1dc1b39461e6fd3b881426dbd149bb3e2063 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Fri, 28 Aug 2026 11:36:52 +0200 Subject: [PATCH 4/4] fix data race on the spec-scoped waiter channels in work-on session tests --- CHANGELOG.md | 1 + pkg/ops/claude_session_test.go | 47 ++++++++++++++++-------- pkg/ops/workon_session_writeback_test.go | 12 +++++- 3 files changed, 43 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c83aca7..dab16dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Please choose versions by [Semantic Versioning](http://semver.org/). - fix: non-interactive `task work-on` / `goal work-on` now wait for the detached headless turn to finish before persisting `claude_session_id`, so the Vault UI only offers Resume against a complete transcript — previously the id landed within ~10s while the turn was still writing, and `claude --resume` failed with "session not found" or replayed partial output. The wait is bounded by a 30m turn timeout (a wait bound, never a kill — the child stays detached), and the turn's JSON result is now validated on both branches, so a failed or zero-turn session persists no id at all. The interactive TTY branch is unchanged. - fix: a failed session-id persist (re-read or write error) no longer reports the id back to the caller — nothing landed on disk, so returning it advertised a session the Vault UI could not resume. Affects `task work-on` and `goal work-on` on every branch, including the cached-session path. +- test: fix a data race in the work-on session specs — the blocking-waiter and turn-bound channels were `Describe`-scoped and reassigned per spec, while the waiter goroutine parked on them outlives the spec that started it (`StartSession` can return via the child-exit branch first). Both are now captured spec-locally, and the turn bound is read through a channel so the assertion has a real happens-before edge. `go test ./... -race` is clean. ## v0.117.0 diff --git a/pkg/ops/claude_session_test.go b/pkg/ops/claude_session_test.go index d9f8270..192a0ba 100644 --- a/pkg/ops/claude_session_test.go +++ b/pkg/ops/claude_session_test.go @@ -245,12 +245,12 @@ var _ = Describe("ClaudeSessionStarter", func() { Context("non-interactive branch", func() { var ( - detachArgs []string - detachDir string - doneCh chan error - detachErr error - capturedWindow libtime.Duration - blockWaiter chan struct{} + detachArgs []string + detachDir string + doneCh chan error + detachErr error + windowCh chan libtime.Duration + blockWaiter chan struct{} ) // validTurnJSON is what a clean headless turn writes to the captured stdout @@ -263,16 +263,22 @@ var _ = Describe("ClaudeSessionStarter", func() { detachDir = "" doneCh = make(chan error, 1) detachErr = nil - capturedWindow = 0 + windowCh = make(chan libtime.Duration, 1) // The waiter must BLOCK on the success paths. StartSession selects on the // child-exit channel and the waiter channel; a waiter that returns // immediately makes both ready and the select picks nondeterministically, // flipping between success and a spurious turn-timeout error. + // Every waiter closure below captures blockWaiter and windowCh as + // spec-local values rather than reading these variables. StartSession's + // select can return via the child-exit branch while the waiter goroutine is + // still parked on the channel, so that goroutine outlives the spec; reading + // either outer variable would race the next spec's reassignment here. blockWaiter = make(chan struct{}) DeferCleanup(func() { close(blockWaiter) }) }) JustBeforeEach(func() { + bw, wc := blockWaiter, windowCh starter = ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, @@ -285,8 +291,8 @@ var _ = Describe("ClaudeSessionStarter", func() { return doneCh, detachErr }, libtime.WaiterDurationFunc(func(_ context.Context, d libtime.Duration) error { - capturedWindow = d - <-blockWaiter + wc <- d + <-bw return nil }), ) @@ -318,15 +324,20 @@ var _ = Describe("ClaudeSessionStarter", func() { Consistently(returned, "100ms").ShouldNot(Receive()) doneCh <- nil Eventually(returned).Should(Receive(BeNil())) + // Received once, asserted twice: the send happens-before this receive, which + // is what makes reading the bound race-free. + var window libtime.Duration + Expect(windowCh).To(Receive(&window)) // Locks the wiring: StartSession hands the constant, not a stray literal. - Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout)) + Expect(window).To(Equal(ops.SessionTurnTimeout)) // Locks the value: SessionTurnTimeout is an alias, so the line above moves // with the constant and would survive any retune. This line is the one that // fails when the bound is changed. - Expect(capturedWindow).To(Equal(30 * libtime.Minute)) + Expect(window).To(Equal(30 * libtime.Minute)) }) It("validates the turn and rejects a zero-turn result", func() { + bw := blockWaiter starter = ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, @@ -337,7 +348,7 @@ var _ = Describe("ClaudeSessionStarter", func() { return done, nil }, libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { - <-blockWaiter + <-bw return nil }), ) @@ -347,6 +358,7 @@ var _ = Describe("ClaudeSessionStarter", func() { }) It("validates the turn and rejects an is_error result", func() { + bw := blockWaiter starter = ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, @@ -357,7 +369,7 @@ var _ = Describe("ClaudeSessionStarter", func() { return done, nil }, libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { - <-blockWaiter + <-bw return nil }), ) @@ -367,6 +379,7 @@ var _ = Describe("ClaudeSessionStarter", func() { }) It("rejects an unparseable turn result", func() { + bw := blockWaiter starter = ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, @@ -376,7 +389,7 @@ var _ = Describe("ClaudeSessionStarter", func() { return done, nil }, libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { - <-blockWaiter + <-bw return nil }), ) @@ -388,6 +401,7 @@ var _ = Describe("ClaudeSessionStarter", func() { It("treats a child exit error as an error", func() { earlyDone := make(chan error, 1) earlyDone <- errors.New("exit status 1") + bw := blockWaiter starter = ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, @@ -395,7 +409,7 @@ var _ = Describe("ClaudeSessionStarter", func() { return earlyDone, nil }, libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { - <-blockWaiter + <-bw return nil }), ) @@ -437,6 +451,7 @@ var _ = Describe("ClaudeSessionStarter", func() { }) It("wraps a spawn failure", func() { + bw := blockWaiter starter = ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, @@ -444,7 +459,7 @@ var _ = Describe("ClaudeSessionStarter", func() { return nil, ErrTest }, libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { - <-blockWaiter + <-bw return nil }), ) diff --git a/pkg/ops/workon_session_writeback_test.go b/pkg/ops/workon_session_writeback_test.go index 86a8be4..77153d9 100644 --- a/pkg/ops/workon_session_writeback_test.go +++ b/pkg/ops/workon_session_writeback_test.go @@ -42,14 +42,24 @@ var _ = Describe("work-on session write-back", func() { // (via blockWaiter, closed in AfterEach) — a waiter that returns immediately // races the select against the child's buffered exit and makes the outcome // nondeterministic. + // + // StartSession's select can return via the child-exit branch while the + // waiter goroutine is still blocked on <-blockWaiter; that goroutine only + // unblocks (and exits) when this spec's DeferCleanup closes it, which can + // still be in flight when the next spec's BeforeEach reassigns the shared + // blockWaiter variable. Capturing the channel into a spec-local "bw" here + // (instead of letting the waiter closure read the outer, reassignable + // variable directly) gives the leaked goroutine its own private channel to + // finish reading, so it never races against a later spec's reassignment. var blockWaiter chan struct{} newStarter := func(detachRun func(args []string, dir string, stdout *os.File) (<-chan error, error)) ops.ClaudeSessionStarter { + bw := blockWaiter return ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", nil, detachRun, libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { - <-blockWaiter + <-bw return nil }), )