Fix race condition where Prompt() returns before SessionUpdate handlers complete #5
Conversation
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
Add a failing test that surfaces a race condition where Prompt() can return before all SessionUpdate notification handlers have finished processing. The issue occurs because: - SessionUpdate notifications are handled asynchronously (goroutines) - PromptResponse is handled synchronously - The receive loop spawns notification handlers but doesn't track them When a server sends multiple SessionUpdate notifications followed by a PromptResponse, the client's Prompt() call returns immediately upon receiving the response, even though notification handlers may still be queued or running. The test expects all SessionUpdate handlers to complete before Prompt() returns, which represents the intended semantic contract: a prompt operation includes all its updates. Currently fails with 0/10 handlers completed when Prompt() returns. This will be fixed in a subsequent commit.
Fix race condition where Prompt() could return before all SessionUpdate notification handlers finished processing. The issue occurred because notification/request handlers were spawned asynchronously while responses were processed synchronously. This meant the receive loop would: 1. Read SessionUpdate, then spawn goroutine G1 2. Read SessionUpdate, then spawn goroutine G2 3. Read PromptResponse, then handle synchronously, unblock Prompt() At step 3, goroutines G1/G2 _could_ be queued, running, or complete. Solution: - Add notificationWg to Connection to track in-flight handlers - Wrap notification handlers with WaitGroup Add/Done - Call notificationWg.Wait() in SendRequest/SendRequestNoResult after receiving response but before returning to caller This ensures the semantic contract that a prompt operation includes all its updates: when Prompt() returns, all SessionUpdate notifications sent before the PromptResponse have been fully processed. Fixes the test added in previous commit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes a race condition where
Prompt()could return before allSessionUpdatenotification handlers finished processing.Notifications are handled asynchronously via goroutines, but responses are handled synchronously. This meant
Prompt()would return as soon as the response arrived, even if notification handlers were still queued or running.Now tracks in-flight notification handlers with a
WaitGroupand waits for them to complete inSendRequest/SendRequestNoResultbefore returning. Ensures callers see all updates before the method returns.Includes test that verifies all (10)
SessionUpdatehandlers complete beforePrompt()returns; it previously failed with none completed.