PRD: fix data race on ManagedSession.Messages between turn and external readers#88
Merged
Conversation
…ed accessors (#85) Encapsulates the conversation history and todo list behind Messages() and Todos() so external readers (status, todos commands) take a consistent snapshot under sess.mu instead of racing with an in-flight turn. - messages and todos fields are now unexported. - Messages() returns a shallow copy via slices.Clone under the lock; the append-only invariant on the slice is documented at the field. - Todos() returns the (internally thread-safe) list pointer under the lock. - statusCmd and todosCmd in daemon.go switch to the accessors. - runTurnInternal and Close() use the unexported fields; Close() semantics unchanged in this slice (handled in slice 2, #86).
Close() previously deleted the session from the manager map first and then read sess.messages without synchronization, racing with an in-flight turn's append. It also blocked on whatever the running turn was doing because there was no cancellation step. The new flow is: 1. Look up the session under sm.mu. 2. Cancel any in-flight turn so it releases sess.mu promptly. 3. Take sess.mu and run agent.Finalize on the now-stable message history (the lock is held across Finalize so the snapshot is consistent for the entire call). 4. Remove the session from the manager map. The duplicated cancel-under-mutex pattern shared by Close() and Interrupt() is factored into a new (*ManagedSession).cancelInflight() helper. The redundant len(messages)==0 guard is dropped: Finalize already short-circuits on empty messages and the prior read was racy. Reset() inherits the new behavior unchanged.
Adds TestSessionConcurrency in pkg/daemon/sessions_concurrency_test.go to lock in the behavior introduced by slices 1 and 2 (#85, #86) and provide regression protection under -race. Subtests: - messages snapshot serializes with in-flight turn: Messages() blocks while a turn holds sess.mu and returns a coherent slice once the turn completes. - close cancels in-flight turn and removes session: Close() unblocks the turn, finalizes, and deletes the session from the manager map. - interrupt unblocks in-flight turn: Interrupt() cancels the turn and subsequent readers proceed. - stress: hammered readers and cancellers do not race or panic: 16 hammers x 3 op kinds x 10 iters racing against an active turn plus Close(); under -race the assertion is "no race report, no panic". Helpers: - gatedProvider exposes started/release channels so subtests can deterministically synchronize on an in-flight turn without time.Sleep. - slowProvider models a turn that finishes naturally if no one cancels, used by the stress subtest so both natural-completion and cancellation paths are exercised. - newConcurrencyTestSM uses fakeProvider for agentCfg.Provider so Close()'s Finalize step never blocks regardless of the chat provider.
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
1 participant
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.
Summary
Implements PRD #84: PRD: fix data race on ManagedSession.Messages between turn and external readers
Resolved sub-issues
Closes #84