Skip to content

Fix run_in_terminal wedging when cached terminal is disposed #311019

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
meganrogge merged 1 commit into from
Apr 17, 2026
+25 −1
Merged

Fix run_in_terminal wedging when cached terminal is disposed #311019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1840,7 +1840,7 @@ export class RunInTerminalTool extends Disposable implements IToolImpl {
// For foreground mode, try to reuse cached terminal (but not if it was a background terminal)
if (!isBackground) {
const cachedTerminal = this._sessionTerminalAssociations.get(chatSessionResource);
if (cachedTerminal && !cachedTerminal.isBackground) {
if (cachedTerminal && !cachedTerminal.isBackground && !cachedTerminal.instance.isDisposed) {
this._logService.debug(`RunInTerminalTool: Using cached terminal with session resource \`${chatSessionResource}\``);
this._terminalToolCreator.refreshShellIntegrationQuality(cachedTerminal);
this._terminalChatService.registerTerminalInstanceWithToolSession(terminalToolSessionId, cachedTerminal.instance);
Expand Down
24 changes: 24 additions & 0 deletions ...ch/contrib/terminalContrib/chatAgentTools/test/electron-browser/runInTerminalTool.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1900,6 +1900,30 @@ suite('RunInTerminalTool', () => {
chatServiceDisposeEmitter.fire({ sessionResources: [LocalChatSessionUri.forSession('non-existent-session')], reason: 'cleared' });
strictEqual(runInTerminalTool.sessionTerminalAssociations.size, 0, 'No associations should exist after handling non-existent session');
});

test('should not reuse a disposed cached terminal', () => {
const sessionResource = LocalChatSessionUri.forSession('disposed-terminal-session');
const disposedTerminal = {
isDisposed: true,
dispose: () => { },
processId: 99999,
} as unknown as ITerminalInstance;
runInTerminalTool.sessionTerminalAssociations.set(sessionResource, {
instance: disposedTerminal,
shellIntegrationQuality: ShellIntegrationQuality.None,
isBackground: false,
});

// A disposed cached terminal should not be returned by the association lookup
const cachedTerminal = runInTerminalTool.sessionTerminalAssociations.get(sessionResource);
ok(cachedTerminal, 'Cached terminal should exist in the map');
strictEqual(cachedTerminal!.instance.isDisposed, true, 'Cached terminal should be disposed');

// 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');
Comment on lines +1922 to +1925
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
});
});

test('should dedupe rapid repeated background input-needed notifications', () => {
Expand Down
Loading