fix(server): serve /events and /summary from the store after a daemon restart - #1383
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
dennisonbertram
force-pushed
the
issue-1375-durable-run-routes
branch
2 times, most recently
from
September 5, 2026 15:24
1d16b8d to
7ecf614
Compare
…allback
Behavioral tests added: run completed against one Runner+Store, then a
second, unrelated Runner sharing the same persistent store (simulating a
daemon restart) must serve GET /v1/runs/{id}/events and GET
/v1/runs/{id}/summary from the store, matching GET /v1/runs/{id}'s existing
fallback behavior.
Test runner output (expected: all failing):
=== RUN TestRunEventsAndSummary_ServedFromStoreAfterRunnerForgets
=== PAUSE TestRunEventsAndSummary_ServedFromStoreAfterRunnerForgets
=== CONT TestRunEventsAndSummary_ServedFromStoreAfterRunnerForgets
http_durable_run_routes_test.go:150: expected 200 from /events after restart, got 404: {"error":{"code":"not_found","message":"run \"run_431bae81-0ec6-4806-8be0-45da03f159a7\" not found"}}
--- FAIL: TestRunEventsAndSummary_ServedFromStoreAfterRunnerForgets (0.07s)
FAIL
FAIL go-agent-harness/internal/server 0.615s
FAIL
This is a meaningful failure: /events 404s exactly as issue #1375 describes,
not an import/compile error. The implementation in the next commit adds the
store fallback for both routes.
These tests will pass after the implementation in the next commit.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WJGxhoFhA8JjkwZFcLGdS5
…ner has forgotten a run Implementation for tests added in 966b3b8. GET /v1/runs/{id}/events and GET /v1/runs/{id}/summary only ever asked the in-memory Runner, so a daemon restart with the same run/conversation SQLite stores made both routes 404 "run not found" for any historical run, even though GET /v1/runs/{id} already fell back to the store (handleGetRun) and GET /v1/conversations/{cid}/events replayed the same run's durable events. - handleRunEvents now falls back to handleDurableRunEvents when runner.Subscribe fails: it resolves the run from the store, replays its durable event log via the existing per-run store.Store.GetEvents(runID, afterSeq) query (honoring Last-Event-ID the same way the live path does), and closes the stream after replay -- the run is necessarily terminal, so there is no live tail to wait for. Event IDs, types, and payload shapes are unchanged (same store.Event -> harness.Event conversion Runner. conversationReplay already uses for durable conversation replay). - handleRunSummary now falls back to durableRunSummary when runner.GetRunSummary returns ErrRunNotFound: it applies the same completed/failed status gate and steps/tool-call event scan as Runner.GetRunSummary, but reads usage and cost totals from the run's last usage.delta event payload (cumulative_usage / cumulative_cost_usd / cost_status), which already carries the same cumulative totals the live in-memory accumulator would report -- store.Run's UsageTotalsJSON/ CostTotalsJSON columns are never populated by the writer today, so they are not a usable source for this. Test runner output (expected: all passing): === RUN TestRunEventsAndSummary_ServedFromStoreAfterRunnerForgets === PAUSE TestRunEventsAndSummary_ServedFromStoreAfterRunnerForgets === CONT TestRunEventsAndSummary_ServedFromStoreAfterRunnerForgets --- PASS: TestRunEventsAndSummary_ServedFromStoreAfterRunnerForgets (0.03s) PASS ok go-agent-harness/internal/server (cached) Full-package regression check: go test ./internal/server ./internal/harness -race ok go-agent-harness/internal/server ok go-agent-harness/internal/harness go vet ./internal/server/... ./internal/harness/... — clean Behavioral tests covered: run present only in the store -> /events 200 with replayed durable events ending on run.completed, /summary 200 with totals matching the live run (steps, prompt/completion tokens, cost, tool calls, cache hit rate). Files changed: internal/server/http_runs.go Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJGxhoFhA8JjkwZFcLGdS5
… fallback Regression tests added that would fail if the change in 9dbdf7c is reverted: - TestDurableRunEvents_LastEventIDSkipsSeenEvents: reconnects to the durable /events fallback with Last-Event-ID set to an already-seen event and asserts the resumed replay skips it (and still ends on run.completed). If handleDurableRunEvents were simplified to an unconditional full replay (ignoring the header), this test catches the duplicate-event regression. - TestDurableRunSummary_StillRunningReturnsConflict: a run seeded directly into the store with status "running" (never touched by the local runner) must still 409 from /summary, not 200 with a misleading partial summary. If durableRunSummary dropped its completed/failed status gate, this test catches it. Also updates README.md (documents the store fallback for GET /v1/runs/{id}, /events, and /summary) and docs/logs/engineering-log.md (cause/fix/regression entry) per the repo's documentation-and-handoff requirement for this issue. Full test suite output: go test ./internal/server ./internal/harness -race ok go-agent-harness/internal/server 16.892s ok go-agent-harness/internal/harness 7.154s go vet ./internal/server/... ./internal/harness/... — clean Regression scenarios covered: - Last-Event-ID resumption against the durable (store-only) replay path does not repeat already-delivered events and still terminates on run.completed. - A store-only run whose status is still "running" returns 409 from /summary instead of a fabricated 200. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJGxhoFhA8JjkwZFcLGdS5
dennisonbertram
force-pushed
the
issue-1375-durable-run-routes
branch
from
September 5, 2026 15:29
7ecf614 to
6ef32d1
Compare
This was referenced Sep 5, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1375
Summary
After a daemon restart with the same run/conversation SQLite stores,
GET /v1/runs/{id}already falls back to the persistent store (handleGetRun), butGET /v1/runs/{id}/eventsandGET /v1/runs/{id}/summaryonly ever asked the in-memoryRunnerand returned 404"run not found"for any historical run — even thoughGET /v1/conversations/{cid}/eventsreplays the same run's durable events. Any client that streams or summarizes by run id (TUI session resume, macapp, ACP) hit a 404 for historical runs.Both routes now fall back to the persistent store when the runner has no live state for the run:
/eventsreplays the run's durable event log via the existing per-runstore.Store.GetEvents(runID, afterSeq)query, honoringLast-Event-IDthe same way the live path does, and closes the stream after replay (a store-only run is necessarily terminal, so there is no live tail)./summaryapplies the same completed/failed status gate and steps/tool-call event scan asRunner.GetRunSummary, reading usage/cost totals from the run's lastusage.deltaevent payload (cumulative_usage/cumulative_cost_usd/cost_status) — those are durable and already carry the same cumulative totals the live in-memory accumulator reports. (store.Run'sUsageTotalsJSON/CostTotalsJSONcolumns exist but are never populated by the writer today, so they were not a usable source.)Event IDs, event types, and payload/response shapes are unchanged.
Scope and issue reconciliation
In scope per the issue: store fallback for the two routes when the runner has no live state, reusing existing seams (
store.Store.GetEvents, the same store→harness.Event conversionRunner.conversationReplayalready performs). Out of scope and untouched: event IDs, replay semantics for live runs, and every other run route. No deviations from the issue's fix boundaries.Impact analysis reconciliation
Changed:
internal/server/http_runs.goonly (handleRunEvents,handleRunSummary, and three new unexported helpers:handleDurableRunEvents,durableRunSummary,durableRunSummaryFromEvents, plusstoreEventToHarness/intFromEventPayload). No changes tointernal/harnessorinternal/store— both already exposed the needed seams (store.Store.GetEvents,harness.ParseEventID,harness.IsTerminalEvent... the last unused here since durable replay always ends the stream).Consumers of these routes (
cmd/harnesscli/tui/api.go,internal/acp/client.go) are unaffected: the live-runner success path is byte-for-byte unchanged, and the only behavior change is that a previously-404 historical-run request now succeeds — a strict improvement with no wire-shape change for them to handle differently.Architecture and duplication check
Searched
internal/server/http_runs.gofor the existing store-fallback pattern (handleGetRun/storeRunToHarness) and reused its shape for/eventsand/summaryrather than inventing a new one. Searchedinternal/harness/runner.gofor existing durable-replay seams (Runner.conversationReplay,Runner.SubscribeConversationFrom) andinternal/store/store.gofor theStoreinterface; choseStore.GetEvents(runID, afterSeq)— the existing single-run durable query — over the conversation-scopedConversationEventReaderpath, since the latter is conversation-wide (would need extra per-run filtering and pagination handling) and the former is already exactly scoped to one run. No new interface, no new store method, no parallel catalog or duplicate wiring; this is additive fallback logic co-located with the siblinghandleGetRunfallback it mirrors.Test-first evidence
Red command:
Observed failure:
Why the failure proved the missing/incorrect behavior: the assertion failed on the exact HTTP status/body the issue describes (404
not_foundfrom/eventsfor a run known only to the store), not an import or compile error — a real behavioral gap, not broken test scaffolding.Green command:
Regression evidence: added
TestDurableRunEvents_LastEventIDSkipsSeenEvents(Last-Event-ID resumption against the durable path doesn't repeat events) andTestDurableRunSummary_StillRunningReturnsConflict(a store-only run still "running" 409s from/summaryinstead of a fabricated 200) — both pass against the implementation and would fail if it were reverted or simplified.Verification evidence
Targeted:
PASS (all three).
Full regression + race, both packages the issue named:
clean (no output).
This is local
go test/go vetverification only — no live daemon restart was exercised against a runningharnessdprocess in this change; the regression test simulates the restart scenario in-process (two independentRunnerinstances sharing onestore.MemoryStore), which is the same simulation pattern the existingTestStoreRunFallbacktest uses forGET /v1/runs/{id}.Rollout and rollback
No migration, no schema change, no new config. Purely additive read-path fallback behind the existing
s.runStore != nilcheck thathandleGetRunalready relies on — when no store is configured, behavior is byte-for-byte unchanged (still 404 as before). Rollback: revert this PR; the routes return to runner-only lookups (restoring the original 404-after-restart bug, with no data loss since nothing is written by this change).Documentation
README.md: added a note under the HTTP surface section thatGET /v1/runs/{id},/events, and/summaryall fall back to the persistent store when the runner has no live state, and that event IDs/wire shapes are unchanged.docs/logs/engineering-log.md: added a 2026-09-05 (Issue bug(server): /v1/runs/{id}/events and /summary return 404 for persisted runs after daemon restart #1375) cause/fix/regression entry following the existing log format.Contract checklist
harnessdprocess; only in-processhttptestsimulation (see Verification evidence)🤖 Generated with Claude Code
https://claude.ai/code/session_01WJGxhoFhA8JjkwZFcLGdS5