fix: isolate environment consumers from single-manager failures by StellaHuang95 · Pull Request #13 · StellaHuang95/vscode-python-environments

StellaHuang95 · 2026-08-22T02:37:39Z

Problem

Two user-visible failures where a single environment manager could break unrelated functionality:

Python API — getEnvironments('all' | 'global') and refreshEnvironments(undefined) used Promise.all, so one manager's rejection discarded every other manager's completed results.
Environment picker — pickEnvironment awaited every manager's getEnvironments('all') sequentially before showing the QuickPick, so opening latency was additive and any manager rejection prevented the picker from opening at all.

Root cause

Both paths fanned out to managers without isolating per-manager failures: Promise.all rejects as soon as any input rejects, and the picker's sequential await loop both serialized discovery and propagated the first rejection before the UI was ever shown.

Fix

API: a private, type-safe collectFromManagers() runs managers concurrently with Promise.allSettled, returns the successful managers' results in original manager order, logs each failure with its manager id, and throws a minimal local AggregateEnvironmentError only when every manager fails. An empty manager list resolves with []/void, and single-manager scope paths are unchanged.
Picker: the QuickPick opens immediately with Browse/Create (and any recommended item), then loads all managers concurrently after it is shown via a small optional onDidShow controller seam on showQuickPickWithButtons. The seam reuses the existing accept/back/cancel/button/hide/token wiring; controller updates no-op once the picker is accepted, dismissed, or disposed. A manager that rejects is logged and skipped without hiding the others, and sections are built in fixed manager order.

Tests

API failure isolation (pythonApi.failureIsolation.unit.test.ts): partial success with manager order, synchronous-throw isolation, total failure throwing AggregateEnvironmentError with all reasons in order, empty-list success, per-manager logging, global scope forwarding, and the refreshEnvironments equivalents.
Picker (pickEnvironment.unit.test.ts): opens before slow managers resolve, fixed manager order regardless of completion order, one manager failing does not hide the others (and is logged), every manager failing still leaves a usable Browse/Create picker, a synchronous recommended item, an empty manager list, and late results after close are ignored.
Seam (showQuickPickWithButtons.unit.test.ts): existing static callers (accept/hide/back/custom button/token) are unchanged, and the onDidShow controller populates items / toggles busy and no-ops after settle.

Copilot

Pull request overview

Improves resilience when individual environment managers are slow or fail, allowing API consumers and the environment picker to continue using successful results.

Changes:

Adds concurrent, failure-isolated manager collection for API operations.
Streams picker sections as managers resolve, with deterministic ordering and deduplication.
Adds controller lifecycle support and comprehensive unit tests.

Show a summary per file

File	Description
src/test/features/pythonApi.failureIsolation.unit.test.ts	Updated as part of this pull request.
src/test/common/showQuickPickWithButtons.unit.test.ts	Updated as part of this pull request.
src/test/common/pickEnvironmentStreaming.unit.test.ts	Updated as part of this pull request.
src/test/common/fakeQuickPick.ts	Updated as part of this pull request.
src/features/pythonApi.ts	Updated as part of this pull request.
src/common/window.apis.ts	Updated as part of this pull request.
src/common/pickers/environments.ts	Updated as part of this pull request.
src/common/errors/AggregateEnvironmentError.ts	Updated as part of this pull request.

Review details

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

Files reviewed: 8/8 changed files
Comments generated: 1
Review effort level: Lite

StellaHuang95 · 2026-08-22T03:45:39Z

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

Addresses the Copilot review comment on the global scope. The 'global' scope shares the same collectFromManagers aggregation as 'all', so total failure throws AggregateEnvironmentError per spec (aggregation behavior is intentionally unchanged). Adds explicit regression coverage: - partial success for 'global' isolates a failing manager, returns the successful results, and proves the scope is forwarded to each manager. - total 'global' failure throws AggregateEnvironmentError with all reasons in manager order. The PR description's partial-vs-total failure semantics were corrected to state that total failure throws for both 'all' and 'global' (it never resolves with [] on total failure; [] is only the empty-manager-list result). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

