[Bug]: Publish terminal run event before terminal status becomes observable

[dennisonbertram]

Work type

Bug / regression

Observed behavior

During the unchanged full regression gate for PR #1060, the race pass failed TestCausalGraphSnapshotAbsentOnProviderErrorFailRun. GetRun already returned status failed, but Subscribe history ended at llm.turn.requested and did not yet contain run.failed. Exact failure: expected a run.failed event on the provider-error path; got [run.started provider.resolved run.step.started llm.turn.requested]. The focused test passed 100 isolated race repetitions, so the window is aggregate-load sensitive rather than deterministic under low load.

Expected behavior

A terminal run status must not become observable before its matching terminal event and any terminal causal snapshot required by that path are committed to replay history. Once GetRun reports completed, failed, or cancelled, an immediate Subscribe replay must contain the matching terminal event. No client should observe terminal state with an incomplete transcript.

Reproduction

Preconditions: macOS, repository full regression load, race instrumentation.

Exact command: ./scripts/test-regression.sh

Actual result: the race stage failed internal/harness TestCausalGraphSnapshotAbsentOnProviderErrorFailRun because status was failed while replay history lacked run.failed.

Focused diagnostic: go test -race ./internal/harness -run TestCausalGraphSnapshotAbsentOnProviderErrorFailRun -count=100 passed, confirming a narrow load-dependent publication window.

Reproduction rate: observed once in the aggregate full-race gate; the ordering is visible in the current failRun implementation and should be made deterministic with a test seam or synchronization barrier.

User and operational impact

GUI, TUI, SSE reconnect, and API polling clients can see a terminal run while the transcript/replay stream still lacks the terminal event. This can leave progress UI, failure messaging, causal evidence, and conversation continuation briefly inconsistent. Severity is high for trustworthy external-run monitoring; no data confidentiality issue is known. Retrying replay later is a workaround, not a valid contract.

Suspected seam and search evidence

Owning packages/types/functions: internal/harness Runner terminal helpers, especially failRun, completeRun, cancelledRun, emit, GetRun, and Subscribe.

Callers/consumers searched: terminal status polling and event replay tests in internal/harness, server run/event routes, TUI and GUI transcript consumers.

Similar code paths checked: provider-error failRun versus normal completion, max-step failure, cancellation, and causal snapshot emission.

Commands/symbol searches used: rg for waitForStatus, collectEvents, EventRunFailed, failRun, completeRun, and terminal status assignment.

Current hypothesis: terminal helpers mutate state.run.Status under the runner lock, release it, and append/fan out the terminal event in a separate operation, allowing GetRun to win between those actions.

Evidence that rules out adjacent seams: the failing test reads the same in-memory Runner with no HTTP, persistence, cron, or provider transport boundary.

Blast-radius impact map

Callers and data flow: provider/tool error -> terminal helper -> run state plus event history -> GetRun/Subscribe/SSE/TUI/GUI.

Config/env/defaults: None; no configuration controls publication ordering.

API/CLI/wire formats/tools: no schema change; observable ordering contract changes.

Persistence/schema/cache: inspect ordering of durable event/status writes; no schema change expected.

Concurrency/lifecycle/cancellation/retries: primary surface; every terminal path and concurrent poll/replay must be audited.

Security/auth/permissions/privacy: None after search; no authorization boundary changes.

TUI/web/macOS/other clients: terminal badges and transcripts must not disagree.

Provider/model/tool catalog: provider-error path reproduces, but provider selection is not the owner.

Deployment/observability/runbooks: terminal monitoring becomes reliable; add engineering evidence.

Compatibility: preserve existing event types, status values, ordering relative to workspace cleanup, and causal snapshots.

Existing tests/fixtures: runner_causalgraph_test, terminal lifecycle tests, Subscribe replay tests, race suite.

Documentation: engineering log, active plan, impact map, and indexes if new docs are added.

Regression test first

Test file and test name: internal/harness terminal lifecycle test, e.g. TestTerminalStatusNeverPrecedesTerminalReplayEvent.

Red command: go test -race ./internal/harness -run TestTerminalStatusNeverPrecedesTerminalReplayEvent -count=100.

Expected failure: a deterministic barrier pauses between status mutation and terminal event append; concurrent GetRun observes terminal and Subscribe lacks the matching event.

