fix: retry failed manager initialization by StellaHuang95 · Pull Request #16 · StellaHuang95/vscode-python-environments

StellaHuang95 · 2026-08-22T03:01:32Z

Summary

Make all six built-in Python environment manager initialize() methods retryable after a failed initialization attempt. A successful initialization stays shared and runs once, and each manager keeps its existing throw-vs-swallow behavior.

Problem

Every manager memoizes initialization in a _initialized deferred and, in finally, resolves it and leaves it set even when discovery threw. The top-of-method guard if (this._initialized) return this._initialized.promise; then returns that settled promise forever, so a single transient failure (e.g. a tool momentarily unavailable during the first call) permanently poisons discovery for the rest of the session with no way to retry.

Affected managers: venv, system, conda, pipenv, poetry, pyenv.

Reproduction: call initialize() while discovery fails (venv/system reject; conda/pipenv/poetry/pyenv swallow and log), then fix the environment and call initialize() again. No rediscovery happens and the manager stays empty for the session.

Root cause

finally { this._initialized.resolve(); } runs on both success and failure and never clears the guard, so the guard short-circuits every later call.

Fix

Mirror the existing reset-on-failure pattern in fastPath.ts. In each initialize():

Capture the deferred locally: const initialized = createDeferred<void>(); this._initialized = initialized;.
On an actual thrown exception, clear the guard so a later call retries, but only if this run still owns it: if (this._initialized === initialized) { this._initialized = undefined; }.
Always settle the captured deferred in finally (initialized.resolve()) so concurrent waiters unblock.

State transitions:

success: undefined -> deferred -> resolved (stays set; shared, one-time).
failure: undefined -> deferred -> resolved with the guard cleared (retryable).
non-throwing tool_not_found / manager-absent: treated as completed init (no wasteful rediscovery).

Preserved behavior:

throw-style (venv, system) still rethrow; swallow-style (conda, pipenv, poetry, pyenv) still log, emit telemetry, and never throw to callers.
the ownership check (=== initialized) stops a late failing run from clearing a guard that a concurrent clearCache() + reinit installed.

Tests

New focused initialize() suites for venv, system, pyenv, pipenv, and poetry, plus an extended conda suite. They cover:

a failed run clears state so a later call retries and succeeds;
a successful init is not re-run;
concurrent callers share one run and all settle (throw-style leader rejects while waiters resolve; swallow-style all resolve);
non-throwing tool_not_found is treated as completed and is not retried;
a late failing run does not clobber a newer deferred installed by clearCache() + reinit.

Copilot

🟡 Changes recommended

The swallow-style managers’ finally blocks still resolve the deferred after sendTelemetryEvent(...), so a telemetry exception could prevent waiter settlement and unexpectedly throw from initialize().

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a long-standing initialization-guard bug across the built-in environment managers where a first failed initialize() permanently short-circuited later discovery for the rest of the session. The change ensures failures clear the _initialized guard (with an identity check to avoid clobbering newer runs) while still settling concurrent waiters.

Changes:

Update _initialized handling in all six managers to capture a local deferred, clear the guard on real discovery exceptions, and always resolve the captured deferred in finally.
Add/extend unit tests to validate retry-after-failure behavior, concurrent waiter settling, and “tool not found” outcomes not being retried.
Add a targeted pipenv test to validate the identity-guard behavior under concurrent clearCache() + initialize().

File summaries