Address the environment identity deduplication and cached recommendation ownership issues.

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

Review details

Files reviewed: 9/9 changed files
Comments generated: 3
Review effort level: Lite

The streaming picker seeds its recommendation synchronously from getLastKnownEnvironment so it can open before manager.get() resolves. That cache is scope-keyed (global / project URI), not manager-keyed, so it can return an environment owned by a since-changed manager. Such a seed is silently dropped by setEnvironments on selection (no manager matches its managerId). Only seed an entry the current global/project manager actually owns; otherwise start unseeded and let the authoritative resolveRecommended fill it in after show. Adds a setEnvironmentCommand seeding-ownership suite (global + project, mismatch vs match) proving unowned last-known envs are not seeded. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Review details

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

Refine the explanatory comments on the seed ownership gate: its primary purpose is keeping the synchronous seed consistent with the authoritative resolveRecommended (manager.get()) and the pre-streaming semantics; the silent-drop by setEnvironments only strictly occurs when the stale env's manager has been unregistered. Comment-only; no behavior change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

StellaHuang95 · 2026-08-22T05:19:16Z

+            await collectFromManagers(this.envManagers.managers, 'refreshEnvironments(all)', (manager) =>
+                manager.refresh(currentScope),
+            );
            return Promise.resolve();


Warning · Non-blocking recommendation

This Promise<void> completes successfully when one or more managers fail to refresh, leaving their environment state stale without exposing partial-failure information to callers. Please separate concurrent settlement from refresh policy so callers can distinguish a fully refreshed result from a partial failure.

StellaHuang95 · 2026-08-22T05:44:46Z

