fix(session): interrupt a running prompt when a new one is submitted - #41748
fix(session): interrupt a running prompt when a new one is submitted#41748ShinjukuZhu wants to merge 3 commits into
Conversation
When the user submits a new prompt while a run is active, the Runner's ensureRunning simply awaited the current run's completion and dropped the new work. If the assistant was blocked on a long-running tool like `sleep`, the new prompt was not answered until the tool finished. Now ensureRunning interrupts the active run and starts the new work, so a new prompt is handled promptly and the current tool (e.g. a shell command) is cancelled. The interrupted caller still resolves through onInterrupt. Adds a regression test that submits a second prompt while a bash `sleep` is running and asserts the second prompt completes immediately.
|
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. |
github-actions
Bot
added
the
needs:compliance
|
The following comment was made by an LLM, it may be inaccurate: Based on my search, I found one potentially related PR that's worth noting: Related PRPR #36375: This PR appears to address a similar issue with the Runner handling multiple work submissions. However, it focuses on queuing work rather than interrupting the current run like PR #41748 does. The approaches are different:
These represent different strategies for handling concurrent prompts in a session runner. Other related PRs (different scope):
Conclusion: PR #41748 appears to be addressing a distinct improvement to the session runner behavior that's not covered by existing open PRs. |
github-actions
Bot
removed
needs:issue
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
| const old = st.run | ||
| yield* Deferred.fail(old.done, new Cancelled()).pipe(Effect.asVoid) | ||
| const done = yield* Deferred.make<A, E | Cancelled>() | ||
| const run = yield* startRun(work, done) |
There was a problem hiding this comment.
Could we prevent the old run from publishing status after the replacement starts? Since startRun happens before the old fiber is interrupted, its interruption handler can publish idle after the replacement publishes busy, and CLI/TUI consumers may treat the active replacement as complete. Is this a concern?
There was a problem hiding this comment.
Good point. I addressed this by making the replacement work wait until the old run has finished interruption cleanup. The interruption still runs on an independent fiber so finishRun can acquire the SynchronizedRef without deadlocking. In addition, finishRun keeps its run-ID guard, so a replaced run cannot publish idle for the active replacement. I added regression coverage that holds the old cleanup open and verifies that no stale idle is published while the replacement is active.
| // `finishRun` acquires this same ref, so interrupting it while holding | ||
| // the lock would deadlock. | ||
| const old = st.run | ||
| yield* Deferred.fail(old.done, new Cancelled()).pipe(Effect.asVoid) |
There was a problem hiding this comment.
Could we defer failing old.done until the old fiber has completed interruption cleanup? Failing it here wakes onInterrupt (lastAssistant) before interruption begins, so a synchronous prompt caller can receive an assistant message whose tool is still running and whose completion/error metadata is not finalized. Is this a concern?
There was a problem hiding this comment.
Yes, this was a real concern. I removed the eager failure of old.done; it is now completed by the old fiber through the existing finishRun path after interruption cleanup has finished. This prevents onInterrupt / lastAssistant from returning while the tool and its metadata are still being finalized. I also added regression coverage for this ordering, successive replacements, and cancellation while a replacement is waiting.
2 participants
Issue for this PR
Closes #41753
Type of change
What does this PR do?
Problem. When the assistant is mid-task and calls the bash tool with a long-running command (e.g.
sleep 30), submitting a new prompt is not handled until the command finishes. The new prompt is saved but no response arrives until the running tool exits — the user has to wait for the whole command before their prompt is answered.Root cause.
Runner.ensureRunninginpackages/opencode/src/effect/runner.tstreats an active run as a single-flight/join point:A second
ensureRunningcall (the new prompt's loop) simply awaits the current run's completion and drops the new work. The new prompt is only eventually answered because the current run loop happens to pick up the newly-saved user message after the tool finishes.Fix. When a new run is requested while one is active, interrupt the current run and start the new work immediately so the new prompt is answered promptly and the current tool (e.g. a shell command) is cancelled. The interrupted caller resolves through
onInterrupt(for prompt loops that islastAssistant). The fiber interrupt runs on a separate fiber because the interrupted run'sfinishRunacquires the sameSynchronizedRef— interrupting while holding the lock would deadlock.Note on approach vs. #36375: that PR queues the new work behind the running run, which fixes dropped work but still makes a new prompt wait for a long-running tool. This PR interrupts instead, which is what the linked issue asks for. The two are mutually exclusive implementations of the
Runningcase; if the maintainers prefer queueing semantics over interrupting, this PR can be reworked accordingly.How did you verify your code works?
a new prompt takes over a running bash tool instead of waiting for it) that starts a first prompt loop executingsleep 30via the bash tool, then submits a second prompt. Before the fix the second loop times out waiting for the sleep; after the fix it completes immediately and the interrupted bash tool is cancelled.packages/opencode:bun test test/effect/runner.test.ts test/session/pass (one pre-existingglob tool keeps instance contexttimeout fails on a clean tree in this environment as well, unrelated to this change).bun typecheckandoxlintpass forpackages/opencode.Screenshots / recordings
N/A — no UI changes.
Checklist