File	Description
src/managers/builtin/venvManager.ts	Clear `_initialized` on thrown discovery errors while resolving the captured deferred to unblock concurrent waiters.
src/managers/builtin/sysPythonManager.ts	Same retryable initialization-guard behavior as venv for throw-style manager.
src/managers/conda/condaEnvManager.ts	Swallow-style manager now clears `_initialized` on thrown discovery exceptions to allow later retries.
src/managers/pipenv/pipenvManager.ts	Swallow-style manager now clears `_initialized` on thrown discovery exceptions; includes identity-guard behavior.
src/managers/poetry/poetryManager.ts	Swallow-style manager now clears `_initialized` on thrown discovery exceptions to allow later retries.
src/managers/pyenv/pyenvManager.ts	Swallow-style manager now clears `_initialized` on thrown discovery exceptions to allow later retries.
src/test/managers/builtin/venvManager.initialize.unit.test.ts	New tests validating retry + concurrent waiter behavior for throw-style venv.
src/test/managers/builtin/sysPythonManager.initialize.unit.test.ts	New tests validating retry + concurrent waiter behavior for throw-style system manager.
src/test/managers/conda/condaEnvManager.initialize.unit.test.ts	Extended tests validating retryability, concurrent waiter settling, and tool-not-found behavior for conda.
src/test/managers/pipenv/pipenvManager.initialize.unit.test.ts	New tests validating swallow-style retry + identity-guard behavior under concurrent clearCache/init.
src/test/managers/poetry/poetryManager.initialize.unit.test.ts	New tests validating swallow-style retry + concurrent waiter settling + tool-not-found behavior.
src/test/managers/pyenv/pyenvManager.initialize.unit.test.ts	New tests validating swallow-style retry + concurrent waiter settling + tool-not-found behavior.

Review details

Files reviewed: 12/12 changed files
Comments generated: 4
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Address Copilot review on PR #16: in the four swallow-style managers (conda, pipenv, poetry, pyenv) the finally block ran sendTelemetryEvent before initialized.resolve(). Since sendTelemetryEvent can throw in production, a telemetry failure would skip resolve() -- deadlocking concurrent waiters -- and propagate out of initialize(), breaking the swallow-style never-throw contract. Resolve the captured deferred first (resolve() is non-throwing/idempotent), then wrap the telemetry call in try/catch and log failures via traceError. Adds a telemetry-failure regression test to each of the four swallow-manager initialize suites. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🟢 Approval recommended

The state-transition fix is consistently applied across all affected managers and is backed by targeted unit tests covering the key failure, concurrency, and telemetry edge cases.

Review details

Files reviewed: 12/12 changed files
Comments generated: 0 new
Review effort level: Lite

StellaHuang95 · 2026-08-22T04:33:51Z

🔒 Automated review in progress — @StellaHuang95 is auto-reviewing this PR.

StellaHuang95 · 2026-08-22T05:20:13Z

        }
-        this._initialized = createDeferred();
+        const initialized = createDeferred<void>();
+        this._initialized = initialized;


Warning · Non-blocking recommendation

📍 src/managers/pipenv/pipenvManager.ts:84
This ownership protocol is duplicated across six managers while tryFastPathGet and clearCache() also mutate _initialized. Please track a shared attempt-state abstraction such as begin(), complete(), and resetIfCurrent() to prevent future lifecycle drift; this is a follow-up design concern, not a correctness blocker for this fix.

[verified]