Why that failure proves the bug: it directly observes the forbidden externally visible state at the owning Runner boundary.

False-positive controls: cover failed, completed, and cancelled paths; assert nonterminal states need no terminal event; avoid fixed sleeps by using a test hook/barrier.

Fix boundaries

In scope: make terminal status, matching terminal event, and required causal snapshot publication linearizable from GetRun/Subscribe consumers; cover all terminal helpers and persistence/fanout ordering.

Out of scope: cron/callback behavior, issue #1003 remote cronsd implementation, event schema redesign, GUI visual redesign, workflow-specific timing issue #1049.

Existing abstraction to repair: Runner terminal transition and emit/replay boundary.

Why no new parallel abstraction is needed: the Runner already owns status and event history; its transition ordering must be corrected centrally.

Diagnostic and observability evidence

Preserve the aggregate red output and add a deterministic test seam that records transition phases without logging prompts or secrets. Green evidence must show no terminal status can be read before the corresponding event is replayable. If durable store ordering differs from memory ordering, record both explicitly.

Verification plan

1. Capture deterministic red for failed, completed, and cancelled transitions.
2. Run focused normal and race stress at least 100 times.
3. Run go test ./internal/harness ./internal/server -count=1.
4. Run go test -race ./internal/harness ./internal/server -count=1.
5. Run go vet on affected packages.
6. Run unchanged ./scripts/test-regression.sh in the established foreground non-TTY environment.
7. Exercise an API poll followed immediately by event replay and confirm the terminal event is present whenever terminal status is returned.
8. Verify no change to issue [Bug]: Execute harness jobs through remote cronsd instead of the shell executor #1003 or [Change]: Link cron executions to terminal harness runs and prevent conversation overlap #1004 product boundaries.

Rollout and rollback

Release as a separate PR before PR #1060 can use the full regression gate. Monitor terminal status/event mismatch tests and hosted fast/race checks. Roll back the isolated transition-order commit if terminal fanout deadlocks, reorders workspace cleanup, or regresses cancellation. No data repair or schema migration is expected.

Dependencies: blocks clean full-gate acceptance of PR #1060; must not be combined into #1060. Issue #1049 is test-only workflow timing and does not cover this engine ordering invariant.

Documentation and handoff

Update docs/logs/engineering-log.md, docs/logs/observational-log.md, docs/logs/system-log.md, docs/logs/long-term-thinking-log.md, docs/plans/active-plan.md, a one-page impact map for this bug, and relevant indexes for any new document. Record symptom, root cause, fix, red-green evidence, and rollback.

Definition of done

• Deterministic reproduction recorded
• Regression test failed before the fix
• Root terminal-transition seam fixed rather than test timing patched
• Failed, completed, and cancelled paths audited
• GetRun terminal implies matching terminal event is immediately replayable
• Targeted, race, vet, hosted, and full regression green
• API poll plus replay real path proven
• Engineering log, impact map, rollout, rollback, and indexes current
• Separate PR closes this issue and does not modify fix: dispatch cronsd harness jobs remotely #1060 scope

Process acknowledgement

• I will preserve the failing regression test, update the issue if the root cause changes, and record any newly discovered bug as its own issue.

Review contract addendum (2026-07-31)

This section supersedes any ambiguous implication above that the current Store
interface provides a two-record transaction.

Durability and availability policy:

• For a retained terminal event, the Runner prepares and seals the terminal
  ledger entry, attempts AppendEvent with a bounded context outside the global
  conversation mutex, records process-local replay, drains the recorder, and
  only then conditionally attempts terminal UpdateRun.
• If terminal AppendEvent returns an error, the Runner must not attempt to
  persist terminal status. The built-in stores therefore retain a non-terminal
  durable run row. In-memory terminal status, replay, and subscriber fanout must
  still complete so live clients do not hang.
• If terminal append succeeds but final UpdateRun errors or reaches its
  context deadline, the durable terminal event may lead the durable run row.
  In-memory terminal status and fanout must still complete. This is an explicit
  one-way guarantee, not two-way durable atomicity; a third-party Store is
  responsible for not applying a write before returning an error if it needs
  the same durable-read characterization.
