Do not block a second consumer on an already-drained halt signal - #937
Merged
Quim Muntal (qmuntal) merged 1 commit intoAug 28, 2026
Conversation
The off-thread streaming run loop emits a single one-shot internalHaltSignal per completion epoch, then parks awaiting input. TakeEventStream, when it is not expecting fresh work, targets that already-emitted halt (myEpoch = currentEpoch). If a prior consumer already drained the signal, a later consumer blocks in nextEvent forever - the run loop is parked and never re-emits it. This is reachable through the public API: Environment.Run already drives one RunToNextHalt internally, so a caller that calls Run.RunToNextHalt again (or drains StreamingRun.WatchUntilHalt twice) on an already-Idle run deadlocks. The lockstep stream returns promptly in the same situation. When not expecting fresh work, drain any still-queued events non-blockingly and stop at the terminal (Idle/Ended) or - for RunToNextHalt semantics - pending-request halt, instead of blocking on a signal that will never be re-emitted. The blockOnPendingRequest path still falls through to block for serviced input. Verified against the full workflow suite under -race.
Copilot started reviewing on behalf of
PratikDhanave (PratikDhanave)
August 28, 2026 05:52
View session
github-actions
Bot
added
area:workflow
Contributor
|
github-actions
Bot
added
risk:medium
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a deadlock in the off-thread (streaming) execution event stream where taking/consuming the event stream a second time on an already-halted run could block forever because the per-epoch halt signal is a one-shot queue item that may have been consumed by a prior consumer.
Changes:
- Add a non-blocking “already halted/no fresh work” path in
streamingRunEventStream.TakeEventStreamthat drains queued events and terminates based on current status (Idle/Ended, or PendingRequests when not blocking). - Add a regression test ensuring a second
RunToNextHalton an already-Idle run returns promptly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| workflow/internal/execution/eventstream.go | Adds a fast path to avoid blocking on an already-drained one-shot halt signal when re-consuming a halted stream. |
| workflow/inproc/binding_test.go | Adds a regression test covering the previously hanging “second RunToNextHalt on halted run” scenario. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1172
to
+1182
|
|
||
| done := make(chan struct{}) | ||
| go func() { | ||
| _, _ = run.RunToNextHalt(context.Background()) | ||
| close(done) | ||
| }() | ||
| select { | ||
| case <-done: | ||
| case <-time.After(5 * time.Second): | ||
| t.Fatal("second RunToNextHalt on an already-halted run blocked") | ||
| } |
Quim Muntal (qmuntal)
approved these changes
Aug 28, 2026
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.
Problem
The off-thread streaming run loop (
workflow/internal/execution/eventstream.go) emits a single one-shotinternalHaltSignalper completion epoch, then parks inwaitForInput. InTakeEventStream, a consumer that isn't expecting fresh work targets that already-emitted halt (myEpoch = currentEpoch). If a prior consumer already dequeued that one-shot signal, a later consumer blocks innextEventforever — the run loop is parked and never re-emits it.This is a deadlock reachable through the public API:
Environment.Runalready drives oneRunToNextHaltinternally, so callingRun.RunToNextHalt(ctx)again on the returned (already-Idle) run hangs.StreamingRun.WatchUntilHaltonce (to Idle) then again with no new input hangs.The sibling lockstep
RunEventStreamreturns promptly in the exact same scenario; only the off-thread/default stream hangs. The stream is explicitly designed to be re-taken (RunHandleresetsisEventStreamTakenafter each consumer), so re-consuming a terminal state is a supported operation.Fix
When
TakeEventStreamis not expecting fresh work (the run has already halted), drain any still-queued events non-blockingly and stop at the terminal (Idle/Ended) halt — or, whenblockOnPendingRequestis false, the pending-request halt — instead of blocking on a signal that will never be re-emitted. TheblockOnPendingRequestpath still falls through to the blocking read so it can await serviced input.Test
TestRun_SecondRunToNextHaltOnHaltedRunReturnscallsRunToNextHalta second time on an already-Idle run under a watchdog; it hangs before the fix and returns promptly after. Validated against the full./workflow/...suite under-race -shuffle(no regressions).