docs(runtime): document StreamStopped ordering and teardown trade-offs (#3074)#3275
Conversation
Capture the decisions left open by #3070: keep StreamStopped emitted before session-end hooks (Option B), since no StreamStoppedEvent consumer relies on hooks having finished and emit-before-hooks keeps the TUI responsive. Document that the channel close, not the event, is the terminal signal and that the event is best-effort. Document the accepted goroutine-leak-over-panic trade-off in restoreAndClose. Add a regression test asserting StreamStopped is the last event delivered before the channel close. Refs #3074
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This is a documentation-only PR adding doc comments to three runtime files and one regression test. No production logic changes.
Drafter findings: The single medium-severity hypothesis (StreamStopped potentially dropped due to buffer exhaustion in the new test) was dismissed by verification: defaultEventChannelCapacity = 128 leaves 126 free slots after the 2 pre-seeded events, so the non-blocking emit succeeds without issue.
The remaining low-severity observations (doc understatement about best-effort delivery, latent test fragility around synchronous channel close) are pre-existing design characteristics, not problems introduced by this PR.
The new test correctly pins the Option B ordering decision from #3074: StreamStopped is emitted before session-end hooks, it is the last event before channel close, and the assertion structure is sound.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR is a documentation-only change — no production behaviour is altered. The new doc comments on restoreAndClose, StreamStoppedEvent, and finalizeEventChannel accurately describe the existing code, correctly document the #3069/#3070/#3074 design decisions, and follow the project's "explain the why" comment philosophy from AGENTS.md.
The new regression test (TestLocalRuntime_FinalizeEventChannelStreamStoppedIsLastBeforeClose) correctly exercises the ordering guarantee using a buffered channel (defaultEventChannelCapacity = 128) and a real runtime instance, with no race conditions identified.
No bugs introduced by the + lines were found.
3 participants
What
Follow-up to #3074, the documentation and decision debt left by #3070 (fix for #3069). No production behavior change: the only code edits are doc comments; the runtime ordering is kept as-is (Option B) and a regression test is added.
Decision: Option B (keep current order)
StreamStoppedstays emitted before the session-end hooks and telemetry, rather than moving it after (Option A). An audit of everyStreamStoppedEventconsumer (TUI chat, sidebar, supervisor, leantui, A2A adapter, API server) shows none rely on session-end hooks having completed, so the order is safe, and emit-before-hooks keeps the TUI responsive (spinner stop and queue drain are not delayed behind user-configuredsession_endhooks). The authoritative terminal signal remains the channel close, not the event. Full rationale and the consumer audit are in #3074.This is opened as a draft because #3074 listed Option A as the recommended option; a maintainer decision on B versus A would unblock marking it ready. Option A is a one-line move and the docs plus test apply unchanged either way.
Changes
pkg/runtime/elicitation.gorestoreAndClose: close-under-write-lock is the #3069 fix, and the accepted trade-off that teardown blocks (goroutine leak) instead of panicking when a parked sender cannot drainpkg/runtime/loop.gofinalizeEventChannel: ordering decision (#3074) and best-effort delivery; inline note on the non-blocking emit warning against reintroducing the #3070 deadlockpkg/runtime/event.goStreamStoppedEvent: best-effort delivery, channel close is the terminal signal, hooks run after the eventpkg/runtime/elicitation_test.goTestLocalRuntime_FinalizeEventChannelStreamStoppedIsLastBeforeClose: asserts exactly one StreamStopped and that it is the last event delivered before the channel closeIssue scope mapping
restoreAndCloseleak-over-panic trade-offValidation
task buildgolangci-lint run ./pkg/runtime/...go test ./pkg/runtime/go test -race ./pkg/runtime/ -run 'Elicitation|FinalizeEventChannel|RunStreamClosesChannel'Note:
go test -race ./pkg/runtime/(full package) reports a data race inTestHandleStream_ContextCancellation. It is pre-existing and unrelated to this change (reproduced on a tree with these edits stashed) and lives in thestalledStream.Close()test double instreaming_test.go, which is invoked concurrently without synchronization. It does not surface under the standardtask testrun (no-race). Out of scope here; a separate fix can add a mutex or atomic tostalledStream.Close().Relations