From ec685641dc2622f6b44bdcb9171ccf6296ba049c Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 8 Sep 2026 17:42:24 -0700 Subject: [PATCH 1/6] Discard cancelled inline-script environments instead of quarantining 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> --- src/common/inlineScript/routingRegistry.ts | 10 ++- src/features/inlineScript/setupEnvironment.ts | 51 ++++++++++- .../builtin/inlineScript/envManager.ts | 83 ++++++++++++----- .../setupEnvironment.unit.test.ts | 90 +++++++++++++++++++ .../inlineScript/envManager.unit.test.ts | 56 ++++++++---- 5 files changed, 247 insertions(+), 43 deletions(-) diff --git a/src/common/inlineScript/routingRegistry.ts b/src/common/inlineScript/routingRegistry.ts index 6387bf8f..de8ee940 100644 --- a/src/common/inlineScript/routingRegistry.ts +++ b/src/common/inlineScript/routingRegistry.ts @@ -3,10 +3,10 @@ import * as path from 'path'; import { Disposable, Event, EventEmitter, Uri } from 'vscode'; +import type { InlineScriptEnvErrorCategory } from '../telemetry/constants'; +import { normalizePath } from '../utils/pathUtils'; import { normalizeDependency } from './cacheKey'; import { InlineScriptMetadata } from './metadata'; -import { normalizePath } from '../utils/pathUtils'; -import type { InlineScriptEnvErrorCategory } from '../telemetry/constants'; export interface InlineScriptRouteabilityChangeEvent { readonly uri: Uri; @@ -32,6 +32,7 @@ export type InlineScriptSetupOutcome = readonly category: InlineScriptEnvErrorCategory; readonly requiresPython?: string; } + | { readonly kind: 'cancelled' } | { readonly kind: 'skipped' }; interface ScriptRoutingState { @@ -150,6 +151,11 @@ export class InlineScriptRoutingRegistry implements Disposable { } } + public getSetupOutcome(script: Uri | string): InlineScriptSetupOutcome | undefined { + const scriptPath = getInlineScriptRoutingKey(script); + return scriptPath ? this.setupOutcomes.get(scriptPath) : undefined; + } + public takeSetupOutcome(script: Uri | string): InlineScriptSetupOutcome | undefined { const scriptPath = getInlineScriptRoutingKey(script); if (!scriptPath) { diff --git a/src/features/inlineScript/setupEnvironment.ts b/src/features/inlineScript/setupEnvironment.ts index 0088864d..bebad836 100644 --- a/src/features/inlineScript/setupEnvironment.ts +++ b/src/features/inlineScript/setupEnvironment.ts @@ -124,12 +124,16 @@ function setupInlineScriptEnvironmentHandler( }; } -function notifyInlineScriptSetupOutcome(uri: Uri, routing: InlineScriptRoutingRegistry): void { - const outcome = routing.takeSetupOutcome(uri); +export function notifyInlineScriptSetupOutcome(uri: Uri, routing: InlineScriptRoutingRegistry): void { + const outcome = routing.getSetupOutcome(uri); if (outcome?.kind === 'skipped') { // Env built but intentionally not associated (metadata changed mid-setup); stay silent. return; } + if (outcome?.kind === 'cancelled') { + showInformationMessage(l10n.t('Environment setup was canceled.')); + return; + } if (outcome?.kind === 'failed') { if (outcome.category === 'compatible-python-declined') { // User declined the install prompt; don't nag. @@ -223,16 +227,57 @@ export async function setUpInlineScriptEnvironmentsInWorkspace( return; } let succeeded = 0; + let attempted = 0; + let failed = 0; + let cancelled = false; for (const pick of picks) { + attempted += 1; try { if (await setUpInlineScriptEnvironment(pick.uri, em, routing)) { succeeded += 1; + continue; + } + const outcome = routing.getSetupOutcome(pick.uri); + if (outcome?.kind === 'cancelled') { + // Cancelling one script's installer stops the whole run rather than immediately + // starting the next script's install. + cancelled = true; + break; + } + if (outcome?.kind !== 'skipped') { + failed += 1; } } catch (error) { + failed += 1; traceError(`Failed to set up the inline-script environment for ${pick.uri.fsPath}:`, error); } } - traceInfo(`Inline-script bulk setup: created or reused ${succeeded} of ${picks.length} environment(s).`); + traceInfo( + `Inline-script bulk setup: created or reused ${succeeded} of ${picks.length} environment(s)` + + `${cancelled ? ' (canceled)' : ''}.`, + ); + if (cancelled) { + showWarningMessage( + l10n.t( + 'Environment setup was canceled. Set up {0} of {1} selected inline script environment(s); the remaining {2} were not started.', + succeeded, + picks.length, + picks.length - attempted, + ), + ); + return; + } + if (failed > 0) { + showWarningMessage( + l10n.t( + 'Set up {0} of {1} selected inline script environment(s). {2} failed — see the Python Environments output for details.', + succeeded, + picks.length, + failed, + ), + ); + return; + } showInformationMessage(l10n.t('Set up {0} of {1} selected inline script environment(s).', succeeded, picks.length)); } diff --git a/src/managers/builtin/inlineScript/envManager.ts b/src/managers/builtin/inlineScript/envManager.ts index 35a51d03..5b426d3f 100644 --- a/src/managers/builtin/inlineScript/envManager.ts +++ b/src/managers/builtin/inlineScript/envManager.ts @@ -51,6 +51,7 @@ import { inspectMetaJson, inspectOwnedCacheEntry, mergeSourceMetadataIdentityHashes, + META_JSON_FILENAME, META_SCHEMA_VERSION, resolveCacheEntryPath, restoreMetaJsonBackupUnderLock, @@ -74,6 +75,7 @@ import { } from '../../../common/lockfile.apis'; import { EventNames, InlineScriptEnvErrorCategory } from '../../../common/telemetry/constants'; import { sendTelemetryEvent } from '../../../common/telemetry/sender'; +import { timeout } from '../../../common/utils/asyncUtils'; import { createDeferred, Deferred } from '../../../common/utils/deferred'; import { isFileNotFoundError } from '../../../common/utils/filesystem'; import { normalizePath } from '../../../common/utils/pathUtils'; @@ -96,6 +98,9 @@ const CACHE_TTL_MS = 14 * 24 * 60 * 60 * 1000; const CACHED_ASSOCIATION_VALIDATION_INTERVAL_MS = 5_000; const DISCOVERY_RETRY_DELAYS_MS = [1_000, 5_000, 30_000] as const; const PERSISTED_ASSOCIATION_SCHEMA_VERSION = 1 as const; +/** Bounded retry for deleting a cache entry whose files may still be briefly held by a stopped installer. */ +const CACHE_ENTRY_REMOVAL_ATTEMPTS = 4; +const CACHE_ENTRY_REMOVAL_RETRY_MS = 150; interface SelectedBaseInterpreter { readonly environment: PythonEnvironment; @@ -113,7 +118,6 @@ interface CreateOrReuseEnvironmentOptions { interface BuildCacheEntryResult { readonly environment?: PythonEnvironment; - readonly retainLock?: boolean; readonly errorCategory?: InlineScriptEnvErrorCategory; } @@ -2658,7 +2662,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { try { await fs.ensureDir(cacheRoot.fsPath); - return await this.withCacheEntryLock(envDir, async (lock) => { + return await this.withCacheEntryLock(envDir, async () => { const cached = await this.inspectCacheEntry(cacheRoot, envDir, metadata, selectedBase, pendingCreation); if (cached.kind === 'reusable') { this.sendInlineScriptEnvReuseHitTelemetry(dependencyCount); @@ -2672,7 +2676,9 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return undefined; } if (cached.kind === 'stale') { - if (!(await this.removeCacheEntry(envDir))) { + if (!(await this.discardCacheEntry(envDir))) { + // The sidecar is gone, so the entry is inert; a later attempt retries the + // deletion once the files are released. this.sendInlineScriptEnvErrorTelemetry('setup-failure'); return undefined; } @@ -2687,15 +2693,6 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { pendingCreation, scriptUri, ); - if (build.retainLock) { - try { - await lock.retain(); - } catch (error) { - this.log.error( - `Failed to mark the inline-script cache lock as retained: ${getErrorMessage(error)}`, - ); - } - } if (build.environment) { this.sendInlineScriptEnvCreatedTelemetry(buildStartAtMs, dependencyCount); return build.environment; @@ -2847,10 +2844,12 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { } if (result?.pkgInstallationCancelled) { - this.log.warn( - 'Inline-script package installation was cancelled; retaining the cache lock until explicit cleanup.', + this.log.info( + 'Inline-script package installation was cancelled; discarding the incomplete environment.', ); - return { retainLock: true, errorCategory: 'package-install-cancelled' }; + await this.discardCacheEntry(envDir); + this.routingRegistry.noteSetupOutcome(scriptUri, { kind: 'cancelled' }); + return { errorCategory: 'package-install-cancelled' }; } if (!result?.environment || result.envCreationErr || result.pkgInstallationErr) { const error = @@ -3484,14 +3483,58 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return new Map(Array.from(scriptPaths, (scriptPath) => [scriptPath, this.fsPathToEnv.get(scriptPath)])); } - private async removeCacheEntry(envDir: Uri): Promise { + /** + * Make a cache entry permanently unreusable, then best-effort delete it. + * + * The sidecar is removed first and is the correctness guarantee: `inspectCacheEntry` treats a + * missing/invalid `.meta.json` 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 + * briefly because a just-stopped installer can hold file handles for a short while, + * especially on Windows. + */ + private async discardCacheEntry(envDir: Uri): Promise { + await this.removeCacheEntrySidecars(envDir); + return this.removeCacheEntry(envDir); + } + + /** Delete `.meta.json` and any backup sidecars so a surviving directory cannot be revalidated. */ + private async removeCacheEntrySidecars(envDir: Uri): Promise { + let entries: string[]; try { - await fs.remove(envDir.fsPath); - return true; + entries = await fs.readdir(envDir.fsPath); } catch (error) { - this.log.error(`Failed to remove incomplete inline-script environment: ${getErrorMessage(error)}`); - return false; + if (!isFileNotFoundError(error)) { + this.log.warn(`Failed to scan inline-script cache sidecars: ${getErrorMessage(error)}`); + } + return; + } + const sidecars = entries.filter( + (entry) => entry === META_JSON_FILENAME || entry.startsWith(`${META_JSON_FILENAME}.backup-`), + ); + for (const sidecar of sidecars) { + try { + await fs.remove(path.join(envDir.fsPath, sidecar)); + } catch (error) { + this.log.warn(`Failed to remove inline-script cache sidecar ${sidecar}: ${getErrorMessage(error)}`); + } + } + } + + private async removeCacheEntry(envDir: Uri): Promise { + let lastError: unknown; + for (let attempt = 0; attempt < CACHE_ENTRY_REMOVAL_ATTEMPTS; attempt += 1) { + try { + await fs.remove(envDir.fsPath); + return true; + } catch (error) { + lastError = error; + if (attempt < CACHE_ENTRY_REMOVAL_ATTEMPTS - 1) { + await timeout(CACHE_ENTRY_REMOVAL_RETRY_MS * (attempt + 1)); + } + } } + this.log.error(`Failed to remove incomplete inline-script environment: ${getErrorMessage(lastError)}`); + return false; } private isDefinitivelyStalePathError(error: unknown): boolean { diff --git a/src/test/features/inlineScript/setupEnvironment.unit.test.ts b/src/test/features/inlineScript/setupEnvironment.unit.test.ts index 4a16c5c5..73ac5867 100644 --- a/src/test/features/inlineScript/setupEnvironment.unit.test.ts +++ b/src/test/features/inlineScript/setupEnvironment.unit.test.ts @@ -13,6 +13,7 @@ import { InlineScriptRoutingRegistry } from '../../../common/inlineScript/routin import * as winapi from '../../../common/window.apis'; import * as wapi from '../../../common/workspace.apis'; import { + notifyInlineScriptSetupOutcome, setUpInlineScriptEnvironment, setUpInlineScriptEnvironmentsInWorkspace, } from '../../../features/inlineScript/setupEnvironment'; @@ -209,4 +210,93 @@ suite('setUpInlineScriptEnvironmentsInWorkspace', () => { manager.verify((m) => m.create(withoutMeta, undefined), typemoq.Times.never()); em.verify((m) => m.setEnvironment(withMeta, env), typemoq.Times.once()); }); + + test('stops the whole run and reports when a script setup is cancelled', async () => { + const warningStub = sinon.stub(winapi, 'showWarningMessage').resolves(undefined); + // The picker sorts by label, so `a_` runs before `z_`. + const first = Uri.file('/workspace/a_first.py'); + const second = Uri.file('/workspace/z_second.py'); + findFilesStub.resolves([first, second]); + readMetadataStub.resolves(makeMetadata(['requests'])); + quickPickStub.callsFake((items) => items); + manager + .setup((m) => m.create(first, undefined)) + .returns(async () => { + routing.noteSetupOutcome(first, { kind: 'cancelled' }); + return undefined; + }); + + await setUpInlineScriptEnvironmentsInWorkspace(em.object, routing); + + manager.verify((m) => m.create(first, undefined), typemoq.Times.once()); + manager.verify((m) => m.create(second, undefined), typemoq.Times.never()); + assert.match(warningStub.firstCall.args[0], /canceled/i); + sinon.assert.notCalled(infoStub); + }); + + test('reports failures instead of silently counting them as successes', async () => { + const warningStub = sinon.stub(winapi, 'showWarningMessage').resolves(undefined); + findFilesStub.resolves([withMeta]); + readMetadataStub.resolves(makeMetadata(['requests'])); + quickPickStub.callsFake((items) => items); + manager + .setup((m) => m.create(withMeta, undefined)) + .returns(async () => { + routing.noteSetupOutcome(withMeta, { kind: 'failed', category: 'install-failure' }); + return undefined; + }); + + await setUpInlineScriptEnvironmentsInWorkspace(em.object, routing); + + assert.match(warningStub.firstCall.args[0], /failed/i); + sinon.assert.notCalled(infoStub); + }); +}); + +suite('notifyInlineScriptSetupOutcome', () => { + const scriptUri = Uri.file('/workspace/script.py'); + let routing: InlineScriptRoutingRegistry; + + setup(() => { + routing = new InlineScriptRoutingRegistry(); + }); + + teardown(() => { + routing.dispose(); + sinon.restore(); + }); + + test('reports cancellation as information, not a failure', () => { + const infoStub = sinon.stub(winapi, 'showInformationMessage').resolves(undefined); + const errorStub = sinon.stub(winapi, 'showErrorMessage').resolves(undefined); + const warningStub = sinon.stub(winapi, 'showWarningMessage').resolves(undefined); + routing.noteSetupOutcome(scriptUri, { kind: 'cancelled' }); + + notifyInlineScriptSetupOutcome(scriptUri, routing); + + assert.match(infoStub.firstCall.args[0], /setup was canceled/i); + sinon.assert.notCalled(errorStub); + sinon.assert.notCalled(warningStub); + }); + + test('never asks the user to clean anything up after a cancellation', () => { + const infoStub = sinon.stub(winapi, 'showInformationMessage').resolves(undefined); + routing.noteSetupOutcome(scriptUri, { kind: 'cancelled' }); + + notifyInlineScriptSetupOutcome(scriptUri, routing); + + assert.doesNotMatch(infoStub.firstCall.args[0], /clean up|quarantin|retry|lock/i); + }); + + test('lets coalesced setup callers observe the same outcome', () => { + const infoStub = sinon.stub(winapi, 'showInformationMessage').resolves(undefined); + const errorStub = sinon.stub(winapi, 'showErrorMessage').resolves(undefined); + routing.noteSetupOutcome(scriptUri, { kind: 'cancelled' }); + + notifyInlineScriptSetupOutcome(scriptUri, routing); + notifyInlineScriptSetupOutcome(scriptUri, routing); + + sinon.assert.calledTwice(infoStub); + sinon.assert.notCalled(errorStub); + }); }); diff --git a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts index 7d8c71e2..c3e2d941 100644 --- a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts +++ b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts @@ -2301,7 +2301,8 @@ suite('InlineScriptEnvManager', () => { }); suite('transaction rollback', () => { - test('retains the partial environment and lock when package installation is cancelled', async () => { + test('discards the partial environment when package installation is cancelled', async () => { + const uri = scriptUri(); createWithProgressStub.callsFake(async (...args: unknown[]) => { const target = args[6] as string; await fs.outputFile(venvPythonPath(target), ''); @@ -2317,29 +2318,48 @@ suite('InlineScriptEnvManager', () => { }; }); - assert.strictEqual(await manager.create(scriptUri()), undefined); - assert.strictEqual(await fs.pathExists(envDir().fsPath), true); + assert.strictEqual(await manager.create(uri), undefined); + assert.strictEqual(await fs.pathExists(envDir().fsPath), false); assert.strictEqual(writeMetaStub.callCount, 0); - assert.ok(retainLockStub.calledOnce); + sinon.assert.notCalled(retainLockStub); assert.ok(releaseLockStub.calledOnce); + assert.deepStrictEqual(routingRegistry.takeSetupOutcome(uri), { kind: 'cancelled' }); }); - test('keeps a failed lock-retain transition fail-closed', async () => { - createWithProgressStub.resolves({ - environment: makeEnvironment( - 'ms-python.python:inline-script', - '3.12.4', - venvPythonPath(envDir().fsPath), - envDir().fsPath, - ), - pkgInstallationErr: 'Canceled', - pkgInstallationCancelled: true, + test('leaves a cancelled entry unreusable when its directory cannot be removed', async () => { + const uri = scriptUri(); + createWithProgressStub.callsFake(async (...args: unknown[]) => { + const target = args[6] as string; + await fs.outputFile(venvPythonPath(target), ''); + await fs.outputFile(path.join(target, '.meta.json'), '{}'); + await fs.outputFile(path.join(target, '.meta.json.backup-0123456789ab'), '{}'); + return { + environment: makeEnvironment( + 'ms-python.python:inline-script', + '3.12.4', + venvPythonPath(target), + target, + ), + pkgInstallationErr: 'Canceled', + pkgInstallationCancelled: true, + }; }); - retainLockStub.rejects(Object.assign(new Error('retention failed'), { code: 'EACCES' })); + const internalManager = manager as unknown as { + removeCacheEntry(candidate: Uri): Promise; + }; + sinon.stub(internalManager, 'removeCacheEntry').resolves(false); - assert.strictEqual(await manager.create(scriptUri()), undefined); - assert.ok(retainLockStub.calledOnce); - assert.ok(releaseLockStub.calledOnce); + assert.strictEqual(await manager.create(uri), undefined); + + // The directory survives, but every sidecar is gone so it can never be revalidated. + assert.strictEqual(await fs.pathExists(envDir().fsPath), true); + assert.strictEqual(await fs.pathExists(path.join(envDir().fsPath, '.meta.json')), false); + assert.strictEqual( + await fs.pathExists(path.join(envDir().fsPath, '.meta.json.backup-0123456789ab')), + false, + ); + sinon.assert.notCalled(retainLockStub); + assert.deepStrictEqual(routingRegistry.takeSetupOutcome(uri), { kind: 'cancelled' }); }); test('removes the partial environment when package installation fails', async () => { From 4088e386a75616d553c9e283a52102759c6ca2a2 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 8 Sep 2026 18:14:43 -0700 Subject: [PATCH 2/6] Follow renames instead of dropping inline-script associations 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> --- .../builtin/inlineScript/envManager.ts | 129 +++++++++++++++++- .../inlineScript/envManager.unit.test.ts | 42 +++++- 2 files changed, 168 insertions(+), 3 deletions(-) diff --git a/src/managers/builtin/inlineScript/envManager.ts b/src/managers/builtin/inlineScript/envManager.ts index 5b426d3f..c2d83df4 100644 --- a/src/managers/builtin/inlineScript/envManager.ts +++ b/src/managers/builtin/inlineScript/envManager.ts @@ -62,6 +62,7 @@ import { extractLowerBoundVersion, pickCompatibleInterpreter } from '../../../co import { InlineScriptMetadata, readInlineScriptMetadataFromFile } from '../../../common/inlineScript/metadata'; import { getInlineScriptMetadataRoutingIdentity, + getInlineScriptRoutingKey, InlineScriptMetadataChangeEvent, InlineScriptRoutingRegistry, } from '../../../common/inlineScript/routingRegistry'; @@ -260,9 +261,9 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { }); }), onDidRenameFiles((event) => { - void this.clearAssociationsForScripts(event.files.map((file) => file.oldUri)).catch((error) => { + void this.handleRenamedScripts(event.files).catch((error) => { this.log.warn( - `Failed to clear inline-script associations for renamed files: ${getErrorMessage(error)}`, + `Failed to update inline-script associations for renamed files: ${getErrorMessage(error)}`, ); }); }), @@ -2232,6 +2233,121 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return run; } + /** + * Follow a rename instead of dropping the association. + * + * A cache entry is keyed by the script's dependencies and base interpreter, never by its path, + * so renaming a script does not invalidate its environment. `PythonProjectManagerImpl` already + * rewrites the matching `python-envs.pythonProjects` entry to the new path, so clearing the + * association here would leave a managed inline-script project entry with no environment behind + * it. + * + * The moved record is re-validated afterwards rather than trusted: its 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. + */ + private async handleRenamedScripts( + files: readonly { readonly oldUri: Uri; readonly newUri: Uri }[], + ): Promise { + const transfers = await this.enqueueSelection(async () => { + await this.persistedAssociationsLoaded; + + const moves: RenamedScriptTransfer[] = []; + const clears: ScriptReference[] = []; + const seen = new Set(); + for (const { oldUri, newUri } of files) { + if (oldUri.scheme !== 'file') { + continue; + } + const oldPath = normalizePath(oldUri.fsPath); + if (seen.has(oldPath)) { + continue; + } + seen.add(oldPath); + if (!this.fsPathToEnv.has(oldPath) && !this.fsPathToPersistedAssociation.has(oldPath)) { + continue; + } + const record = this.fsPathToPersistedAssociation.get(oldPath); + // Only follow the rename when the destination is still a routable local `.py` file; + // renaming to another extension (or off the local filesystem) drops the association. + const newPath = getInlineScriptRoutingKey(newUri); + if (!record || newPath === undefined || newPath === oldPath) { + clears.push({ uri: oldUri, scriptPath: oldPath }); + continue; + } + moves.push({ + oldUri, + oldPath, + newUri, + newPath, + record, + environment: this.fsPathToEnv.get(oldPath), + }); + } + + if (moves.length === 0 && clears.length === 0) { + return []; + } + + await this.updatePersistedAssociations([ + ...clears.map(({ scriptPath }) => ({ scriptPath })), + // Remove the old key and write the new one in the same transaction. Writing the new + // key unconditionally means a rename onto an already-associated script replaces it. + ...moves.flatMap((move) => [ + { scriptPath: move.oldPath, expectedPersistedAssociation: move.record }, + { scriptPath: move.newPath, persistedAssociation: move.record }, + ]), + ]); + + for (const clear of clears) { + this.forgetScriptAssociationState(clear.scriptPath); + this.clearValidatedRouteableState(clear.uri); + } + for (const move of moves) { + this.forgetScriptAssociationState(move.oldPath); + this.clearValidatedRouteableState(move.oldUri); + // Reset validation bookkeeping for the destination without discarding the record + // `updatePersistedAssociations` just wrote for it. + this.bumpAssociationRevision(move.newPath); + this.pendingRehydrations.delete(move.newPath); + this.pendingMetadataRefreshes.delete(move.newPath); + if (move.environment) { + this.fsPathToEnv.set(move.newPath, move.environment); + } else { + this.fsPathToEnv.delete(move.newPath); + } + this.clearValidatedRouteableState(move.newUri); + this.log.info(`Moved inline-script association from ${move.oldPath} to ${move.newPath}.`); + } + return moves; + }); + + // Re-validate outside the selection queue, mirroring how activation primes associations. + for (const move of transfers) { + await this.seedRoutingMetadataFromSavedFile(move.newUri, move.newPath); + const metadata = this.routingRegistry.getMetadata(move.newPath); + const metadataIdentity = metadata ? getInlineScriptMetadataRoutingIdentity(metadata) : undefined; + if (!metadata || !metadataIdentity) { + continue; + } + const uri = this.routingRegistry.getUri(move.newPath) ?? move.newUri; + await this.refreshValidatedAssociationForMetadata( + uri, + metadata, + metadataIdentity, + this.routingRegistry.getMetadataRevision(uri), + ); + } + } + + private forgetScriptAssociationState(scriptPath: string): void { + this.bumpAssociationRevision(scriptPath); + this.pendingRehydrations.delete(scriptPath); + this.pendingMetadataRefreshes.delete(scriptPath); + this.fsPathToEnv.delete(scriptPath); + this.fsPathToPersistedAssociation.delete(scriptPath); + } + private clearAssociationsForScripts(scripts: readonly Uri[]): Promise { return this.enqueueSelection(async () => { await this.persistedAssociationsLoaded; @@ -3640,6 +3756,15 @@ interface ScriptReference { readonly scriptPath: string; } +interface RenamedScriptTransfer { + readonly oldUri: Uri; + readonly oldPath: string; + readonly newUri: Uri; + readonly newPath: string; + readonly record: PersistedAssociationRecord; + readonly environment: PythonEnvironment | undefined; +} + interface PendingScriptUpdate extends ScriptReference { readonly before: PythonEnvironment | undefined; readonly persistedAssociation?: PersistedAssociationRecord; diff --git a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts index c3e2d941..d1354a8d 100644 --- a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts +++ b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts @@ -5230,7 +5230,7 @@ suite('InlineScriptEnvManager', () => { assert.strictEqual(routingRegistry.hasValidatedAssociation(uri), false); }); - test('clears persisted association state for the old path when a script is renamed', async () => { + test('moves the persisted association to the new path when a script is renamed', async () => { const oldUri = scriptUri('old.py'); const newUri = scriptUri('new.py'); const environment = await createOwnedEnvironment(); @@ -5240,11 +5240,51 @@ suite('InlineScriptEnvManager', () => { await nextTurn(); await nextTurn(); + // The cache entry is keyed by dependencies + interpreter, not by path, so the + // environment survives the rename and follows the file. + assert.deepStrictEqual(Object.keys(persistedAssociations ?? {}), [normalizePath(newUri.fsPath)]); + assert.strictEqual(await manager.get(oldUri), undefined); + assert.strictEqual(routingRegistry.hasValidatedAssociation(oldUri), false); + assert.strictEqual(await manager.get(newUri), environment); + }); + + test('drops the association when a script is renamed to a non-python file', async () => { + const oldUri = scriptUri('old.py'); + const newUri = scriptUri('old.txt'); + const environment = await createOwnedEnvironment(); + await manager.set(oldUri, environment); + + fireRename(oldUri, newUri); + await nextTurn(); + await nextTurn(); + assert.deepStrictEqual(persistedAssociations, {}); assert.strictEqual(await manager.get(oldUri), undefined); assert.strictEqual(routingRegistry.hasValidatedAssociation(oldUri), false); }); + test('replaces an existing association when a script is renamed onto it', async () => { + const oldUri = scriptUri('old.py'); + const targetUri = scriptUri('target.py'); + const movedEnvironment = await createOwnedEnvironment(); + const replacedEnvironment = await createOwnedEnvironment('bbbbbbbbbbbbbbbb'); + await manager.set(targetUri, replacedEnvironment); + await manager.set(oldUri, movedEnvironment); + + fireRename(oldUri, targetUri); + await nextTurn(); + await nextTurn(); + + assert.deepStrictEqual(Object.keys(persistedAssociations ?? {}), [normalizePath(targetUri.fsPath)]); + const moved = (persistedAssociations as Record)[ + normalizePath(targetUri.fsPath) + ]; + assert.strictEqual( + normalizePath(moved.environmentPath), + normalizePath(movedEnvironment.environmentPath.fsPath), + ); + }); + test('removes and notifies for a warm association whose executable was deleted', async () => { const uri = scriptUri(); const environment = await createOwnedEnvironment(); From b9fb08fac8543710b510187516efbe065aafe481 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Wed, 9 Sep 2026 12:20:10 -0700 Subject: [PATCH 3/6] Report inline-script cancellation to every script sharing the build 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> --- .../builtin/inlineScript/envManager.ts | 51 ++++++++++++++++++- .../inlineScript/envManager.unit.test.ts | 46 +++++++++++++++++ 2 files changed, 95 insertions(+), 2 deletions(-) diff --git a/src/managers/builtin/inlineScript/envManager.ts b/src/managers/builtin/inlineScript/envManager.ts index c2d83df4..4ca350b8 100644 --- a/src/managers/builtin/inlineScript/envManager.ts +++ b/src/managers/builtin/inlineScript/envManager.ts @@ -147,6 +147,15 @@ interface PendingCreationContext { sourceMetadataIdentityHashes?: readonly string[]; hasStartedRecordingSourceMetadataIdentityHashes: boolean; recordedSourceMetadataIdentityHashes?: readonly string[]; + /** + * Outcome of the shared build, recorded once and read by every caller joined to it. Cancelling + * a build that several scripts joined must report cancellation to all of them, not just to the + * script that happened to start it. + */ + failure?: { + readonly category: InlineScriptEnvErrorCategory; + readonly cancelled?: boolean; + }; } interface MergeCacheEntrySourceMetadataIdentityHashResult { @@ -367,6 +376,9 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { pending.hasStartedRecordingSourceMetadataIdentityHashes; this.addPendingCreationSourceMetadataIdentityHash(pending, sourceMetadataIdentityHash); const environment = await pending.promise; + if (!environment) { + this.notePendingCreationFailure(scriptUri, metadata, pending); + } return await this.finalizeCreateForScript( cacheKey, environment, @@ -392,6 +404,9 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { this.pendingCreations.set(cacheKey, pendingCreation); try { const environment = await creation; + if (!environment) { + this.notePendingCreationFailure(scriptUri, metadata, pendingCreation); + } return await this.finalizeCreateForScript( cacheKey, environment, @@ -406,6 +421,31 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { } } + /** + * Translate the shared build's recorded failure into a routing outcome for one caller. Called + * for every script joined to the build so a cancellation is reported as a cancellation to all + * of them rather than surfacing as a generic failure for the joiners. + */ + private notePendingCreationFailure( + scriptUri: Uri, + metadata: InlineScriptMetadata, + pendingCreation: PendingCreationContext, + ): void { + const failure = pendingCreation.failure; + if (!failure) { + return; + } + if (failure.cancelled) { + this.routingRegistry.noteSetupOutcome(scriptUri, { kind: 'cancelled' }); + return; + } + this.routingRegistry.noteSetupOutcome(scriptUri, { + kind: 'failed', + category: failure.category, + requiresPython: metadata.requiresPython, + }); + } + private getPendingSetupKey( scriptUri: Uri, metadata: InlineScriptMetadata, @@ -2788,6 +2828,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { this.log.warn( `Preserving an inline-script cache entry that could not be safely inspected: ${envDir.fsPath}`, ); + pendingCreation.failure = { category: 'setup-failure' }; this.sendInlineScriptEnvErrorTelemetry('setup-failure'); return undefined; } @@ -2795,6 +2836,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { if (!(await this.discardCacheEntry(envDir))) { // The sidecar is gone, so the entry is inert; a later attempt retries the // deletion once the files are released. + pendingCreation.failure = { category: 'setup-failure' }; this.sendInlineScriptEnvErrorTelemetry('setup-failure'); return undefined; } @@ -2814,12 +2856,18 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return build.environment; } if (build.errorCategory) { + pendingCreation.failure = { + category: build.errorCategory, + ...(build.errorCategory === 'package-install-cancelled' ? { cancelled: true } : {}), + }; this.sendInlineScriptEnvErrorTelemetry(build.errorCategory); } return undefined; }); } catch (error) { - this.sendInlineScriptEnvErrorTelemetry(this.getCreateOrReuseErrorCategory(error)); + const category = this.getCreateOrReuseErrorCategory(error); + pendingCreation.failure = { category }; + this.sendInlineScriptEnvErrorTelemetry(category); this.log.error(`Failed to create or reuse inline-script cache entry: ${getErrorMessage(error)}`); return undefined; } @@ -2964,7 +3012,6 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { 'Inline-script package installation was cancelled; discarding the incomplete environment.', ); await this.discardCacheEntry(envDir); - this.routingRegistry.noteSetupOutcome(scriptUri, { kind: 'cancelled' }); return { errorCategory: 'package-install-cancelled' }; } if (!result?.environment || result.envCreationErr || result.pkgInstallationErr) { diff --git a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts index d1354a8d..614f2699 100644 --- a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts +++ b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts @@ -2362,6 +2362,52 @@ suite('InlineScriptEnvManager', () => { assert.deepStrictEqual(routingRegistry.takeSetupOutcome(uri), { kind: 'cancelled' }); }); + test('reports cancellation to every script joined to the same build', async () => { + // Two scripts with identical dependencies resolve to one cache entry and therefore join + // a single in-flight build. Cancelling it must be reported as a cancellation to both, + // not just to the script that started it. + const starter = scriptUri('shared_starter.py'); + const joiner = scriptUri('shared_joiner.py'); + let notifyBuildStarted: () => void = () => undefined; + let releaseBuild: () => void = () => undefined; + const buildStarted = new Promise((resolve) => { + notifyBuildStarted = resolve; + }); + const buildGate = new Promise((resolve) => { + releaseBuild = resolve; + }); + createWithProgressStub.callsFake(async (...args: unknown[]) => { + const target = args[6] as string; + notifyBuildStarted(); + await buildGate; + await fs.outputFile(venvPythonPath(target), ''); + return { + environment: makeEnvironment( + 'ms-python.python:inline-script', + '3.12.4', + venvPythonPath(target), + target, + ), + pkgInstallationErr: 'Canceled', + pkgInstallationCancelled: true, + }; + }); + + const starterCreate = manager.create(starter); + await buildStarted; + const joinerCreate = manager.create(joiner); + // Let the second request reach the shared pending creation before the build settles. + await nextTurn(); + await nextTurn(); + releaseBuild(); + + assert.strictEqual(await starterCreate, undefined); + assert.strictEqual(await joinerCreate, undefined); + assert.strictEqual(createWithProgressStub.callCount, 1, 'both scripts should share one build'); + assert.deepStrictEqual(routingRegistry.takeSetupOutcome(starter), { kind: 'cancelled' }); + assert.deepStrictEqual(routingRegistry.takeSetupOutcome(joiner), { kind: 'cancelled' }); + }); + test('removes the partial environment when package installation fails', async () => { createWithProgressStub.callsFake(async (...args: unknown[]) => { const target = args[6] as string; From 714c16c4cc38b4d4b45a5ccb978006fd06037036 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Wed, 9 Sep 2026 12:43:04 -0700 Subject: [PATCH 4/6] Resolve shell startup variables at folder scope, not from the event payload 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> --- .../shellStartupActivationVariablesManager.ts | 32 ++-- ...tupActivationVariablesManager.unit.test.ts | 142 ++++++++++++++++++ 2 files changed, 160 insertions(+), 14 deletions(-) create mode 100644 src/test/features/terminal/shellStartupActivationVariablesManager.unit.test.ts diff --git a/src/features/terminal/shellStartupActivationVariablesManager.ts b/src/features/terminal/shellStartupActivationVariablesManager.ts index 3ea63a09..317b9ff7 100644 --- a/src/features/terminal/shellStartupActivationVariablesManager.ts +++ b/src/features/terminal/shellStartupActivationVariablesManager.ts @@ -48,21 +48,25 @@ export class ShellStartupActivationVariablesManagerImpl implements ShellStartupA private async handleEnvironmentChange(e: DidChangeEnvironmentEventArgs) { const autoActType = getAutoActivationType(); - if (autoActType === ACT_TYPE_SHELL && e.uri) { - const wf = getWorkspaceFolder(e.uri); - if (wf) { - const envVars = this.envCollection.getScoped({ workspaceFolder: wf }); - if (envVars) { - this.shellEnvsProviders.forEach((provider) => { - if (e.new) { - provider.updateEnvVariables(envVars, e.new); - } else { - provider.removeEnvVariables(envVars); - } - }); - } - } + if (autoActType !== ACT_TYPE_SHELL || !e.uri) { + return; } + const wf = getWorkspaceFolder(e.uri); + if (!wf) { + return; + } + const envVars = this.envCollection.getScoped({ workspaceFolder: wf }); + if (!envVars) { + return; + } + const folderEnvironment = await this.api.getEnvironment(wf.uri); + this.shellEnvsProviders.forEach((provider) => { + if (folderEnvironment) { + provider.updateEnvVariables(envVars, folderEnvironment); + } else { + provider.removeEnvVariables(envVars); + } + }); } private async initializeInternal(): Promise { diff --git a/src/test/features/terminal/shellStartupActivationVariablesManager.unit.test.ts b/src/test/features/terminal/shellStartupActivationVariablesManager.unit.test.ts new file mode 100644 index 00000000..9ff6b1cd --- /dev/null +++ b/src/test/features/terminal/shellStartupActivationVariablesManager.unit.test.ts @@ -0,0 +1,142 @@ +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import { Disposable, GlobalEnvironmentVariableCollection, Uri, WorkspaceFolder } from 'vscode'; + +import { DidChangeEnvironmentEventArgs, PythonEnvironment, PythonProjectEnvironmentApi } from '../../../api'; +import * as workspaceApis from '../../../common/workspace.apis'; +import { ShellStartupActivationVariablesManagerImpl } from '../../../features/terminal/shellStartupActivationVariablesManager'; +import { ShellEnvsProvider } from '../../../features/terminal/shells/startupProvider'; +import * as terminalUtils from '../../../features/terminal/utils'; + +function makeEnvironment(id: string, managerId: string): PythonEnvironment { + return { + envId: { id, managerId }, + name: id, + displayName: id, + displayPath: `/envs/${id}`, + version: '3.12.4', + environmentPath: Uri.file(`/envs/${id}/bin/python`), + sysPrefix: `/envs/${id}`, + execInfo: { run: { executable: `/envs/${id}/bin/python` } }, + } as unknown as PythonEnvironment; +} + +class RecordingEnvsProvider implements ShellEnvsProvider { + public readonly shellType = 'pwsh'; + public readonly updated: PythonEnvironment[] = []; + public removeCalls = 0; + + updateEnvVariables(_collection: unknown, env: PythonEnvironment): void { + this.updated.push(env); + } + + removeEnvVariables(): void { + this.removeCalls += 1; + } + + getEnvVariables(): Map | undefined { + return undefined; + } +} + +suite('ShellStartupActivationVariablesManager', () => { + const folderUri = Uri.file('/workspace'); + const workspaceFolder = { uri: folderUri, name: 'workspace', index: 0 } as WorkspaceFolder; + const folderEnvironment = makeEnvironment('folder-venv', 'ms-python.python:venv'); + const scriptEnvironment = makeEnvironment('script-env', 'ms-python.python:inline-script'); + + let provider: RecordingEnvsProvider; + let scopedCollection: object; + let envCollection: GlobalEnvironmentVariableCollection; + let getEnvironmentStub: sinon.SinonStub; + let changeListener: ((e: DidChangeEnvironmentEventArgs) => Promise) | undefined; + let manager: ShellStartupActivationVariablesManagerImpl; + + setup(() => { + provider = new RecordingEnvsProvider(); + scopedCollection = {}; + envCollection = { + description: undefined, + getScoped: () => scopedCollection, + } as unknown as GlobalEnvironmentVariableCollection; + + sinon.stub(terminalUtils, 'getAutoActivationType').returns(terminalUtils.ACT_TYPE_SHELL); + sinon.stub(workspaceApis, 'getWorkspaceFolder').returns(workspaceFolder); + sinon.stub(workspaceApis, 'onDidChangeConfiguration').returns(new Disposable(() => undefined)); + + getEnvironmentStub = sinon.stub().resolves(folderEnvironment); + const api = { + getEnvironment: getEnvironmentStub, + setEnvironment: sinon.stub().resolves(), + onDidChangeEnvironment: (listener: (e: DidChangeEnvironmentEventArgs) => Promise) => { + changeListener = listener; + return new Disposable(() => undefined); + }, + } as unknown as PythonProjectEnvironmentApi; + + manager = new ShellStartupActivationVariablesManagerImpl(envCollection, [provider], api); + }); + + teardown(() => { + manager.dispose(); + sinon.restore(); + }); + + test('does not write a file-scoped inline-script environment into folder startup variables', async () => { + assert.ok(changeListener, 'expected the manager to subscribe to environment changes'); + + // A PEP 723 script selection fires with the `.py` file uri and the script's own environment. + await changeListener!({ + uri: Uri.file('/workspace/script.py'), + new: scriptEnvironment, + old: undefined, + }); + + assert.deepStrictEqual( + provider.updated.map((env) => env.envId.id), + ['folder-venv'], + 'the folder default should be written, never the script environment', + ); + sinon.assert.calledOnceWithExactly(getEnvironmentStub, folderUri); + }); + + test('writes the folder environment resolved at folder scope, not the event payload', async () => { + await changeListener!({ + uri: folderUri, + new: makeEnvironment('stale-payload', 'ms-python.python:venv'), + old: undefined, + }); + + assert.deepStrictEqual( + provider.updated.map((env) => env.envId.id), + ['folder-venv'], + ); + }); + + test('removes startup variables when the folder has no environment', async () => { + getEnvironmentStub.resolves(undefined); + + await changeListener!({ + uri: Uri.file('/workspace/script.py'), + new: scriptEnvironment, + old: undefined, + }); + + assert.strictEqual(provider.updated.length, 0); + assert.strictEqual(provider.removeCalls, 1); + }); + + test('ignores environment changes when shell startup activation is off', async () => { + (terminalUtils.getAutoActivationType as sinon.SinonStub).returns('command'); + + await changeListener!({ + uri: Uri.file('/workspace/script.py'), + new: scriptEnvironment, + old: undefined, + }); + + assert.strictEqual(provider.updated.length, 0); + assert.strictEqual(provider.removeCalls, 0); + sinon.assert.notCalled(getEnvironmentStub); + }); +}); From 56aa0c7cc3b8a039cc85322f13a96773aa2e9e09 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Wed, 9 Sep 2026 13:43:49 -0700 Subject: [PATCH 5/6] Invalidate an inline-script environment when its packages are edited 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> --- src/common/inlineScript/cacheLayout.ts | 12 +- .../builtin/inlineScript/envManager.ts | 90 +++++++++++++- .../inlineScript/envManager.unit.test.ts | 112 ++++++++++++++++++ .../builtin/inlineScript/main.unit.test.ts | 1 + 4 files changed, 209 insertions(+), 6 deletions(-) diff --git a/src/common/inlineScript/cacheLayout.ts b/src/common/inlineScript/cacheLayout.ts index 7554ed08..45854223 100644 --- a/src/common/inlineScript/cacheLayout.ts +++ b/src/common/inlineScript/cacheLayout.ts @@ -42,6 +42,8 @@ export interface InlineScriptEnvMeta { readonly baseInterpreterVersion: string; /** Last successful use as a canonical UTC string produced by `Date.toISOString()`. */ readonly lastUsedAt: string; + /** Set when packages changed outside setup; the entry no longer matches its declared dependencies. */ + readonly manuallyModified?: boolean; /** Bounded SHA-256 hashes of metadata identities proven for this cache entry. */ readonly sourceMetadataIdentityHashes?: readonly string[]; } @@ -447,11 +449,7 @@ function validateMeta(value: unknown): InlineScriptEnvMeta | 'unsupported' | und return undefined; } const obj = value as Record; - if ( - typeof obj.schemaVersion !== 'number' || - !Number.isSafeInteger(obj.schemaVersion) || - obj.schemaVersion <= 0 - ) { + if (typeof obj.schemaVersion !== 'number' || !Number.isSafeInteger(obj.schemaVersion) || obj.schemaVersion <= 0) { return undefined; } if (obj.schemaVersion > META_SCHEMA_VERSION) { @@ -469,6 +467,9 @@ function validateMeta(value: unknown): InlineScriptEnvMeta | 'unsupported' | und if (!isCanonicalIsoTimestamp(obj.lastUsedAt)) { return undefined; } + if (obj.manuallyModified !== undefined && typeof obj.manuallyModified !== 'boolean') { + return undefined; + } const sourceMetadataIdentityHashes = validateSourceMetadataIdentityHashes(obj.sourceMetadataIdentityHashes); if (obj.sourceMetadataIdentityHashes !== undefined && sourceMetadataIdentityHashes === undefined) { return undefined; @@ -479,6 +480,7 @@ function validateMeta(value: unknown): InlineScriptEnvMeta | 'unsupported' | und baseInterpreterPath: obj.baseInterpreterPath, baseInterpreterVersion: obj.baseInterpreterVersion, lastUsedAt: obj.lastUsedAt, + ...(obj.manuallyModified === true ? { manuallyModified: true } : {}), ...(sourceMetadataIdentityHashes ? { sourceMetadataIdentityHashes } : {}), }; } diff --git a/src/managers/builtin/inlineScript/envManager.ts b/src/managers/builtin/inlineScript/envManager.ts index 4ca350b8..79ffa1b7 100644 --- a/src/managers/builtin/inlineScript/envManager.ts +++ b/src/managers/builtin/inlineScript/envManager.ts @@ -20,6 +20,7 @@ import { CreateEnvironmentScope, DidChangeEnvironmentEventArgs, DidChangeEnvironmentsEventArgs, + DidChangePackagesEventArgs, EnvironmentChangeKind, EnvironmentManager, GetEnvironmentScope, @@ -79,7 +80,7 @@ import { sendTelemetryEvent } from '../../../common/telemetry/sender'; import { timeout } from '../../../common/utils/asyncUtils'; import { createDeferred, Deferred } from '../../../common/utils/deferred'; import { isFileNotFoundError } from '../../../common/utils/filesystem'; -import { normalizePath } from '../../../common/utils/pathUtils'; +import { isSameOrParentPath, normalizePath } from '../../../common/utils/pathUtils'; import { PythonVersion } from '../../../common/pythonVersion'; import { PythonVersionSpecifier, splitClause } from '../../../common/pythonVersionSpecifier'; import { getVenvPythonPath } from '../../../common/utils/virtualEnvironment'; @@ -276,6 +277,13 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { ); }); }), + this.api.onDidChangePackages((event) => { + void this.handlePackagesChanged(event).catch((error) => { + this.log.warn( + `Failed to record an inline-script package change: ${getErrorMessage(error)}`, + ); + }); + }), ); this.persistedAssociationsLoaded = this.loadPersistedAssociations(); void this.initializePersistedAssociations().catch((error) => { @@ -2286,6 +2294,79 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { * content-derived, so if the file at the new path no longer matches, ordinary validation clears * the association and the setup CodeLens returns. */ + /** + * Editing packages outside setup silently affects every script sharing the entry, so mark the + * entry non-reusable and un-route each of them; the next setup rebuilds from declared metadata. + */ + private async handlePackagesChanged(event: DidChangePackagesEventArgs): Promise { + if (this.disposed || event.environment.envId.managerId !== INLINE_SCRIPT_MANAGER_ID) { + return; + } + if (event.changes.length === 0) { + return; + } + const envDirPath = event.environment.sysPrefix; + // Setup installs through `managePackages`, which fires this event too. The emitter is + // synchronous, so a build still registered here owns this change and must not self-mark. + if (this.isBuildInFlightFor(envDirPath)) { + return; + } + const cacheRoot = getScriptEnvCacheRoot(this.globalStorageUri); + const envDir = Uri.file(envDirPath); + if ((await inspectOwnedCacheEntry(event.environment, cacheRoot, envDir)) !== 'expected') { + return; + } + if (!(await this.markCacheEntryManuallyModified(envDir))) { + return; + } + this.unrouteScriptsUsingEnvironment(envDirPath); + } + + private isBuildInFlightFor(envDirPath: string): boolean { + return this.pendingCreations.has(path.basename(envDirPath)); + } + + /** Mark under the entry lock so it cannot race a concurrent build. */ + private async markCacheEntryManuallyModified(envDir: Uri): Promise { + try { + return await this.withCacheEntryLock(envDir, async () => { + // Re-check now that the build, if any, has released the lock. + if (this.isBuildInFlightFor(envDir.fsPath)) { + return false; + } + const sidecar = await inspectMetaJson(envDir); + if (sidecar.kind !== 'valid' || sidecar.metadata.manuallyModified) { + return false; + } + await writeMetaJson(envDir, { ...sidecar.metadata, manuallyModified: true }); + this.cacheMutationRevision += 1; + this.log.info( + `Inline-script environment packages were modified outside setup; it will be rebuilt on next setup: ${envDir.fsPath}`, + ); + return true; + }); + } catch (error) { + // A build holding the lock rewrites the sidecar anyway. + this.log.warn( + `Failed to record an inline-script package modification for ${envDir.fsPath}: ${getErrorMessage(error)}`, + ); + return false; + } + } + + /** Un-route every script associated with the given cache entry. */ + private unrouteScriptsUsingEnvironment(envDirPath: string): void { + for (const [scriptPath, association] of this.fsPathToPersistedAssociation.entries()) { + if (!isSameOrParentPath(envDirPath, association.environmentPath)) { + continue; + } + this.invalidateCachedAssociationValidation(scriptPath); + const uri = this.routingRegistry.getUri(scriptPath) ?? Uri.file(scriptPath); + this.routingRegistry.setValidatedAssociation(uri, false); + this.log.info(`Inline-script association for ${scriptPath} needs setup again after a package change.`); + } + } + private async handleRenamedScripts( files: readonly { readonly oldUri: Uri; readonly newUri: Uri }[], ): Promise { @@ -2920,6 +3001,13 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { }; } const sidecar = sidecarResult.metadata; + if (sidecar.manuallyModified) { + // Packages changed outside setup, so the entry no longer matches its dependencies. + this.log.info( + `Rebuilding an inline-script cache entry whose packages were modified outside setup: ${envDir.fsPath}`, + ); + return { kind: 'stale' }; + } if (!this.matchesSelectedBase(sidecar, selectedBase)) { return { kind: 'stale' }; } diff --git a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts index 614f2699..815bf95a 100644 --- a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts +++ b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts @@ -9,6 +9,7 @@ import * as path from 'path'; import * as sinon from 'sinon'; import { Disposable, LogOutputChannel, Memento, TextDocument, Uri } from 'vscode'; import { + DidChangePackagesEventArgs, EnvironmentChangeKind, EnvironmentManager, PythonEnvironment, @@ -101,6 +102,7 @@ suite('InlineScriptEnvManager', () => { let api: PythonEnvironmentApi; let apiGetEnvironmentsStub: sinon.SinonStub; let apiRefreshEnvironmentsStub: sinon.SinonStub; + let packagesChangedListener: ((e: DidChangePackagesEventArgs) => unknown) | undefined; let baseEnvironment: PythonEnvironment; let baseExecutable: string; let baseManager: EnvironmentManager; @@ -150,6 +152,10 @@ suite('InlineScriptEnvManager', () => { api = { getEnvironments: apiGetEnvironmentsStub, refreshEnvironments: apiRefreshEnvironmentsStub, + onDidChangePackages: (listener: (e: DidChangePackagesEventArgs) => unknown) => { + packagesChangedListener = listener; + return new Disposable(() => undefined); + }, } as unknown as PythonEnvironmentApi; nativeFinder = {} as NativePythonFinder; routingRegistry = new InlineScriptRoutingRegistry(); @@ -2300,6 +2306,112 @@ suite('InlineScriptEnvManager', () => { }); }); + suite('package drift', () => { + // The listener is fire-and-forget, so wait for its async work rather than the call. + function firePackagesChanged(environment: PythonEnvironment): void { + assert.ok(packagesChangedListener, 'expected the manager to subscribe to package changes'); + packagesChangedListener!({ + environment, + manager: {} as never, + changes: [{ kind: 0 as never, pkg: {} as never }], + }); + } + + function sidecarFor(environment: PythonEnvironment): cacheLayout.InlineScriptEnvMeta | undefined { + const entry = sidecarsByEnvDir.get(normalizePath(environment.sysPrefix)); + return typeof entry === 'string' ? undefined : entry; + } + + test('marks an owned entry manually modified and un-routes every script using it', async () => { + const first = scriptUri('shared_one.py'); + const second = scriptUri('shared_two.py'); + const environment = await createOwnedEnvironment(); + await manager.set(first, environment); + await manager.set(second, environment); + routingRegistry.setValidatedAssociation(first, true); + routingRegistry.setValidatedAssociation(second, true); + + firePackagesChanged(environment); + await waitForCondition( + () => sidecarFor(environment)?.manuallyModified === true, + 'the modified cache entry should be marked in its sidecar', + ); + + // Both scripts sharing the entry lose routing, not just the edited one. + assert.strictEqual(routingRegistry.hasValidatedAssociation(first), false); + assert.strictEqual(routingRegistry.hasValidatedAssociation(second), false); + }); + + test('rebuilds instead of reusing an entry whose packages were modified', async () => { + const uri = scriptUri(); + const environment = await createOwnedEnvironment(); + await manager.set(uri, environment); + firePackagesChanged(environment); + await waitForCondition( + () => sidecarFor(environment)?.manuallyModified === true, + 'the modified cache entry should be marked in its sidecar', + ); + createWithProgressStub.resetHistory(); + + await manager.create(uri); + + assert.strictEqual( + createWithProgressStub.callCount, + 1, + 'a drifted entry must be rebuilt, not reused', + ); + }); + + test('ignores package changes for environments it does not own', async () => { + const uri = scriptUri(); + const environment = await createOwnedEnvironment(); + await manager.set(uri, environment); + routingRegistry.setValidatedAssociation(uri, true); + + firePackagesChanged({ + ...environment, + envId: { managerId: 'ms-python.python:venv', id: 'some-venv' }, + }); + await nextTurn(); + await nextTurn(); + await nextTurn(); + + assert.strictEqual(sidecarFor(environment)?.manuallyModified, undefined); + assert.strictEqual(routingRegistry.hasValidatedAssociation(uri), true); + }); + test('does not mark an entry while its own build is in flight', async () => { + // The real createWithProgress installs through managePackages, which fires this same + // event while the build still holds the cache-entry lock, so setup must not mark the + // entry it is building. Two entries are used so the outcomes are distinguishable: the + // unguarded entry settling proves the guarded one had at least as long to settle. + const building = await createOwnedEnvironment(); + const edited = await createOwnedEnvironment('bbbbbbbbbbbbbbbb'); + const buildingScript = scriptUri('building.py'); + await manager.set(buildingScript, building); + routingRegistry.setValidatedAssociation(buildingScript, true); + const pendingCreations = ( + manager as unknown as { pendingCreations: Map } + ).pendingCreations; + pendingCreations.set(CACHE_KEY, {}); + + firePackagesChanged(building); + firePackagesChanged(edited); + await waitForCondition( + () => sidecarFor(edited)?.manuallyModified === true, + 'a package change outside a build should be recorded', + ); + await nextTurn(); + await nextTurn(); + + assert.strictEqual( + sidecarFor(building)?.manuallyModified, + undefined, + 'setup must not mark the entry it is building', + ); + assert.strictEqual(routingRegistry.hasValidatedAssociation(buildingScript), true); + }); + }); + suite('transaction rollback', () => { test('discards the partial environment when package installation is cancelled', async () => { const uri = scriptUri(); diff --git a/src/test/managers/builtin/inlineScript/main.unit.test.ts b/src/test/managers/builtin/inlineScript/main.unit.test.ts index d62def49..7483fba0 100644 --- a/src/test/managers/builtin/inlineScript/main.unit.test.ts +++ b/src/test/managers/builtin/inlineScript/main.unit.test.ts @@ -67,6 +67,7 @@ suite('registerInlineScriptFeatures (feature-flag gate)', () => { .returns(new Disposable(() => undefined)); getPythonApiStub = sinon.stub(pythonApi, 'getPythonApi').resolves({ registerEnvironmentManager: registerEnvironmentManagerStub, + onDidChangePackages: () => new Disposable(() => undefined), } as unknown as PythonEnvironmentApi); }); From 9631d32c49c841b9c15968ae56b5507da57e459f Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Wed, 9 Sep 2026 14:19:31 -0700 Subject: [PATCH 6/6] Address review feedback on inline-script fixes 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> --- src/features/inlineScript/setupEnvironment.ts | 3 ++ .../builtin/inlineScript/envManager.ts | 26 ++++++------- .../setupEnvironment.unit.test.ts | 39 ++++++++++++------- 3 files changed, 42 insertions(+), 26 deletions(-) diff --git a/src/features/inlineScript/setupEnvironment.ts b/src/features/inlineScript/setupEnvironment.ts index bebad836..6a8a0cb0 100644 --- a/src/features/inlineScript/setupEnvironment.ts +++ b/src/features/inlineScript/setupEnvironment.ts @@ -75,6 +75,9 @@ export async function setUpInlineScriptEnvironment( return undefined; } await em.setEnvironment(scriptUri, environment); + // Outcomes are read non-consumingly, so a success must retire the previous attempt's outcome + // rather than relying on the next `create` to clear it on entry. + routing.clearSetupOutcome(scriptUri); return environment; } diff --git a/src/managers/builtin/inlineScript/envManager.ts b/src/managers/builtin/inlineScript/envManager.ts index 79ffa1b7..e15906a0 100644 --- a/src/managers/builtin/inlineScript/envManager.ts +++ b/src/managers/builtin/inlineScript/envManager.ts @@ -2281,19 +2281,6 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return run; } - /** - * Follow a rename instead of dropping the association. - * - * A cache entry is keyed by the script's dependencies and base interpreter, never by its path, - * so renaming a script does not invalidate its environment. `PythonProjectManagerImpl` already - * rewrites the matching `python-envs.pythonProjects` entry to the new path, so clearing the - * association here would leave a managed inline-script project entry with no environment behind - * it. - * - * The moved record is re-validated afterwards rather than trusted: its 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. - */ /** * Editing packages outside setup silently affects every script sharing the entry, so mark the * entry non-reusable and un-route each of them; the next setup rebuilds from declared metadata. @@ -2367,6 +2354,19 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { } } + /** + * Follow a rename instead of dropping the association. + * + * A cache entry is keyed by the script's dependencies and base interpreter, never by its path, + * so renaming a script does not invalidate its environment. `PythonProjectManagerImpl` already + * rewrites the matching `python-envs.pythonProjects` entry to the new path, so clearing the + * association here would leave a managed inline-script project entry with no environment behind + * it. + * + * The moved record is re-validated afterwards rather than trusted: its 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. + */ private async handleRenamedScripts( files: readonly { readonly oldUri: Uri; readonly newUri: Uri }[], ): Promise { diff --git a/src/test/features/inlineScript/setupEnvironment.unit.test.ts b/src/test/features/inlineScript/setupEnvironment.unit.test.ts index 73ac5867..9f3ff1a8 100644 --- a/src/test/features/inlineScript/setupEnvironment.unit.test.ts +++ b/src/test/features/inlineScript/setupEnvironment.unit.test.ts @@ -92,6 +92,20 @@ suite('setUpInlineScriptEnvironment', () => { em.verify((m) => m.setEnvironment(scriptUri, env), typemoq.Times.once()); }); + test('retires a previous cancellation once setup succeeds', async () => { + // Outcomes are read non-consumingly, so a stale cancellation must not survive a later + // successful setup and be reported against it. + const env = makeEnv(); + routing.noteSetupOutcome(scriptUri, { kind: 'cancelled' }); + manager.setup((m) => m.create(scriptUri, undefined)).returns(() => Promise.resolve(env)); + em.setup((m) => m.setEnvironment(scriptUri, env)).returns(() => Promise.resolve()); + + const result = await setUpInlineScriptEnvironment(scriptUri, em.object, routing); + + assert.strictEqual(result, env); + assert.strictEqual(routing.getSetupOutcome(scriptUri), undefined); + }); + test('publishes saved metadata for a closed script so its project can route', async () => { // The lazy detector only observes open documents, so bulk setup of a closed script would // otherwise leave the registry with no metadata and the script permanently non-routeable. @@ -230,7 +244,10 @@ suite('setUpInlineScriptEnvironmentsInWorkspace', () => { manager.verify((m) => m.create(first, undefined), typemoq.Times.once()); manager.verify((m) => m.create(second, undefined), typemoq.Times.never()); - assert.match(warningStub.firstCall.args[0], /canceled/i); + assert.strictEqual( + warningStub.firstCall.args[0], + 'Environment setup was canceled. Set up 0 of 2 selected inline script environment(s); the remaining 1 were not started.', + ); sinon.assert.notCalled(infoStub); }); @@ -248,7 +265,10 @@ suite('setUpInlineScriptEnvironmentsInWorkspace', () => { await setUpInlineScriptEnvironmentsInWorkspace(em.object, routing); - assert.match(warningStub.firstCall.args[0], /failed/i); + assert.strictEqual( + warningStub.firstCall.args[0], + 'Set up 0 of 1 selected inline script environment(s). 1 failed — see the Python Environments output for details.', + ); sinon.assert.notCalled(infoStub); }); }); @@ -266,7 +286,7 @@ suite('notifyInlineScriptSetupOutcome', () => { sinon.restore(); }); - test('reports cancellation as information, not a failure', () => { + test('reports cancellation as an information message with nothing to clean up', () => { const infoStub = sinon.stub(winapi, 'showInformationMessage').resolves(undefined); const errorStub = sinon.stub(winapi, 'showErrorMessage').resolves(undefined); const warningStub = sinon.stub(winapi, 'showWarningMessage').resolves(undefined); @@ -274,20 +294,13 @@ suite('notifyInlineScriptSetupOutcome', () => { notifyInlineScriptSetupOutcome(scriptUri, routing); - assert.match(infoStub.firstCall.args[0], /setup was canceled/i); + // The exact message matters: cancelling must not ask the user to clean up, retry, or + // mention quarantined state, because the incomplete environment is already discarded. + assert.strictEqual(infoStub.firstCall.args[0], 'Environment setup was canceled.'); sinon.assert.notCalled(errorStub); sinon.assert.notCalled(warningStub); }); - test('never asks the user to clean anything up after a cancellation', () => { - const infoStub = sinon.stub(winapi, 'showInformationMessage').resolves(undefined); - routing.noteSetupOutcome(scriptUri, { kind: 'cancelled' }); - - notifyInlineScriptSetupOutcome(scriptUri, routing); - - assert.doesNotMatch(infoStub.firstCall.args[0], /clean up|quarantin|retry|lock/i); - }); - test('lets coalesced setup callers observe the same outcome', () => { const infoStub = sinon.stub(winapi, 'showInformationMessage').resolves(undefined); const errorStub = sinon.stub(winapi, 'showErrorMessage').resolves(undefined);