Fix RunInTerminalTool hang when shell exits before/during execute()#313249
Merged
meganrogge merged 4 commits intomainfrom Apr 29, 2026
Merged
Fix RunInTerminalTool hang when shell exits before/during execute()#313249meganrogge merged 4 commits intomainfrom
meganrogge merged 4 commits intomainfrom
Conversation
The rich execute strategy subscribed to onExit/onDisposed AFTER awaiting xtermReadyPromise. If the pty had already exited before execute() was entered, those emitters had already fired and been disposed, so Event.toPromise() subscribed to a dead emitter and never resolved - hanging the run-in-terminal tool until the agent's 60-minute outer timeout (16 X_AGENT_STILL_RESPONDING failures observed in eval run 25073061392). - Add synchronous up-front check for instance.isDisposed / exitCode in both rich and basic strategies; resolve immediately with the captured exit code rather than subscribing to a fired-and-disposed emitter. - In the rich strategy, move the Promise.race lifecycle subscription setup BEFORE 'await this._instance.xtermReadyPromise' so onExit / onDisposed are wired synchronously at function entry, closing the race window where the pty exits during xterm initialization. - Add unit tests for both branches in rich and basic strategies. Fixes #313248
meganrogge
added
the
~requires-eval-assessment
meganrogge
removed
the
~requires-eval-assessment
Collaborator
Author
|
/requires-eval-assessment terminalbench2 gpt-5.4,claude-opus-4.6,claude-opus-4.7 |
Contributor
|
⏳ Queued vscode build for
|
vs-code-engineering
Bot
added
the
~requires-eval-assessment
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a hang in the run_in_terminal tool’s rich/basic execute strategies when the terminal process has already exited (or exits during strategy setup), by ensuring lifecycle completion can’t be missed due to late event subscription.
Changes:
- Add synchronous “already disposed / already exited” short-circuit handling at the start of
execute()in both strategies. - In rich strategy, subscribe to lifecycle events before awaiting
xtermReadyPromiseto avoid missingonExit/onDisposed. - Add unit tests covering the “already exited” and “already disposed” early-out behavior for both strategies.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/executeStrategy/richExecuteStrategy.ts | Adds early-out checks and moves lifecycle subscriptions ahead of await xtermReadyPromise to prevent hangs. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/executeStrategy/basicExecuteStrategy.ts | Adds early-out checks to avoid waiting on already-fired lifecycle events. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/richExecuteStrategy.test.ts | Adds tests for immediate return on captured exit code and for disposed-instance rejection. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/basicExecuteStrategy.test.ts | Adds analogous tests for the basic strategy. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 2
…r/executeStrategy/richExecuteStrategy.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…r/executeStrategy/basicExecuteStrategy.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
pwang347
approved these changes
Apr 29, 2026
Contributor
|
🚀 Queued eval-assessment publish build for
|
Contributor
|
🔬 Queued eval-assessment benchmark for
Results will be posted back here when the run completes. |
Contributor
|
✅ Eval-assessment build published.
|
vs-code-engineering
Bot
removed
the
~requires-eval-assessment
Contributor
|
📊 Eval-assessment benchmark complete.
Analysis ResultsResolution Rate
Token Usage
Step Counts
|
Contributor
|
📊 Eval-assessment benchmark complete.
Analysis ResultsResolution Rate
Token Usage
Step Counts
|
Contributor
|
📊 Eval-assessment benchmark complete.
Analysis ResultsResolution Rate
Token Usage
Step Counts
|
56 tasks
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.
Fixes #313248
Problem
In benchmark eval run
25073061392, 16 tests failed withX_AGENT_STILL_RESPONDING(60-minute outer timeout). All 16 traced to the same hang inRunInTerminalToolrich execute strategy. The same bug was independently confirmed in run25115115244, where 13/89 tasks timed out with the identical pattern — shell process died (exit codes 1/2/22/127/130),onExitalready fired,Event.toPromisehung forever.The build under test already had #312827 (
onExitlistener) and #312854 (downgrade rich → basic on broken shell integration), so the listener wasn't missing. The actual bug was a subscription-ordering / already-fired-emitter race.RichExecuteStrategy.execute()wiredonExit/onDisposedafterawait this._instance.xtermReadyPromise. Two failure modes from this:execute()was entered,_onExithad already fired and been disposed (seeterminalInstance._onProcessExit).Event.toPromise(onExit)then subscribed to a dead emitter and never resolved — hang until the agent's 60-min outer timeout.xtermReadyPromise. If the pty exits during xterm init, the events fire before our subscription is attached and are missed.BasicExecuteStrategyalready wired its race before the await, but had the same upfront-already-dead hole.Fix
In both
richExecuteStrategy.tsandbasicExecuteStrategy.ts:execute():instance.isDisposed→ throw"The terminal was closed"(matches existingonDisposedrace branch).instance.exitCode !== undefined→ resolve immediately with the captured exit code andadditionalInformation: 'Command exited with code N'.Promise.race([...])lifecycle subscriptions beforeawait xtermReadyPromisesoonExit/onDisposedare wired synchronously at function entry. Closes the in-flight death race.Tests
Added unit tests to both
richExecuteStrategy.test.tsandbasicExecuteStrategy.test.ts:returns immediately with captured exit code when pty has already exited before execute()throws "The terminal was closed" when instance is already disposed before execute()runCommand/sendTextare stubbed to throw, proving the early-out path doesn't try to write to a dead shell.