Skip to content

Commit bb6e0f4

Browse files
Fix false-positive "environment manager not registered" warning on startup (#1533)
### Summary Fixes a startup race that produces a spurious warning popup: > Default environment manager `pypa.hatch:hatch` could not be applied: Environment manager `pypa.hatch:hatch` is not registered Even though the manager is actually registered and working correctly seconds later. The same race affects any third-party environment-manager extension (e.g. `PyPA.hatch`, `pixi-code`). Fixes #1517. The same root cause likely produces an equivalent symptom in other contributing extensions (e.g. the `pixi-code` extension, where a similar warning has been reported); this fix is generic and applies to any third-party manager that registers via the public API. --- ### The bug, in plain language If a user sets `python-envs.defaultEnvManager` to a manager contributed by a separate extension (Hatch, Pixi, etc.), they sometimes see a warning popup at startup claiming the manager is not registered — even though the manager *is* registered and works fine. The popup is a false positive. It does not reproduce on every machine. Some users hit it every launch, some never; on at least one user's machine, uninstalling system-level `conda` + `poetry` made the popup stop appearing (see issue thread). --- ### Root cause: a race between two concurrent `activate()` timelines Once `vscode-python-environments` returns its API (`extension.ts` line 712), VS Code starts activating any dependent extensions that declare `"extensionDependencies": ["ms-python.vscode-python-envs"]` (Hatch and Pixi do exactly this). At that moment two `async` functions are running concurrently on the same Node.js event loop in the extension host: **Timeline A** — our `setImmediate(async () => { … })` block in `extension.ts` (line 568+): ```ts await createNativePythonFinder(...); // spawns pet binary await Promise.all([ safeRegister('system', ...), safeRegister('conda', ...), safeRegister('pyenv', ...), safeRegister('pipenv', ...), safeRegister('poetry', ...), safeRegister('shellStartupVars', ...), ]); await applyInitialEnvironmentSelection(...); // <-- consults the registry ``` **Timeline B** — `PyPA.hatch.activate()` (or any other contributing extension): ```ts const api = await getPythonApi(); // ...possibly runs `hatch --version` or other async setup... api.registerEnvironmentManager(hatchManager); // <-- writes to the registry ``` Both timelines yield at every `await`. The race target is the registry Map (`src/features/envManagers.ts` line 125, `this._environmentManagers.set(managerId, mgr)` — synchronous, the source of truth). `applyInitialEnvironmentSelection` → `resolvePriorityChainCore` (`src/features/interpreterSelection.ts` lines 99–114) calls `envManagers.getEnvironmentManager('pypa.hatch:hatch')`. If Timeline B hasn't yet reached `registerEnvironmentManager` when Timeline A executes that lookup, an error is recorded with kind `managerNotRegistered`. Several more `await`s then run; by the time `notifyUserOfSettingErrors` is invoked, the manager often *has* registered — but the warning is shown anyway because the previously-recorded error is never re-evaluated. #### Why timing varies by machine The race outcome depends on: - How quickly `pet` (the bundled Rust binary) streams JSON-RPC discovery messages back to the extension host. Each message is an I/O callback on the main event loop. More installed Python tooling (conda, poetry) → more environments to discover → more event-loop traffic → Hatch's `await child_process` for `hatch --version` resumes later. - How long the third-party extension's own startup `await`s take. - General OS-level CPU and disk I/O contention. Removing conda and poetry from the user's system doesn't "fix" anything in our code path — it just shifts the timing enough that Timeline B usually wins. --- ### The fix Re-check the live manager registry at notification time, and drop "manager not registered" errors that became stale during the `await`s between when the error was recorded and when the warning is about to be shown. --- ### Why not "fix the race itself"? This is a real race condition, not just a stale-data bug, so the most architecturally pure fix would be to remove the race entirely — for example, by deferring `applyInitialEnvironmentSelection` until either (a) an expected set of third-party manager registrations has been observed, or (b) some deadline has elapsed. That kind of fix was considered and rejected for this change because: 1. **The set of "expected" managers is unknown to us.** Third-party env-manager extensions opt in by calling `registerEnvironmentManager` at their own pace. We have no manifest of who is going to register, so any deadline is arbitrary. 2. **Adding deadlines couples activation time to extension-host scheduling.** A safe deadline on a slow machine could delay the user's interpreter selection visibly; an aggressive deadline still has the same race, just on a smaller timescale. 3. **Existing real not-registered errors must still be surfaced quickly.** If a user genuinely typos a manager id, they want the warning fast — not after a multi-second wait. 4. **The blast radius would be much larger.** Reordering activation/selection touches the whole initialization sequence, including pet startup, terminal management, project resolution, and telemetry timing — all of which are currently sequenced around the existing `setImmediate` boundary. The chosen fix instead changes the *gate* (the notification), not the *race* (the order of activation). It is essentially: "before yelling at the user, confirm the problem is still real." Future work could pursue the architectural fix as a follow-up, but it should be evaluated independently of #1517. --- ### Tradeoffs and known limits - If a third-party manager genuinely takes longer to activate than `applyInitialEnvironmentSelection` (e.g. the manager extension's own `activate()` is unusually slow), the popup can still fire. After this fix the user can dismiss it and the manager will continue to work normally; no permanent state is corrupted. - The fix only filters errors whose `kind === 'managerNotRegistered'`. Real not-registered errors (typo'd ids, uninstalled extension, etc.), unresolved-variable errors, and path-resolution errors still notify as before. - This applies to both the awaited workspace-scope notification path and the deferred background global-scope notification path; both call sites pass `envManagers` to the updated helper. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 4aaf742 commit bb6e0f4

2 files changed

Lines changed: 146 additions & 16 deletions

File tree

‎src/features/interpreterSelection.ts‎

Lines changed: 55 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,10 @@ export interface PriorityChainResult {
4444
export interface SettingResolutionError {
4545
/** The setting that failed */
4646
setting: 'pythonProjects' | 'defaultEnvManager' | 'defaultInterpreterPath';
47+
/** Structured failure kind, used for localization and stale-error re-checks. */
48+
kind: 'managerNotRegistered' | 'pathUnresolvedVariables' | 'pathCannotResolve';
4749
/** The configured value */
4850
configuredValue: string;
49-
/** Reason for failure */
50-
reason: string;
5151
}
5252

5353
/**
@@ -81,8 +81,8 @@ async function resolvePriorityChainCore(
8181
}
8282
const error: SettingResolutionError = {
8383
setting: 'pythonProjects',
84+
kind: 'managerNotRegistered',
8485
configuredValue: projectManagerId,
85-
reason: `Environment manager '${projectManagerId}' is not registered`,
8686
};
8787
errors.push(error);
8888
traceWarn(`${logPrefix} pythonProjects[] manager '${projectManagerId}' not found, trying next priority`);
@@ -99,8 +99,8 @@ async function resolvePriorityChainCore(
9999
}
100100
const error: SettingResolutionError = {
101101
setting: 'defaultEnvManager',
102+
kind: 'managerNotRegistered',
102103
configuredValue: userConfiguredManager,
103-
reason: `Environment manager '${userConfiguredManager}' is not registered`,
104104
};
105105
errors.push(error);
106106
traceWarn(`${logPrefix} defaultEnvManager '${userConfiguredManager}' not found, trying next priority`);
@@ -118,8 +118,8 @@ async function resolvePriorityChainCore(
118118
);
119119
const error: SettingResolutionError = {
120120
setting: 'defaultInterpreterPath',
121+
kind: 'pathUnresolvedVariables',
121122
configuredValue: userInterpreterPath,
122-
reason: l10n.t('Path contains unresolved variables'),
123123
};
124124
errors.push(error);
125125
} else {
@@ -138,8 +138,8 @@ async function resolvePriorityChainCore(
138138
);
139139
const error: SettingResolutionError = {
140140
setting: 'defaultInterpreterPath',
141+
kind: 'pathUnresolvedVariables',
141142
configuredValue: userInterpreterPath,
142-
reason: l10n.t('Path contains unresolved variables'),
143143
};
144144
errors.push(error);
145145
} else {
@@ -155,8 +155,8 @@ async function resolvePriorityChainCore(
155155
}
156156
const error: SettingResolutionError = {
157157
setting: 'defaultInterpreterPath',
158+
kind: 'pathCannotResolve',
158159
configuredValue: userInterpreterPath,
159-
reason: `Could not resolve interpreter path '${userInterpreterPath}'`,
160160
};
161161
errors.push(error);
162162
traceWarn(
@@ -383,8 +383,9 @@ export async function applyInitialEnvironmentSelection(
383383
}
384384
resolveGlobalScope()
385385
.then(async (globalErrors) => {
386-
if (globalErrors.length > 0) {
387-
await notifyUserOfSettingErrors(globalErrors);
386+
const liveGlobalErrors = filterLiveSettingErrors(globalErrors, envManagers);
387+
if (liveGlobalErrors.length > 0) {
388+
await notifyUserOfSettingErrors(liveGlobalErrors);
388389
}
389390
})
390391
.catch((err) => traceError(`[interpreterSelection] Background global scope resolution failed: ${err}`));
@@ -397,9 +398,11 @@ export async function applyInitialEnvironmentSelection(
397398
allErrors.push(...globalErrors);
398399
}
399400

400-
// Notify user if any settings could not be applied (workspace + global when awaited)
401-
if (allErrors.length > 0) {
402-
await notifyUserOfSettingErrors(allErrors);
401+
// Drop errors that became stale during the awaits above; see filterLiveSettingErrors.
402+
const liveErrors = filterLiveSettingErrors(allErrors, envManagers);
403+
404+
if (liveErrors.length > 0) {
405+
await notifyUserOfSettingErrors(liveErrors);
403406
}
404407

405408
// Numeric values must go via the measures argument (properties are dropped).
@@ -409,7 +412,7 @@ export async function applyInitialEnvironmentSelection(
409412
duration: selectionStopWatch.elapsedTime,
410413
workspaceFolderCount: folders.length,
411414
resolvedFolderCount,
412-
settingErrorCount: allErrors.length,
415+
settingErrorCount: liveErrors.length,
413416
},
414417
{
415418
globalScopeDeferred: workspaceFolderResolved,
@@ -430,7 +433,42 @@ export function resetSettingWarnings(): void {
430433
warnedSettings.clear();
431434
}
432435

436+
function localizedReasonFor(error: SettingResolutionError): string {
437+
switch (error.kind) {
438+
case 'managerNotRegistered':
439+
return l10n.t("Environment manager '{0}' is not registered", error.configuredValue);
440+
case 'pathUnresolvedVariables':
441+
return l10n.t('Path contains unresolved variables');
442+
case 'pathCannotResolve':
443+
return l10n.t("Could not resolve interpreter path '{0}'", error.configuredValue);
444+
}
445+
}
446+
447+
/**
448+
* Re-check the live registry to drop `managerNotRegistered` errors that became
449+
* stale while later awaits ran — a third-party manager extension may have
450+
* registered in the meantime, in which case the warning would be a false positive.
451+
*/
452+
function filterLiveSettingErrors(
453+
errors: SettingResolutionError[],
454+
envManagers: EnvironmentManagers,
455+
): SettingResolutionError[] {
456+
return errors.filter((e) => {
457+
if (e.kind === 'managerNotRegistered' && envManagers.getEnvironmentManager(e.configuredValue)) {
458+
traceVerbose(
459+
`[interpreterSelection] Manager '${e.configuredValue}' is now registered; suppressing stale warning`,
460+
);
461+
return false;
462+
}
463+
return true;
464+
});
465+
}
466+
433467
async function notifyUserOfSettingErrors(errors: SettingResolutionError[]): Promise<void> {
468+
if (errors.length === 0) {
469+
return;
470+
}
471+
434472
// Group errors by setting type to avoid spamming the user
435473
const uniqueSettings = [...new Set(errors.map((e) => e.setting))];
436474

@@ -442,6 +480,7 @@ async function notifyUserOfSettingErrors(errors: SettingResolutionError[]): Prom
442480

443481
const settingErrors = errors.filter((e) => e.setting === setting);
444482
const firstError = settingErrors[0];
483+
const reason = localizedReasonFor(firstError);
445484

446485
let message: string;
447486
let settingKey: string;
@@ -451,23 +490,23 @@ async function notifyUserOfSettingErrors(errors: SettingResolutionError[]): Prom
451490
message = l10n.t(
452491
"Python project setting for environment manager '{0}' could not be applied: {1}",
453492
firstError.configuredValue,
454-
firstError.reason,
493+
reason,
455494
);
456495
settingKey = 'python-envs.pythonProjects';
457496
break;
458497
case 'defaultEnvManager':
459498
message = l10n.t(
460499
"Default environment manager '{0}' could not be applied: {1}",
461500
firstError.configuredValue,
462-
firstError.reason,
501+
reason,
463502
);
464503
settingKey = 'python-envs.defaultEnvManager';
465504
break;
466505
case 'defaultInterpreterPath':
467506
message = l10n.t(
468507
"Default interpreter path '{0}' could not be resolved: {1}",
469508
firstError.configuredValue,
470-
firstError.reason,
509+
reason,
471510
);
472511
settingKey = 'python.defaultInterpreterPath';
473512
break;

‎src/test/features/interpreterSelection.unit.test.ts‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -968,6 +968,97 @@ suite('Interpreter Selection - applyInitialEnvironmentSelection', () => {
968968
const warningMessage = showWarnStub.firstCall.args[0] as string;
969969
assert.ok(warningMessage.includes('/nonexistent/python'), 'Warning message should include the configured path');
970970
});
971+
972+
test('should suppress stale warning when manager registers during await (issue #1517)', async () => {
973+
// Simulate the race: defaultEnvManager is set to 'pypa.hatch:hatch', which is not
974+
// registered at priority-chain evaluation time, but becomes registered before
975+
// notifyUserOfSettingErrors runs. The warning must NOT be shown — neither from the
976+
// awaited workspace path nor from the deferred global `.then(...)` path.
977+
const hatchManagerId = 'pypa.hatch:hatch';
978+
979+
sandbox.stub(workspaceApis, 'getWorkspaceFolders').returns([{ uri: testUri, name: 'test', index: 0 }]);
980+
sandbox.stub(workspaceApis, 'getConfiguration').returns(createMockConfig([]) as WorkspaceConfiguration);
981+
sandbox.stub(helpers, 'getUserConfiguredSetting').callsFake((section: string, key: string) => {
982+
if (section === 'python-envs' && key === 'defaultEnvManager') {
983+
return hatchManagerId;
984+
}
985+
return undefined;
986+
});
987+
988+
// Hatch is not registered when the priority chain runs (returns undefined),
989+
// but by the time notifyUserOfSettingErrors is called, it is available.
990+
const mockHatchManager = {
991+
id: hatchManagerId,
992+
name: 'hatch',
993+
displayName: 'Hatch',
994+
get: sandbox.stub().resolves(undefined),
995+
set: sandbox.stub().resolves(),
996+
} as unknown as sinon.SinonStubbedInstance<InternalEnvironmentManager>;
997+
998+
let hatchRegistered = false;
999+
// Override getEnvironmentManager: returns undefined until hatchRegistered is set, then returns the manager
1000+
mockEnvManagers.getEnvironmentManager.callsFake((scope: unknown) => {
1001+
const id = typeof scope === 'string' ? scope : undefined;
1002+
if (id === hatchManagerId && hatchRegistered) {
1003+
return mockHatchManager;
1004+
}
1005+
if (id === 'ms-python.python:venv') {
1006+
return mockVenvManager;
1007+
}
1008+
if (id === 'ms-python.python:system') {
1009+
return mockSystemManager;
1010+
}
1011+
return undefined;
1012+
});
1013+
1014+
// Simulate Hatch registering during the venv discovery await
1015+
mockVenvManager.get.callsFake(async () => {
1016+
hatchRegistered = true; // Hatch "registers" while venv discovery is awaited
1017+
return mockVenvEnv;
1018+
});
1019+
1020+
// The workspace folder will resolve (venv found), so global scope is deferred to
1021+
// a background `.then(...)`. Use a deferred promise to deterministically wait for
1022+
// that background work to complete before asserting on showWarningMessage —
1023+
// otherwise a regression in the deferred-path filtering would fire AFTER the test
1024+
// returned and silently slip through.
1025+
let resolveGlobalDone!: () => void;
1026+
const globalDone = new Promise<void>((resolve) => {
1027+
resolveGlobalDone = resolve;
1028+
});
1029+
mockEnvManagers.setEnvironments.callsFake(async () => {
1030+
resolveGlobalDone();
1031+
});
1032+
1033+
const showWarnStub = sandbox.stub(windowApis, 'showWarningMessage').resolves(undefined);
1034+
1035+
await applyInitialEnvironmentSelection(
1036+
mockEnvManagers as unknown as EnvironmentManagers,
1037+
mockProjectManager as unknown as PythonProjectManager,
1038+
mockNativeFinder as unknown as NativePythonFinder,
1039+
mockApi as unknown as PythonEnvironmentApi,
1040+
);
1041+
1042+
// Wait for the background global scope to call setEnvironments, then flush
1043+
// microtasks so the `.then(...)` handler that would call notifyUserOfSettingErrors
1044+
// has a chance to run before we assert.
1045+
await globalDone;
1046+
await new Promise<void>((resolve) => process.nextTick(resolve));
1047+
1048+
// Workspace folder should have resolved (venv found) and global scope was attempted
1049+
assert.ok(mockEnvManagers.setEnvironment.called, 'setEnvironment should be called for workspace folder');
1050+
assert.ok(
1051+
mockEnvManagers.setEnvironments.called,
1052+
'setEnvironments should be called for deferred global scope',
1053+
);
1054+
1055+
// Warning must NOT be shown because the error was stale on both paths
1056+
assert.strictEqual(
1057+
showWarnStub.called,
1058+
false,
1059+
'showWarningMessage must NOT be called when manager registered before notification',
1060+
);
1061+
});
9711062
});
9721063

9731064
suite('Interpreter Selection - resolveGlobalEnvironmentByPriority', () => {

0 commit comments

Comments
 (0)