Add uv fallback for inline script environments (PEP 723 PR 6/16) - #1652
Add uv fallback for inline script environments (PEP 723 PR 6/16)#1652Stella Huang (StellaHuang95) wants to merge 4 commits into
Conversation
| @@ -1,27 +1,65 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
There was a problem hiding this comment.
Copilot generated:
📍 src/managers/builtin/inlineScriptEnvManager.ts (buildCacheEntry / createOrReuseEnvironment)
A single user Cancel during package install permanently poisons this cache key with no recovery. The Skeptic and Architect both traced this (High): pkgInstallationCancelled → buildCacheEntry returns { retainLock: true } without removeCacheEntry, createOrReuseEnvironment calls lock.retain(), and every future create() for the same deps+interpreter then hits mkdir→EEXIST→isRetainedLock()→throws ELOCKRETAINED→caught→returns undefined. The half-built env has no sidecar, so it never gets a lastUsedAt and TTL cleanup can't reclaim it either. While the Advocate notes retain-on-interruption is intentional fail-closed design, conflating a deliberate user Cancel with a crash makes this unrecoverable without manual globalStorage surgery. Treat cancellation distinctly: removeCacheEntry(envDir) + normal release() to allow retry, or make a retained-lock acquisition reclaim-and-rebuild. Add a cross-call regression test (the existing rollback suite stubs the lock, so it misses this).
[verified]
There was a problem hiding this comment.
The recoverability concern makes sense, but I kept the fail-closed behavior here. Cancellation currently rejects immediately after proc.kill(), before the child reports close, and a package-manager child may still be writing into the cache path. Removing the entry and releasing the lock at that point could race a surviving pip/uv process and allow a retry to rebuild underneath it. PR 13's guarded clear-cache flow is the planned recovery path and needs to land before the feature becomes user-facing.
|
GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for src/test/managers/builtin/inlineScriptEnvManager.unit.test.ts:L1. Copilot generated: Review-rule violation ( [verified] |
| @@ -340,9 +358,9 @@ export async function createWithProgress( | |||
| venvRoot: Uri, | |||
There was a problem hiding this comment.
Copilot generated:
📍 src/managers/builtin/venvUtils.ts (createWithProgress)
The Architect flags (Medium) that createWithProgress now takes 9 positional parameters, and the new tests assert against firstCall.args[4]/args[6]/args[7]/args[8] throughout. Any future reordering breaks many tests with opaque index failures, and the long positional list is itself a smell. Consider grouping the trailing arguments (packages, options, and the env/root URIs) into an options object to decouple callers and tests from arg order.
[verified]
There was a problem hiding this comment.
Agreed that the positional API is worth improving, but I am deferring this refactor. The helper already had eight positional parameters, and converting it now would touch all four production callers plus unrelated tests without changing behavior. A request/options-object refactor will be safer as a dedicated change when another option is needed.
Cross-process file lock, venv Python-path helper, cancellation-safe process runner, and createWithProgress tracking options that inline-script environment creation builds on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39dcc6a3-0fbd-4f36-9d0f-68677de49c27
Cache-key tail normalization, cache-layout ownership/status checks with typed sidecar reads, and interpreter-constraint trimming that inline-script environment creation builds on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39dcc6a3-0fbd-4f36-9d0f-68677de49c27
Implement InlineScriptEnvManager.create(): select a compatible base interpreter and build or reuse a dependency-keyed virtual environment, with cache-ownership validation, cross-process locking, and cancellation-safe creation. Wire the manager's collaborators. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39dcc6a3-0fbd-4f36-9d0f-68677de49c27
Prompt to install a compatible Python via uv when no installed base satisfies PEP 723 metadata, then refresh and reselect before creating the environment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 22cdcbcc-09ff-41d0-9fcf-59a6a5117ff1
e7ead2d to
fa3de47
Compare
|
Addressed the platform-helper feedback: inherited PR 5 tests now use isWindows()/getVenvPythonPath, and the three PR 6 uv-executable cases use isWindows(). |
> Part of #1602 (PEP 723 inline script env support). Design doc: #1601. > > This replaces the earlier closed draft #1652 with the finalized implementation rebased on `main`. ### Roadmap context This is **PR 6 of 16** in the PEP 723 inline-script roadmap. It extends the PR 5 `create()` happy path with the missing-compatible-interpreter fallback. | Phase 2: Manager | PR | Status | |---|---|---| | | PR 4: `InlineScriptEnvManager` skeleton | merged (#1610) | | | PR 5a: generic env-creation utilities | merged (#1651) | | | PR 5b: inline-script cache + interpreter utilities | merged (#1655) | | | PR 5c: `create()` happy path | merged (#1656) | | | **PR 6: `create()` uv-install fallback** | **this PR** | | | PR 7: persistence (`get` / `set` + Memento) | separate follow-up | ### Why this PR PR 5 can create or reuse an inline-script environment when an installed base interpreter already satisfies the script's `requires-python`. It deliberately stops when no compatible interpreter exists. This PR adds the consent-gated fallback for that case: 1. Re-check installed interpreters after entering a narrow fallback queue. 2. Select a safe Python version satisfying the complete PEP 440 requirement. 3. Ask the user before installing uv or Python. 4. Refresh environment discovery after installation. 5. If discovery is stale or unavailable, resolve and validate uv's returned executable directly. 6. Continue through the existing PR 5 cache create/reuse path. ### What this PR does **Adds the inline-script fallback to `InlineScriptEnvManager.create()`** - Keeps the installed-interpreter path unchanged and only enters the fallback when no compatible base exists. - Skips installation prompts for quick-create/noninteractive calls. - Serializes only fallback installations, then re-checks discovery before prompting so concurrent compatible requests reuse one installation. - Retains successfully direct-resolved uv interpreters so queued requests can reuse them even while discovery remains stale. **Selects a safe uv target from `requires-python`** - Uses simple safe selectors directly, such as `>=3.13` → `3.13` and `==3.13.1` → `3.13.1`. - Queries uv's advertised versions for bounded or exclusion-heavy constraints. - Restricts catalog candidates to default CPython 3 builds satisfying the complete specifier. - Handles exclusions such as `>=3.13.2,!=3.13.2` without installing the excluded floor. - Uses an advertised release for bounded ranges rather than fabricating a potentially unavailable patch-zero release. - Applies full PEP 440 prerelease semantics consistently across discovery, catalog selection, direct resolution, and cache validation. - Normalizes accepted prerelease aliases (for example, `c1` → `rc1`) before passing a version to uv. **Extends the uv installer's consent flow** - Adds an inline-script-specific prompt that shows both the script requirement and selected Python version. - Sanitizes and caps script-controlled prompt details. - Validates install selectors before forwarding them to uv. - If catalog lookup is required and uv is missing, asks for consent to install uv first. - Re-checks whether a newly installed uv is usable by the current extension host and surfaces the existing restart-required message when needed. **Handles stale discovery after installation** - Refreshes environment discovery after uv installs Python. - Treats refresh/discovery failures as recoverable. - Resolves the executable returned by uv directly, verifies that it satisfies `requires-python`, and canonicalizes its path before creating the cached environment. ### Examples | `requires-python` | Fallback behavior | |---|---| | `>=3.13` | Request uv's `3.13` selector | | `==3.13.1` | Request exactly `3.13.1` without requiring a catalog lookup | | `>=3.11,<3.12` | Choose an advertised compatible `3.11.x` release | | `>=3.13.2,!=3.13.2` | Skip `3.13.2` and choose a compatible advertised release | | `>=3.15.0a1,<3.16` | Permit an explicitly requested prerelease | | `>=3.14,<3.16` | Do not select a prerelease implicitly | ### Safety and concurrency - No uv or Python installation occurs without explicit user consent. - Script-derived values cannot inject arbitrary uv arguments. - Declined, cancelled, or failed installations do not mutate the script-environment cache. - The fallback queue does not globally serialize environment selection or normal cache creation. - Existing cross-process cache locking and rollback behavior from PR 5 remains unchanged. ### Tests Coverage includes: - simple, exact, bounded, exclusion, and prerelease requirements; - uv catalog filtering and consented uv bootstrap; - declined and failed installations; - refresh and discovery failures with direct resolution; - simultaneous same- and different-constraint requests; - quick-create prompt suppression; - strict post-install and cache compatibility checks; - prompt sanitization and install-selector validation. `npm run compile-tests`, `npm run lint`, the full unit suite, and the focused inline-script/uv suites are clean. ### User impact **No default-path user impact yet.** This completes an internal Phase 2 manager capability. Automatic routing and user-facing entry points arrive in later roadmap PRs. When those entry points are wired, users whose scripts require an unavailable Python will be able to approve installing a compatible interpreter rather than having environment creation stop. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1a9f6ba1-9bd3-4664-bc25-a0d34d7a2e91
No description provided.