Fix run_in_terminal wedging when cached terminal is disposed#311019
Fix run_in_terminal wedging when cached terminal is disposed#311019meganrogge merged 1 commit intomainfrom
run_in_terminal wedging when cached terminal is disposed#311019Conversation
|
cc @anthonykim1 |
Screenshot ChangesBase: Changed (2)
|
| Before | After |
|---|---|
 |
 |
editor/inlineCompletions/other/JumpToHint/Dark
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
Pull request overview
Fixes a race where run_in_terminal could permanently wedge by reusing a stale cached terminal instance after the terminal was disposed (eg. via terminal.killAll), by ensuring disposed cached terminals are not reused.
Changes:
- Add an
isDisposedguard when reusing the cached foreground terminal in_initTerminal. - Add a unit test intended to cover the disposed-cached-terminal scenario.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts | Prevents reuse of cached foreground terminal instances once they’ve been disposed. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/runInTerminalTool.test.ts | Adds test coverage for the disposed cached terminal reuse regression (currently needs strengthening). |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
| // Verify the guard condition that _initTerminal uses: | ||
| // cachedTerminal && !cachedTerminal.isBackground && !cachedTerminal.instance.isDisposed | ||
| const wouldReuse = cachedTerminal !== undefined && !cachedTerminal.isBackground && !cachedTerminal.instance.isDisposed; | ||
| strictEqual(wouldReuse, false, 'Should not reuse a disposed cached terminal'); |
There was a problem hiding this comment.
This test doesn’t actually exercise RunInTerminalTool behavior: it recomputes the guard condition locally, so it will still pass even if _initTerminal stops checking instance.isDisposed (or if the reuse logic changes). To make this regression-proof, call into the tool (e.g. via prepareToolInvocation/invoke or by invoking the private _initTerminal through a cast) and assert a new terminal is created/returned when the cached association’s instance.isDisposed is true, and that the association map is updated accordingly.
Fixes #296892
Fixes #308610
When
terminal.killAll(or other terminal disposal) races with arun_in_terminalcall,_initTerminalcould return a stale cached terminal from_sessionTerminalAssociationsbecause theonDidDisposeInstancecleanup hadn't propagated yet. This caused all subsequent terminal operations to fail permanently.Added an
isDisposedcheck before returning the cached terminal so_initTerminalfalls through and creates a fresh terminal instead.