Fix five PEP 723 inline-script defects: cancellation, rename, shared builds, terminal scope, and package drift - #1772
Merged
Stella Huang (StellaHuang95) merged 6 commits intoSep 9, 2026
Conversation
Stella Huang (StellaHuang95)
added
the
bug
…them Cancelling a PEP 723 environment setup used to retain the cache-entry lock with no cleanup attempt, so the cancellation surfaced as a generic "Failed to set up the environment for this script" error and the next attempt failed with "Lock was retained after an interrupted operation". Recovering required clearing the entire inline-script cache. Cancellation now cleans up automatically: - `discardCacheEntry` removes `.meta.json` and any `.meta.json.backup-*` first, which is the correctness guarantee: `inspectCacheEntry` treats a missing sidecar as stale, so an entry whose directory survives is inert and gets rebuilt or swept by TTL eviction rather than reused. - Directory removal is retried with a short backoff, because a just-stopped installer can briefly hold file handles (most visibly on Windows). - No lock is retained on cancellation, so `ELOCKRETAINED` can no longer be reached from this path and retrying the CodeLens simply rebuilds. The user-visible result is a single informational "Environment setup was canceled." message with nothing to clean up or confirm. Also fixes two bulk-setup bugs. `setUpInlineScriptEnvironmentsInWorkspace` only counted successes and never reported outcomes, so a cancellation mid-run was invisible and the next script's install started immediately. It now stops the run on cancellation and reports failures distinctly. This matters because cache keys are shared by design: scripts whose dependencies normalize to the same list resolve to the same cache entry, so one cancellation could previously poison a sibling script in the same run. `getSetupOutcome` is added as a non-consuming read so callers coalesced onto a single `create` attempt all observe the same outcome; `create` already clears it on entry, so it stays scoped to one attempt. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Renaming a PEP 723 script cleared its environment association, so the file needed setting up again even though nothing about the environment had changed. A cache entry is keyed by the script's normalized dependencies and its base interpreter (the script path is not an input), so a rename cannot invalidate it, and neither the inline metadata block nor the cached environment is touched by the rename. This also left two subsystems disagreeing. PythonProjectManagerImpl already follows renames via updatePythonProjectSettingPath, which rewrites the python-envs.pythonProjects entry to the new path and preserves its _inlineScriptRegistration marker. Clearing the association here therefore produced a managed inline-script project entry pointing at a file with no environment behind it. The rename handler now transfers the persisted record from the old path to the new one in a single persistence transaction, and re-validates it afterwards rather than trusting it: the record's metadata binding is content-derived, so if the file at the new path no longer matches, ordinary validation clears the association and the setup CodeLens returns. Guards: - The destination must still be a routable local .py file. Renaming to another extension, or off the local filesystem, drops the association as before. - Renaming onto an already-associated script replaces that association, since the moved file's contents are what now live at the destination. - Deletes are unchanged and still clear the association. Directory renames are not covered here. VS Code reports a single event for the folder rather than one per file, so associations under a moved folder are still stranded; that needs its own change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Scripts whose dependencies normalize to the same list resolve to the same cache entry, so a second script requesting setup while a build is in flight joins that build instead of starting its own. Cancelling the build only recorded an outcome for the script that started it. The joined script fell through to the generic "Failed to set up the environment for this script" error, and in a bulk run its outcome was not a cancellation, so the run kept installing its remaining selections instead of stopping. The shared PendingCreationContext now carries the build's failure, and each caller translates it into its own routing outcome after awaiting the shared promise. Cancellation therefore reaches every joined script, and the other failure paths (uncertain entry, undeletable stale entry, lock errors) now reach joiners as well instead of being reported only to the initiator. Verified by removing the joiner-side propagation and confirming the new test fails: the joined script's outcome was undefined. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ayload Shell-startup activation keeps a single activation command per workspace folder (for example VSCODE_PYTHON_PWSH_ACTIVATE), which VS Code injects into every terminal opened in that folder. handleEnvironmentChange wrote whichever environment the change event carried into that slot, after collapsing the event's uri to its containing folder. Selecting a PEP 723 inline-script environment fires that event with the `.py` file's uri and the script's own environment, so a per-file selection became the folder default. A general workspace terminal opened afterwards would activate the last configured script's environment instead of the folder's, and installs run there would land in the inline-script cache. The handler now resolves the folder's own environment via getEnvironment(workspaceFolder.uri) rather than trusting the payload. This matches what initializeInternal already did, so the two paths no longer disagree, and it fixes the whole class rather than special-casing the inline-script manager: any file-scoped environment is excluded. A folder whose startup variables were already written from a file-scoped selection is repaired on the next environment change. Removal semantics are unchanged: variables are cleared only when the folder genuinely has no environment. Adds the first unit coverage for this manager. Verified the tests fail when the handler is reverted to writing the event payload. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cache entries are shared: scripts whose dependencies normalize to the same list resolve to the same key and therefore the same physical environment. Changing packages there through the package manager UI silently affected every script bound to it. Nothing validated installed versions, so the entry stayed "valid", the setup CodeLens stayed hidden, and a script the user never opened would quietly run the wrong version while its own header still declared the original pin. Re-running setup reused the modified entry rather than repairing it, so the only recovery was clearing the whole cache. A package change on an owned entry now records `manuallyModified` in the sidecar and un-routes every script associated with it, so the setup CodeLens returns for each affected script. `inspectCacheEntry` treats a marked entry as stale, so the next setup discards and rebuilds it from the script's declared metadata instead of handing back the drifted environment. The marker is written under the cache-entry lock. Setup installs through `managePackages`, which fires this same event, so a build still registered in `pendingCreations` is skipped: the emitter is synchronous, so a build owning the change is recognised before any await, and the check is repeated once the lock is held. Without that guard every setup would mark the environment it had just built and rebuild it endlessly. Not addressed here: sharing is still not surfaced before an edit, a deliberate ad-hoc install is discarded by the next setup without explanation, and package changes made outside VS Code fire no event and remain undetected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stella Huang (StellaHuang95)
force-pushed
the
fix/inline-script-cancel-recovery
branch
from
September 9, 2026 20:50
669cb8e to
56aa0c7
Compare
Contributor
|
🔒 Automated review in progress — Rich Chiodo (@rchiodo) is auto-reviewing this PR. |
Contributor
|
Result: 🔴 Verification detailsVerification: Isolated verification observed failures that were not classified as caused by this PR: Node dependency and test discovery preflight. The relevant tests could not be fully run in the isolated environment; this review is not fully verified. Summary: Verification could not proceed because the container lacks Node.js (`node: not found`). Consequently, compilation and all three targeted Mocha suites were not run. I identified 19 tests added or substantially rewritten by the PR. Confidence is low until they run in a Node-capable environment. Test runs: 1 failed, 3 not run
|
Rich Chiodo (rchiodo)
added
the
review-auto:changes-requested
Retire a stored setup outcome when setup succeeds. Outcomes are read non-consumingly so that callers coalesced onto one attempt all observe the same result, but nothing replaced the outcome on success: only the next `create` cleared it on entry. A cancelled attempt followed by a successful one therefore left a stale `cancelled` outcome behind, which a later read could report against the successful setup. `setUpInlineScriptEnvironment` now clears it once the environment is associated, and a regression test covers cancel-then-success. Assert complete user-facing strings in the inline-script setup UI tests instead of matching fragments, so an accidental wording change is caught. The cancellation test now asserts the exact message, which also subsumes the separate "does not mention cleanup" assertion that it replaces. Move the rename handler's documentation back above `handleRenamedScripts`. It was left attached to `handlePackagesChanged` when that handler was inserted ahead of it, so each method now documents its own invariant again. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Result: 🔴 Verification detailsVerification: Isolated verification observed failures that were not classified as caused by this PR: Runtime, dependency, test, and change discovery. The relevant tests could not be fully run in the isolated environment; this review is not fully verified. Summary: Verification could not proceed because the disposable container has no Node.js executable (`node: not found`). Consequently, none of the four targeted unit-test files could be compiled or executed. The PR adds or rewrites 19 focused tests across cancellation, rename, shared builds, terminal scoping, and package drift. Confidence is limited to test discovery and source inspection. Test runs: 1 failed, 4 not run
|
Rich Chiodo (rchiodo)
approved these changes
Sep 9, 2026
Rich Chiodo (rchiodo)
left a comment
Rich Chiodo (rchiodo)
left a comment
Contributor
There was a problem hiding this comment.
Approved via Review Center.
Rich Chiodo (rchiodo)
added
review-auto:approved
Stella Huang (StellaHuang95)
enabled auto-merge (squash)
September 9, 2026 21:29
Stella Huang (StellaHuang95)
added a commit
that referenced
this pull request
Sep 11, 2026
…nments (#1780) > Part of #1602. Design: #1601. Follow-up to #1772. ## Summary Fix PEP 723 inline-script cache and routing issues, make cached environments read-only through the package-management UI, and support deleting one cached environment without clearing the entire script cache. The feature remains behind the internal, undeclared `python-envs.inlineScripts.enabled` flag, which defaults to `false` and is latched at activation. This does not enable inline scripts by default or add automatic installation on Run/Debug. ## Commits and user-visible changes | Commit | Problem or missing behavior | Change | |---|---|---| | `64960273` | The first package enumeration could be mistaken for an installation, so expanding Packages or refreshing inventory invalidated a working script environment. Case-only Windows renames could also lose the association. | Replace package-event-based drift detection with a recorded inventory hash; hide package mutation actions and reject the palette install path for inline environments; preserve Windows case-only rename associations, including dirty aliases and pending reads. | | `669ffe18` | A lookup could detect drift and return no environment while routing still advertised the cache as valid and the setup CodeLens remained hidden. Other scripts sharing the entry could retain stale state. | Confirm drift against the current entry, invalidate affected routing/cached validation, and notify environment consumers without requiring a save. Protect newer repairs and selections from stale observations. | | `1d3ea92a` | Delete Environment was offered in the Projects view, but the inline manager did not implement removal and returned "Remove Environment not supported". | Delete the selected cache entry without a confirmation dialog, clear its known workspace associations, and update the environment collection. Preserve script files, project entries/settings, the base interpreter, and unrelated cache entries. | ## Implementation and behavior notes ### Record package inventory instead of interpreting package events - When inventory is readable, setup records an `installedPackagesHash` in the entry's `.meta.json`, under the existing cache-entry lock. The hash is computed from sorted, case-normalized `*.dist-info` names, which include distribution names and versions. - Validation and reuse compare the recorded inventory with disk. Merely expanding or refreshing the package list no longer has authority to invalidate the environment. - A missing baseline or unreadable inventory is **unknown**, not evidence that packages were added. Existing `manuallyModified` sidecars remain honored. - The package list stays visible, but inline environments do not offer Install, Uninstall, or Change Version actions in the trees. The palette install command explains that dependencies should be edited in the script's metadata and setup rerun. - No new package-provider event metadata, per-package operation queues, or provider opt-in interfaces are introduced. The pip, Conda, Poetry, shared package-change helper, and package-watcher implementations are unchanged. ### Keep routing and the setup action consistent - A confirmed mismatch clears the affected scripts' cached environments and validation state, advances revisions, restores their setup CodeLenses, and publishes environment changes. - Invalidation rechecks the current sidecar/inventory under a non-waiting cache-entry lock. An observation made before a repair must not invalidate the repaired entry. - Busy entries and unavailable confirmation reads do not publish stale invalidations. A newer selection wins when an older lookup finishes later. - Detection is on demand, after the existing five-second validation cache expires; this is not a new background polling service. ### Delete one cached environment - Delete means physical deletion whether the entry has one associated script or several. It is not silently changed into "detach this script" for shared entries. - The action has **no confirmation dialog**. Scripts sharing the deleted entry need setup again. - Reuse the existing cache maintenance, ownership/path checks, locks, and association-cleanup machinery, with a single-entry target. Do not invoke the bulk project-settings cleanup. - Route an inline Projects-view item through the manager owning the clicked environment, rather than the file's potentially changed fallback manager. Ordinary environment removal keeps its existing routing. - Reject missing arguments, foreign/out-of-cache targets, redirected entries, and active creation conflicts. Preserve unrelated and unrecognized association records. - Invalidate a valid sidecar before filesystem removal so an interrupted deletion does not leave surviving cache files trusted as healthy. Report filesystem/persistence failures and update local state for what was actually removed. ## Testing Local validation before the rebase/signing update (`1afe4bc9`): - `npm run compile-tests`: passed. - `npm run compile` (webpack extension bundle): passed. - `npm run lint`: passed. - Full unit suite: **2,074 passing, 6 pending**. Coverage includes get-only drift detection and CodeLens recovery, shared versus unrelated entries, stale observations after newer repairs/selections, real cache-entry locks, unavailable inventory, Windows case-only renames, palette restrictions, targeted deletion without prompting, protected source/settings/base-Python files, already-missing entries, unsafe paths, partial failures, and concurrent create/delete/rehydration. ### Suggested manual verification These are reviewer checks, not a claim that a full live VS Code end-to-end pass has been completed: 1. Enable `python-envs.inlineScripts.enabled` and reload. Use the Python Environments integration and companion builds that support per-file interpreters. 2. Set up two scripts with the same metadata so they share a cached environment, plus a third script using a different cache entry. 3. Expand/refresh Packages: setup remains valid and inline package mutation actions are absent. The palette install action for an inline project should show guidance rather than install. 4. In a disposable test environment, change installed packages externally. After the validation cache expires, request the script's environment without saving. Verify the affected setup actions return and explicit setup repairs the entry. 5. On Windows, rename `job.py` to `Job.py`; verify the existing environment remains associated and unsaved metadata is not replaced with stale disk metadata. 6. Delete a shared inline environment from the Projects view. Verify there is no confirmation, only that cache entry is deleted, affected scripts need setup again, and the unrelated environment, scripts, project settings, and base Python remain. 7. Exercise a busy/failed deletion and verify it reports failure rather than success. Recheck normal package management/removal for an ordinary venv. ## Scope and known follow-ups - Inventory comparison is not a full package-integrity audit: it does not inspect package contents or legacy `.egg-info`-only installations. - Shared cache entries can be referenced by other workspaces. Association cleanup/notifications here are local to the current workspace; other windows notice deletion on revalidation. Cache locks do not track arbitrary running Python processes, so running jobs should be stopped before deletion. - The current unknown-inventory behavior is retained. The case where an entirely missing `site-packages` directory is accepted during reuse remains a follow-up; deleting the cache entry and running setup provides a recovery path. - A rejected inline selection can still briefly show the temporary "selected" badge; the actual selection is not changed. That feedback fix is separate from the routing invalidation addressed here. - Companion-version guidance is covered separately by #1777. No installer-backend, retention-policy, or Run/Debug auto-setup changes are included. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a4277008-9dd3-4beb-b9ac-4197eb085404
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Five fixes to the PEP 723 inline-script feature, found while working through reported issues against the testbed. Each is independently reviewable and has unit coverage that was checked to fail when the corresponding guard is removed, not merely to pass.
The feature remains behind the undeclared
python-envs.inlineScripts.enabledflag (defaultfalse), so nothing here is user-visible until that is enabled.Commits and the issue each addresses
0a21a2c1Lock was retained after an interrupted operation887548d8eaa7142322628e1f669cb8edTwo earlier commits on this branch (
9acf5a21,cd7d214a) predate this work and are unrelated to the above.0a21a2c1— Discard cancelled environments instead of quarantining themCancelling package installation retained the cache-entry lock and made no cleanup attempt. The cancellation surfaced as a generic "Failed to set up the environment for this script", and the next attempt failed with
ELOCKRETAINED. Recovery required clearing the entire cache.Cancellation now cleans up automatically.
discardCacheEntrydeletes.meta.jsonand any.meta.json.backup-*first, then removes the directory with bounded retry.The key invariant for reviewers: the sidecar is the correctness guarantee, not the directory.
inspectCacheEntrytreats a missing sidecar asstale, andwriteMetaJsonis the only writer of that file — four call sites, all under the cache-entry lock (see the comment abovewithCacheEntryLock). A surviving installer writes intosite-packagesand cannot recreate a sidecar. So an entry whose directory survives deletion is inert, and gets rebuilt or TTL-swept rather than reused.Retry exists because a just-stopped installer can hold file handles briefly, most visibly on Windows.
Also fixes two bulk-setup bugs:
setUpInlineScriptEnvironmentsInWorkspaceonly counted successes and never surfaced outcomes, so a mid-run cancellation was invisible and the next install started immediately. It now stops the run on cancellation and reports failures distinctly.887548d8— Follow renames instead of dropping associationsA cache entry is keyed by normalized dependencies and base interpreter — the script path is not an input — so a rename cannot invalidate it. Neither the metadata block nor the environment changes.
This also left two subsystems disagreeing.
PythonProjectManagerImplalready follows renames viaupdatePythonProjectSettingPath, which rewrites thepythonProjectsentry and preserves its_inlineScriptRegistrationmarker. Clearing the association therefore produced a managed inline-script project entry pointing at a file with no environment behind it.The record is transferred in one persistence transaction and then re-validated rather than trusted: its metadata binding is content-derived, so if the file at the new path no longer matches, ordinary validation clears it and the CodeLens returns.
Guards: destination must still be a routable local
.py; renaming onto an already-associated script replaces it; deletes are unchanged.Known gap: directory renames are not covered — VS Code reports one event per folder rather than one per contained file. Moving an individual file already works, since VS Code reports a move as a rename.
eaa71423— Cancellation reaches every script sharing a buildScripts whose dependencies normalize to the same list resolve to the same cache key, so a second request joins the in-flight build via
pendingCreationsinstead of starting its own. Cancelling recorded an outcome only against the initiating URI. The joined script fell through to a generic failure, and in a bulk run its outcome was not a cancellation — so the run kept installing.The failure is now recorded on the shared
PendingCreationContextand translated per caller after awaiting the shared promise.Note for reviewers: same-file coalescing (
pendingSetups) and different-file shared builds (pendingCreations) are separate mechanisms. Only the second was broken. Bulk setup is sequential, so selecting both files in one bulk run does not reproduce it — the second request must overlap the first build.22628e1f— Shell startup variables resolve at folder scopeShell-startup activation keeps a single activation command per workspace folder (for example
VSCODE_PYTHON_PWSH_ACTIVATE), injected into every terminal opened there.handleEnvironmentChangewrote whichever environment the event carried into that slot after collapsing the event's URI to its containing folder — so a per-file selection became the folder default.The handler now resolves the folder's own environment via
getEnvironment(workspaceFolder.uri). This matches whatinitializeInternalalready did, so the two paths no longer disagree, and it fixes the whole class rather than special-casing the inline-script manager: any file-scoped environment is excluded. A folder already polluted by an earlier session self-repairs on the next environment change.Only reachable with
python-envs.terminal.autoActivationType: "shellStartup"; the default is"command". Adds the first unit coverage for this manager.669cb8ed— Invalidate an environment when its packages are editedCache entries are shared by design. Editing packages there through the package UI silently affected every bound script: nothing validated installed versions, so the entry stayed valid, the CodeLens stayed hidden, and a script the user never opened would run the wrong version while its own header still declared the original pin. Re-running setup reused the modified entry rather than repairing it.
A package change on an owned entry now records
manuallyModifiedin the sidecar and un-routes every associated script, so the CodeLens returns for each.inspectCacheEntrytreats a marked entry as stale, so the next setup rebuilds from declared metadata.Please look closely at the in-flight guard. Setup installs through
managePackages, which fires this same event. Without the guard, every setup would mark the environment it had just built and rebuild endlessly. The guard relies on VS Code'sEventEmitterbeing synchronous, so a build still registered inpendingCreationsis recognised before anyawait; the check is repeated once the lock is held.Known gaps, deliberately not addressed: sharing is still not surfaced before an edit; a deliberate ad-hoc install is discarded by the next setup without explanation; package changes made outside VS Code fire no event and remain undetected.