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
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.