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)#1652StellaHuang95 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(). |
1 participant
No description provided.