• StorageModeNone is the explicit no-terminal-event exception. Its status may
  persist without a terminal append, but the recorder must close and drain
  before status visibility.
• Every status snapshot/persist/commit sequence shares a per-run serialization
  boundary so delayed running or waiting writes cannot overwrite terminal
  state.
• A refcounted per-conversation sequence lock prevents same-conversation
  overtaking while external terminal I/O is in flight, permits unrelated
  conversations to progress, and reclaims the key after owners and queued
  waiters drain.

Additional acceptance criteria:

• Terminal append failure never attempts terminal status persistence and
  still completes bounded in-memory publication/fanout.
• Final status update error or context timeout leaves the built-in durable
  run non-terminal while live publication/fanout completes.
• Delayed non-terminal status cannot overwrite a terminal transition.
• Unrelated conversations progress during blocked terminal store I/O;
  later events on the target conversation cannot overtake terminal fanout.
• Suppressed terminal events drain the recorder before status visibility.
• Contended and many-distinct-key sequence locks reclaim to zero.

Durability retention and degraded-admission addendum (2026-07-31)

The exact-head review found a second persistence seam: a successful terminal AppendEvent set the event-persisted marker, but the result of terminal UpdateRun was discarded. Pruning could therefore evict the only truthful terminal status while the durable run row remained non-terminal. Permanent append or update failures also created protected terminal states which could grow without bound.

Revised acceptance and recovery contract:

• A store-backed terminal run is prunable only after both the terminal event and final terminal status are durably acknowledged. An explicit StorageModeNone suppression resolves the event side by policy, but still requires final status persistence. No-store runs remain process-local, are not pruned as durable, and do not activate durability admission backpressure.
• Both unacknowledged terminal appends and unacknowledged final status updates count toward one backlog capped by MaxCompletedRetention. Already-admitted running or queued work may finish and remains truthfully visible, so the temporary excess is bounded by the finite population admitted before the outage was detected.
• Once the unresolved backlog reaches the cap, StartRun and ContinueRun retry only status-only gaps under one shared deadline of at most 250 ms, with no store I/O under Runner, status, event-journal, or conversation locks. A successful retry clears the gap; a still-full backlog returns a typed error.
• Terminal append errors are not retried in-process because a third-party store may have applied an append before returning an error; retrying could duplicate the forensic event. They remain protected and keep admission fail-closed until process/operator recovery.
• HTTP StartRun and ContinueRun map the typed degraded-admission error to 503 with code terminal_durability_unavailable. ContinueRun still reports unknown and non-completed source errors before the global durability gate.
• Deterministic tests cover several already-admitted UpdateRun-failed completions at retention 1, durable rows remaining non-terminal, append-failure and status-failure boundedness, concurrent rejected and recovered admissions under race, the shared retry deadline and unlocked I/O, intentional StorageModeNone suppression, and no-store behavior.

Rollout and rollback update:

• Monitor terminal AppendEvent/UpdateRun errors together with HTTP 503 terminal_durability_unavailable. Recovery of status persistence is probed on later admissions; acknowledged retries reopen admission automatically.
• A permanent append ambiguity requires operator recovery or restart rather than an unsafe duplicate append. Existing admitted work remains queryable in memory.
• Roll back if healthy stores spuriously return 503, admission retry exceeds the single deadline, source error precedence changes, or unrelated runner/conversation operations block. Before rollback during a real persistence outage, stop new producers and preserve the process because reverting the gate would reintroduce either truthful-state eviction or unbounded protected memory growth.

[dennisonbertram]

Exact-head release review of local commit 92fd8f9212894887b4debb75912d710fb16d3980 requested changes before any push or merge.

Additional acceptance evidence required:

• terminal event persistence must not hold the global conversation journal lock; unrelated conversations must continue while the target append or final status write is blocked;
• terminal status must never be written durably when the matching terminal event append failed;
• final status persistence must use a bounded context and the nonfatal failure policy must keep in-memory clients moving without overstating durable success;
• terminal and ordinary status writes must share one per-run serialization and terminal-immutability boundary so delayed nonterminal or waiting writes cannot overwrite the winner;
• StorageModeNone must still drain previously queued recorder work before terminal status visibility;
• per-conversation sequencing locks must be reclaimed safely after the final holder or waiter releases.

