Skip to content

Commit df05e69

Browse files
Fix five PEP 723 inline-script defects: cancellation, rename, shared builds, terminal scope, and package drift (#1772)
## 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.enabled` flag (default `false`), so nothing here is user-visible until that is enabled. ## Commits and the issue each addresses | Commit | Issue addressed | |---|---| | `0a21a2c1` | Cancelling setup retained the cache lock with no cleanup, so the retry failed with `Lock was retained after an interrupted operation` | | `887548d8` | Renaming a script dropped its environment association even though nothing about the environment changed | | `eaa71423` | Cancelling a build shared by two scripts reported cancellation to only one of them | | `22628e1f` | Selecting a script environment could leak into general workspace terminals | | `669cb8ed` | Editing packages on a shared environment silently broke other scripts | Two earlier commits on this branch (`9acf5a21`, `cd7d214a`) predate this work and are unrelated to the above. --- ### `0a21a2c1` — Discard cancelled environments instead of quarantining them Cancelling 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. `discardCacheEntry` deletes `.meta.json` and 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. `inspectCacheEntry` treats a missing sidecar as `stale`, and `writeMetaJson` is the only writer of that file — four call sites, all under the cache-entry lock (see the comment above `withCacheEntryLock`). A surviving installer writes into `site-packages` and 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: `setUpInlineScriptEnvironmentsInWorkspace` only 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 associations A 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. `PythonProjectManagerImpl` already follows renames via `updatePythonProjectSettingPath`, which rewrites the `pythonProjects` entry and preserves its `_inlineScriptRegistration` marker. 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 build Scripts whose dependencies normalize to the same list resolve to the same cache key, so a second request joins the in-flight build via `pendingCreations` instead 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 `PendingCreationContext` and 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 scope Shell-startup activation keeps a single activation command per workspace folder (for example `VSCODE_PYTHON_PWSH_ACTIVATE`), injected into every terminal opened there. `handleEnvironmentChange` wrote 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 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 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 edited Cache 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 `manuallyModified` in the sidecar and un-routes every associated script, so the CodeLens returns for each. `inspectCacheEntry` treats 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's `EventEmitter` being synchronous, so a build still registered in `pendingCreations` is recognised before any `await`; 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. --- --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 66a6bc6 commit df05e69

9 files changed

Lines changed: 894 additions & 67 deletions

File tree

src/common/inlineScript/cacheLayout.ts

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@ export interface InlineScriptEnvMeta {
4242
readonly baseInterpreterVersion: string;
4343
/** Last successful use as a canonical UTC string produced by `Date.toISOString()`. */
4444
readonly lastUsedAt: string;
45+
/** Set when packages changed outside setup; the entry no longer matches its declared dependencies. */
46+
readonly manuallyModified?: boolean;
4547
/** Bounded SHA-256 hashes of metadata identities proven for this cache entry. */
4648
readonly sourceMetadataIdentityHashes?: readonly string[];
4749
}
@@ -447,11 +449,7 @@ function validateMeta(value: unknown): InlineScriptEnvMeta | 'unsupported' | und
447449
return undefined;
448450
}
449451
const obj = value as Record<string, unknown>;
450-
if (
451-
typeof obj.schemaVersion !== 'number' ||
452-
!Number.isSafeInteger(obj.schemaVersion) ||
453-
obj.schemaVersion <= 0
454-
) {
452+
if (typeof obj.schemaVersion !== 'number' || !Number.isSafeInteger(obj.schemaVersion) || obj.schemaVersion <= 0) {
455453
return undefined;
456454
}
457455
if (obj.schemaVersion > META_SCHEMA_VERSION) {
@@ -469,6 +467,9 @@ function validateMeta(value: unknown): InlineScriptEnvMeta | 'unsupported' | und
469467
if (!isCanonicalIsoTimestamp(obj.lastUsedAt)) {
470468
return undefined;
471469
}
470+
if (obj.manuallyModified !== undefined && typeof obj.manuallyModified !== 'boolean') {
471+
return undefined;
472+
}
472473
const sourceMetadataIdentityHashes = validateSourceMetadataIdentityHashes(obj.sourceMetadataIdentityHashes);
473474
if (obj.sourceMetadataIdentityHashes !== undefined && sourceMetadataIdentityHashes === undefined) {
474475
return undefined;
@@ -479,6 +480,7 @@ function validateMeta(value: unknown): InlineScriptEnvMeta | 'unsupported' | und
479480
baseInterpreterPath: obj.baseInterpreterPath,
480481
baseInterpreterVersion: obj.baseInterpreterVersion,
481482
lastUsedAt: obj.lastUsedAt,
483+
...(obj.manuallyModified === true ? { manuallyModified: true } : {}),
482484
...(sourceMetadataIdentityHashes ? { sourceMetadataIdentityHashes } : {}),
483485
};
484486
}

src/common/inlineScript/routingRegistry.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,10 @@
33

44
import * as path from 'path';
55
import { Disposable, Event, EventEmitter, Uri } from 'vscode';
6+
import type { InlineScriptEnvErrorCategory } from '../telemetry/constants';
7+
import { normalizePath } from '../utils/pathUtils';
68
import { normalizeDependency } from './cacheKey';
79
import { InlineScriptMetadata } from './metadata';
8-
import { normalizePath } from '../utils/pathUtils';
9-
import type { InlineScriptEnvErrorCategory } from '../telemetry/constants';
1010

1111
export interface InlineScriptRouteabilityChangeEvent {
1212
readonly uri: Uri;
@@ -32,6 +32,7 @@ export type InlineScriptSetupOutcome =
3232
readonly category: InlineScriptEnvErrorCategory;
3333
readonly requiresPython?: string;
3434
}
35+
| { readonly kind: 'cancelled' }
3536
| { readonly kind: 'skipped' };
3637

3738
interface ScriptRoutingState {
@@ -150,6 +151,11 @@ export class InlineScriptRoutingRegistry implements Disposable {
150151
}
151152
}
152153

154+
public getSetupOutcome(script: Uri | string): InlineScriptSetupOutcome | undefined {
155+
const scriptPath = getInlineScriptRoutingKey(script);
156+
return scriptPath ? this.setupOutcomes.get(scriptPath) : undefined;
157+
}
158+
153159
public takeSetupOutcome(script: Uri | string): InlineScriptSetupOutcome | undefined {
154160
const scriptPath = getInlineScriptRoutingKey(script);
155161
if (!scriptPath) {

src/features/inlineScript/setupEnvironment.ts

Lines changed: 51 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,9 @@ export async function setUpInlineScriptEnvironment(
7575
return undefined;
7676
}
7777
await em.setEnvironment(scriptUri, environment);
78+
// Outcomes are read non-consumingly, so a success must retire the previous attempt's outcome
79+
// rather than relying on the next `create` to clear it on entry.
80+
routing.clearSetupOutcome(scriptUri);
7881
return environment;
7982
}
8083

@@ -124,12 +127,16 @@ function setupInlineScriptEnvironmentHandler(
124127
};
125128
}
126129

127-
function notifyInlineScriptSetupOutcome(uri: Uri, routing: InlineScriptRoutingRegistry): void {
128-
const outcome = routing.takeSetupOutcome(uri);
130+
export function notifyInlineScriptSetupOutcome(uri: Uri, routing: InlineScriptRoutingRegistry): void {
131+
const outcome = routing.getSetupOutcome(uri);
129132
if (outcome?.kind === 'skipped') {
130133
// Env built but intentionally not associated (metadata changed mid-setup); stay silent.
131134
return;
132135
}
136+
if (outcome?.kind === 'cancelled') {
137+
showInformationMessage(l10n.t('Environment setup was canceled.'));
138+
return;
139+
}
133140
if (outcome?.kind === 'failed') {
134141
if (outcome.category === 'compatible-python-declined') {
135142
// User declined the install prompt; don't nag.
@@ -223,16 +230,57 @@ export async function setUpInlineScriptEnvironmentsInWorkspace(
223230
return;
224231
}
225232
let succeeded = 0;
233+
let attempted = 0;
234+
let failed = 0;
235+
let cancelled = false;
226236
for (const pick of picks) {
237+
attempted += 1;
227238
try {
228239
if (await setUpInlineScriptEnvironment(pick.uri, em, routing)) {
229240
succeeded += 1;
241+
continue;
242+
}
243+
const outcome = routing.getSetupOutcome(pick.uri);
244+
if (outcome?.kind === 'cancelled') {
245+
// Cancelling one script's installer stops the whole run rather than immediately
246+
// starting the next script's install.
247+
cancelled = true;
248+
break;
249+
}
250+
if (outcome?.kind !== 'skipped') {
251+
failed += 1;
230252
}
231253
} catch (error) {
254+
failed += 1;
232255
traceError(`Failed to set up the inline-script environment for ${pick.uri.fsPath}:`, error);
233256
}
234257
}
235-
traceInfo(`Inline-script bulk setup: created or reused ${succeeded} of ${picks.length} environment(s).`);
258+
traceInfo(
259+
`Inline-script bulk setup: created or reused ${succeeded} of ${picks.length} environment(s)` +
260+
`${cancelled ? ' (canceled)' : ''}.`,
261+
);
262+
if (cancelled) {
263+
showWarningMessage(
264+
l10n.t(
265+
'Environment setup was canceled. Set up {0} of {1} selected inline script environment(s); the remaining {2} were not started.',
266+
succeeded,
267+
picks.length,
268+
picks.length - attempted,
269+
),
270+
);
271+
return;
272+
}
273+
if (failed > 0) {
274+
showWarningMessage(
275+
l10n.t(
276+
'Set up {0} of {1} selected inline script environment(s). {2} failed — see the Python Environments output for details.',
277+
succeeded,
278+
picks.length,
279+
failed,
280+
),
281+
);
282+
return;
283+
}
236284
showInformationMessage(l10n.t('Set up {0} of {1} selected inline script environment(s).', succeeded, picks.length));
237285
}
238286

src/features/terminal/shellStartupActivationVariablesManager.ts

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -48,21 +48,25 @@ export class ShellStartupActivationVariablesManagerImpl implements ShellStartupA
4848

4949
private async handleEnvironmentChange(e: DidChangeEnvironmentEventArgs) {
5050
const autoActType = getAutoActivationType();
51-
if (autoActType === ACT_TYPE_SHELL && e.uri) {
52-
const wf = getWorkspaceFolder(e.uri);
53-
if (wf) {
54-
const envVars = this.envCollection.getScoped({ workspaceFolder: wf });
55-
if (envVars) {
56-
this.shellEnvsProviders.forEach((provider) => {
57-
if (e.new) {
58-
provider.updateEnvVariables(envVars, e.new);
59-
} else {
60-
provider.removeEnvVariables(envVars);
61-
}
62-
});
63-
}
64-
}
51+
if (autoActType !== ACT_TYPE_SHELL || !e.uri) {
52+
return;
6553
}
54+
const wf = getWorkspaceFolder(e.uri);
55+
if (!wf) {
56+
return;
57+
}
58+
const envVars = this.envCollection.getScoped({ workspaceFolder: wf });
59+
if (!envVars) {
60+
return;
61+
}
62+
const folderEnvironment = await this.api.getEnvironment(wf.uri);
63+
this.shellEnvsProviders.forEach((provider) => {
64+
if (folderEnvironment) {
65+
provider.updateEnvVariables(envVars, folderEnvironment);
66+
} else {
67+
provider.removeEnvVariables(envVars);
68+
}
69+
});
6670
}
6771

6872
private async initializeInternal(): Promise<void> {

0 commit comments

Comments
 (0)