Skip to content

Commit 036e883

Browse files
Coalesce concurrent native finder refreshes with the same key (microsoft#1598)
## Summary Fixes microsoft#1587. At startup, several environment managers (venv, system, pyenv, pipenv, poetry, conda) each call `nativeFinder.refresh(true, undefined)` concurrently — typically triggered when the Python extension calls `api.refreshEnvironments(undefined)` via its `triggerRefresh` on activation. Every one of those calls resolves to the same cache key (`'all'`) but is queued as a distinct task on the native finder's worker pool, which runs with `concurrency = 1`. Neither the worker pool nor `doRefresh` deduplicates identical submissions, so N parallel callers become N sequential full PET discoveries — same input, same output, N-1 wasted scans. The reporter (pytorch workspace) saw the result of this stacking: ``` [Pipenv] Environment discovery took 199.2s (found 0 environments). [Poetry] Environment discovery took 253.8s (found 0 environments). ``` plus the `Setup appears hung during stage: managerRegistration` watchdog firing, and the same `Discovered env: <path>` lines repeating 4-5× in the log — the smoking gun that PET ran the same scan multiple times back-to-back. ## Fix Add an in-flight tracking map (`Map<string, Promise<NativeInfo[]>>`) in `NativePythonFinderImpl` and a guard at the top of `handleHardRefresh`: if a refresh for the same key is already running, return the existing promise instead of queueing another task. The slot is cleared in `.finally` so a rejected refresh does not poison future callers — sequential refreshes still each run a fresh PET scan. `handleSoftRefresh` already falls through to `handleHardRefresh` on cache miss, so it picks up the dedup automatically — a concurrent soft + hard for the same key now share one PET scan. ## Why this layer - All duplicate-refresh paths converge at `handleHardRefresh` (manager refreshes, conda's `getConda` fallback via `native.refresh(false)`, the Python extension's `triggerRefresh`, the `Refresh All Environment Managers` command, external API consumers). Fixing here covers every caller in one place. - No public API change, no observable semantic change for sequential refreshes, no test breakage. - Mirrors the existing `inFlight: Map<string, Promise<…>>` idiom in `src/features/inlineScriptLazyDetector.ts`. ## Expected impact for microsoft#1587 | Metric | Before | After | |---|---:|---:| | PET scans triggered at startup | ~6-7 | **1** | | Pipenv discovery warning | 106-199 s | ~25-35 s | | Poetry discovery warning | 121-253 s | ~25-35 s | | `Setup appears hung during stage: managerRegistration` | fires | should not fire | | Total startup-to-ready | ~150 s | **~40-50 s** (~3× wall-clock) | The fix eliminates duplicate-scan stacking; it does not speed up a single PET scan (intrinsic ~25-35 s on pytorch on the reporter''s machine — separate from this change). ## Verification - ✅ Webpack production build succeeds. - ✅ ESLint clean on the changed file. - ✅ Unit tests: 1215 pass (same 3 unrelated pre-existing failures on `main`). - ✅ Built a VSIX with this branch and a VSIX from `main`, diffed the minified `handleHardRefresh` body — the fix VSIX has the new structure (`inFlightRefreshes.get → guard → addToQueue.then.finally`), `main` has the old (`await pool.addToQueue`). Tested manually by inspecting the minified output of both builds; would appreciate eyes from someone with the pytorch repro to confirm wall-clock improvement on a real run. ## What this does NOT address (intentionally) - Single PET scan latency on large workspaces (separate, PET-side). - Cross-session cache misses on remote (issue microsoft#1581, separate fix in flight). - The Python extension''s `triggerRefresh` on every activation (worth a follow-up — could skip when our cache is non-empty). - The double-init in `InternalEnvironmentManager.refresh`''s tail-call to `getEnvironments(''all'')` (separate small follow-up; produces the "phantom" lowercase-only refresh log lines). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 2645a48 commit 036e883

1 file changed

Lines changed: 32 additions & 8 deletions

File tree

src/managers/common/nativePythonFinder.ts

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,11 @@ class NativePythonFinderImpl implements NativePythonFinder {
326326
private connection: rpc.MessageConnection;
327327
private readonly pool: WorkerPool<NativePythonEnvironmentKind | Uri[] | undefined, NativeInfo[]>;
328328
private cache: Map<string, NativeInfo[]> = new Map();
329+
/**
330+
* Tracks in-flight hard refreshes by cache key so concurrent callers share a
331+
* single PET scan instead of queueing duplicate work.
332+
*/
333+
private inFlightRefreshes: Map<string, Promise<NativeInfo[]>> = new Map();
329334
private startDisposables: Disposable[] = [];
330335
private proc: ChildProcess | undefined;
331336
private processExited: boolean = false;
@@ -555,20 +560,39 @@ class NativePythonFinderImpl implements NativePythonFinder {
555560

556561
private async handleHardRefresh(options?: NativePythonEnvironmentKind | Uri[]): Promise<NativeInfo[]> {
557562
const key = this.getKey(options);
563+
564+
const inFlight = this.inFlightRefreshes.get(key);
565+
if (inFlight) {
566+
this.outputChannel.debug(`[Finder] Coalescing hard refresh with in-flight request for key: ${key}`);
567+
return inFlight;
568+
}
569+
558570
this.cache.delete(key);
559571
if (!options) {
560572
this.outputChannel.debug('[Finder] Refreshing all environments');
561573
} else {
562574
this.outputChannel.debug(`[Finder] Hard refresh for key: ${key}`);
563575
}
564-
const result = await this.pool.addToQueue(options);
565-
// Validate result from worker pool
566-
if (!result || !Array.isArray(result)) {
567-
this.outputChannel.warn(`[pet] Worker pool returned invalid result type: ${typeof result}`);
568-
return [];
569-
}
570-
this.cache.set(key, result);
571-
return result;
576+
577+
// .finally clears the in-flight slot on both success AND failure paths so
578+
// a rejected refresh does not poison the cache — the next call after a
579+
// failure starts a fresh attempt, matching today's behavior.
580+
const refreshPromise = this.pool
581+
.addToQueue(options)
582+
.then((result) => {
583+
if (!result || !Array.isArray(result)) {
584+
this.outputChannel.warn(`[pet] Worker pool returned invalid result type: ${typeof result}`);
585+
return [] as NativeInfo[];
586+
}
587+
this.cache.set(key, result);
588+
return result;
589+
})
590+
.finally(() => {
591+
this.inFlightRefreshes.delete(key);
592+
});
593+
594+
this.inFlightRefreshes.set(key, refreshPromise);
595+
return refreshPromise;
572596
}
573597

574598
private async handleSoftRefresh(options?: NativePythonEnvironmentKind | Uri[]): Promise<NativeInfo[]> {

0 commit comments

Comments
 (0)