Skip to content

Commit 6da90bb

Browse files
edvilmeCopilot
andcommitted
fix: only show install prompt after manager timeout, not during startup
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>
1 parent c5a1430 commit 6da90bb

2 files changed

Lines changed: 260 additions & 57 deletions

File tree

src/features/common/managerReady.ts

Lines changed: 90 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { Disposable, l10n, Uri } from 'vscode';
2-
import { allExtensions, getExtension } from '../../common/extension.apis';
2+
import { getExtension } from '../../common/extension.apis';
33
import { WorkbenchStrings } from '../../common/localize';
44
import { traceError, traceInfo, traceWarn } from '../../common/logging';
55
import { EventNames } from '../../common/telemetry/constants';
@@ -31,6 +31,10 @@ function getExtensionId(managerId: string): string | undefined {
3131
* Wraps a deferred with a timeout so a missing/dead manager cannot block the API forever.
3232
* On timeout the deferred is resolved (not rejected) so callers proceed with degraded results
3333
* instead of hanging.
34+
*
35+
* Also attempts to activate the manager's extension early (via setImmediate) so it has the
36+
* best chance of registering before the timeout. If the timeout does fire, prompts the user
37+
* to install the extension only if it's genuinely missing.
3438
*/
3539
export function withManagerTimeout(
3640
deferred: Deferred<void>,
@@ -40,6 +44,10 @@ export function withManagerTimeout(
4044
if (deferred.completed) {
4145
return deferred.promise;
4246
}
47+
48+
// Attempt to activate the extension early so the manager registers sooner
49+
tryActivateExtension(managerId);
50+
4351
return new Promise<void>((resolve) => {
4452
const timer = setTimeout(() => {
4553
if (!deferred.completed) {
@@ -53,6 +61,7 @@ export function withManagerTimeout(
5361
managerKind: kind,
5462
});
5563
deferred.resolve();
64+
promptInstallExtensionIfMissing(managerId);
5665
}
5766
}, MANAGER_READY_TIMEOUT_MS);
5867

@@ -69,10 +78,81 @@ export function withManagerTimeout(
6978
});
7079
}
7180

81+
/**
82+
* Attempts to activate the extension for a manager if it's installed but not yet active.
83+
* This is a best-effort nudge — if it fails, the timeout will handle it.
84+
*/
85+
function tryActivateExtension(managerId: string): void {
86+
const extId = getExtensionId(managerId);
87+
if (!extId) {
88+
return;
89+
}
90+
setImmediate(async () => {
91+
const ext = getExtension(extId);
92+
if (ext && !ext.isActive) {
93+
traceInfo(`Extension for manager ${managerId} is not active: Activating...`);
94+
try {
95+
await ext.activate();
96+
traceInfo(`Extension for manager ${managerId} is now active.`);
97+
} catch (err) {
98+
traceError(`Failed to activate extension ${extId}, required for: ${managerId}`, err);
99+
}
100+
}
101+
});
102+
}
103+
104+
/**
105+
* Shows an install prompt only if the extension for the given manager ID
106+
* is genuinely not available. Called after the timeout expires, giving the
107+
* extension host ample time to initialize.
108+
*/
109+
async function promptInstallExtensionIfMissing(managerId: string): Promise<void> {
110+
const extId = getExtensionId(managerId);
111+
if (!extId) {
112+
showErrorMessage(l10n.t(`Extension for {0} is not installed or enabled for this workspace.`, managerId));
113+
return;
114+
}
115+
116+
const ext = getExtension(extId);
117+
if (ext) {
118+
// Extension is installed but the manager never registered — don't prompt to install.
119+
// This can happen if the extension activated but failed to register its manager.
120+
traceWarn(
121+
`Extension ${extId} is installed but manager "${managerId}" never registered. ` +
122+
`The extension may have failed during activation or manager registration.`,
123+
);
124+
return;
125+
}
126+
127+
traceError(`Extension for manager ${managerId} is not installed. Looked up extId="${extId}" via getExtension().`);
128+
const result = await showErrorMessage(
129+
l10n.t(`Do you want to install extension {0} to enable {1} support.`, extId, managerId),
130+
WorkbenchStrings.installExtension,
131+
);
132+
if (result === WorkbenchStrings.installExtension) {
133+
traceInfo(`Installing extension: ${extId}`);
134+
try {
135+
await installExtension(extId);
136+
traceInfo(`Extension ${extId} installed.`);
137+
} catch (err) {
138+
traceError(`Failed to install extension: ${extId}`, err);
139+
}
140+
141+
try {
142+
const installedExt = getExtension(extId);
143+
if (installedExt && !installedExt.isActive) {
144+
traceInfo(`Extension for manager ${managerId} is not active: Activating...`);
145+
await installedExt.activate();
146+
}
147+
} catch (err) {
148+
traceError(`Failed to activate extension ${extId}, required for: ${managerId}`, err);
149+
}
150+
}
151+
}
152+
72153
class ManagerReadyImpl implements ManagerReady {
73154
private readonly envManagers: Map<string, Deferred<void>> = new Map();
74155
private readonly pkgManagers: Map<string, Deferred<void>> = new Map();
75-
private readonly checked: Set<string> = new Set();
76156
private readonly disposables: Disposable[] = [];
77157

78158
constructor(
@@ -101,58 +181,6 @@ class ManagerReadyImpl implements ManagerReady {
101181
);
102182
}
103183

104-
private checkExtension(managerId: string) {
105-
const installed = allExtensions().some((ext) => managerId.startsWith(`${ext.id}:`));
106-
if (this.checked.has(managerId)) {
107-
return;
108-
}
109-
this.checked.add(managerId);
110-
const extId = getExtensionId(managerId);
111-
if (extId) {
112-
setImmediate(async () => {
113-
if (installed) {
114-
const ext = getExtension(extId);
115-
if (ext && !ext.isActive) {
116-
traceInfo(`Extension for manager ${managerId} is not active: Activating...`);
117-
try {
118-
await ext.activate();
119-
traceInfo(`Extension for manager ${managerId} is now active.`);
120-
} catch (err) {
121-
traceError(`Failed to activate extension ${extId}, required for: ${managerId}`, err);
122-
}
123-
}
124-
} else {
125-
traceError(`Extension for manager ${managerId} is not installed.`);
126-
const result = await showErrorMessage(
127-
l10n.t(`Do you want to install extension {0} to enable {1} support.`, extId, managerId),
128-
WorkbenchStrings.installExtension,
129-
);
130-
if (result === WorkbenchStrings.installExtension) {
131-
traceInfo(`Installing extension: ${extId}`);
132-
try {
133-
await installExtension(extId);
134-
traceInfo(`Extension ${extId} installed.`);
135-
} catch (err) {
136-
traceError(`Failed to install extension: ${extId}`, err);
137-
}
138-
139-
try {
140-
const ext = getExtension(extId);
141-
if (ext && !ext.isActive) {
142-
traceInfo(`Extension for manager ${managerId} is not active: Activating...`);
143-
await ext.activate();
144-
}
145-
} catch (err) {
146-
traceError(`Failed to activate extension ${extId}, required for: ${managerId}`, err);
147-
}
148-
}
149-
}
150-
});
151-
} else {
152-
showErrorMessage(l10n.t(`Extension for {0} is not installed or enabled for this workspace.`, managerId));
153-
}
154-
}
155-
156184
public dispose(): void {
157185
this.disposables.forEach((d) => d.dispose());
158186
this.envManagers.clear();
@@ -188,7 +216,6 @@ class ManagerReadyImpl implements ManagerReady {
188216
}
189217

190218
public async waitForEnvManagerId(managerIds: string[]): Promise<void> {
191-
managerIds.forEach((managerId) => this.checkExtension(managerId));
192219
await Promise.all(managerIds.map((managerId) => this._waitForEnvManager(managerId)));
193220
}
194221

@@ -237,7 +264,6 @@ class ManagerReadyImpl implements ManagerReady {
237264
await this.waitForPkgManagerId(Array.from(ids));
238265
}
239266
public async waitForPkgManagerId(managerIds: string[]): Promise<void> {
240-
managerIds.forEach((managerId) => this.checkExtension(managerId));
241267
await Promise.all(managerIds.map((managerId) => this._waitForPkgManager(managerId)));
242268
}
243269
}
@@ -251,6 +277,14 @@ export function createManagerReady(em: EnvironmentManagers, pm: PythonProjectMan
251277
}
252278
}
253279

280+
/**
281+
* Reset the module-level singleton so `createManagerReady` can be called again.
282+
* Only for use in tests.
283+
*/
284+
export function _resetManagerReadyForTesting(): void {
285+
_deferred = createDeferred<ManagerReady>();
286+
}
287+
254288
export async function waitForEnvManager(uris?: Uri[]): Promise<void> {
255289
const mr = await _deferred.promise;
256290
return mr.waitForEnvManager(uris);

0 commit comments

Comments
 (0)