From 5f80e813d37a5230c63fb1da95179fd53278be73 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Fri, 27 Feb 2026 12:16:57 -0800 Subject: [PATCH 1/3] feat: implement timeout handling for environment and package manager registration --- src/common/telemetry/constants.ts | 12 ++++++ src/features/common/managerReady.ts | 61 +++++++++++++++++++++++++---- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/src/common/telemetry/constants.ts b/src/common/telemetry/constants.ts index bde80901..6c48d7ee 100644 --- a/src/common/telemetry/constants.ts +++ b/src/common/telemetry/constants.ts @@ -41,6 +41,7 @@ export enum EventNames { * - errorType: string (error class name, on failure only) */ ENVIRONMENT_DISCOVERY = 'ENVIRONMENT_DISCOVERY', + MANAGER_READY_TIMEOUT = 'MANAGER_READY.TIMEOUT', } // Map all events to their properties @@ -210,4 +211,15 @@ export interface IEventNamePropertyMapping { envCount?: number; errorType?: string; }; + + /* __GDPR__ + "manager_ready.timeout": { + "managerId": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "eleanorjboyd" }, + "managerKind": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "eleanorjboyd" } + } + */ + [EventNames.MANAGER_READY_TIMEOUT]: { + managerId: string; + managerKind: 'environment' | 'package'; + }; } diff --git a/src/features/common/managerReady.ts b/src/features/common/managerReady.ts index a7341b80..c85b3134 100644 --- a/src/features/common/managerReady.ts +++ b/src/features/common/managerReady.ts @@ -1,12 +1,16 @@ import { Disposable, l10n, Uri } from 'vscode'; -import { EnvironmentManagers, PythonProjectManager } from '../../internal.api'; -import { createDeferred, Deferred } from '../../common/utils/deferred'; import { allExtensions, getExtension } from '../../common/extension.apis'; -import { traceError, traceInfo } from '../../common/logging'; -import { showErrorMessage } from '../../common/window.apis'; -import { getDefaultEnvManagerSetting, getDefaultPkgManagerSetting } from '../settings/settingHelpers'; import { WorkbenchStrings } from '../../common/localize'; +import { traceError, traceInfo, traceWarn } from '../../common/logging'; +import { EventNames } from '../../common/telemetry/constants'; +import { sendTelemetryEvent } from '../../common/telemetry/sender'; +import { createDeferred, Deferred } from '../../common/utils/deferred'; +import { showErrorMessage } from '../../common/window.apis'; import { installExtension } from '../../common/workbenchCommands'; +import { EnvironmentManagers, PythonProjectManager } from '../../internal.api'; +import { getDefaultEnvManagerSetting, getDefaultPkgManagerSetting } from '../settings/settingHelpers'; + +const MANAGER_READY_TIMEOUT_MS = 30_000; interface ManagerReady extends Disposable { waitForEnvManager(uris?: Uri[]): Promise; @@ -29,7 +33,10 @@ class ManagerReadyImpl implements ManagerReady { private readonly checked: Set = new Set(); private readonly disposables: Disposable[] = []; - constructor(em: EnvironmentManagers, private readonly pm: PythonProjectManager) { + constructor( + em: EnvironmentManagers, + private readonly pm: PythonProjectManager, + ) { this.disposables.push( em.onDidChangeEnvironmentManager((e) => { if (this.envManagers.has(e.manager.id)) { @@ -104,6 +111,44 @@ class ManagerReadyImpl implements ManagerReady { } } + /** + * Wraps a deferred with a timeout so a missing/dead manager cannot block the API forever. + * On timeout the deferred is resolved (not rejected) so callers proceed with degraded results + * instead of hanging. + */ + private _withTimeout(deferred: Deferred, managerId: string, kind: string): Promise { + if (deferred.completed) { + return deferred.promise; + } + return new Promise((resolve) => { + const timer = setTimeout(() => { + if (!deferred.completed) { + traceWarn( + `Timed out after ${MANAGER_READY_TIMEOUT_MS / 1000}s waiting for ${kind} manager "${managerId}" to register. ` + + `The manager may not be installed or its extension failed to activate. Proceeding without it. ` + + `To prevent this, check your "python-envs.defaultEnvManager" and "python-envs.pythonProjects" settings.`, + ); + sendTelemetryEvent(EventNames.MANAGER_READY_TIMEOUT, undefined, { + managerId, + managerKind: kind as 'environment' | 'package', + }); + deferred.resolve(); + } + }, MANAGER_READY_TIMEOUT_MS); + + deferred.promise.then( + () => { + clearTimeout(timer); + resolve(); + }, + () => { + clearTimeout(timer); + resolve(); + }, + ); + }); + } + public dispose(): void { this.disposables.forEach((d) => d.dispose()); this.envManagers.clear(); @@ -116,7 +161,7 @@ class ManagerReadyImpl implements ManagerReady { } const deferred = createDeferred(); this.envManagers.set(managerId, deferred); - return deferred.promise; + return this._withTimeout(deferred, managerId, 'environment'); } public async waitForEnvManager(uris?: Uri[]): Promise { @@ -165,7 +210,7 @@ class ManagerReadyImpl implements ManagerReady { } const deferred = createDeferred(); this.pkgManagers.set(managerId, deferred); - return deferred.promise; + return this._withTimeout(deferred, managerId, 'package'); } public async waitForPkgManager(uris?: Uri[]): Promise { From a7d5d40139283085df3c5eb5cdd5d3f0bddb484b Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Fri, 27 Feb 2026 15:17:53 -0800 Subject: [PATCH 2/3] add withManagerTimeout unit tests for manager readiness --- .vscode/settings.json | 3 +- src/features/common/managerReady.ts | 86 +++++++------- .../features/common/managerReady.unit.test.ts | 106 ++++++++++++++++++ 3 files changed, 153 insertions(+), 42 deletions(-) create mode 100644 src/test/features/common/managerReady.unit.test.ts diff --git a/.vscode/settings.json b/.vscode/settings.json index 031b2d19..a777efbd 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -31,6 +31,7 @@ "git.branchProtectionPrompt": "alwaysCommitToNewBranch", "chat.tools.terminal.autoApprove": { "npx tsc": true, - "mkdir": true + "mkdir": true, + "npx mocha": true } } diff --git a/src/features/common/managerReady.ts b/src/features/common/managerReady.ts index c85b3134..a0b71c26 100644 --- a/src/features/common/managerReady.ts +++ b/src/features/common/managerReady.ts @@ -10,7 +10,7 @@ import { installExtension } from '../../common/workbenchCommands'; import { EnvironmentManagers, PythonProjectManager } from '../../internal.api'; import { getDefaultEnvManagerSetting, getDefaultPkgManagerSetting } from '../settings/settingHelpers'; -const MANAGER_READY_TIMEOUT_MS = 30_000; +export const MANAGER_READY_TIMEOUT_MS = 30_000; interface ManagerReady extends Disposable { waitForEnvManager(uris?: Uri[]): Promise; @@ -27,6 +27,48 @@ function getExtensionId(managerId: string): string | undefined { return parts ? parts[1] : undefined; } +/** + * Wraps a deferred with a timeout so a missing/dead manager cannot block the API forever. + * On timeout the deferred is resolved (not rejected) so callers proceed with degraded results + * instead of hanging. + */ +export function withManagerTimeout( + deferred: Deferred, + managerId: string, + kind: 'environment' | 'package', +): Promise { + if (deferred.completed) { + return deferred.promise; + } + return new Promise((resolve) => { + const timer = setTimeout(() => { + if (!deferred.completed) { + traceWarn( + `Timed out after ${MANAGER_READY_TIMEOUT_MS / 1000}s waiting for ${kind} manager "${managerId}" to register. ` + + `The manager may not be installed or its extension failed to activate. Proceeding without it. ` + + `To prevent this, check your "python-envs.defaultEnvManager" and "python-envs.pythonProjects" settings.`, + ); + sendTelemetryEvent(EventNames.MANAGER_READY_TIMEOUT, undefined, { + managerId, + managerKind: kind, + }); + deferred.resolve(); + } + }, MANAGER_READY_TIMEOUT_MS); + + deferred.promise.then( + () => { + clearTimeout(timer); + resolve(); + }, + () => { + clearTimeout(timer); + resolve(); + }, + ); + }); +} + class ManagerReadyImpl implements ManagerReady { private readonly envManagers: Map> = new Map(); private readonly pkgManagers: Map> = new Map(); @@ -111,44 +153,6 @@ class ManagerReadyImpl implements ManagerReady { } } - /** - * Wraps a deferred with a timeout so a missing/dead manager cannot block the API forever. - * On timeout the deferred is resolved (not rejected) so callers proceed with degraded results - * instead of hanging. - */ - private _withTimeout(deferred: Deferred, managerId: string, kind: string): Promise { - if (deferred.completed) { - return deferred.promise; - } - return new Promise((resolve) => { - const timer = setTimeout(() => { - if (!deferred.completed) { - traceWarn( - `Timed out after ${MANAGER_READY_TIMEOUT_MS / 1000}s waiting for ${kind} manager "${managerId}" to register. ` + - `The manager may not be installed or its extension failed to activate. Proceeding without it. ` + - `To prevent this, check your "python-envs.defaultEnvManager" and "python-envs.pythonProjects" settings.`, - ); - sendTelemetryEvent(EventNames.MANAGER_READY_TIMEOUT, undefined, { - managerId, - managerKind: kind as 'environment' | 'package', - }); - deferred.resolve(); - } - }, MANAGER_READY_TIMEOUT_MS); - - deferred.promise.then( - () => { - clearTimeout(timer); - resolve(); - }, - () => { - clearTimeout(timer); - resolve(); - }, - ); - }); - } - public dispose(): void { this.disposables.forEach((d) => d.dispose()); this.envManagers.clear(); @@ -161,7 +165,7 @@ class ManagerReadyImpl implements ManagerReady { } const deferred = createDeferred(); this.envManagers.set(managerId, deferred); - return this._withTimeout(deferred, managerId, 'environment'); + return withManagerTimeout(deferred, managerId, 'environment'); } public async waitForEnvManager(uris?: Uri[]): Promise { @@ -210,7 +214,7 @@ class ManagerReadyImpl implements ManagerReady { } const deferred = createDeferred(); this.pkgManagers.set(managerId, deferred); - return this._withTimeout(deferred, managerId, 'package'); + return withManagerTimeout(deferred, managerId, 'package'); } public async waitForPkgManager(uris?: Uri[]): Promise { diff --git a/src/test/features/common/managerReady.unit.test.ts b/src/test/features/common/managerReady.unit.test.ts new file mode 100644 index 00000000..7cef4b54 --- /dev/null +++ b/src/test/features/common/managerReady.unit.test.ts @@ -0,0 +1,106 @@ +import assert from 'assert'; +import * as sinon from 'sinon'; +import * as logging from '../../../common/logging'; +import { EventNames } from '../../../common/telemetry/constants'; +import * as telemetrySender from '../../../common/telemetry/sender'; +import { createDeferred } from '../../../common/utils/deferred'; +import { MANAGER_READY_TIMEOUT_MS, withManagerTimeout } from '../../../features/common/managerReady'; + +suite('withManagerTimeout', () => { + let clock: sinon.SinonFakeTimers; + let traceWarnStub: sinon.SinonStub; + let sendTelemetryStub: sinon.SinonStub; + + setup(() => { + clock = sinon.useFakeTimers(); + traceWarnStub = sinon.stub(logging, 'traceWarn'); + sendTelemetryStub = sinon.stub(telemetrySender, 'sendTelemetryEvent'); + }); + + teardown(() => { + clock.restore(); + sinon.restore(); + }); + + test('deferred never resolves → timeout fires, logs warning, sends telemetry', async () => { + const deferred = createDeferred(); + const promise = withManagerTimeout(deferred, 'test-ext:venv', 'environment'); + + // Advance past the timeout + clock.tick(MANAGER_READY_TIMEOUT_MS); + await clock.tickAsync(0); // flush microtasks + + await promise; + + // Warning was logged with manager ID + assert.ok(traceWarnStub.calledOnce, 'traceWarn should be called once'); + assert.ok(traceWarnStub.firstCall.args[0].includes('test-ext:venv'), 'warning should contain the manager ID'); + + // Telemetry was sent + assert.ok(sendTelemetryStub.calledOnce, 'sendTelemetryEvent should be called once'); + const [eventName, , properties] = sendTelemetryStub.firstCall.args; + assert.strictEqual(eventName, EventNames.MANAGER_READY_TIMEOUT); + assert.strictEqual(properties.managerId, 'test-ext:venv'); + assert.strictEqual(properties.managerKind, 'environment'); + }); + + test('deferred resolves before timeout → no warning, no telemetry', async () => { + const deferred = createDeferred(); + const promise = withManagerTimeout(deferred, 'test-ext:conda', 'environment'); + + // Resolve before timeout + deferred.resolve(); + await clock.tickAsync(0); + + await promise; + + // Advance past the timeout to confirm it was cleared + clock.tick(MANAGER_READY_TIMEOUT_MS); + await clock.tickAsync(0); + + assert.ok(traceWarnStub.notCalled, 'traceWarn should not be called'); + assert.ok(sendTelemetryStub.notCalled, 'sendTelemetryEvent should not be called'); + }); + + test('timeout resolves (not rejects) the deferred', async () => { + const deferred = createDeferred(); + const promise = withManagerTimeout(deferred, 'test-ext:missing', 'environment'); + + clock.tick(MANAGER_READY_TIMEOUT_MS); + await clock.tickAsync(0); + + // This must resolve — if it rejects, the test fails + await promise; + + assert.ok(deferred.resolved, 'deferred should be resolved, not rejected'); + assert.ok(!deferred.rejected, 'deferred should not be rejected'); + }); + + test('already-completed deferred returns immediately without timeout', async () => { + const deferred = createDeferred(); + deferred.resolve(); + + const promise = withManagerTimeout(deferred, 'test-ext:venv', 'environment'); + await promise; + + // No timer was set, so nothing should fire + clock.tick(MANAGER_READY_TIMEOUT_MS); + await clock.tickAsync(0); + + assert.ok(traceWarnStub.notCalled, 'traceWarn should not be called for completed deferred'); + assert.ok(sendTelemetryStub.notCalled, 'sendTelemetryEvent should not be called for completed deferred'); + }); + + test('package manager kind is passed through to telemetry', async () => { + const deferred = createDeferred(); + const promise = withManagerTimeout(deferred, 'test-ext:pip', 'package'); + + clock.tick(MANAGER_READY_TIMEOUT_MS); + await clock.tickAsync(0); + await promise; + + const [, , properties] = sendTelemetryStub.firstCall.args; + assert.strictEqual(properties.managerId, 'test-ext:pip'); + assert.strictEqual(properties.managerKind, 'package'); + }); +}); From 1e14374b620bfe1ae881a15e6963a31843fc5c9c Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Fri, 27 Feb 2026 15:48:58 -0800 Subject: [PATCH 3/3] fix merge conflicts --- src/features/common/managerReady.ts | 50 ----------------------------- 1 file changed, 50 deletions(-) diff --git a/src/features/common/managerReady.ts b/src/features/common/managerReady.ts index b2c078d1..a0b71c26 100644 --- a/src/features/common/managerReady.ts +++ b/src/features/common/managerReady.ts @@ -10,11 +10,7 @@ import { installExtension } from '../../common/workbenchCommands'; import { EnvironmentManagers, PythonProjectManager } from '../../internal.api'; import { getDefaultEnvManagerSetting, getDefaultPkgManagerSetting } from '../settings/settingHelpers'; -<<<<<<< smart-donkey export const MANAGER_READY_TIMEOUT_MS = 30_000; -======= -const MANAGER_READY_TIMEOUT_MS = 30_000; ->>>>>>> main interface ManagerReady extends Disposable { waitForEnvManager(uris?: Uri[]): Promise; @@ -157,44 +153,6 @@ class ManagerReadyImpl implements ManagerReady { } } - /** - * Wraps a deferred with a timeout so a missing/dead manager cannot block the API forever. - * On timeout the deferred is resolved (not rejected) so callers proceed with degraded results - * instead of hanging. - */ - private _withTimeout(deferred: Deferred, managerId: string, kind: string): Promise { - if (deferred.completed) { - return deferred.promise; - } - return new Promise((resolve) => { - const timer = setTimeout(() => { - if (!deferred.completed) { - traceWarn( - `Timed out after ${MANAGER_READY_TIMEOUT_MS / 1000}s waiting for ${kind} manager "${managerId}" to register. ` + - `The manager may not be installed or its extension failed to activate. Proceeding without it. ` + - `To prevent this, check your "python-envs.defaultEnvManager" and "python-envs.pythonProjects" settings.`, - ); - sendTelemetryEvent(EventNames.MANAGER_READY_TIMEOUT, undefined, { - managerId, - managerKind: kind as 'environment' | 'package', - }); - deferred.resolve(); - } - }, MANAGER_READY_TIMEOUT_MS); - - deferred.promise.then( - () => { - clearTimeout(timer); - resolve(); - }, - () => { - clearTimeout(timer); - resolve(); - }, - ); - }); - } - public dispose(): void { this.disposables.forEach((d) => d.dispose()); this.envManagers.clear(); @@ -207,11 +165,7 @@ class ManagerReadyImpl implements ManagerReady { } const deferred = createDeferred(); this.envManagers.set(managerId, deferred); -<<<<<<< smart-donkey return withManagerTimeout(deferred, managerId, 'environment'); -======= - return this._withTimeout(deferred, managerId, 'environment'); ->>>>>>> main } public async waitForEnvManager(uris?: Uri[]): Promise { @@ -260,11 +214,7 @@ class ManagerReadyImpl implements ManagerReady { } const deferred = createDeferred(); this.pkgManagers.set(managerId, deferred); -<<<<<<< smart-donkey return withManagerTimeout(deferred, managerId, 'package'); -======= - return this._withTimeout(deferred, managerId, 'package'); ->>>>>>> main } public async waitForPkgManager(uris?: Uri[]): Promise {