From 6857c5999ffcc20214979efe2912d0f57129d9f5 Mon Sep 17 00:00:00 2001 From: Eduardo Villalpando Mello Date: Wed, 29 Apr 2026 12:53:23 -0700 Subject: [PATCH 1/2] fix: only show install prompt after manager timeout, not during startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, checkExtension would show the 'install extension' prompt inside a setImmediate callback — just one tick after startup. If the extension host hadn't fully initialized yet, getExtension() could return undefined even for installed extensions, causing a false prompt. Now the install prompt is only shown after the full 30-second timeout expires AND getExtension() still returns undefined. This gives the extension host ample time to initialize. Additionally, if the extension IS installed but the manager never registered (activation failure), we log a warning instead of prompting to install. Changes: - Move install prompt logic from checkExtension into promptInstallExtensionIfMissing, called only after timeout - checkExtension now only attempts activation (no install prompts) - Add guard: if extension is installed but manager never registered, log a warning instead of prompting to install - Add 5 regression tests for the race condition scenarios - Add _resetManagerReadyForTesting() helper for test isolation Fixes #1465 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/features/common/managerReady.ts | 117 +++++++----- .../features/common/managerReady.unit.test.ts | 171 +++++++++++++++++- 2 files changed, 246 insertions(+), 42 deletions(-) diff --git a/src/features/common/managerReady.ts b/src/features/common/managerReady.ts index a0b71c26..fef4b2d4 100644 --- a/src/features/common/managerReady.ts +++ b/src/features/common/managerReady.ts @@ -1,5 +1,5 @@ import { Disposable, l10n, Uri } from 'vscode'; -import { allExtensions, getExtension } from '../../common/extension.apis'; +import { getExtension } from '../../common/extension.apis'; import { WorkbenchStrings } from '../../common/localize'; import { traceError, traceInfo, traceWarn } from '../../common/logging'; import { EventNames } from '../../common/telemetry/constants'; @@ -30,7 +30,8 @@ function getExtensionId(managerId: string): string | 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. + * instead of hanging. If the manager's extension is genuinely not installed when the timeout + * fires, the user is prompted to install it. */ export function withManagerTimeout( deferred: Deferred, @@ -40,6 +41,7 @@ export function withManagerTimeout( if (deferred.completed) { return deferred.promise; } + return new Promise((resolve) => { const timer = setTimeout(() => { if (!deferred.completed) { @@ -53,6 +55,7 @@ export function withManagerTimeout( managerKind: kind, }); deferred.resolve(); + promptInstallExtensionIfMissing(managerId); } }, MANAGER_READY_TIMEOUT_MS); @@ -69,6 +72,55 @@ export function withManagerTimeout( }); } +/** + * Shows an install prompt only if the extension for the given manager ID + * is genuinely not available. Called after the timeout expires, giving the + * extension host ample time to initialize. + */ +async function promptInstallExtensionIfMissing(managerId: string): Promise { + const extId = getExtensionId(managerId); + if (!extId) { + showErrorMessage(l10n.t(`Extension for {0} is not installed or enabled for this workspace.`, managerId)); + return; + } + + const ext = getExtension(extId); + if (ext) { + // Extension is installed but the manager never registered — don't prompt to install. + // This can happen if the extension activated but failed to register its manager. + traceWarn( + `Extension ${extId} is installed but manager "${managerId}" never registered. ` + + `The extension may have failed during activation or manager registration.`, + ); + return; + } + + traceError(`Extension for manager ${managerId} is not installed. Looked up extId="${extId}" via getExtension().`); + const result = await showErrorMessage( + l10n.t(`Do you want to install extension {0} to enable {1} support.`, extId, managerId), + WorkbenchStrings.installExtension, + ); + if (result === WorkbenchStrings.installExtension) { + traceInfo(`Installing extension: ${extId}`); + try { + await installExtension(extId); + traceInfo(`Extension ${extId} installed.`); + } catch (err) { + traceError(`Failed to install extension: ${extId}`, err); + } + + try { + const installedExt = getExtension(extId); + if (installedExt && !installedExt.isActive) { + traceInfo(`Extension for manager ${managerId} is not active: Activating...`); + await installedExt.activate(); + } + } catch (err) { + traceError(`Failed to activate extension ${extId}, required for: ${managerId}`, err); + } + } +} + class ManagerReadyImpl implements ManagerReady { private readonly envManagers: Map> = new Map(); private readonly pkgManagers: Map> = new Map(); @@ -101,8 +153,12 @@ class ManagerReadyImpl implements ManagerReady { ); } - private checkExtension(managerId: string) { - const installed = allExtensions().some((ext) => managerId.startsWith(`${ext.id}:`)); + /** + * Best-effort activation nudge for the extension that provides the given manager. + * Uses setImmediate so the extension host has a chance to finish initializing before + * we query it. Deduplicated per manager ID to avoid redundant activation calls. + */ + private checkExtension(managerId: string): void { if (this.checked.has(managerId)) { return; } @@ -110,46 +166,17 @@ class ManagerReadyImpl implements ManagerReady { const extId = getExtensionId(managerId); if (extId) { setImmediate(async () => { - if (installed) { - const ext = getExtension(extId); - if (ext && !ext.isActive) { - traceInfo(`Extension for manager ${managerId} is not active: Activating...`); - try { - await ext.activate(); - traceInfo(`Extension for manager ${managerId} is now active.`); - } catch (err) { - traceError(`Failed to activate extension ${extId}, required for: ${managerId}`, err); - } - } - } else { - traceError(`Extension for manager ${managerId} is not installed.`); - const result = await showErrorMessage( - l10n.t(`Do you want to install extension {0} to enable {1} support.`, extId, managerId), - WorkbenchStrings.installExtension, - ); - if (result === WorkbenchStrings.installExtension) { - traceInfo(`Installing extension: ${extId}`); - try { - await installExtension(extId); - traceInfo(`Extension ${extId} installed.`); - } catch (err) { - traceError(`Failed to install extension: ${extId}`, err); - } - - try { - const ext = getExtension(extId); - if (ext && !ext.isActive) { - traceInfo(`Extension for manager ${managerId} is not active: Activating...`); - await ext.activate(); - } - } catch (err) { - traceError(`Failed to activate extension ${extId}, required for: ${managerId}`, err); - } + const ext = getExtension(extId); + if (ext && !ext.isActive) { + traceInfo(`Extension for manager ${managerId} is not active: Activating...`); + try { + await ext.activate(); + traceInfo(`Extension for manager ${managerId} is now active.`); + } catch (err) { + traceError(`Failed to activate extension ${extId}, required for: ${managerId}`, err); } } }); - } else { - showErrorMessage(l10n.t(`Extension for {0} is not installed or enabled for this workspace.`, managerId)); } } @@ -251,6 +278,14 @@ export function createManagerReady(em: EnvironmentManagers, pm: PythonProjectMan } } +/** + * Reset the module-level singleton so `createManagerReady` can be called again. + * Only for use in tests. + */ +export function _resetManagerReadyForTesting(): void { + _deferred = createDeferred(); +} + export async function waitForEnvManager(uris?: Uri[]): Promise { const mr = await _deferred.promise; return mr.waitForEnvManager(uris); diff --git a/src/test/features/common/managerReady.unit.test.ts b/src/test/features/common/managerReady.unit.test.ts index 7cef4b54..c861c7de 100644 --- a/src/test/features/common/managerReady.unit.test.ts +++ b/src/test/features/common/managerReady.unit.test.ts @@ -1,10 +1,29 @@ import assert from 'assert'; import * as sinon from 'sinon'; +import { Disposable, EventEmitter } from 'vscode'; +import * as extensionApis from '../../../common/extension.apis'; 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'; +import * as windowApis from '../../../common/window.apis'; +import { + MANAGER_READY_TIMEOUT_MS, + _resetManagerReadyForTesting, + createManagerReady, + waitForEnvManagerId, + waitForPkgManagerId, + withManagerTimeout, +} from '../../../features/common/managerReady'; +import * as settingHelpers from '../../../features/settings/settingHelpers'; +import { + DidChangeEnvironmentManagerEventArgs, + DidChangePackageManagerEventArgs, + EnvironmentManagers, + InternalEnvironmentManager, + InternalPackageManager, + PythonProjectManager, +} from '../../../internal.api'; suite('withManagerTimeout', () => { let clock: sinon.SinonFakeTimers; @@ -14,7 +33,11 @@ suite('withManagerTimeout', () => { setup(() => { clock = sinon.useFakeTimers(); traceWarnStub = sinon.stub(logging, 'traceWarn'); + sinon.stub(logging, 'traceError'); sendTelemetryStub = sinon.stub(telemetrySender, 'sendTelemetryEvent'); + // Stub dependencies used by promptInstallExtensionIfMissing (called on timeout) + sinon.stub(extensionApis, 'getExtension').returns(undefined); + sinon.stub(windowApis, 'showErrorMessage').returns(Promise.resolve(undefined)); }); teardown(() => { @@ -104,3 +127,149 @@ suite('withManagerTimeout', () => { assert.strictEqual(properties.managerKind, 'package'); }); }); + +suite('ManagerReady - race condition handling', () => { + let envManagerEmitter: EventEmitter; + let pkgManagerEmitter: EventEmitter; + let clock: sinon.SinonFakeTimers; + let disposables: Disposable[]; + + setup(() => { + clock = sinon.useFakeTimers(); + disposables = []; + + _resetManagerReadyForTesting(); + + envManagerEmitter = new EventEmitter(); + pkgManagerEmitter = new EventEmitter(); + + // Stub logging and telemetry to keep test output clean + sinon.stub(logging, 'traceWarn'); + sinon.stub(logging, 'traceError'); + sinon.stub(logging, 'traceInfo'); + sinon.stub(telemetrySender, 'sendTelemetryEvent'); + sinon.stub(windowApis, 'showErrorMessage').returns(Promise.resolve(undefined)); + sinon.stub(extensionApis, 'getExtension').returns({ + id: 'ms-python.python', + isActive: true, + } as unknown as ReturnType); + sinon.stub(settingHelpers, 'getDefaultEnvManagerSetting').returns('ms-python.python:venv'); + sinon.stub(settingHelpers, 'getDefaultPkgManagerSetting').returns('ms-python.python:pip'); + + const mockEm = { + onDidChangeEnvironmentManager: envManagerEmitter.event, + onDidChangePackageManager: pkgManagerEmitter.event, + } as unknown as EnvironmentManagers; + + const mockPm = { + getProjects: () => [], + } as unknown as PythonProjectManager; + + createManagerReady(mockEm, mockPm, disposables); + }); + + teardown(() => { + clock.restore(); + disposables.forEach((d) => d.dispose()); + envManagerEmitter.dispose(); + pkgManagerEmitter.dispose(); + sinon.restore(); + _resetManagerReadyForTesting(); + }); + + test('no install prompt when manager registers before timeout', async () => { + const waitPromise = waitForEnvManagerId(['ms-python.python:venv']); + // Flush microtasks so the internal await _deferred.promise completes + // and the timeout/deferred is set up + await clock.tickAsync(0); + + // Manager registers before timeout + envManagerEmitter.fire({ + kind: 'registered', + manager: { id: 'ms-python.python:venv' } as unknown as InternalEnvironmentManager, + }); + + await clock.tickAsync(0); + await waitPromise; + + // Advance past timeout to ensure no late prompt + clock.tick(MANAGER_READY_TIMEOUT_MS); + await clock.tickAsync(0); + + const showErrorStub = windowApis.showErrorMessage as sinon.SinonStub; + assert.ok(!showErrorStub.called, 'should not prompt to install when manager registered successfully'); + }); + + test('no install prompt on timeout when extension is installed but manager never registered', async () => { + // Extension IS installed (getExtension returns it), but manager never fires registration event + const waitPromise = waitForEnvManagerId(['ms-python.python:venv']); + // Flush microtasks so internal await completes and timeout is armed + await clock.tickAsync(0); + + // Advance past timeout — manager never registers + clock.tick(MANAGER_READY_TIMEOUT_MS); + await clock.tickAsync(0); + await waitPromise; + + // Should NOT prompt to install because getExtension finds the extension + const showErrorStub = windowApis.showErrorMessage as sinon.SinonStub; + assert.ok(!showErrorStub.called, 'should not prompt to install when extension is installed'); + + // Should log a warning instead + const traceWarnStub = logging.traceWarn as sinon.SinonStub; + const warnAboutManager = traceWarnStub.getCalls().find( + (c: sinon.SinonSpyCall) => typeof c.args[0] === 'string' && c.args[0].includes('never registered'), + ); + assert.ok(warnAboutManager, 'should warn that manager never registered despite extension being installed'); + }); + + test('install prompt shown on timeout only when extension is genuinely not installed', async () => { + const getExtensionStub = extensionApis.getExtension as sinon.SinonStub; + getExtensionStub.returns(undefined); // Extension not installed + + const waitPromise = waitForEnvManagerId(['ms-python.python:venv']); + // Flush microtasks so internal await completes and timeout is armed + await clock.tickAsync(0); + + // Advance past timeout — manager never registers + clock.tick(MANAGER_READY_TIMEOUT_MS); + await clock.tickAsync(0); + await waitPromise; + + // NOW the install prompt should appear (after 30s, not immediately) + const showErrorStub = windowApis.showErrorMessage as sinon.SinonStub; + assert.ok(showErrorStub.called, 'should prompt to install after timeout when extension is missing'); + }); + + test('manager registered before wait resolves immediately without prompt', async () => { + envManagerEmitter.fire({ + kind: 'registered', + manager: { id: 'ms-python.python:venv' } as unknown as InternalEnvironmentManager, + }); + + await clock.tickAsync(0); + + // Wait should resolve immediately since the manager already registered + const waitPromise = waitForEnvManagerId(['ms-python.python:venv']); + await clock.tickAsync(0); + await waitPromise; + + const showErrorStub = windowApis.showErrorMessage as sinon.SinonStub; + assert.ok(!showErrorStub.called, 'should not prompt when manager already registered'); + }); + + test('pkg manager wait resolves when registration event fires', async () => { + const waitPromise = waitForPkgManagerId(['ms-python.python:pip']); + + pkgManagerEmitter.fire({ + kind: 'registered', + manager: { id: 'ms-python.python:pip' } as unknown as InternalPackageManager, + }); + + await clock.tickAsync(0); + await waitPromise; + + const showErrorStub = windowApis.showErrorMessage as sinon.SinonStub; + assert.ok(!showErrorStub.called, 'should not prompt when pkg manager registered'); + }); +}); From 2151164003b5d9535616ab946e73cd6943ca9d6e Mon Sep 17 00:00:00 2001 From: Eduardo Villalpando Mello Date: Thu, 30 Apr 2026 11:15:33 -0700 Subject: [PATCH 2/2] fix: address PR review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Wrap promptInstallExtensionIfMissing with void/.catch for unhandled rejections - Attempt activation post-timeout when extension is installed but inactive - Fix question punctuation (period → question mark) - Deduplicate install prompts per extension ID per session via prompted Set Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/features/common/managerReady.ts | 38 +++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/src/features/common/managerReady.ts b/src/features/common/managerReady.ts index fef4b2d4..335e5176 100644 --- a/src/features/common/managerReady.ts +++ b/src/features/common/managerReady.ts @@ -55,7 +55,9 @@ export function withManagerTimeout( managerKind: kind, }); deferred.resolve(); - promptInstallExtensionIfMissing(managerId); + void promptInstallExtensionIfMissing(managerId).catch((err) => { + traceError(`Failed to prompt installation for manager extension "${managerId}".`, err); + }); } }, MANAGER_READY_TIMEOUT_MS); @@ -75,8 +77,10 @@ export function withManagerTimeout( /** * Shows an install prompt only if the extension for the given manager ID * is genuinely not available. Called after the timeout expires, giving the - * extension host ample time to initialize. + * extension host ample time to initialize. Deduplicated per extension ID + * so each missing extension only prompts once per session. */ +const prompted: Set = new Set(); async function promptInstallExtensionIfMissing(managerId: string): Promise { const extId = getExtensionId(managerId); if (!extId) { @@ -87,17 +91,34 @@ async function promptInstallExtensionIfMissing(managerId: string): Promise const ext = getExtension(extId); if (ext) { // Extension is installed but the manager never registered — don't prompt to install. - // This can happen if the extension activated but failed to register its manager. - traceWarn( - `Extension ${extId} is installed but manager "${managerId}" never registered. ` + - `The extension may have failed during activation or manager registration.`, - ); + // Attempt activation as a recovery step since the extension host is reliable at this point. + if (!ext.isActive) { + traceWarn( + `Extension ${extId} is installed but manager "${managerId}" never registered. Attempting activation...`, + ); + try { + await ext.activate(); + traceInfo(`Extension ${extId} activated post-timeout for manager "${managerId}".`); + } catch (err) { + traceError(`Failed to activate extension ${extId} post-timeout for: ${managerId}`, err); + } + } else { + traceWarn( + `Extension ${extId} is installed and active but manager "${managerId}" never registered. ` + + `The extension may have failed during manager registration.`, + ); + } + return; + } + + if (prompted.has(extId)) { return; } + prompted.add(extId); traceError(`Extension for manager ${managerId} is not installed. Looked up extId="${extId}" via getExtension().`); const result = await showErrorMessage( - l10n.t(`Do you want to install extension {0} to enable {1} support.`, extId, managerId), + l10n.t(`Do you want to install extension {0} to enable {1} support?`, extId, managerId), WorkbenchStrings.installExtension, ); if (result === WorkbenchStrings.installExtension) { @@ -284,6 +305,7 @@ export function createManagerReady(em: EnvironmentManagers, pm: PythonProjectMan */ export function _resetManagerReadyForTesting(): void { _deferred = createDeferred(); + prompted.clear(); } export async function waitForEnvManager(uris?: Uri[]): Promise {