Confirmed as an intentional follow-up. This correctness-focused change deliberately keeps the reset/ownership protocol inline (see the PR's "no shared helper extraction" note and the constraint to avoid introducing a new abstraction in this fix). Notably, this round extends the same ownership pattern into fastPath.ts (getInitialized) and pipenv's discoverAndCommit — which strengthens the case for a future shared begin()/complete()/resetIfCurrent() abstraction. Tracking it as a separate design follow-up, not a correctness blocker for #16.

Follow-up to the failed-manager-initialization retry fix, closing three concurrency/ownership correctness gaps found in final review: - Gap 1 (venv/system, throw style): initialize() now resolves the captured deferred on success and rejects it on failure, then returns that same promise, so the leader and every concurrent waiter observe the identical settled result (both reject with the same error) instead of a leader-throws/waiter-resolves split. The guard is still cleared for retry and no orphan unhandled rejection is introduced. - Gap 2 (fastPath): background-init failure resets _initialized only when it still points at the deferred this run installed, so a stale run cannot erase a newer initialization. FastPathOptions gains getInitialized and all five call sites supply it. - Gap 3 (pipenv): discoverAndCommit builds the collection, project map and global env into locals and commits them synchronously only if the run still owns initialization, preventing a late/superseded run from overwriting newer state and preventing partial map/global writes on a mid-way throw. A superseded run also skips post-discovery bookkeeping. Cross-manager audit: only pipenv permitted the late-overwrite/partial-commit race (its clearCache() resets _initialized and its map load accumulates); conda, pyenv, venv, system and poetry are verified unaffected (no in-session _initialized reset plus clear-first map rebuild). pipenv refresh() keeps its existing accumulate flow (separate, out of scope). Adds deterministic regression tests for all three gaps (leader+waiter share one rejection, ownership-aware fastPath reset, pipenv late-overwrite, mid-map failure with changed retry, and superseded-run skip), each verified to fail without its fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

The changes alter concurrency/initialization semantics across multiple managers and fast-path behavior, which is high-impact and warrants final human review despite strong targeted unit coverage.

Review details

Files reviewed: 14/14 changed files
Comments generated: 0 new
Review effort level: Lite

StellaHuang95 · 2026-08-22T07:37:21Z

+                    }
                }
            }
        } catch (ex) {


Info · Optional note

discoverAndCommit publishes add events before the subsequent tool lookup and missing-manager notification. If either later operation throws, the catch clears initialization and a retry republishes the same add events. Please narrow retryable failures to discovery/commit or defer event publication until the fallible bookkeeping completes.

StellaHuang95 · 2026-08-22T07:37:23Z

+            // discovering) committed nothing and no longer owns the current state, so skip the
+            // post-discovery bookkeeping: don't re-run tool lookup or emit a duplicate
+            // missing-manager notification for a collection it did not write.
+            if (committed) {


Info · Optional note

A superseded attempt skips bookkeeping but still emits result: 'success' with envCount: 0, which can distort initialization telemetry. Skip telemetry for discarded attempts or record a distinct superseded outcome.

StellaHuang95 · 2026-08-22T07:37:25Z

+                    envCount,
+                    toolSource,
+                    errorType,
+                });


Warning · Non-blocking recommendation

The four swallow-style managers duplicate the resolve-before-best-effort-telemetry boundary. Please consider a shared best-effort telemetry helper, while retaining the strict sender where propagation is desired, so one manager cannot regress independently.

All six built-in manager initialize() methods memoized initialization in a _initialized deferred and resolved it in finally even on failure, leaving the guard set. The top guard then returned that settled promise forever, so a single transient discovery failure permanently poisoned discovery for the rest of the session with no way to retry. Capture the deferred locally, clear the guard on a thrown exception only if this run still owns it so a later call retries, and always settle the captured deferred in finally so concurrent waiters unblock. Each manager's throw-vs-swallow behavior is preserved, and a non-throwing tool_not_found stays a completed init. Affected: venv, system, conda, pipenv, poetry, pyenv. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

StellaHuang95 · 2026-08-23T02:32:55Z

            traceError('Pipenv lazy initialization failed', ex);
+            if (this._initialized === initialized) {
+                this._initialized = undefined;
+            }


Issue · Please address or respond

Clearing _initialized for every caught error also makes failures after discoverAndCommit has published add events retryable. A retry republishes those add events. Restrict reset to failures before publication or finish fallible bookkeeping before publishing events.

[verified]

StellaHuang95 · 2026-08-23T02:32:58Z

+        } as any);
+    }
+
+    test('rethrows on failure but clears state so a later call retries', async () => {


Warning · Non-blocking recommendation

These new manager test suites define createManager() before their tests, contrary to the required test-helper placement. Please move this helper to the end of each affected suite (sysPython, venv, pipenv, poetry, and pyenv).

[verified]

StellaHuang95 · 2026-08-23T03:07:31Z

                errorType,
            });
-            this._initialized.resolve();
+            initialized.resolve();


Issue · Please address or respond

Telemetry still runs before initialized.resolve(). A telemetry exception can strand concurrent waiters and escape the swallow-style contract; resolve first and make telemetry best-effort.

StellaHuang95 · 2026-08-23T03:07:33Z

                errorType,
            });
-            this._initialized.resolve();
+            initialized.resolve();


Issue · Please address or respond

Telemetry still runs before initialized.resolve(). A telemetry exception can strand concurrent waiters and reject callers despite this manager's swallow-style contract; resolve first and make telemetry best-effort.

