From 3c4cf92b5bc87acef3e3acae0cc2c953691d15c4 Mon Sep 17 00:00:00 2001 From: Simon Siefke Date: Tue, 1 Sep 2026 21:45:36 +0000 Subject: [PATCH 1/2] fix: cancel terminal telemetry timeout on disposal --- .../telemetry/browser/terminalTelemetry.ts | 4 +- .../test/browser/terminalTelemetry.test.ts | 78 +++++++++++++++++++ 2 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 src/vs/workbench/contrib/terminalContrib/telemetry/test/browser/terminalTelemetry.test.ts diff --git a/src/vs/workbench/contrib/terminalContrib/telemetry/browser/terminalTelemetry.ts b/src/vs/workbench/contrib/terminalContrib/telemetry/browser/terminalTelemetry.ts index f6570068e6336..6a2caceb51819 100644 --- a/src/vs/workbench/contrib/terminalContrib/telemetry/browser/terminalTelemetry.ts +++ b/src/vs/workbench/contrib/terminalContrib/telemetry/browser/terminalTelemetry.ts @@ -6,6 +6,7 @@ import { getWindowById } from '../../../../../base/browser/dom.js'; import { isAuxiliaryWindow } from '../../../../../base/browser/window.js'; import { timeout } from '../../../../../base/common/async.js'; +import { cancelOnDispose } from '../../../../../base/common/cancellation.js'; import { Event } from '../../../../../base/common/event.js'; import { Disposable, DisposableStore } from '../../../../../base/common/lifecycle.js'; import { basename } from '../../../../../base/common/path.js'; @@ -32,12 +33,13 @@ export class TerminalTelemetryContribution extends Disposable implements IWorkbe this._register(terminalService.onDidCreateInstance(async instance => { const store = new DisposableStore(); this._store.add(store); + const cancellationToken = cancelOnDispose(store); await Promise.race([ // Wait for process ready so the shell launch config is fully resolved, then // allow another 10 seconds for the shell integration to be fully initialized instance.processReady.then(() => { - return timeout(10000); + return timeout(10000, cancellationToken); }), // If the terminal is disposed, it's ready to report on immediately Event.toPromise(instance.onDisposed, store), diff --git a/src/vs/workbench/contrib/terminalContrib/telemetry/test/browser/terminalTelemetry.test.ts b/src/vs/workbench/contrib/terminalContrib/telemetry/test/browser/terminalTelemetry.test.ts new file mode 100644 index 0000000000000..9be20823f8d81 --- /dev/null +++ b/src/vs/workbench/contrib/terminalContrib/telemetry/test/browser/terminalTelemetry.test.ts @@ -0,0 +1,78 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { Emitter } from '../../../../../../base/common/event.js'; +import { URI } from '../../../../../../base/common/uri.js'; +import { upcastPartial } from '../../../../../../base/test/common/mock.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js'; +import { captureGlobalTimeApi, type TimeoutId } from '../../../../../../base/test/common/virtualScheduling/timeApi.js'; +import { pushGlobalTimeApi } from '../../../../../../base/test/common/virtualScheduling/globalTimeApi.js'; +import { TerminalCapabilityStore } from '../../../../../../platform/terminal/common/capabilities/terminalCapabilityStore.js'; +import { TerminalLocation } from '../../../../../../platform/terminal/common/terminal.js'; +import { ITelemetryService } from '../../../../../../platform/telemetry/common/telemetry.js'; +import { ILifecycleService, type WillShutdownEvent } from '../../../../../services/lifecycle/common/lifecycle.js'; +import { ITerminalEditorService, type ITerminalInstance, ITerminalService } from '../../../../terminal/browser/terminal.js'; +import { TerminalTelemetryContribution } from '../../browser/terminalTelemetry.js'; + +suite('TerminalTelemetryContribution', () => { + const store = ensureNoDisposablesAreLeakedInTestSuite(); + + test('cancels the shell integration timeout when the terminal is disposed', async () => { + const onDidCreateInstance = store.add(new Emitter()); + const onAnyInstanceShellTypeChanged = store.add(new Emitter()); + const onDisposed = store.add(new Emitter()); + const onWillShutdown = store.add(new Emitter()); + const timeoutHandle = 1 as unknown as TimeoutId; + let scheduledTimeout: number | undefined; + let clearedTimeout: TimeoutId | undefined; + let telemetryEvents = 0; + + const timeApi = captureGlobalTimeApi(); + store.add(pushGlobalTimeApi({ + ...timeApi, + setTimeout: (_handler, timeout) => { + scheduledTimeout = timeout; + return timeoutHandle; + }, + clearTimeout: handle => clearedTimeout = handle, + })); + + const terminalService = upcastPartial({ + onDidCreateInstance: onDidCreateInstance.event, + onAnyInstanceShellTypeChanged: onAnyInstanceShellTypeChanged.event, + }); + const lifecycleService = upcastPartial({ onWillShutdown: onWillShutdown.event }); + const terminalEditorService = upcastPartial({ + getInputFromResource: () => { throw new Error('Not an editor terminal'); }, + }); + const telemetryService = upcastPartial({ + publicLog2: () => { telemetryEvents++; }, + }); + store.add(new TerminalTelemetryContribution(lifecycleService, terminalService, terminalEditorService, telemetryService)); + + const instance = upcastPartial({ + resource: URI.parse('terminal:test'), + target: TerminalLocation.Panel, + processReady: Promise.resolve(), + onDisposed: onDisposed.event, + shellLaunchConfig: {}, + capabilities: store.add(new TerminalCapabilityStore()), + hasRemoteAuthority: false, + usedShellIntegrationInjection: false, + shellIntegrationInjectionFailureReason: undefined, + sessionId: 'test', + }); + + onDidCreateInstance.fire(instance); + onDisposed.fire(instance); + await Promise.resolve(); + await Promise.resolve(); + + assert.strictEqual(scheduledTimeout, 10_000); + assert.strictEqual(clearedTimeout, timeoutHandle); + assert.strictEqual(telemetryEvents, 1); + }); +}); From 85eedf8ce7451436f17407642345f59f6f92751f Mon Sep 17 00:00:00 2001 From: Simon Siefke Date: Wed, 2 Sep 2026 10:19:35 +0000 Subject: [PATCH 2/2] fix: handle terminal telemetry cancellation --- .../telemetry/browser/terminalTelemetry.ts | 30 ++++++++++++------- .../test/browser/terminalTelemetry.test.ts | 26 ++++++++++++---- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/src/vs/workbench/contrib/terminalContrib/telemetry/browser/terminalTelemetry.ts b/src/vs/workbench/contrib/terminalContrib/telemetry/browser/terminalTelemetry.ts index 6a2caceb51819..5b7f1f87da33e 100644 --- a/src/vs/workbench/contrib/terminalContrib/telemetry/browser/terminalTelemetry.ts +++ b/src/vs/workbench/contrib/terminalContrib/telemetry/browser/terminalTelemetry.ts @@ -7,6 +7,7 @@ import { getWindowById } from '../../../../../base/browser/dom.js'; import { isAuxiliaryWindow } from '../../../../../base/browser/window.js'; import { timeout } from '../../../../../base/common/async.js'; import { cancelOnDispose } from '../../../../../base/common/cancellation.js'; +import { isCancellationError } from '../../../../../base/common/errors.js'; import { Event } from '../../../../../base/common/event.js'; import { Disposable, DisposableStore } from '../../../../../base/common/lifecycle.js'; import { basename } from '../../../../../base/common/path.js'; @@ -35,17 +36,24 @@ export class TerminalTelemetryContribution extends Disposable implements IWorkbe this._store.add(store); const cancellationToken = cancelOnDispose(store); - await Promise.race([ - // Wait for process ready so the shell launch config is fully resolved, then - // allow another 10 seconds for the shell integration to be fully initialized - instance.processReady.then(() => { - return timeout(10000, cancellationToken); - }), - // If the terminal is disposed, it's ready to report on immediately - Event.toPromise(instance.onDisposed, store), - // If the app is shutting down, flush - Event.toPromise(lifecycleService.onWillShutdown, store), - ]); + try { + await Promise.race([ + // Wait for process ready so the shell launch config is fully resolved, then + // allow another 10 seconds for the shell integration to be fully initialized + instance.processReady.then(() => { + return timeout(10000, cancellationToken); + }), + // If the terminal is disposed, it's ready to report on immediately + Event.toPromise(instance.onDisposed, store), + // If the app is shutting down, flush + Event.toPromise(lifecycleService.onWillShutdown, store), + ]); + } catch (error) { + if (cancellationToken.isCancellationRequested && isCancellationError(error)) { + return; + } + throw error; + } // Determine window status, this is done some time after the process is ready and could // reflect the terminal being moved. diff --git a/src/vs/workbench/contrib/terminalContrib/telemetry/test/browser/terminalTelemetry.test.ts b/src/vs/workbench/contrib/terminalContrib/telemetry/test/browser/terminalTelemetry.test.ts index 9be20823f8d81..219538551a02d 100644 --- a/src/vs/workbench/contrib/terminalContrib/telemetry/test/browser/terminalTelemetry.test.ts +++ b/src/vs/workbench/contrib/terminalContrib/telemetry/test/browser/terminalTelemetry.test.ts @@ -20,7 +20,7 @@ import { TerminalTelemetryContribution } from '../../browser/terminalTelemetry.j suite('TerminalTelemetryContribution', () => { const store = ensureNoDisposablesAreLeakedInTestSuite(); - test('cancels the shell integration timeout when the terminal is disposed', async () => { + test('cancels the shell integration timeout when the terminal or contribution is disposed', async () => { const onDidCreateInstance = store.add(new Emitter()); const onAnyInstanceShellTypeChanged = store.add(new Emitter()); const onDisposed = store.add(new Emitter()); @@ -51,7 +51,7 @@ suite('TerminalTelemetryContribution', () => { const telemetryService = upcastPartial({ publicLog2: () => { telemetryEvents++; }, }); - store.add(new TerminalTelemetryContribution(lifecycleService, terminalService, terminalEditorService, telemetryService)); + const contribution = store.add(new TerminalTelemetryContribution(lifecycleService, terminalService, terminalEditorService, telemetryService)); const instance = upcastPartial({ resource: URI.parse('terminal:test'), @@ -71,8 +71,24 @@ suite('TerminalTelemetryContribution', () => { await Promise.resolve(); await Promise.resolve(); - assert.strictEqual(scheduledTimeout, 10_000); - assert.strictEqual(clearedTimeout, timeoutHandle); - assert.strictEqual(telemetryEvents, 1); + assert.deepStrictEqual({ scheduledTimeout, clearedTimeout, telemetryEvents }, { + scheduledTimeout: 10_000, + clearedTimeout: timeoutHandle, + telemetryEvents: 1, + }); + + scheduledTimeout = undefined; + clearedTimeout = undefined; + onDidCreateInstance.fire(instance); + await Promise.resolve(); + contribution.dispose(); + await Promise.resolve(); + await Promise.resolve(); + + assert.deepStrictEqual({ scheduledTimeout, clearedTimeout, telemetryEvents }, { + scheduledTimeout: 10_000, + clearedTimeout: timeoutHandle, + telemetryEvents: 1, + }); }); });