Each item needs a deterministic failing-first regression, repeated race coverage, and an honest documented store-failure contract. PR #1070 remains unapproved and its remote head remains intentionally stale until these checks pass.

[dennisonbertram]

Local review-hardening verification

Verified locally on commit 28bcf5052ccc17c10d4e676357d9f766fa1451fe (not pushed; PR #1070 intentionally remains on its older head pending parent composition with #1055).

All review-addendum acceptance cases pass:

• append failure skips terminal status persistence while in-memory replay/status/fanout complete;
• status update error and context timeout leave the built-in durable run non-terminal while live publication completes;
• delayed non-terminal status cannot overwrite terminal state;
• blocked terminal I/O permits unrelated conversations but prevents same-conversation overtaking;
• StorageModeNone waits for recorder drain;
• contended and 100 distinct keyed locks reclaim to zero.

Evidence: terminal suite normal/race -count=100; HTTP poll/replay normal/race -count=100; complete harness/server normal/race and vet; fresh uninterrupted ./scripts/test-regression.sh PASS with 85.6% total coverage and zero uncovered functions. Hosted checks remain pending until the composed head is pushed.

[dennisonbertram]

Final cheap exact-head review of 28bcf505 found one remaining P1 in the documented UpdateRun-failure policy.

A successful terminal event append sets terminalEventPersisted, but the result of terminal status UpdateRun is not retained. pruneCompletedRunsLocked therefore treats the run as safely durable based on the event alone. Under MaxCompletedRetention pressure, it can evict the only truthful in-memory terminal state even though the durable run row remains nonterminal. API pollers and conversation subscribers do not pin the run.

Additional acceptance criterion:

• track terminal status persistence separately;
• a terminal run whose final status was not persisted must not be considered safely prunable merely because its terminal event persisted;
• add a deterministic retention-pressure regression with terminal UpdateRun failures across multiple completed runs, proving the truthful in-memory terminal states remain available while durable rows remain nonterminal;
• document the bounded retention/recovery implications honestly and rerun the full terminal and repository gates.

PR #1070 remains unapproved and unpushed until this invariant passes cheap and Sol re-review.

[dennisonbertram]

Exact-head re-review: concurrent continuation source pruning

Independent cheap-tier review of local commit cad182fcbd4d11dc66ddd6eef2155243e1f56afb found one remaining P1, so PR #1070 remains unapproved and unpushed.

ContinueRunWithOptions validates its completed source and releases Runner.mu before entering the terminal-durability recovery gate. During that unlocked window, a concurrent StartRun can recover status-only durability gaps and invoke an unqualified prune. That prune can delete the already-validated continuation source, after which the continuation's required revalidation returns a spurious ErrRunNotFound.

Required correction within #1067:

• reserve or pin the continuation source before releasing the initial validation lock, and make every pruning path respect that reservation; an equivalent admission/pruning serialization is acceptable if it preserves concurrency and error precedence;
• reclaim any reservation state after success or failure;
• add a deterministic concurrent StartRun recovery plus ContinueRun regression, not a sleep-based test;
• retain the existing bounded-retry, no-lock-held-store-I/O, source-error precedence, and single-winner continuation contracts;
• rerun focused normal/race stress, full harness/server normal/race/vet, and the foreground non-TTY repository regression.

The existing sequential source-preservation test is insufficient for this interleaving. The exact reviewed SHA is not approved. origin/main also advanced to 7f8b2c92, so the eventual corrected head must be rebased and re-reviewed again before promotion.

[dennisonbertram]

Exact-head P1 follow-up (cad182fc): Continue validated a completed source, released Runner.mu for bounded durability recovery, and could then lose that source to a concurrent Start recovery/prune before its write-lock revalidation.

Deterministic red added: TestRunner_ConcurrentStartRecoveryCannotPruneValidatedContinuationSource pauses Continue at the post-validation boundary, lets Start recover/prune at retention 1, then resumes Continue. Against cad182fc it fails with ContinueRun after concurrent Start recovery: run not found.

Repair contract: reserve the validated source under Runner.mu across unlocked recovery and the single-winner handoff; every completed-run prune path must respect the reservation; release/re-prune on all exits; preserve error precedence, one shared retry deadline, and no store I/O under Runner/status/conversation locks.