diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/monitoring/outputMonitor.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/monitoring/outputMonitor.ts index 5b417b85289..4c8a910714c 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/monitoring/outputMonitor.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/monitoring/outputMonitor.ts @@ -102,6 +102,45 @@ export class OutputMonitor extends Disposable implements IOutputMonitor { private _command = ''; private _invocationContext: IToolInvocationContext | undefined; private _currentMonitoringCts: CancellationTokenSource | undefined; + /** + * Tracks whether onDidFinishCommand has fired so the event is delivered at + * most once. The event must fire synchronously during dispose so consumers + * awaiting `Event.toPromise(onDidFinishCommand)` are unblocked before the + * underlying emitter is torn down by super.dispose(). + */ + private _didFinish = false; + + private _fireFinishedOnce(): void { + if (this._didFinish) { + return; + } + this._didFinish = true; + this._onDidFinishCommand.fire(); + } + + override dispose(): void { + // Deliver onDidFinishCommand to consumers BEFORE super.dispose() tears + // down the emitter. Field-initialized disposables (including + // _onDidFinishCommand) are registered before any disposable added in + // the constructor body and are disposed first by DisposableStore in + // insertion order. Without this override, consumers awaiting + // `Event.toPromise(onDidFinishCommand)` would race with emitter + // teardown and hang when dispose lands while _startMonitoring is still + // in flight. + if (!this._didFinish) { + // Synthesize a Cancelled pollingResult so consumers that read + // `monitor.pollingResult` after awaiting onDidFinishCommand always + // see a defined value with the output collected so far. + this._pollingResult ??= { + state: OutputMonitorState.Cancelled, + output: this._execution.getOutput(), + pollDurationMs: 0, + resources: undefined, + }; + } + this._fireFinishedOnce(); + super.dispose(); + } constructor( private readonly _execution: IExecution, @@ -231,7 +270,10 @@ export class OutputMonitor extends Disposable implements IOutputMonitor { }; // Clean up idle input listener if still active this._userInputListener.clear(); - this._onDidFinishCommand.fire(); + // Fire at most once. If dispose() already fired the event synchronously + // (e.g. the monitor was torn down before this async loop reached its + // finally), skip firing on a potentially disposed emitter. + this._fireFinishedOnce(); } } diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/outputMonitor.test.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/outputMonitor.test.ts index be36b8feb70..f4fcd47d4d8 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/outputMonitor.test.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/outputMonitor.test.ts @@ -500,6 +500,28 @@ suite('OutputMonitor', () => { monitor.dispose(); }); }); + + test('disposing while monitoring is in-flight still resolves onDidFinishCommand', async () => { + // Regression: if dispose() races the async _startMonitoring loop, the loop's + // finally block fires onDidFinishCommand AFTER super.dispose() has already + // torn down the emitter. Consumers awaiting Event.toPromise(onDidFinishCommand) + // would never resolve and the agent would hang on the run_in_terminal call. + // + // Fix: dispose() must fire onDidFinishCommand synchronously, before the + // emitter is disposed. It must also surface a Cancelled pollingResult so + // consumers that read monitor.pollingResult after awaiting the event see a + // defined value rather than undefined. + return runWithFakedTimers({}, async () => { + monitor = store.add(instantiationService.createInstance(OutputMonitor, execution, undefined, createTestContext('1'), cts.token, 'test command')); + const finished = Event.toPromise(monitor.onDidFinishCommand); + // Dispose immediately, before the deferred _startMonitoring even starts. + monitor.dispose(); + // Must resolve — would hang prior to the synchronous-fire-on-dispose fix. + await finished; + assert.ok(monitor.pollingResult, 'pollingResult should be defined after dispose-induced finish'); + assert.strictEqual(monitor.pollingResult!.state, OutputMonitorState.Cancelled); + }); + }); }); suite('detectsGenericPressAnyKeyPattern', () => {