fix: only show install prompt after manager timeout, not during startup

edvilme · Copilot · edvilme · commit 6857c5999ffc · 2026-04-29T13:53:31.000-07:00
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>
diff --git 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<void>,
@@ -40,6 +41,7 @@ export function withManagerTimeout(
     if (deferred.completed) {
         return deferred.promise;
     }
+
     return new Promise<void>((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<void> {
+    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<string, Deferred<void>> = new Map();
     private readonly pkgManagers: Map<string, Deferred<void>> = new Map();
@@ -101,55 +153,30 @@ 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;
         }
         this.checked.add(managerId);
         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<ManagerReady>();
+}
+
 export async function waitForEnvManager(uris?: Uri[]): Promise<void> {
     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
@@ -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<DidChangeEnvironmentManagerEventArgs>;
+    let pkgManagerEmitter: EventEmitter<DidChangePackageManagerEventArgs>;
+    let clock: sinon.SinonFakeTimers;
+    let disposables: Disposable[];
+
+    setup(() => {
+        clock = sinon.useFakeTimers();
+        disposables = [];
+
+        _resetManagerReadyForTesting();
+
+        envManagerEmitter = new EventEmitter<DidChangeEnvironmentManagerEventArgs>();
+        pkgManagerEmitter = new EventEmitter<DidChangePackageManagerEventArgs>();
+
+        // 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<typeof extensionApis.getExtension>);
+        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');
+    });
+});