+        controller.setItems(buildItems());
+    };
+
+    const onDidShow = (controller: QuickPickController<EnvironmentPickItem>) => {


Warning · Non-blocking recommendation

When resolveRecommended rejects, the catch retains any synchronously seeded recommendation, whereas an undefined successful resolution clears it. Please add a seeded-rejection case to establish and protect this fallback behavior explicitly.

[verified]

#13) Two final race/correctness fixes for the streaming environment picker. - Guard every loader/recommendation completion on controller.settled before any section or canonical-item mutation. buildItems() rewrites the canonical item objects in place, so a late duplicate or late recommendation resolving after the user has accepted could otherwise change the already-accepted item's result and return a different environment than the one selected. deferred.completed flips synchronously on accept/back/cancel, so checking settled before any build reliably short-circuits the late continuation. - Remove synchronous last-known recommendation seeding, along with the envId.managerId === manager.id ownership check, which was invalid for delegating managers (e.g. VenvManager legitimately returning a System/base environment whose managerId differs). The picker now opens immediately with Browse/Create and resolves the authoritative recommendation via manager.get() only after onDidShow, streamed through the same guarded controller. Removed the now-unused recommended option field and the seed-ownership code/comments. Tests: added accept-vs-late-higher-priority-duplicate and accept-vs-late-recommendation races (verified non-vacuous: both fail when the settled guard is removed), a delegated (different managerId) recommendation that opens immediately and appears when resolved, a resolver-returns-undefined case, and replaced the seeding-ownership suite with "no synchronous seed" tests proving both setEnvironmentCommand paths open without awaiting a pending default-manager get(). Full unit suite green (1734 passing). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🟢 Approval recommended

No final review comments identify unresolved blocking issues.

Review details

Suppressed comments (1)

src/test/common/pickEnvironmentStreaming.unit.test.ts:22

These fixtures use hard-coded POSIX paths for every identity key. The new deduplication logic is path-sensitive, so this suite never exercises Windows backslashes, drive letters, or case folding; a Windows-only regression in environmentIdentityKey could pass. Build the fixture paths with platform-aware Uri.file(...).fsPath/path helpers and include an equivalent Windows path case.

function makeEnv(id: string, execPath: string, displayName = id, managerId = 'test-manager'): PythonEnvironment {
    return {
        envId: { id, managerId },
        name: id,
        displayName,
        displayPath: execPath,
        execInfo: { run: { executable: execPath } },
    } as unknown as PythonEnvironment;

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

The streaming picker's environmentIdentityKey dedup is path-sensitive, but the suite only used POSIX fixtures, so a Windows-path regression could pass. Add a deterministic test proving the same prefix expressed with backslashes vs forward slashes collapses to a single entry (normalizePath folds '\' to '/' on every platform; the test asserts separator folding only, not Windows-only case folding, so it passes identically on Windows and POSIX runners). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

The concurrency and race-sensitive picker changes warrant final human review.

Review details

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

StellaHuang95 · 2026-08-23T01:38:15Z

+    constructor(message: string, errors: unknown[]) {
+        super(message);
+        this.name = 'AggregateEnvironmentError';
+        this.errors = [...errors];


Warning · Non-blocking recommendation

AggregateEnvironmentError is thrown by public getEnvironments and refreshEnvironments paths but is not exported through the public API. Consumers therefore cannot type-safely recognize or inspect the new errors payload. Please export and document this error contract, or keep the public failure shape to a standard Error.

The Python API's getEnvironments('all'|'global') and refreshEnvironments(undefined) used Promise.all, so a single manager's rejection hid every other manager's completed results. The environment picker also awaited every manager sequentially before showing, so latency was additive and any rejection stopped the picker from opening at all. - Add a private, type-safe collectFromManagers() helper that runs managers concurrently via Promise.allSettled, returns successful results in original manager order, logs each failure with its manager id, and throws AggregateEnvironmentError only when every manager fails (an empty manager list resolves with []). - Open the picker immediately with Browse/Create and load managers concurrently after it is shown, through a small optional onDidShow controller seam on showQuickPickWithButtons that reuses the existing accept/back/cancel/button wiring. A failed manager is logged and skipped without blocking the others, and late updates no-op once the picker is closed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

StellaHuang95 · 2026-08-23T03:21:24Z

-            label: manager.displayName,
-            kind: QuickPickItemKind.Separator,
+    // Load every manager's environments concurrently after the picker is shown so opening never waits
+    // on the slowest manager, and a single manager that rejects can't hide the others' environments.


Issue · Please address or respond

manager.getEnvironments('all') is invoked while constructing the Promise.allSettled input. A synchronous throw therefore escapes before allSettled is created, rejects onDidShow, and closes the picker. Wrap each invocation in an async callback and add a synchronous-throw picker regression test.

StellaHuang95 · 2026-08-23T03:21:27Z

-            label: manager.displayName,
-            kind: QuickPickItemKind.Separator,
+    // Load every manager's environments concurrently after the picker is shown so opening never waits
+    // on the slowest manager, and a single manager that rejects can't hide the others' environments.


Warning · Non-blocking recommendation

Items are applied only after every manager settles. If one manager never resolves, the picker remains busy and successful managers' environments never appear. Consider cancellation/timeout handling or progressively publishing settled sections, with a never-settling-manager regression test.

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

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

Copilot AI reviewed Aug 22, 2026

View reviewed changes

Comment thread src/features/pythonApi.ts

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

StellaHuang95 changed the title ~~feat: isolate environment consumers from single-manager failures~~ fix: isolate environment consumers from single-manager failures Aug 22, 2026

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

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

Copilot AI reviewed Aug 22, 2026

View reviewed changes

Comment thread src/common/pickers/environments.ts Outdated

Comment thread src/features/envCommands.ts Outdated

Comment thread src/features/envCommands.ts Outdated

StellaHuang95 requested a lite review from Copilot August 22, 2026 04:29

Copilot started reviewing on behalf of StellaHuang95 August 22, 2026 04:29 View session