fix: defer install prompt until after manager timeout to prevent false positives (#1496)

edvilme · Copilot · web-flow · commit 9fbeb75921b4 · 2026-05-01T09:55:16.000-07:00
## Fix: Only show install prompt after manager timeout, not during startup Fixes #1465 ### Problem Users consistently see a false "Do you want to install extension ms-python.python to enable ms-python.python:venv support" prompt despite having the extension installed. This happens across Windows/Linux, persists despite reinstalls, and is a race condition during startup. ### Root Cause The race condition: - **A)** The extension host is loading all extensions (takes variable time) - **B)** Our code calls `checkExtension()` which queries `getExtension()` after just 1 tick (`setImmediate`) B can execute BEFORE A finishes — `getExtension()` returns `undefined` even though the extension IS installed, triggering a false install prompt. ### The Fix Separate the two concerns: 1. **`checkExtension()`** — fires early, but only does activation (harmless if it sees stale state — worst case it's a no-op). **Never prompts to install.** 2. **`promptInstallExtensionIfMissing()`** — fires ONLY after the 30s timeout in `withManagerTimeout()`, when `getExtension()` is guaranteed to be reliable. The install prompt can no longer race against extension host initialization because it's gated behind the same 30s timeout that already exists for "manager didn't register." ### Flow (annotated) ``` // BEFORE (broken): waitForEnvManagerId(["ms-python.python:venv"]) ├─ checkExtension() │ └─ setImmediate (1ms) → getExtension() → undefined (host still loading!) │ └─ ❌ showErrorMessage("Do you want to install...?") // FALSE PROMPT └─ setTimeout(30s) → ... // AFTER (fixed): waitForEnvManagerId(["ms-python.python:venv"]) ├─ checkExtension() │ └─ setImmediate (1ms) → getExtension() │ └─ ✅ Only activates if found inactive. Never prompts. └─ withManagerTimeout(30s) ├─ Manager registers at ~200ms → deferred resolves → timer cleared → done ✅ └─ 30s expire → promptInstallExtensionIfMissing() ├─ getExtension() → found? → log warning, NO prompt ✅ └─ getExtension() → not found? → show install prompt ✅ (genuinely missing) ``` ### Changes - **`withManagerTimeout`**: Now calls `promptInstallExtensionIfMissing()` on timeout expiry - **`promptInstallExtensionIfMissing`** (new): Checks `getExtension()` after 30s; only prompts if extension truly missing; logs warning if installed but manager never registered - **`checkExtension`**: Simplified to only attempt activation — no install prompts - **Added 5 regression tests** covering: timeout with/without extension installed, manager registering before timeout, pre-registered managers, package manager wait --------- 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,9 @@ export function withManagerTimeout(
                     managerKind: kind,
                 });
                 deferred.resolve();
+                void promptInstallExtensionIfMissing(managerId).catch((err) => {
+                    traceError(`Failed to prompt installation for manager extension "${managerId}".`, err);
+                });
             }
         }, MANAGER_READY_TIMEOUT_MS);
 
@@ -69,6 +74,74 @@ 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. Deduplicated per extension ID
+ * so each missing extension only prompts once per session.
+ */
+const prompted: Set<string> = new Set();
+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.
+        // 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),
+        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 +174,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 +299,15 @@ 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>();
+    prompted.clear();
+}
+
 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');
+    });
+});