StellaHuang95 · 2026-08-23T03:07:35Z

                errorType,
            });
-            this._initialized.resolve();
+            initialized.resolve();


Issue · Please address or respond

Telemetry still precedes deferred settlement. Resolve the captured deferred first and catch/log telemetry failures so this manager preserves its never-throw behavior and concurrent waiters cannot hang.

StellaHuang95 · 2026-08-23T03:07:37Z

                errorType,
            });
-            this._initialized.resolve();
+            initialized.resolve();


Issue · Please address or respond

Telemetry can still throw before the captured deferred is resolved, leaving concurrent waiters pending. Settle first and make telemetry best-effort.

StellaHuang95 · 2026-08-23T03:07:39Z

@@ -129,6 +130,9 @@ export class PipenvManager implements EnvironmentManager, Disposable {
            result = 'error';
            errorType = classifyError(ex);
            traceError('Pipenv lazy initialization failed', ex);


Issue · Please address or respond

This ownership check protects only the failure path. If clearCache() starts a newer initialization while this older attempt later succeeds, the older attempt can still publish stale environments and events over the newer result. Keep discovery results local and commit them only while this attempt still owns _initialized.

StellaHuang95 · 2026-08-23T03:07:42Z

Separate from this diff, fastPath.ts still unconditionally clears its initialization guard after failures; its clear-cache/reinitialization race should be addressed separately if this hardening is intended to cover the shared pattern.

StellaHuang95 requested a lite review from Copilot August 22, 2026 03:02

Copilot started reviewing on behalf of StellaHuang95 August 22, 2026 03:02 View session

Copilot AI reviewed Aug 22, 2026

View reviewed changes

Comment thread src/managers/conda/condaEnvManager.ts

Comment thread src/managers/pipenv/pipenvManager.ts

Comment thread src/managers/poetry/poetryManager.ts

Comment thread src/managers/pyenv/pyenvManager.ts

StellaHuang95 requested a lite review from Copilot August 22, 2026 03:33

Copilot started reviewing on behalf of StellaHuang95 August 22, 2026 03:33 View session

Copilot AI reviewed Aug 22, 2026

View reviewed changes

StellaHuang95 added the bug Something isn't working label Aug 22, 2026

StellaHuang95 commented Aug 22, 2026

View reviewed changes

StellaHuang95 added the review-auto:approved Automated review: no blocking findings (approval posted). label Aug 22, 2026

StellaHuang95 changed the title ~~fix: retry failed manager initialization~~ fix: retryable + ownership-aware manager initialization (races) Aug 22, 2026

StellaHuang95 requested a lite review from Copilot August 22, 2026 07:17

Copilot started reviewing on behalf of StellaHuang95 August 22, 2026 07:17 View session

Copilot AI reviewed Aug 22, 2026

View reviewed changes

StellaHuang95 commented Aug 22, 2026

View reviewed changes

StellaHuang95 force-pushed the stellahuang-microsoft-refactored-enigma branch from fac452e to 3e76695 Compare August 23, 2026 01:55

StellaHuang95 changed the title ~~fix: retryable + ownership-aware manager initialization (races)~~ fix: retry failed manager initialization Aug 23, 2026

StellaHuang95 removed the review-auto:approved Automated review: no blocking findings (approval posted). label Aug 23, 2026

StellaHuang95 force-pushed the stellahuang-microsoft-refactored-enigma branch from 3e76695 to 2785083 Compare August 23, 2026 02:09

StellaHuang95 force-pushed the stellahuang-microsoft-refactored-enigma branch from 2785083 to fe868b5 Compare August 23, 2026 02:17

StellaHuang95 commented Aug 23, 2026

View reviewed changes

StellaHuang95 added the review-auto:changes-requested Automated review: posted blocking findings to address. label Aug 23, 2026

StellaHuang95 commented Aug 23, 2026

View reviewed changes

StellaHuang95 closed this Aug 23, 2026

Conversation

StellaHuang95 commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Root cause

Fix

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

StellaHuang95 commented Aug 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StellaHuang95 commented Aug 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

StellaHuang95 commented Aug 22, 2026 •

edited

Loading