fix: defer extension lookup in checkExtension to avoid stale state race condition

edvilme · Copilot · edvilme · commit 49e05cb8bae2 · 2026-04-29T13:28:03.000-07:00
The checkExtension method previously captured the extension install state synchronously (via allExtensions().some()) before the setImmediate callback ran. By the time the callback executed, the extension state could have changed, leading to false 'install extension' prompts. Changes: - Replace allExtensions().some() with getExtension() inside the setImmediate callback, so the lookup uses fresh state - Remove unused allExtensions import - Add _resetManagerReadyForTesting() helper for test isolation - Add 4 regression tests for the race condition behavior 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';
@@ -102,17 +102,16 @@ class ManagerReadyImpl implements ManagerReady {
     }
 
     private checkExtension(managerId: string) {
-        const installed = allExtensions().some((ext) => managerId.startsWith(`${ext.id}:`));
         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) {
+                const ext = getExtension(extId);
+                if (ext) {
+                    if (!ext.isActive) {
                         traceInfo(`Extension for manager ${managerId} is not active: Activating...`);
                         try {
                             await ext.activate();
@@ -122,7 +121,10 @@ class ManagerReadyImpl implements ManagerReady {
                         }
                     }
                 } else {
-                    traceError(`Extension for manager ${managerId} is not installed.`);
+                    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,
@@ -251,6 +253,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;
@@ -104,3 +123,133 @@ 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');
+        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('checkExtension defers getExtension lookup to avoid stale state at startup', async () => {
+        const getExtensionStub = extensionApis.getExtension as sinon.SinonStub;
+        // Initially the extension is not available (simulating early startup)
+        getExtensionStub.withArgs('ms-python.python').returns(undefined);
+
+        // Trigger checkExtension via waitForEnvManagerId
+        const waitPromise = waitForEnvManagerId(['ms-python.python:venv']);
+
+        // Extension becomes available before setImmediate fires
+        getExtensionStub.withArgs('ms-python.python').returns({
+            id: 'ms-python.python',
+            isActive: true,
+        });
+
+        // Resolve the manager deferred
+        envManagerEmitter.fire({
+            kind: 'registered',
+            manager: { id: 'ms-python.python:venv' } as unknown as InternalEnvironmentManager,
+        });
+
+        // Flush setImmediate and microtasks
+        clock.tick(0);
+        await clock.tickAsync(0);
+        await waitPromise;
+
+        // showErrorMessage should NOT have been called because getExtension is
+        // evaluated inside setImmediate (after the extension became available)
+        const showErrorStub = windowApis.showErrorMessage as sinon.SinonStub;
+        assert.ok(!showErrorStub.called, 'should not prompt to install when extension becomes available before setImmediate fires');
+    });
+
+    test('checkExtension does not show install prompt when extension is found via getExtension', async () => {
+        const getExtensionStub = extensionApis.getExtension as sinon.SinonStub;
+        getExtensionStub.withArgs('ms-python.python').returns({
+            id: 'ms-python.python',
+            isActive: true,
+        });
+
+        const waitPromise = waitForEnvManagerId(['ms-python.python:venv']);
+
+        envManagerEmitter.fire({
+            kind: 'registered',
+            manager: { id: 'ms-python.python:venv' } as unknown as InternalEnvironmentManager,
+        });
+
+        clock.tick(0);
+        await clock.tickAsync(0);
+        await waitPromise;
+
+        assert.ok(getExtensionStub.called, 'getExtension should be used for extension lookup');
+        const showErrorStub = windowApis.showErrorMessage as sinon.SinonStub;
+        assert.ok(!showErrorStub.called, 'should not prompt to install when extension is found');
+    });
+
+    test('manager registered before wait resolves immediately', 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;
+    });
+
+    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;
+    });
+});
