fix(runner): queue work when already running instead of discarding#36375
Open
dreampuf wants to merge 1 commit into
Open
fix(runner): queue work when already running instead of discarding#36375dreampuf wants to merge 1 commit into
dreampuf wants to merge 1 commit into
Conversation
When a background subagent completes and calls ops.prompt() to notify the parent session, the parent's Runner is often still Running (processing the previous turn). The old ensureRunning discarded the new work item and just awaited the current run's completion, so the notification was lost and the parent session would hang until the user typed 'continue'. Add a RunningThenRun state to the Runner state machine. When ensureRunning is called while Running, the new work is stored as pending and the runner transitions to RunningThenRun. When the current run finishes, finishRun starts the pending work and transitions back to Running. Cancel in the RunningThenRun state interrupts both the current and pending work. This fixes the root cause of the parent-not-notified bug. The alternative approach in PR anomalyco#35041 moves the notification to the session completion path but still races through ensureRunning and can lose the notification when the parent is busy. Refs: anomalyco#35066
Contributor
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
Contributor
|
The following comment was made by an LLM, it may be inaccurate: Potential related PR found:
This is related but takes a different approach. PR #36375 is the more comprehensive fix. No other duplicate PRs found. |
Contributor
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
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.
Issue for this PR
Closes #35066
Type of change
What does this PR do?
When a background subagent completes and calls
ops.prompt()to notify the parent session, the parent'sRunneris often stillRunning(processing the previous turn). The oldensureRunningdiscarded the new work item and just awaited the current run's completion — so the notification was lost and the parent session would hang until the user typed "continue".Root cause:
Runner.ensureRunninginpackages/opencode/src/effect/runner.tshad this behavior when state wasRunning:It returned the existing run's deferred and threw away the new
workargument. So if the parent was busy when the subagent finished, theops.prompt()call that should have queued a new turn was silently dropped.Commit dabf2dc ("remove the need for polling from experimental background agents" #29179) removed the old polling-based idle check (
resumeWhenIdle), replacing it with a directops.prompt()call that races withensureRunning— which is exactly where the race lives.Fix: Add a
RunningThenRunstate to the Runner state machine (mirroring the existingShellThenRunpattern):ensureRunningwhenRunning: store the new work aspending, transition toRunningThenRun, return a deferred that resolves when the pending work completes.finishRunwhenRunningThenRun: when the current run finishes, start the pending work and transition back toRunning.cancelwhenRunningThenRun: interrupt both the current run's fiber and fail both deferreds (current + pending), then go idle.Why this approach over PR #35041: PR #35041 (draft) moves the notification to the session completion path, but still has the same
ensureRunningrace — it doesn't fix the root cause. If the parent is busy when the notification fires,ensureRunningstill discards the work. This PR fixes the root cause in the Runner itself, so all callers ofensureRunning(not just the subagent notification path) benefit.How did you verify your code works?
"concurrent callers share the same run"— now expectscalls=2because the queued run also increments."second ensureRunning ignores new work"→"second ensureRunning queues behind running and executes after"— now asserts the second work actually runs and returns its own result.ensureRunning queues behind running and transitions through RunningThenRunthird ensureRunning shares pending in RunningThenRuncancel in RunningThenRun cancels both current and pendingScreenshots / recordings
N/A — no UI changes.
Checklist