From d5f4f2848cf46b9f2f361c6f6131d61d430d4ea7 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 25 Aug 2026 12:20:16 -0700 Subject: [PATCH 1/7] fix: harden inline script cache clearing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/common/constants.ts | 1 + src/common/errors/NotSupportedError.ts | 6 + src/common/lockfile.apis.ts | 193 +++++++- src/common/persistentState.ts | 46 +- src/extension.ts | 13 +- src/features/envCommands.ts | 14 +- src/features/envManagers.ts | 20 +- src/features/settings/settingHelpers.ts | 6 +- .../builtin/inlineScript/envManager.ts | 428 ++++++++++++++++-- src/test/common/lockfile.apis.unit.test.ts | 197 ++++++++ src/test/common/persistentState.unit.test.ts | 200 ++++++++ src/test/features/envCommands.unit.test.ts | 76 +++- src/test/features/envManagers.unit.test.ts | 44 ++ .../settings/settingHelpers.unit.test.ts | 25 + .../inlineScript/envManager.unit.test.ts | 342 +++++++++++++- 15 files changed, 1497 insertions(+), 114 deletions(-) create mode 100644 src/test/common/persistentState.unit.test.ts diff --git a/src/common/constants.ts b/src/common/constants.ts index 31894953c..087834ae9 100644 --- a/src/common/constants.ts +++ b/src/common/constants.ts @@ -4,6 +4,7 @@ export const ENVS_EXTENSION_ID = 'ms-python.vscode-python-envs'; export const PYTHON_EXTENSION_ID = 'ms-python.python'; export const CONDA_MANAGER_ID = `${PYTHON_EXTENSION_ID}:conda`; export const INLINE_SCRIPT_MANAGER_ID = `${PYTHON_EXTENSION_ID}:inline-script`; +export const INLINE_SCRIPT_ENVS_KEY = `${ENVS_EXTENSION_ID}:inline-script:SCRIPT_ENVIRONMENTS`; export const PYENV_MANAGER_ID = `${PYTHON_EXTENSION_ID}:pyenv`; export const JUPYTER_EXTENSION_ID = 'ms-toolsai.jupyter'; export const EXTENSION_ROOT_DIR = path.dirname(__dirname); diff --git a/src/common/errors/NotSupportedError.ts b/src/common/errors/NotSupportedError.ts index da1677058..a8bb427a6 100644 --- a/src/common/errors/NotSupportedError.ts +++ b/src/common/errors/NotSupportedError.ts @@ -11,3 +11,9 @@ export class RemoveEnvironmentNotSupported extends BaseError { super('NotSupported', message); } } + +export class ClearCacheNotSupported extends BaseError { + constructor(message: string) { + super('NotSupported', message); + } +} diff --git a/src/common/lockfile.apis.ts b/src/common/lockfile.apis.ts index cb8ff8032..10648cc71 100644 --- a/src/common/lockfile.apis.ts +++ b/src/common/lockfile.apis.ts @@ -19,6 +19,9 @@ export interface AcquiredFileLock { export const FILE_LOCK_DIR_SUFFIX = '.lock'; export const FILE_LOCK_OWNER_MARKER_PREFIX = 'owner-'; export const FILE_LOCK_RETAINED_MARKER_PREFIX = 'retained-'; +export const FILE_LOCK_RECLAIM_MARKER_PREFIX = '.reclaim-'; +export const FILE_LOCK_RELEASE_MARKER_PREFIX = '.release-'; +export const FILE_LOCK_RETIRED_DIR_INFIX = '.retired-'; /** Legacy retained marker. It remains recognizable but cannot be safely reclaimed. */ export const FILE_LOCK_RETAINED_MARKER = 'retained'; @@ -30,6 +33,8 @@ export interface InspectFileLockOptions { } type LockState = 'held' | 'released' | 'retained'; +const LOCK_RETIRE_MAX_ATTEMPTS = 3; +const LOCK_RETIRE_RETRY_MS = 10; export function getFileLockPath(filePath: string): string { return `${path.resolve(filePath)}${FILE_LOCK_DIR_SUFFIX}`; @@ -43,6 +48,8 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock `${FILE_LOCK_OWNER_MARKER_PREFIX}${process.pid}-${crypto.randomBytes(16).toString('hex')}`, ); const retainedMarker = path.join(lockPath, getRetainedMarkerName(path.basename(ownerMarker))); + const releaseMarkerName = + `${FILE_LOCK_RELEASE_MARKER_PREFIX}${process.pid}-${crypto.randomBytes(16).toString('hex')}-${path.basename(ownerMarker)}`; const deadline = Date.now() + options.timeoutMs; while (true) { @@ -80,16 +87,36 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock if (state !== 'held') { return; } - state = 'released'; try { - await fsapi.unlink(ownerMarker); + await fsapi.rename(ownerMarker, path.join(lockPath, releaseMarkerName)); } catch (error) { if (hasErrorCode(error, 'ENOENT')) { throw createLockError('Lock ownership was compromised', 'ECOMPROMISED', lockPath); } throw error; } - await fsapi.rmdir(lockPath); + const retiredPath = getRetiredLockPath(lockPath); + try { + await retireCanonicalLockDirectory(lockPath, retiredPath); + } catch (error) { + const restored = await fsapi + .rename(path.join(lockPath, releaseMarkerName), ownerMarker) + .then( + () => true, + () => false, + ); + if (restored) { + throw createLockError( + 'Failed to retire the lock directory; ownership was restored', + 'ELOCKRELEASEFAILED', + lockPath, + error, + ); + } + throw error; + } + state = 'released'; + await cleanupRetiredLock(retiredPath, releaseMarkerName); }, }; } catch (error) { @@ -114,7 +141,8 @@ export async function inspectFileLock(filePath: string, options?: InspectFileLoc interface FileLockSnapshot { readonly state: FileLockState; readonly marker?: string; - readonly markerKind?: 'owner' | 'retained'; + readonly markerKind?: 'owner' | 'retained' | 'reclaim' | 'release'; + readonly generationMarker?: string; } async function inspectFileLockSnapshot( @@ -137,14 +165,26 @@ async function inspectFileLockSnapshot( return { state: 'malformed' }; } - const entries = await fsapi.readdir(lockPath); + let entries: string[]; + try { + entries = await fsapi.readdir(lockPath); + } catch (error) { + if (hasErrorCode(error, 'ENOENT')) { + return { state: 'missing' }; + } + throw error; + } const ownerEntries = entries.filter((entry) => entry.startsWith(FILE_LOCK_OWNER_MARKER_PREFIX)); const generationRetainedEntries = entries.filter((entry) => entry.startsWith(FILE_LOCK_RETAINED_MARKER_PREFIX)); + const reclaimEntries = entries.filter((entry) => entry.startsWith(FILE_LOCK_RECLAIM_MARKER_PREFIX)); + const releaseEntries = entries.filter((entry) => entry.startsWith(FILE_LOCK_RELEASE_MARKER_PREFIX)); const retainedEntries = entries.filter((entry) => entry === FILE_LOCK_RETAINED_MARKER); const unknownEntries = entries.filter( (entry) => !entry.startsWith(FILE_LOCK_OWNER_MARKER_PREFIX) && !entry.startsWith(FILE_LOCK_RETAINED_MARKER_PREFIX) && + !entry.startsWith(FILE_LOCK_RECLAIM_MARKER_PREFIX) && + !entry.startsWith(FILE_LOCK_RELEASE_MARKER_PREFIX) && entry !== FILE_LOCK_RETAINED_MARKER, ); @@ -152,9 +192,13 @@ async function inspectFileLockSnapshot( unknownEntries.length > 0 || ownerEntries.length > 1 || generationRetainedEntries.length > 1 || + reclaimEntries.length > 1 || + releaseEntries.length > 1 || retainedEntries.length > 1 || - generationRetainedEntries.length + retainedEntries.length > 1 || - generationRetainedEntries.length + ownerEntries.length > 1 + (retainedEntries.length === 1 && + generationRetainedEntries.length + reclaimEntries.length + releaseEntries.length > 0) || + (retainedEntries.length === 0 && + ownerEntries.length + generationRetainedEntries.length + reclaimEntries.length + releaseEntries.length > 1) ) { return { state: 'malformed' }; } @@ -168,6 +212,48 @@ async function inspectFileLockSnapshot( } return { state: 'retained', marker: generationRetainedEntries[0], markerKind: 'retained' }; } + if (reclaimEntries.length === 1) { + const reclaimMarker = parseTransitionMarker(reclaimEntries[0], FILE_LOCK_RECLAIM_MARKER_PREFIX); + if (!reclaimMarker) { + return { state: 'malformed' }; + } + const liveness = await (options?.checkProcessLiveness ?? getProcessLiveness)(reclaimMarker.pid); + if (liveness === 'dead') { + return { + state: 'stale', + marker: reclaimEntries[0], + markerKind: 'reclaim', + generationMarker: reclaimMarker.generationMarker, + }; + } + return { + state: liveness === 'live' ? 'held' : 'unavailable', + marker: reclaimEntries[0], + markerKind: 'reclaim', + generationMarker: reclaimMarker.generationMarker, + }; + } + if (releaseEntries.length === 1) { + const releaseMarker = parseTransitionMarker(releaseEntries[0], FILE_LOCK_RELEASE_MARKER_PREFIX); + if (!releaseMarker) { + return { state: 'malformed' }; + } + const liveness = await (options?.checkProcessLiveness ?? getProcessLiveness)(releaseMarker.pid); + if (liveness === 'dead') { + return { + state: 'stale', + marker: releaseEntries[0], + markerKind: 'release', + generationMarker: releaseMarker.generationMarker, + }; + } + return { + state: liveness === 'live' ? 'held' : 'unavailable', + marker: releaseEntries[0], + markerKind: 'release', + generationMarker: releaseMarker.generationMarker, + }; + } if (ownerEntries.length === 1) { const ownerPid = parseMarkerPid(ownerEntries[0], FILE_LOCK_OWNER_MARKER_PREFIX); if (ownerPid === undefined) { @@ -183,7 +269,7 @@ async function inspectFileLockSnapshot( } /** - * Claim and remove the exact observed stale or retained generation without releasing the lock directory. + * Claim the exact observed stale or retained generation, then atomically retire its canonical directory. */ export async function reclaimFileLock(filePath: string, options?: InspectFileLockOptions): Promise { const lockPath = getFileLockPath(filePath); @@ -196,12 +282,13 @@ export async function reclaimFileLock(filePath: string, options?: InspectFileLoc return false; } - const claimedMarker = path.join( - lockPath, - `.reclaim-${process.pid}-${crypto.randomBytes(16).toString('hex')}-${snapshot.marker}`, - ); + const generationMarker = snapshot.generationMarker ?? snapshot.marker; + const claimedMarkerName = + `${FILE_LOCK_RECLAIM_MARKER_PREFIX}${process.pid}-${crypto.randomBytes(16).toString('hex')}-${generationMarker}`; + const claimedMarker = path.join(lockPath, claimedMarkerName); + const observedMarker = path.join(lockPath, snapshot.marker); try { - await fsapi.rename(path.join(lockPath, snapshot.marker), claimedMarker); + await fsapi.rename(observedMarker, claimedMarker); } catch (error) { if (hasErrorCode(error, 'ENOENT') || hasErrorCode(error, 'EEXIST')) { return false; @@ -209,16 +296,18 @@ export async function reclaimFileLock(filePath: string, options?: InspectFileLoc throw error; } + const retiredPath = getRetiredLockPath(lockPath); try { - await fsapi.unlink(claimedMarker); - await fsapi.rmdir(lockPath); - return true; + await retireCanonicalLockDirectory(lockPath, retiredPath); } catch (error) { + await fsapi.rename(claimedMarker, observedMarker).catch(() => undefined); if (hasErrorCode(error, 'ENOENT') || hasErrorCode(error, 'ENOTEMPTY')) { return false; } throw error; } + await cleanupRetiredLock(retiredPath, claimedMarkerName); + return true; } export async function getProcessLiveness(pid: number): Promise { @@ -269,12 +358,80 @@ function parseMarkerPid(entry: string, prefix: string): number | undefined { return Number.isSafeInteger(pid) && pid > 0 ? pid : undefined; } +function parseTransitionMarker( + entry: string, + transitionPrefix: string, +): { readonly pid: number; readonly generationMarker: string } | undefined { + const match = entry.match( + new RegExp( + `^${escapeRegExp(transitionPrefix)}(\\d+)-[0-9a-f]{32}-((?:${escapeRegExp(FILE_LOCK_OWNER_MARKER_PREFIX)}|${escapeRegExp(FILE_LOCK_RETAINED_MARKER_PREFIX)})\\d+-.+)$`, + ), + ); + if (!match) { + return undefined; + } + const pid = Number(match[1]); + const generationMarker = match[2]; + const generationPrefix = generationMarker.startsWith(FILE_LOCK_OWNER_MARKER_PREFIX) + ? FILE_LOCK_OWNER_MARKER_PREFIX + : FILE_LOCK_RETAINED_MARKER_PREFIX; + return Number.isSafeInteger(pid) && + pid > 0 && + parseMarkerPid(generationMarker, generationPrefix) !== undefined + ? { pid, generationMarker } + : undefined; +} + +function getRetiredLockPath(lockPath: string): string { + return `${lockPath}${FILE_LOCK_RETIRED_DIR_INFIX}${process.pid}-${crypto.randomBytes(16).toString('hex')}`; +} + +async function cleanupRetiredLock(retiredPath: string, markerName: string): Promise { + try { + await fsapi.unlink(path.join(retiredPath, markerName)); + await fsapi.rmdir(retiredPath); + } catch { + await fsapi.remove(retiredPath).catch(() => undefined); + } +} + +async function retireCanonicalLockDirectory(lockPath: string, retiredPath: string): Promise { + for (let attempt = 0; attempt < LOCK_RETIRE_MAX_ATTEMPTS; attempt += 1) { + try { + await fsapi.rename(lockPath, retiredPath); + return; + } catch (error) { + if (!isRetirementContentionError(error) || attempt === LOCK_RETIRE_MAX_ATTEMPTS - 1) { + throw error; + } + await delay(LOCK_RETIRE_RETRY_MS); + } + } +} + +function isRetirementContentionError(error: unknown): boolean { + return ( + hasErrorCode(error, 'EPERM') || + hasErrorCode(error, 'EBUSY') || + hasErrorCode(error, 'EACCES') + ); +} + function escapeRegExp(value: string): string { return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); } -function createLockError(message: string, code: string, lockPath: string): NodeJS.ErrnoException { - return Object.assign(new Error(message), { code, path: lockPath }); +function createLockError( + message: string, + code: string, + lockPath: string, + cause?: unknown, +): NodeJS.ErrnoException { + return Object.assign(new Error(message), { + code, + path: lockPath, + ...(cause === undefined ? {} : { cause }), + }); } async function delay(milliseconds: number): Promise { diff --git a/src/common/persistentState.ts b/src/common/persistentState.ts index c6b0f2632..859e1a43f 100644 --- a/src/common/persistentState.ts +++ b/src/common/persistentState.ts @@ -1,28 +1,32 @@ import { ExtensionContext, Memento } from 'vscode'; import { traceError } from './logging'; -import { createDeferred, Deferred } from './utils/deferred'; +import { createDeferred } from './utils/deferred'; export interface PersistentState { get(key: string, defaultValue?: T): Promise; set(key: string, value: T): Promise; - clear(keys?: string[]): Promise; + clear(keys?: string[], options?: { readonly preserveKeys?: readonly string[] }): Promise; +} + +export interface ClearPersistentStateOptions { + readonly preserveWorkspaceKeys?: readonly string[]; + readonly preserveGlobalKeys?: readonly string[]; } class PersistentStateImpl implements PersistentState { - private clearing: Deferred; - constructor(private readonly momento: Memento) { - this.clearing = createDeferred(); - this.clearing.resolve(); - } + private clearQueue: Promise = Promise.resolve(); + + constructor(private readonly momento: Memento) {} + async get(key: string, defaultValue?: T): Promise { - await this.clearing.promise; + await this.clearQueue; if (defaultValue === undefined) { return this.momento.get(key); } return this.momento.get(key, defaultValue); } async set(key: string, value: T): Promise { - await this.clearing.promise; + await this.clearQueue; await this.momento.update(key, value); const before = JSON.stringify(value); @@ -32,14 +36,15 @@ class PersistentStateImpl implements PersistentState { traceError('Error while updating state for key:', key); } } - async clear(keys?: string[]): Promise { - if (this.clearing.completed) { - this.clearing = createDeferred(); - const _keys = keys ?? this.momento.keys(); - await Promise.all(_keys.map((key) => this.momento.update(key, undefined))); - this.clearing.resolve(); - } - return this.clearing.promise; + async clear(keys?: string[], options?: { readonly preserveKeys?: readonly string[] }): Promise { + const requestedKeys = keys ? [...keys] : undefined; + const preservedKeys = new Set(options?.preserveKeys ?? []); + const operation = this.clearQueue.then(async () => { + const keysToClear = (requestedKeys ?? this.momento.keys()).filter((key) => !preservedKeys.has(key)); + await Promise.all(keysToClear.map((key) => this.momento.update(key, undefined))); + }); + this.clearQueue = operation.catch(() => undefined); + return operation; } } @@ -59,8 +64,11 @@ export function getGlobalPersistentState(): Promise { return _global.promise; } -export async function clearPersistentState(): Promise { +export async function clearPersistentState(options?: ClearPersistentStateOptions): Promise { const [workspace, global] = await Promise.all([_workspace.promise, _global.promise]); - await Promise.all([workspace.clear(), global.clear()]); + await Promise.all([ + workspace.clear(undefined, { preserveKeys: options?.preserveWorkspaceKeys }), + global.clear(undefined, { preserveKeys: options?.preserveGlobalKeys }), + ]); return undefined; } diff --git a/src/extension.ts b/src/extension.ts index 7465f997d..c94aa747b 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -13,7 +13,7 @@ import { PythonEnvironment, PythonEnvironmentApi, PythonProjectCreator } from '. import { ENVS_EXTENSION_ID } from './common/constants'; import { ensureCorrectVersion } from './common/extVersion'; import { registerLogger, traceError, traceInfo, traceWarn } from './common/logging'; -import { clearPersistentState, setPersistentState } from './common/persistentState'; +import { setPersistentState } from './common/persistentState'; import { newProjectSelection } from './common/pickers/managers'; import { StopWatch } from './common/stopWatch'; import { EventNames } from './common/telemetry/constants'; @@ -45,6 +45,7 @@ import { ProjectCreatorsImpl } from './features/creators/projectCreators'; import { addPythonProjectCommand, copyPathToClipboard, + clearEnvironmentCachesCommand, clearScriptEnvironmentCacheCommand, createAnyEnvironmentCommand, createEnvironmentCommand, @@ -78,11 +79,7 @@ import { registerCompletionProvider } from './features/settings/settingCompletio import { migrateGlobalDefaultEnvManagerSetting } from './features/settings/settingHelpers'; import { setActivateMenuButtonContext } from './features/terminal/activateMenuButton'; import { normalizeShellPath } from './features/terminal/shells/common/shellUtils'; -import { - clearShellProfileCache, - createShellEnvProviders, - createShellStartupProviders, -} from './features/terminal/shells/providers'; +import { createShellEnvProviders, createShellStartupProviders } from './features/terminal/shells/providers'; import { ShellStartupActivationVariablesManagerImpl } from './features/terminal/shellStartupActivationVariablesManager'; import { cleanupStartupScripts } from './features/terminal/shellStartupSetupHandlers'; import { TerminalActivationImpl } from './features/terminal/terminalActivationState'; @@ -405,9 +402,7 @@ export async function activate(context: ExtensionContext): Promise { - await clearPersistentState(); - await envManagers.clearCache(undefined); - await clearShellProfileCache(shellStartupProviders); + await clearEnvironmentCachesCommand(envManagers, shellStartupProviders); }), ...(isInlineScriptsFeatureEnabled() ? [ diff --git a/src/features/envCommands.ts b/src/features/envCommands.ts index fabf31702..57a6a6ef2 100644 --- a/src/features/envCommands.ts +++ b/src/features/envCommands.ts @@ -21,6 +21,7 @@ import { isPackageVersionLookupNotSupportedError, } from '../api'; import { traceError, traceInfo, traceVerbose } from '../common/logging'; +import * as persistentState from '../common/persistentState'; import { EnvironmentManagers, InternalEnvironmentManager, @@ -60,9 +61,11 @@ import { showWarningMessage, withProgress, } from '../common/window.apis'; -import { INLINE_SCRIPT_MANAGER_ID } from '../common/constants'; +import { INLINE_SCRIPT_ENVS_KEY, INLINE_SCRIPT_MANAGER_ID } from '../common/constants'; import { runAsTask } from './execution/runAsTask'; import { runInTerminal } from './terminal/runInTerminal'; +import * as shellProviders from './terminal/shells/providers'; +import { ShellStartupScriptProvider } from './terminal/shells/startupProvider'; import { TerminalManager } from './terminal/terminalManager'; import { EnvManagerView } from './views/envManagersView'; import { @@ -680,6 +683,15 @@ export async function removePythonProject( wm.remove(item.project); } +export async function clearEnvironmentCachesCommand( + em: EnvironmentManagers, + startupProviders: ShellStartupScriptProvider[], +): Promise { + await persistentState.clearPersistentState({ preserveWorkspaceKeys: [INLINE_SCRIPT_ENVS_KEY] }); + await em.clearCache(undefined); + await shellProviders.clearShellProfileCache(startupProviders); +} + export async function clearScriptEnvironmentCacheCommand( em: EnvironmentManagers, wm: PythonProjectManager, diff --git a/src/features/envManagers.ts b/src/features/envManagers.ts index a99198e41..755245a87 100644 --- a/src/features/envManagers.ts +++ b/src/features/envManagers.ts @@ -15,6 +15,7 @@ import { EnvironmentManagerAlreadyRegisteredError, PackageManagerAlreadyRegisteredError, } from '../common/errors/AlreadyRegisteredError'; +import { ClearCacheNotSupported } from '../common/errors/NotSupportedError'; import { InlineScriptRouteabilityChangeEvent, InlineScriptRoutingRegistry, @@ -336,12 +337,29 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { return; } + if ( + scope === INLINE_SCRIPT_MANAGER_ID || + (!(scope instanceof Uri) && + typeof scope !== 'string' && + scope.envId.managerId === INLINE_SCRIPT_MANAGER_ID) + ) { + this.throwInlineClearNotSupported(); + } const manager = this.getEnvironmentManager(scope); - if (manager && manager.id !== INLINE_SCRIPT_MANAGER_ID) { + if (manager?.id === INLINE_SCRIPT_MANAGER_ID) { + this.throwInlineClearNotSupported(); + } + if (manager) { await manager.clearCache(); } } + private throwInlineClearNotSupported(): never { + throw new ClearCacheNotSupported( + `Clear Cache for ${INLINE_SCRIPT_MANAGER_ID} requires the dedicated inline-script cache lifecycle.`, + ); + } + /** * Sets the environment for a single scope, scope of undefined checks 'global'. * If given an array of scopes, delegates to setEnvironments for batch setting. diff --git a/src/features/settings/settingHelpers.ts b/src/features/settings/settingHelpers.ts index bbea301db..c8b3322b3 100644 --- a/src/features/settings/settingHelpers.ts +++ b/src/features/settings/settingHelpers.ts @@ -635,9 +635,13 @@ export async function removeInlineScriptPythonProjectSettings( await Promise.all(promises); + const workspaceRootPaths = new Set(workspaceFolders.map((folder) => normalizePath(folder.uri.fsPath))); return Array.from(removedProjects.values()) .map((project) => currentProjectsByUri.get(project.uri.toString())) - .filter((project): project is PythonProject => project !== undefined); + .filter( + (project): project is PythonProject => + project !== undefined && !workspaceRootPaths.has(normalizePath(project.uri.fsPath)), + ); } export async function addPythonProjectSetting(edits: EditProjectSettings[]): Promise { diff --git a/src/managers/builtin/inlineScript/envManager.ts b/src/managers/builtin/inlineScript/envManager.ts index 34a847c3a..865024ae3 100644 --- a/src/managers/builtin/inlineScript/envManager.ts +++ b/src/managers/builtin/inlineScript/envManager.ts @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +import * as crypto from 'crypto'; import * as fs from 'fs-extra'; import * as path from 'path'; import type { Stats } from 'fs'; @@ -49,7 +50,7 @@ import { } from '../../../common/inlineScript/routingRegistry'; import { CONDA_MANAGER_ID, - ENVS_EXTENSION_ID, + INLINE_SCRIPT_ENVS_KEY, INLINE_SCRIPT_MANAGER_ID, PYENV_MANAGER_ID, SYSTEM_MANAGER_ID, @@ -90,10 +91,13 @@ const BASE_INTERPRETER_MANAGER_IDS = new Set([ const CACHE_LOCK_TIMEOUT_MS = 5 * 60 * 1000; const CACHE_LOCK_RETRY_MS = 500; +const CACHE_ROOT_GENERATION_FILENAME = '.root-generation'; +const CACHE_ROOT_GENERATION_ARTIFACT_PATTERN = + /^\.root-generation\.(?:tmp|invalid)-\d+-[0-9a-f]{32}$/; +const CACHE_ROOT_GENERATION_PATTERN = /^[0-9a-f]{32}$/; const CACHED_ASSOCIATION_VALIDATION_INTERVAL_MS = 5_000; const DISCOVERY_RETRY_DELAYS_MS = [1_000, 5_000, 30_000] as const; -/** Workspace-state key for PEP 723 script path to environment executable associations. */ -export const INLINE_SCRIPT_ENVS_KEY = `${ENVS_EXTENSION_ID}:inline-script:SCRIPT_ENVIRONMENTS`; +export { INLINE_SCRIPT_ENVS_KEY }; const PERSISTED_ASSOCIATION_SCHEMA_VERSION = 1 as const; interface SelectedBaseInterpreter { @@ -175,6 +179,11 @@ interface SavedMetadataSnapshot { readonly identity?: string; } +interface PhysicalCacheRoot { + readonly path: string; + readonly generation: string; +} + /** Manages extension-owned PEP 723 script environments. */ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { private readonly pendingSetups = new Map>(); @@ -259,9 +268,9 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { scope: CreateEnvironmentScope, options?: CreateEnvironmentOptions, ): Promise { - this.activeCreateOperations += 1; - try { - return await this.waitForCacheMaintenance(async () => { + return this.waitForCacheMaintenance(async () => { + this.activeCreateOperations += 1; + try { try { const scriptUri = this.getScriptUri(scope); if (!scriptUri) { @@ -304,10 +313,10 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { this.log.error(`Failed to set up inline-script environment: ${getErrorMessage(error)}`); return undefined; } - }); - } finally { - this.activeCreateOperations -= 1; - } + } finally { + this.activeCreateOperations -= 1; + } + }); } private async createForScript( @@ -475,8 +484,11 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { async clearCache(): Promise { const activeCreatesAtStart = this.activeCreateOperations; + const cacheRoot = getScriptEnvCacheRoot(this.globalStorageUri); return this.enqueueCacheMaintenance(() => - this.enqueueSelection(() => this.clearCacheInternal(activeCreatesAtStart)), + this.enqueueSelection(() => + this.withCacheRootLifecycleLock(cacheRoot, () => this.clearCacheInternal(activeCreatesAtStart)), + ), ); } @@ -2524,18 +2536,189 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { envDir: Uri, action: (lock: AcquiredFileLock) => Promise, ): Promise { - const lock = await acquireFileLock(envDir.fsPath, { - timeoutMs: CACHE_LOCK_TIMEOUT_MS, - retryIntervalMs: CACHE_LOCK_RETRY_MS, - }); + const lock = await this.acquireCacheEntryLockForMutation(envDir); try { return await action(lock); } finally { + await this.releaseCacheLockOrRetain(lock, 'inline-script cache entry'); + } + } + + private async acquireCacheEntryLockForMutation(envDir: Uri): Promise { + const cacheRoot = getScriptEnvCacheRoot(this.globalStorageUri); + const deadline = Date.now() + CACHE_LOCK_TIMEOUT_MS; + let contentionError: unknown; + while (true) { + if (contentionError && Date.now() >= deadline) { + throw contentionError; + } + const remainingMs = Math.max(0, deadline - Date.now()); + const lifecycleLock = await this.acquireCacheRootLifecycleLock(cacheRoot, remainingMs); + let entryLock: AcquiredFileLock | undefined; + let admissionError: unknown; try { - await lock.release(); + await fs.ensureDir(cacheRoot.fsPath); + if (!(await this.getPhysicalOwnedCacheRoot(cacheRoot))) { + throw new Error(l10n.t('Unable to establish the script environment cache root generation.')); + } + entryLock = await acquireFileLock(envDir.fsPath, { + timeoutMs: 0, + retryIntervalMs: CACHE_LOCK_RETRY_MS, + }); } catch (error) { - this.log.warn(`Failed to release inline-script cache lock: ${getErrorMessage(error)}`); + admissionError = error; + } + + const lifecycleReleaseError = await this.releaseCacheLockOrRetain( + lifecycleLock, + 'inline-script cache lifecycle', + ); + if (lifecycleReleaseError) { + if (entryLock) { + await this.releaseCacheLockOrRetain(entryLock, 'inline-script cache entry'); + } + throw lifecycleReleaseError; } + if (entryLock) { + return entryLock; + } + if (!this.isEntryLockWaitError(admissionError)) { + throw admissionError; + } + contentionError = admissionError; + if (Date.now() >= deadline) { + throw contentionError; + } + await this.waitForCacheEntryRetry(deadline); + } + } + + private isEntryLockWaitError(error: unknown): boolean { + return ( + typeof error === 'object' && + error !== null && + 'code' in error && + (error as NodeJS.ErrnoException).code === 'ELOCKED' + ); + } + + private async waitForCacheEntryRetry(deadline: number): Promise { + const delayMs = Math.min(CACHE_LOCK_RETRY_MS, Math.max(0, deadline - Date.now())); + if (delayMs > 0) { + await new Promise((resolve) => setTimeout(resolve, delayMs)); + } + } + + // Creation holds this only through entry-lock acquisition; clear holds it for the sweep. + private async withCacheRootLifecycleLock(cacheRoot: Uri, operation: () => Promise): Promise { + const lock = await this.acquireCacheRootLifecycleLock(cacheRoot); + let operationFailed = false; + try { + return await operation(); + } catch (error) { + operationFailed = true; + throw error; + } finally { + const releaseError = await this.releaseCacheLockOrRetain(lock, 'inline-script cache lifecycle'); + if (releaseError && !operationFailed) { + throw releaseError; + } + } + } + + private async acquireCacheRootLifecycleLock( + cacheRoot: Uri, + timeoutMs: number = CACHE_LOCK_TIMEOUT_MS, + ): Promise { + await this.prepareCacheRootLifecycleLock(cacheRoot); + let lastError: unknown; + for (let attempt = 0; attempt < 3; attempt += 1) { + try { + return await acquireFileLock(cacheRoot.fsPath, { + timeoutMs: 0, + retryIntervalMs: CACHE_LOCK_RETRY_MS, + }); + } catch (error) { + lastError = error; + if (!this.isLockContentionError(error)) { + throw error; + } + const currentState = await inspectFileLock(cacheRoot.fsPath); + if ( + (currentState === 'stale' || currentState === 'retained') && + (await reclaimFileLock(cacheRoot.fsPath)) + ) { + continue; + } + if (currentState === 'missing') { + continue; + } + if (currentState === 'held') { + try { + return await acquireFileLock(cacheRoot.fsPath, { + timeoutMs, + retryIntervalMs: CACHE_LOCK_RETRY_MS, + }); + } catch (waitError) { + if (!this.isLockContentionError(waitError)) { + throw waitError; + } + const finalState = await inspectFileLock(cacheRoot.fsPath); + if ( + (finalState === 'stale' || finalState === 'retained') && + (await reclaimFileLock(cacheRoot.fsPath)) + ) { + continue; + } + if (finalState === 'missing') { + continue; + } + throw waitError; + } + } + throw error; + } + } + if (lastError) { + throw lastError; + } + throw new Error(l10n.t('Unable to acquire the script environment cache lifecycle lock.')); + } + + private async prepareCacheRootLifecycleLock(cacheRoot: Uri): Promise { + this.validateCacheRootLocation(cacheRoot); + const globalStoragePath = path.resolve(this.globalStorageUri.fsPath); + await fs.ensureDir(globalStoragePath); + const globalStorageStat = await fs.lstat(globalStoragePath); + if (!globalStorageStat.isDirectory() || globalStorageStat.isSymbolicLink()) { + this.log.error( + `Refusing to use inline-script cache lifecycle lock from redirected globalStorage root: ${globalStoragePath}`, + ); + throw new Error( + l10n.t( + 'Refusing to clear the script environment cache because the global storage root is not a normal directory.', + ), + ); + } + } + + private async releaseCacheLockOrRetain( + lock: AcquiredFileLock, + label: string, + ): Promise { + try { + await lock.release(); + return undefined; + } catch (error) { + try { + await lock.retain(); + } catch (retainError) { + this.log.error( + `Failed to retain ${label} after release failed: ${getErrorMessage(retainError)}`, + ); + } + this.log.warn(`Failed to release ${label}: ${getErrorMessage(error)}`); + return error; } } @@ -2598,7 +2781,6 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { const envDir = getScriptEnvDir(this.globalStorageUri, cacheKey); try { - await fs.ensureDir(cacheRoot.fsPath); return await this.withCacheEntryLock(envDir, async (lock) => { const cached = await this.inspectCacheEntry( cacheRoot, @@ -2840,7 +3022,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { } const cacheRoot = getScriptEnvCacheRoot(this.globalStorageUri); - const physicalCacheRootPath = await this.getPhysicalOwnedCacheRootPath(cacheRoot); + const physicalCacheRoot = await this.getPhysicalOwnedCacheRoot(cacheRoot); const persistedAssociations = await this.getPersistedAssociationSnapshot(); const scriptPaths = new Set([ ...Object.keys(persistedAssociations), @@ -2860,10 +3042,10 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { const removedCacheEntries = new Set(); const deletionErrors: unknown[] = []; - if (physicalCacheRootPath) { + if (physicalCacheRoot) { let entryNames: string[]; try { - entryNames = await fs.readdir(physicalCacheRootPath); + entryNames = await fs.readdir(physicalCacheRoot.path); } catch (error) { if (isFileNotFoundError(error)) { entryNames = []; @@ -2874,13 +3056,16 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { const cacheEntryNames = new Set(); for (const entryName of entryNames) { + if (entryName === CACHE_ROOT_GENERATION_FILENAME) { + continue; + } if (entryName.endsWith(FILE_LOCK_DIR_SUFFIX)) { const envName = entryName.slice(0, -FILE_LOCK_DIR_SUFFIX.length); if (envName.length === 0) { const message = l10n.t( 'Refusing to clear the script environment cache because a lock entry is malformed.', ); - this.log.error(`${message} (${path.join(physicalCacheRootPath, entryName)})`); + this.log.error(`${message} (${path.join(physicalCacheRoot.path, entryName)})`); throw new Error(message); } cacheEntryNames.add(envName); @@ -2893,7 +3078,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { try { const removed = await this.removeCacheEntryForClear( cacheRoot, - physicalCacheRootPath, + physicalCacheRoot, entryName, ); if (removed) { @@ -2902,7 +3087,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { } catch (error) { deletionErrors.push(error); this.log.error( - `Failed to remove inline-script cache entry ${path.join(physicalCacheRootPath, entryName)}: ${getErrorMessage(error)}`, + `Failed to remove inline-script cache entry ${path.join(physicalCacheRoot.path, entryName)}: ${getErrorMessage(error)}`, ); } } @@ -2932,32 +3117,31 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { private async removeCacheEntryForClear( cacheRoot: Uri, - originalPhysicalCacheRootPath: string, + originalPhysicalCacheRoot: PhysicalCacheRoot, entryName: string, ): Promise { - const envDirPath = path.join(originalPhysicalCacheRootPath, entryName); + const envDirPath = path.join(originalPhysicalCacheRoot.path, entryName); let lock: AcquiredFileLock | undefined; try { lock = await this.acquireCacheEntryLockForClear(envDirPath); - const currentPhysicalCacheRootPath = await this.getPhysicalOwnedCacheRootPath(cacheRoot); - if (!currentPhysicalCacheRootPath) { - return undefined; - } + const currentPhysicalCacheRoot = await this.getPhysicalOwnedCacheRoot(cacheRoot); if ( - normalizePath(currentPhysicalCacheRootPath) !== normalizePath(originalPhysicalCacheRootPath) + !currentPhysicalCacheRoot || + normalizePath(currentPhysicalCacheRoot.path) !== normalizePath(originalPhysicalCacheRoot.path) || + currentPhysicalCacheRoot.generation !== originalPhysicalCacheRoot.generation ) { const message = l10n.t( - 'Refusing to clear the script environment cache because its physical root changed during cleanup.', + 'Refusing to clear the script environment cache because its physical root changed during cleanup (a different root generation was observed).', ); this.log.error( - `${message} (${originalPhysicalCacheRootPath} -> ${currentPhysicalCacheRootPath})`, + `${message} (${originalPhysicalCacheRoot.path} -> ${currentPhysicalCacheRoot?.path ?? 'missing'})`, ); throw new Error(message); } const entryPath = await this.getClearableCacheEntryPath( - Uri.file(currentPhysicalCacheRootPath), - path.join(currentPhysicalCacheRootPath, entryName), + Uri.file(currentPhysicalCacheRoot.path), + path.join(currentPhysicalCacheRoot.path, entryName), ); if (!entryPath) { return undefined; @@ -2966,7 +3150,13 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return entryPath; } finally { if (lock) { - await lock.release(); + const releaseError = await this.releaseCacheLockOrRetain( + lock, + 'inline-script cache entry during cleanup', + ); + if (releaseError) { + throw releaseError; + } } } } @@ -3025,17 +3215,153 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { throw new Error(message); } + private async getPhysicalOwnedCacheRoot(cacheRoot: Uri): Promise { + const physicalPath = await this.getPhysicalOwnedCacheRootPath(cacheRoot); + if (!physicalPath) { + return undefined; + } + try { + const stat = await fs.lstat(physicalPath); + if (!stat.isDirectory() || stat.isSymbolicLink()) { + return undefined; + } + return { + path: physicalPath, + generation: await this.getOrCreateCacheRootGenerationUnderLifecycleLock(physicalPath), + }; + } catch (error) { + if (isFileNotFoundError(error)) { + return undefined; + } + throw error; + } + } + + private async getOrCreateCacheRootGenerationUnderLifecycleLock( + physicalCacheRootPath: string, + ): Promise { + const markerPath = path.join(physicalCacheRootPath, CACHE_ROOT_GENERATION_FILENAME); + await this.cleanupCacheRootGenerationArtifacts(physicalCacheRootPath); + let currentGeneration = await this.inspectCacheRootGeneration(markerPath); + if (currentGeneration.kind === 'valid') { + return currentGeneration.generation; + } + if (currentGeneration.kind === 'malformed') { + const invalidPath = path.join( + physicalCacheRootPath, + `${CACHE_ROOT_GENERATION_FILENAME}.invalid-${process.pid}-${crypto.randomBytes(16).toString('hex')}`, + ); + try { + await fs.rename(markerPath, invalidPath); + } catch (error) { + if (!isFileNotFoundError(error)) { + throw error; + } + } + await fs.unlink(invalidPath).catch((error) => { + if (!isFileNotFoundError(error)) { + throw error; + } + }); + currentGeneration = await this.inspectCacheRootGeneration(markerPath); + if (currentGeneration.kind === 'valid') { + return currentGeneration.generation; + } + if (currentGeneration.kind !== 'missing') { + throw new Error( + l10n.t( + 'Refusing to use the script environment cache because its root generation marker could not be recovered.', + ), + ); + } + } + + const proposedGeneration = crypto.randomBytes(16).toString('hex'); + const tempPath = path.join( + physicalCacheRootPath, + `${CACHE_ROOT_GENERATION_FILENAME}.tmp-${process.pid}-${crypto.randomBytes(16).toString('hex')}`, + ); + try { + await fs.writeFile(tempPath, proposedGeneration, { encoding: 'utf8', flag: 'wx' }); + await fs.rename(tempPath, markerPath); + } finally { + await fs.unlink(tempPath).catch(() => undefined); + } + + const publishedGeneration = await this.inspectCacheRootGeneration(markerPath); + if (publishedGeneration.kind !== 'valid') { + throw new Error( + l10n.t('Refusing to use the script environment cache because its root generation could not be published.'), + ); + } + return publishedGeneration.generation; + } + + private async inspectCacheRootGeneration( + markerPath: string, + ): Promise< + | { readonly kind: 'missing' | 'malformed' } + | { readonly kind: 'valid'; readonly generation: string } + > { + let markerStat; + try { + markerStat = await fs.lstat(markerPath); + } catch (error) { + if (isFileNotFoundError(error)) { + return { kind: 'missing' }; + } + throw error; + } + if (!markerStat.isFile() || markerStat.isSymbolicLink()) { + throw new Error( + l10n.t( + 'Refusing to use the script environment cache because its root generation marker is not a normal file.', + ), + ); + } + let generation: string; + try { + generation = await fs.readFile(markerPath, 'utf8'); + } catch (error) { + if (isFileNotFoundError(error)) { + return { kind: 'missing' }; + } + throw error; + } + if (!CACHE_ROOT_GENERATION_PATTERN.test(generation)) { + return { kind: 'malformed' }; + } + return { kind: 'valid', generation }; + } + + private async cleanupCacheRootGenerationArtifacts(physicalCacheRootPath: string): Promise { + const entries = await fs.readdir(physicalCacheRootPath); + for (const entry of entries.filter((candidate) => CACHE_ROOT_GENERATION_ARTIFACT_PATTERN.test(candidate))) { + const artifactPath = path.join(physicalCacheRootPath, entry); + let stat; + try { + stat = await fs.lstat(artifactPath); + } catch (error) { + if (isFileNotFoundError(error)) { + continue; + } + throw error; + } + if (!stat.isFile() || stat.isSymbolicLink()) { + throw new Error( + l10n.t( + 'Refusing to use the script environment cache because a root generation artifact is not a normal file.', + ), + ); + } + await fs.unlink(artifactPath); + } + } + private async getPhysicalOwnedCacheRootPath(cacheRoot: Uri): Promise { + this.validateCacheRootLocation(cacheRoot); const globalStoragePath = path.resolve(this.globalStorageUri.fsPath); const cacheRootPath = path.resolve(cacheRoot.fsPath); - if (path.basename(cacheRootPath) !== INLINE_SCRIPT_CACHE_DIR_NAME || normalizePath(path.dirname(cacheRootPath)) !== normalizePath(globalStoragePath)) { - this.log.error(`Refusing to clear inline-script cache from unsafe root: ${cacheRootPath}`); - throw new Error(l10n.t('Refusing to clear the script environment cache from an unsafe cache root.')); - } - if (isDriveRoot(globalStoragePath) || !hasMinimumPathDepth(cacheRootPath, 3)) { - this.log.error(`Refusing to clear inline-script cache from unsafe root: ${cacheRootPath}`); - throw new Error(l10n.t('Refusing to clear the script environment cache from an unsafe cache root.')); - } let globalStorageStat; try { @@ -3103,6 +3429,20 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return resolvedCacheRootPath; } + private validateCacheRootLocation(cacheRoot: Uri): void { + const globalStoragePath = path.resolve(this.globalStorageUri.fsPath); + const cacheRootPath = path.resolve(cacheRoot.fsPath); + if ( + path.basename(cacheRootPath) !== INLINE_SCRIPT_CACHE_DIR_NAME || + normalizePath(path.dirname(cacheRootPath)) !== normalizePath(globalStoragePath) || + isDriveRoot(globalStoragePath) || + !hasMinimumPathDepth(cacheRootPath, 3) + ) { + this.log.error(`Refusing to clear inline-script cache from unsafe root: ${cacheRootPath}`); + throw new Error(l10n.t('Refusing to clear the script environment cache from an unsafe cache root.')); + } + } + private async getClearableCacheEntryPath(cacheRoot: Uri, entryPath: string): Promise { let stat; try { diff --git a/src/test/common/lockfile.apis.unit.test.ts b/src/test/common/lockfile.apis.unit.test.ts index a0230a230..5094b25c7 100644 --- a/src/test/common/lockfile.apis.unit.test.ts +++ b/src/test/common/lockfile.apis.unit.test.ts @@ -12,8 +12,11 @@ import { acquireFileLock, AcquireFileLockOptions, FILE_LOCK_OWNER_MARKER_PREFIX, + FILE_LOCK_RECLAIM_MARKER_PREFIX, + FILE_LOCK_RELEASE_MARKER_PREFIX, FILE_LOCK_RETAINED_MARKER, FILE_LOCK_RETAINED_MARKER_PREFIX, + FILE_LOCK_RETIRED_DIR_INFIX, getFileLockPath, inspectFileLock, reclaimFileLock, @@ -161,6 +164,79 @@ suite('lockfile APIs', () => { assert.strictEqual(await fs.pathExists(`${path.resolve(targetPath)}.lock`), false); }); + test('release keeps the canonical path reusable when retired-directory rmdir fails', async () => { + const lock = await acquireFileLock(targetPath, OPTIONS); + const rmdirStub = sinon + .stub(fsExtra, 'rmdir') + .rejects(Object.assign(new Error('rmdir failed'), { code: 'EACCES' })); + + await lock.release(); + + assert.strictEqual(await inspectFileLock(targetPath), 'missing'); + sinon.assert.called(rmdirStub); + assert.deepStrictEqual( + (await fs.readdir(tempRoot)).filter((entry) => entry.includes(FILE_LOCK_RETIRED_DIR_INFIX)), + [], + ); + rmdirStub.restore(); + const replacement = await acquireFileLock(targetPath, OPTIONS); + await replacement.release(); + }); + + test('release retries a transient canonical retirement failure while retaining ownership', async () => { + const lock = await acquireFileLock(targetPath, OPTIONS); + const lockPath = getFileLockPath(targetPath); + const originalRename = fsExtra.rename; + let retirementAttempts = 0; + sinon.stub(fsExtra, 'rename').callsFake(async (source, destination) => { + if (path.resolve(String(source)) === path.resolve(lockPath)) { + retirementAttempts += 1; + if (retirementAttempts === 1) { + throw Object.assign(new Error('sharing violation'), { code: 'EBUSY' }); + } + } + await originalRename(source, destination); + }); + + await lock.release(); + + assert.strictEqual(retirementAttempts, 2); + assert.strictEqual(await inspectFileLock(targetPath), 'missing'); + }); + + test('terminal retirement failure restores ownership and permits the same handle to retry later', async () => { + const lock = await acquireFileLock(targetPath, OPTIONS); + const lockPath = getFileLockPath(targetPath); + const originalRename = fsExtra.rename; + let blockRetirement = true; + let retirementAttempts = 0; + const renameStub = sinon.stub(fsExtra, 'rename').callsFake(async (source, destination) => { + if (path.resolve(String(source)) === path.resolve(lockPath)) { + retirementAttempts += 1; + if (blockRetirement) { + throw Object.assign(new Error('access denied'), { code: 'EACCES' }); + } + } + await originalRename(source, destination); + }); + + await assert.rejects( + lock.release(), + (error: NodeJS.ErrnoException) => error.code === 'ELOCKRELEASEFAILED', + ); + assert.strictEqual(retirementAttempts, 3); + assert.strictEqual(await inspectFileLock(targetPath), 'held'); + assert.strictEqual( + (await fs.readdir(lockPath)).filter((entry) => entry.startsWith(FILE_LOCK_OWNER_MARKER_PREFIX)).length, + 1, + ); + + blockRetirement = false; + await lock.release(); + assert.strictEqual(await inspectFileLock(targetPath), 'missing'); + sinon.assert.called(renameStub); + }); + test('retained locks fail fast without waiting for the acquisition timeout', async () => { const lock = await acquireFileLock(targetPath, OPTIONS); await lock.retain(); @@ -246,6 +322,99 @@ suite('lockfile APIs', () => { await replacement.release(); }); + test('recovers an interrupted generation-specific reclaim after its claimant exits', async () => { + const lockPath = getFileLockPath(targetPath); + const generationMarker = `${FILE_LOCK_RETAINED_MARKER_PREFIX}424241-generation`; + const reclaimMarker = + `${FILE_LOCK_RECLAIM_MARKER_PREFIX}424242-${'a'.repeat(32)}-${generationMarker}`; + await fs.ensureDir(lockPath); + await fs.writeFile(path.join(lockPath, reclaimMarker), ''); + const checkProcessLiveness = sinon.stub().withArgs(424242).resolves('dead'); + + assert.strictEqual(await inspectFileLock(targetPath, { checkProcessLiveness }), 'stale'); + assert.strictEqual(await reclaimFileLock(targetPath, { checkProcessLiveness }), true); + assert.strictEqual(await inspectFileLock(targetPath), 'missing'); + }); + + test('does not reclaim an in-progress generation-specific reclaim', async () => { + const lockPath = getFileLockPath(targetPath); + const generationMarker = `${FILE_LOCK_OWNER_MARKER_PREFIX}424241-generation`; + const reclaimMarker = + `${FILE_LOCK_RECLAIM_MARKER_PREFIX}${process.pid}-${'b'.repeat(32)}-${generationMarker}`; + await fs.ensureDir(lockPath); + await fs.writeFile(path.join(lockPath, reclaimMarker), ''); + const checkProcessLiveness = sinon.stub().withArgs(process.pid).resolves('live'); + + assert.strictEqual(await inspectFileLock(targetPath, { checkProcessLiveness }), 'held'); + assert.strictEqual(await reclaimFileLock(targetPath, { checkProcessLiveness }), false); + assert.strictEqual(await fs.pathExists(path.join(lockPath, reclaimMarker)), true); + }); + + test('reclaim keeps the canonical path reusable when retired-directory rmdir fails', async () => { + const lock = await acquireFileLock(targetPath, OPTIONS); + await lock.retain(); + const rmdirStub = sinon + .stub(fsExtra, 'rmdir') + .rejects(Object.assign(new Error('rmdir failed'), { code: 'EACCES' })); + + assert.strictEqual(await reclaimFileLock(targetPath), true); + assert.strictEqual(await inspectFileLock(targetPath), 'missing'); + sinon.assert.called(rmdirStub); + assert.deepStrictEqual( + (await fs.readdir(tempRoot)).filter((entry) => entry.includes(FILE_LOCK_RETIRED_DIR_INFIX)), + [], + ); + rmdirStub.restore(); + const replacement = await acquireFileLock(targetPath, OPTIONS); + await replacement.release(); + }); + + test('reclaims an interrupted release transition only after its claimant is dead', async () => { + const lockPath = getFileLockPath(targetPath); + const generationMarker = `${FILE_LOCK_OWNER_MARKER_PREFIX}424241-generation`; + const releaseMarker = + `${FILE_LOCK_RELEASE_MARKER_PREFIX}424242-${'c'.repeat(32)}-${generationMarker}`; + await fs.ensureDir(lockPath); + await fs.writeFile(path.join(lockPath, releaseMarker), ''); + const liveProbe = { checkProcessLiveness: sinon.stub().withArgs(424242).resolves('live') }; + + assert.strictEqual(await inspectFileLock(targetPath, liveProbe), 'held'); + assert.strictEqual(await reclaimFileLock(targetPath, liveProbe), false); + + const deadProbe = { checkProcessLiveness: sinon.stub().withArgs(424242).resolves('dead') }; + assert.strictEqual(await reclaimFileLock(targetPath, deadProbe), true); + assert.strictEqual(await inspectFileLock(targetPath), 'missing'); + }); + + test('an interrupted retired artifact neither blocks acquisition nor gets mistaken for a live canonical lock', async () => { + const lockPath = getFileLockPath(targetPath); + const lock = await acquireFileLock(targetPath, OPTIONS); + const rmdirStub = sinon + .stub(fsExtra, 'rmdir') + .rejects(Object.assign(new Error('cleanup interrupted'), { code: 'EACCES' })); + const removeStub = sinon + .stub(fsExtra, 'remove') + .rejects(Object.assign(new Error('cleanup interrupted'), { code: 'EACCES' })); + + await lock.release(); + rmdirStub.restore(); + removeStub.restore(); + + const retiredEntries = (await fs.readdir(tempRoot)).filter((entry) => + entry.startsWith(`${path.basename(lockPath)}${FILE_LOCK_RETIRED_DIR_INFIX}`), + ); + assert.strictEqual(retiredEntries.length, 1); + const retiredPath = path.join(tempRoot, retiredEntries[0]); + + assert.strictEqual(await inspectFileLock(targetPath), 'missing'); + const replacement = await acquireFileLock(targetPath, OPTIONS); + assert.strictEqual(await inspectFileLock(targetPath), 'held'); + assert.strictEqual(await fs.pathExists(retiredPath), true); + + await replacement.release(); + assert.strictEqual(await fs.pathExists(retiredPath), true); + }); + test('refuses to reclaim the ambiguous legacy retained marker', async () => { const lockPath = getFileLockPath(targetPath); await fs.ensureDir(lockPath); @@ -300,6 +469,34 @@ suite('lockfile APIs', () => { await replacement.release(); }); + test('treats retirement between lstat and readdir inspection as a missing lock', async () => { + const lock = await acquireFileLock(targetPath, OPTIONS); + const lockPath = getFileLockPath(targetPath); + const originalReaddir = fsExtra.readdir; + let releaseInspection: (() => void) | undefined; + let signalInspectionStarted: (() => void) | undefined; + const inspectionStarted = new Promise((resolve) => { + signalInspectionStarted = resolve; + }); + const inspectionGate = new Promise((resolve) => { + releaseInspection = resolve; + }); + sinon.stub(fsExtra, 'readdir').callsFake(async (candidatePath, options) => { + if (path.resolve(String(candidatePath)) === path.resolve(lockPath)) { + signalInspectionStarted!(); + await inspectionGate; + } + return originalReaddir(candidatePath, options as never); + }); + + const inspection = inspectFileLock(targetPath); + await inspectionStarted; + await lock.release(); + releaseInspection!(); + + assert.strictEqual(await inspection, 'missing'); + }); + test('classifies a dead owner marker as stale using the liveness probe', async () => { const lockPath = getFileLockPath(targetPath); await fs.ensureDir(lockPath); diff --git a/src/test/common/persistentState.unit.test.ts b/src/test/common/persistentState.unit.test.ts new file mode 100644 index 000000000..c199ebff3 --- /dev/null +++ b/src/test/common/persistentState.unit.test.ts @@ -0,0 +1,200 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import assert from 'assert'; +import { ExtensionContext, Memento } from 'vscode'; +import { INLINE_SCRIPT_ENVS_KEY } from '../../common/constants'; +import { + clearPersistentState, + getWorkspacePersistentState, + PersistentState, + setPersistentState, +} from '../../common/persistentState'; + +suite('persistent state clearing', () => { + let workspace: TestMemento; + let global: TestMemento; + let workspaceState: PersistentState; + + suiteSetup(async () => { + workspace = createMemento(); + global = createMemento(); + setPersistentState({ + workspaceState: workspace.memento, + globalState: global.memento, + } as ExtensionContext); + workspaceState = await getWorkspacePersistentState(); + }); + + setup(() => { + workspace.reset(); + global.reset(); + }); + + test('clears selected scopes without snapshotting or rewriting preserved inline associations', async () => { + const inlineAssociations = { 'C:\\workspace\\script.py': 'C:\\cache\\python.exe' }; + workspace.reset({ + [INLINE_SCRIPT_ENVS_KEY]: inlineAssociations, + 'other-workspace-key': 'remove', + }); + global.reset({ 'other-global-key': 'remove' }); + + await clearPersistentState({ preserveWorkspaceKeys: [INLINE_SCRIPT_ENVS_KEY] }); + + assert.deepStrictEqual(workspace.values.get(INLINE_SCRIPT_ENVS_KEY), inlineAssociations); + assert.strictEqual(workspace.values.has('other-workspace-key'), false); + assert.strictEqual(global.values.has('other-global-key'), false); + assert.deepStrictEqual( + workspace.updates.filter((update) => update.key === INLINE_SCRIPT_ENVS_KEY), + [], + 'the preserved value must not be snapshot-restored through Memento.update', + ); + }); + + test('serializes generic preserve before a racing dedicated inline clear', async () => { + workspace.reset({ + [INLINE_SCRIPT_ENVS_KEY]: { script: 'environment' }, + 'other-workspace-key': 'remove', + }); + const gate = createGate(); + workspace.beforeUpdate = async (key) => { + if (key === 'other-workspace-key') { + gate.started.resolve(); + await gate.release.promise; + } + }; + + const genericClear = clearPersistentState({ preserveWorkspaceKeys: [INLINE_SCRIPT_ENVS_KEY] }); + await gate.started.promise; + const dedicatedClear = workspaceState.clear([INLINE_SCRIPT_ENVS_KEY]); + + assert.strictEqual(workspace.values.has(INLINE_SCRIPT_ENVS_KEY), true); + gate.release.resolve(); + await genericClear; + await dedicatedClear; + + assert.strictEqual(workspace.values.has('other-workspace-key'), false); + assert.strictEqual(workspace.values.has(INLINE_SCRIPT_ENVS_KEY), false); + assert.deepStrictEqual( + workspace.updates.map((update) => update.key), + ['other-workspace-key', INLINE_SCRIPT_ENVS_KEY], + ); + }); + + test('serializes dedicated inline clear before a racing generic preserve', async () => { + workspace.reset({ + [INLINE_SCRIPT_ENVS_KEY]: { script: 'environment' }, + 'other-workspace-key': 'remove', + }); + const gate = createGate(); + workspace.beforeUpdate = async (key) => { + if (key === INLINE_SCRIPT_ENVS_KEY) { + gate.started.resolve(); + await gate.release.promise; + } + }; + + const dedicatedClear = workspaceState.clear([INLINE_SCRIPT_ENVS_KEY]); + await gate.started.promise; + const genericClear = clearPersistentState({ preserveWorkspaceKeys: [INLINE_SCRIPT_ENVS_KEY] }); + + gate.release.resolve(); + await dedicatedClear; + await genericClear; + + assert.strictEqual(workspace.values.has(INLINE_SCRIPT_ENVS_KEY), false); + assert.strictEqual(workspace.values.has('other-workspace-key'), false); + assert.deepStrictEqual( + workspace.updates.map((update) => update.key), + [INLINE_SCRIPT_ENVS_KEY, 'other-workspace-key'], + ); + }); + + test('settles the queue tail after update failure so later get, set, and clear succeed', async () => { + workspace.reset({ + 'fail-key': 'keep-after-failure', + 'clear-after-failure': 'remove', + 'read-key': 'readable', + }); + let shouldFail = true; + workspace.beforeUpdate = async (key) => { + if (key === 'fail-key' && shouldFail) { + shouldFail = false; + throw new Error('memento update failed'); + } + }; + + const failedClear = workspaceState.clear(['fail-key']); + const successfulClear = workspaceState.clear(['clear-after-failure']); + + await assert.rejects(failedClear, /memento update failed/); + await successfulClear; + assert.strictEqual(await workspaceState.get('read-key'), 'readable'); + + await workspaceState.set('new-key', 'new-value'); + assert.strictEqual(await workspaceState.get('new-key'), 'new-value'); + await workspaceState.clear(['new-key']); + assert.strictEqual(await workspaceState.get('new-key'), undefined); + }); +}); + +interface TestMemento { + readonly memento: Memento; + readonly values: Map; + readonly updates: Array<{ key: string; value: unknown }>; + beforeUpdate?: (key: string, value: unknown) => Promise; + reset(initial?: Record): void; +} + +function createMemento(): TestMemento { + const values = new Map(); + const updates: Array<{ key: string; value: unknown }> = []; + const result: TestMemento = { + memento: undefined as unknown as Memento, + values, + updates, + reset: (initial = {}) => { + values.clear(); + Object.entries(initial).forEach(([key, value]) => values.set(key, value)); + updates.splice(0, updates.length); + result.beforeUpdate = undefined; + }, + }; + const memento = { + keys: () => Array.from(values.keys()), + get: (key: string, defaultValue?: T): T | undefined => + (values.has(key) ? values.get(key) : defaultValue) as T | undefined, + update: async (key: string, value: unknown): Promise => { + await result.beforeUpdate?.(key, value); + updates.push({ key, value }); + if (value === undefined) { + values.delete(key); + } else { + values.set(key, value); + } + }, + } as Memento; + (result as { memento: Memento }).memento = memento; + return result; +} + +function createGate(): { + readonly started: { readonly promise: Promise; resolve(): void }; + readonly release: { readonly promise: Promise; resolve(): void }; +} { + return { + started: createSignal(), + release: createSignal(), + }; +} + +function createSignal(): { readonly promise: Promise; resolve(): void } { + let resolvePromise: (() => void) | undefined; + const promise = new Promise((resolve) => { + resolvePromise = resolve; + }); + return { + promise, + resolve: () => resolvePromise!(), + }; +} diff --git a/src/test/features/envCommands.unit.test.ts b/src/test/features/envCommands.unit.test.ts index ae2a1c54f..9d3aef267 100644 --- a/src/test/features/envCommands.unit.test.ts +++ b/src/test/features/envCommands.unit.test.ts @@ -4,17 +4,21 @@ import * as typeMoq from 'typemoq'; import { Uri } from 'vscode'; import { PythonEnvironment, PythonProject } from '../../api'; import * as commandApi from '../../common/command.api'; -import { INLINE_SCRIPT_MANAGER_ID } from '../../common/constants'; +import { INLINE_SCRIPT_ENVS_KEY, INLINE_SCRIPT_MANAGER_ID } from '../../common/constants'; +import * as persistentState from '../../common/persistentState'; import * as managerApi from '../../common/pickers/managers'; import * as projectApi from '../../common/pickers/projects'; import * as windowApis from '../../common/window.apis'; import { + clearEnvironmentCachesCommand, clearScriptEnvironmentCacheCommand, createAnyEnvironmentCommand, removePythonProject, revealEnvInManagerView, } from '../../features/envCommands'; import * as settingHelpers from '../../features/settings/settingHelpers'; +import * as shellProviders from '../../features/terminal/shells/providers'; +import { ShellStartupScriptProvider } from '../../features/terminal/shells/startupProvider'; import { EnvManagerView } from '../../features/views/envManagersView'; import { ProjectEnvironment, ProjectItem } from '../../features/views/treeViewItems'; import { EnvironmentManagers, InternalEnvironmentManager, PythonProjectManager } from '../../internal.api'; @@ -252,12 +256,16 @@ suite('Clear Script Environment Cache Command Tests', () => { test('clears cache before inline settings cleanup and unloads removed projects', async () => { const calls: string[] = []; + const selectionEvents: string[] = []; + let associationPresent = true; const inlineProject: PythonProject = { uri: Uri.file('/workspace/script.py'), name: 'script.py', }; const clearCache = sinon.stub().callsFake(async () => { calls.push('clearCache'); + associationPresent = false; + selectionEvents.push('cleared'); }); const envManagers = { getEnvironmentManager: sinon.stub().withArgs(INLINE_SCRIPT_MANAGER_ID).returns({ @@ -280,6 +288,8 @@ suite('Clear Script Environment Cache Command Tests', () => { .callsFake(async (projects) => { calls.push('removeInlineSettings'); assert.deepStrictEqual(projects, [inlineProject]); + assert.strictEqual(associationPresent, false); + assert.deepStrictEqual(selectionEvents, ['cleared']); return [inlineProject]; }); @@ -291,10 +301,10 @@ suite('Clear Script Environment Cache Command Tests', () => { assert.deepStrictEqual(calls, ['clearCache', 'getProjects', 'removeInlineSettings', 'removeProjects']); }); - test('keeps loaded projects when inline settings cleanup leaves them configured', async () => { - const inlineProject: PythonProject = { - uri: Uri.file('/workspace/runner'), - name: 'runner', + test('keeps the intrinsic workspace-root project loaded after its inline setting is cleaned', async () => { + const workspaceRootProject: PythonProject = { + uri: Uri.file('/workspace'), + name: 'workspace', }; const clearCache = sinon.stub().resolves(); const envManagers = { @@ -304,16 +314,21 @@ suite('Clear Script Environment Cache Command Tests', () => { }), } as unknown as EnvironmentManagers; const projectManager = { - getProjects: sinon.stub().returns([inlineProject]), + getProjects: sinon.stub().returns([workspaceRootProject]), remove: sinon.stub(), } as unknown as PythonProjectManager; sinon.stub(windowApis, 'showWarningMessage').resolves('Clear Cache' as never); - const removeInlineSettings = sinon.stub(settingHelpers, 'removeInlineScriptPythonProjectSettings').resolves([]); + const removeInlineSettings = sinon + .stub(settingHelpers, 'removeInlineScriptPythonProjectSettings') + .callsFake(async (projects) => { + assert.deepStrictEqual(projects, [workspaceRootProject]); + return []; + }); await clearScriptEnvironmentCacheCommand(envManagers, projectManager); sinon.assert.calledOnce(clearCache); - sinon.assert.calledOnceWithExactly(removeInlineSettings, [inlineProject]); + sinon.assert.calledOnceWithExactly(removeInlineSettings, [workspaceRootProject]); sinon.assert.notCalled(projectManager.remove as sinon.SinonStub); }); @@ -340,6 +355,51 @@ suite('Clear Script Environment Cache Command Tests', () => { }); }); +suite('Clear Environment Caches Command Tests', () => { + teardown(() => { + sinon.restore(); + }); + + test('generic clear preserves inline association persistence while clearing other state and managers', async () => { + const inlineAssociations = { 'C:\\workspace\\script.py': 'C:\\cache\\python.exe' }; + const workspaceState = new Map([ + [INLINE_SCRIPT_ENVS_KEY, inlineAssociations], + ['other-workspace-state', { stale: true }], + ]); + const calls: string[] = []; + sinon.stub(persistentState, 'clearPersistentState').callsFake(async (options) => { + calls.push('persistent'); + const preserved = new Set(options?.preserveWorkspaceKeys ?? []); + for (const key of workspaceState.keys()) { + if (!preserved.has(key)) { + workspaceState.delete(key); + } + } + }); + const envManagers = { + clearCache: sinon.stub().callsFake(async () => { + calls.push('managers'); + assert.deepStrictEqual(workspaceState.get(INLINE_SCRIPT_ENVS_KEY), inlineAssociations); + }), + } as unknown as EnvironmentManagers; + const startupProvider = { + clearCache: sinon.stub().callsFake(async () => { + calls.push('shells'); + }), + } as unknown as ShellStartupScriptProvider; + sinon.stub(shellProviders, 'clearShellProfileCache').callsFake(async (providers) => { + await Promise.all(providers.map((provider) => provider.clearCache())); + }); + + await clearEnvironmentCachesCommand(envManagers, [startupProvider]); + + assert.deepStrictEqual(calls, ['persistent', 'managers', 'shells']); + assert.deepStrictEqual(workspaceState.get(INLINE_SCRIPT_ENVS_KEY), inlineAssociations); + assert.strictEqual(workspaceState.has('other-workspace-state'), false); + sinon.assert.calledOnceWithExactly(envManagers.clearCache as sinon.SinonStub, undefined); + }); +}); + suite('Reveal Env In Manager View Command Tests', () => { let managerView: typeMoq.IMock; let executeCommandStub: sinon.SinonStub; diff --git a/src/test/features/envManagers.unit.test.ts b/src/test/features/envManagers.unit.test.ts index aafe89f27..31ce15f0c 100644 --- a/src/test/features/envManagers.unit.test.ts +++ b/src/test/features/envManagers.unit.test.ts @@ -7,6 +7,7 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import { Uri } from 'vscode'; import { PythonEnvironment } from '../../api'; +import { ClearCacheNotSupported } from '../../common/errors/NotSupportedError'; import * as frameUtils from '../../common/utils/frameUtils'; import * as workspaceApis from '../../common/workspace.apis'; import { PythonEnvironmentManagers } from '../../features/envManagers'; @@ -393,4 +394,47 @@ suite('PythonEnvironmentManagers - clearCache', () => { sinon.assert.calledOnce(systemClearCache); sinon.assert.notCalled(inlineClearCache); }); + + test('rejects an explicitly scoped inline clear in favor of the dedicated lifecycle', async () => { + const inlineClearCache = sandbox.stub().resolves(); + registerManager('inline-script', inlineClearCache); + + await assert.rejects( + envManagers.clearCache('ms-python.python:inline-script'), + (error: unknown) => + error instanceof ClearCacheNotSupported && + error.category === 'NotSupported' && + /dedicated inline-script cache lifecycle/.test(error.message), + ); + + sinon.assert.notCalled(inlineClearCache); + }); + + test('rejects an explicit inline manager id even when the manager is unregistered', async () => { + await assert.rejects( + envManagers.clearCache('ms-python.python:inline-script'), + (error: unknown) => + error instanceof ClearCacheNotSupported && + error.category === 'NotSupported' && + /dedicated inline-script cache lifecycle/.test(error.message), + ); + }); + + test('rejects an explicit inline environment even when the manager is unregistered', async () => { + const inlineEnvironment = { + envId: { id: 'inline-cache-entry', managerId: 'ms-python.python:inline-script' }, + } as PythonEnvironment; + + await assert.rejects( + envManagers.clearCache(inlineEnvironment), + (error: unknown) => + error instanceof ClearCacheNotSupported && + error.category === 'NotSupported' && + /dedicated inline-script cache lifecycle/.test(error.message), + ); + }); + + test('retains URI routing behavior when no inline manager is registered', async () => { + await envManagers.clearCache(Uri.file('/workspace/script.py')); + }); }); diff --git a/src/test/features/settings/settingHelpers.unit.test.ts b/src/test/features/settings/settingHelpers.unit.test.ts index b454de792..34ea41527 100644 --- a/src/test/features/settings/settingHelpers.unit.test.ts +++ b/src/test/features/settings/settingHelpers.unit.test.ts @@ -858,6 +858,31 @@ suite('Setting Helpers - Project Removal', () => { ]); }); + test('removes an inline root setting without returning the intrinsic workspace project for unloading', async () => { + const rootProject = new PythonProjectsImpl(firstWorkspace.name, firstWorkspace.uri); + const config = createProjectConfig({ + workspaceName: firstWorkspace.name, + workspaceValue: [ + { path: '.', envManager: INLINE_MANAGER_ID, packageManager: PIP_MANAGER_ID }, + ], + }); + sinon.stub(workspaceApis, 'getWorkspaceFolders').returns([firstWorkspace]); + sinon.stub(workspaceApis, 'getWorkspaceFolder').returns(firstWorkspace); + sinon.stub(workspaceApis, 'getConfiguration').returns(config); + + const removedProjects = await removeInlineScriptPythonProjectSettings([rootProject]); + + assert.deepStrictEqual(removedProjects, []); + assert.deepStrictEqual(updateCalls, [ + { + workspace: firstWorkspace.name, + key: 'pythonProjects', + value: undefined, + target: ConfigurationTarget.Workspace, + }, + ]); + }); + test('removes inline-script settings even when the project is not loaded', async () => { const config = createProjectConfig({ workspaceName: firstWorkspace.name, diff --git a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts index c95bfc782..9302150b1 100644 --- a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts +++ b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts @@ -122,6 +122,7 @@ suite('InlineScriptEnvManager', () => { let inspectMetaStub: sinon.SinonStub; let retainLockStub: sinon.SinonStub; let releaseLockStub: sinon.SinonStub; + let releaseRootLifecycleLockStub: sinon.SinonStub; let resolveSystemPythonStub: sinon.SinonStub; let resolveVenvStub: sinon.SinonStub; let routingRegistry: InlineScriptRoutingRegistry; @@ -205,9 +206,12 @@ suite('InlineScriptEnvManager', () => { }); retainLockStub = sinon.stub().resolves(); releaseLockStub = sinon.stub().resolves(); - lockStub = sinon - .stub(lockfileApis, 'acquireFileLock') - .resolves({ release: releaseLockStub, retain: retainLockStub }); + releaseRootLifecycleLockStub = sinon.stub().resolves(); + lockStub = sinon.stub(lockfileApis, 'acquireFileLock').callsFake(async (targetPath: string) => + normalizePath(targetPath) === normalizePath(cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath) + ? { release: releaseRootLifecycleLockStub, retain: sinon.stub().resolves() } + : { release: releaseLockStub, retain: retainLockStub }, + ); resolveSystemPythonStub = sinon.stub(builtinUtils, 'resolveSystemPythonEnvironmentPath').resolves(undefined); resolveVenvStub = sinon.stub(venvUtils, 'resolveVenvPythonEnvironmentPath').callsFake(async (environmentPath: string) => { return environmentsByExecutablePath.get(normalizePath(environmentPath)); @@ -268,6 +272,11 @@ suite('InlineScriptEnvManager', () => { return cacheLayout.getScriptEnvDir(globalStorageUri, CACHE_KEY); } + function cacheEntryLockCalls(): readonly sinon.SinonSpyCall[] { + const cacheRootPath = normalizePath(cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath); + return lockStub.getCalls().filter((call) => normalizePath(call.args[0]) !== cacheRootPath); + } + function getCacheKeyInputKey(dependencies: readonly string[], interpreterPath: string): string { return JSON.stringify({ dependencies: Array.from( @@ -1229,15 +1238,254 @@ suite('InlineScriptEnvManager', () => { assert.ok(releaseLockStub.calledOnce); }); - test('uses a bounded cross-process lock at the final cache path', async () => { + test('uses nonblocking entry admission at the final cache path', async () => { await manager.create(scriptUri()); - assert.strictEqual(lockStub.firstCall.args[0], envDir().fsPath); - const options = lockStub.firstCall.args[1]; - assert.ok(options.timeoutMs > 0); + assert.strictEqual( + lockStub.firstCall.args[0], + cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath, + ); + assert.strictEqual(lockStub.firstCall.args[1].timeoutMs, 0); + const entryLockCall = cacheEntryLockCalls()[0]; + assert.ok(entryLockCall); + assert.strictEqual(entryLockCall.args[0], envDir().fsPath); + const options = entryLockCall.args[1]; + assert.strictEqual(options.timeoutMs, 0); assert.ok(options.retryIntervalMs > 0); }); + test('releases the root lifecycle lock after entry admission while the build continues', async () => { + let continueCreation: (() => void) | undefined; + let signalCreationStarted: (() => void) | undefined; + const creationStarted = new Promise((resolve) => { + signalCreationStarted = resolve; + }); + const creationGate = new Promise((resolve) => { + continueCreation = resolve; + }); + createWithProgressStub.callsFake(async (...args: unknown[]) => { + const target = args[6] as string; + await fs.outputFile(venvPythonPath(target), ''); + signalCreationStarted!(); + await creationGate; + return { + environment: makeEnvironment( + 'ms-python.python:inline-script', + '3.12.4', + venvPythonPath(target), + target, + ), + }; + }); + + const createPromise = manager.create(scriptUri()); + await creationStarted; + + sinon.assert.calledOnce(releaseRootLifecycleLockStub); + sinon.assert.notCalled(releaseLockStub); + + continueCreation!(); + assert.ok(await createPromise); + sinon.assert.calledOnce(releaseLockStub); + }); + + test('retains a lifecycle lock handle when terminal release fails', async () => { + const failedRootRelease = sinon + .stub() + .rejects(Object.assign(new Error('retirement failed'), { code: 'ELOCKRELEASEFAILED' })); + const retainedRoot = sinon.stub().resolves(); + const rootPath = normalizePath(cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath); + lockStub.callsFake(async (targetPath: string) => + normalizePath(targetPath) === rootPath + ? { release: failedRootRelease, retain: retainedRoot } + : { release: releaseLockStub, retain: retainLockStub }, + ); + + assert.strictEqual(await manager.create(scriptUri()), undefined); + + sinon.assert.calledOnce(failedRootRelease); + sinon.assert.calledOnce(retainedRoot); + sinon.assert.calledOnce(releaseLockStub); + assert.strictEqual(createWithProgressStub.callCount, 0); + }); + + test('key-A contention does not block independent key-B admission at the root', async () => { + const otherCacheKey = 'fedcba9876543210'; + const firstUri = scriptUri('a.py'); + const secondUri = scriptUri('b.py'); + const secondMetadata = { ...VALID_METADATA, dependencies: ['flask'] }; + readMetadataStub.callsFake(async (uri: Uri) => + normalizePath(uri.fsPath) === normalizePath(secondUri.fsPath) ? secondMetadata : VALID_METADATA, + ); + registerCacheKey(otherCacheKey, secondMetadata.dependencies, baseExecutable); + + const rootPath = normalizePath(cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath); + const firstEntryPath = normalizePath(envDir().fsPath); + const secondEntryPath = normalizePath( + cacheLayout.getScriptEnvDir(globalStorageUri, otherCacheKey).fsPath, + ); + let rootHeld = false; + let allowFirstEntry = false; + const calls: string[] = []; + lockStub.callsFake(async (targetPath: string) => { + const normalizedTarget = normalizePath(targetPath); + if (normalizedTarget === rootPath) { + assert.strictEqual(rootHeld, false, 'root lifecycle lock must be released before another admission'); + rootHeld = true; + return { + retain: sinon.stub().resolves(), + release: async () => { + calls.push('release-root'); + rootHeld = false; + }, + }; + } + assert.strictEqual(rootHeld, true, 'entry admission must follow root acquisition'); + if (normalizedTarget === firstEntryPath && !allowFirstEntry) { + calls.push('contend-a'); + throw Object.assign(new Error('key A is locked'), { code: 'ELOCKED' }); + } + calls.push(normalizedTarget === secondEntryPath ? 'admit-b' : 'admit-a'); + return { release: sinon.stub().resolves(), retain: sinon.stub().resolves() }; + }); + + let resumeFirstRetry: (() => void) | undefined; + let signalFirstWaiting: (() => void) | undefined; + const firstWaiting = new Promise((resolve) => { + signalFirstWaiting = resolve; + }); + const internalManager = manager as unknown as { + waitForCacheEntryRetry(deadline: number): Promise; + }; + sinon.stub(internalManager, 'waitForCacheEntryRetry').callsFake( + () => + new Promise((resolve) => { + signalFirstWaiting!(); + resumeFirstRetry = resolve; + }), + ); + + const firstCreate = manager.create(firstUri); + await firstWaiting; + assert.deepStrictEqual(calls.slice(0, 2), ['contend-a', 'release-root']); + + const secondEnvironment = await manager.create(secondUri); + assert.ok(secondEnvironment, 'independent key B should be admitted while key A waits'); + assert.ok(calls.indexOf('admit-b') > calls.indexOf('release-root')); + + allowFirstEntry = true; + resumeFirstRetry!(); + assert.ok(await firstCreate); + assert.strictEqual(rootHeld, false); + }); + + test('entry admission contention stops at the shared deadline without spinning', async () => { + const rootPath = normalizePath(cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath); + lockStub.callsFake(async (targetPath: string) => { + if (normalizePath(targetPath) === rootPath) { + return { release: releaseRootLifecycleLockStub, retain: sinon.stub().resolves() }; + } + throw Object.assign(new Error('entry remains locked'), { code: 'ELOCKED' }); + }); + const internalManager = manager as unknown as { + waitForCacheEntryRetry(deadline: number): Promise; + }; + const waitStub = sinon.stub(internalManager, 'waitForCacheEntryRetry').callsFake(async (deadline) => { + clock.tick(Math.max(0, deadline - Date.now())); + }); + + assert.strictEqual(await manager.create(scriptUri()), undefined); + + sinon.assert.calledOnce(waitStub); + assert.strictEqual(cacheEntryLockCalls().length, 1); + sinon.assert.calledOnce(releaseRootLifecycleLockStub); + assert.strictEqual(createWithProgressStub.callCount, 0); + }); + + test('root generation remains stable when entry children mutate ctime-backed directory stats', async () => { + const cacheRoot = cacheLayout.getScriptEnvCacheRoot(globalStorageUri); + await fs.ensureDir(cacheRoot.fsPath); + const internalManager = manager as unknown as { + getPhysicalOwnedCacheRoot( + cacheRoot: Uri, + ): Promise<{ path: string; generation: string } | undefined>; + }; + + const before = await internalManager.getPhysicalOwnedCacheRoot(cacheRoot); + await fs.ensureDir(path.join(cacheRoot.fsPath, 'child.lock')); + await fs.writeFile(path.join(cacheRoot.fsPath, 'child.lock', 'marker'), ''); + const after = await internalManager.getPhysicalOwnedCacheRoot(cacheRoot); + + assert.ok(before); + assert.ok(after); + assert.strictEqual(after!.generation, before!.generation); + }); + + test('root generation changes when the same physical path is deleted and recreated', async () => { + const cacheRoot = cacheLayout.getScriptEnvCacheRoot(globalStorageUri); + await fs.ensureDir(cacheRoot.fsPath); + const internalManager = manager as unknown as { + getPhysicalOwnedCacheRoot( + cacheRoot: Uri, + ): Promise<{ path: string; generation: string } | undefined>; + }; + + const before = await internalManager.getPhysicalOwnedCacheRoot(cacheRoot); + await fs.remove(cacheRoot.fsPath); + await fs.ensureDir(cacheRoot.fsPath); + const after = await internalManager.getPhysicalOwnedCacheRoot(cacheRoot); + + assert.ok(before); + assert.ok(after); + assert.strictEqual(normalizePath(after!.path), normalizePath(before!.path)); + assert.notStrictEqual(after!.generation, before!.generation); + }); + + for (const malformedPayload of ['', 'partial', 'g'.repeat(32)]) { + test(`recovers malformed root generation payload ${JSON.stringify(malformedPayload)}`, async () => { + const cacheRoot = cacheLayout.getScriptEnvCacheRoot(globalStorageUri); + const markerPath = path.join(cacheRoot.fsPath, '.root-generation'); + await fs.outputFile(markerPath, malformedPayload); + + assert.ok(await manager.create(scriptUri())); + + assert.match(await fs.readFile(markerPath, 'utf8'), /^[0-9a-f]{32}$/); + assert.deepStrictEqual( + (await fs.readdir(cacheRoot.fsPath)).filter((entry) => + entry.startsWith('.root-generation.'), + ), + [], + ); + }); + } + + test('cleans interrupted generation temp artifacts before atomically publishing the final marker', async () => { + const cacheRoot = cacheLayout.getScriptEnvCacheRoot(globalStorageUri); + const markerPath = path.join(cacheRoot.fsPath, '.root-generation'); + const interruptedTempPath = path.join( + cacheRoot.fsPath, + `.root-generation.tmp-424242-${'a'.repeat(32)}`, + ); + await fs.outputFile(interruptedTempPath, 'partial'); + const renameSpy = sinon.spy(fsExtra, 'rename'); + + assert.ok(await manager.create(scriptUri())); + + assert.strictEqual(await fs.pathExists(interruptedTempPath), false); + assert.match(await fs.readFile(markerPath, 'utf8'), /^[0-9a-f]{32}$/); + assert.ok( + renameSpy.getCalls().some((call) => { + const source = String(call.args[0]); + const destination = String(call.args[1]); + return ( + destination === markerPath && + path.basename(source).startsWith(`.root-generation.tmp-${process.pid}-`) + ); + }), + 'generation must be published by renaming a complete unique temp file', + ); + }); + test('reuses a restart cache entry from an older backup matching the selected base', async () => { const directory = envDir(); const executable = venvPythonPath(directory.fsPath); @@ -1335,7 +1583,7 @@ suite('InlineScriptEnvManager', () => { const [firstResult, secondResult] = await Promise.all([first, second]); assert.strictEqual(firstResult, secondResult); - assert.strictEqual(lockStub.callCount, 1); + assert.strictEqual(cacheEntryLockCalls().length, 1); assert.strictEqual(createWithProgressStub.callCount, 1); }); @@ -1403,7 +1651,7 @@ suite('InlineScriptEnvManager', () => { assert.ok(firstEnvironment); assert.strictEqual(firstEnvironment, secondEnvironment); - assert.strictEqual(lockStub.callCount, 1); + assert.strictEqual(cacheEntryLockCalls().length, 1); assert.strictEqual(createWithProgressStub.callCount, 1); assert.deepStrictEqual( ( @@ -1540,7 +1788,7 @@ suite('InlineScriptEnvManager', () => { assert.ok(firstEnvironment); assert.strictEqual(firstEnvironment, secondEnvironment); assert.strictEqual(createWithProgressStub.callCount, 1); - assert.strictEqual(lockStub.callCount, 2); + assert.strictEqual(cacheEntryLockCalls().length, 2); assert.deepStrictEqual( ( sidecarsByEnvDir.get( @@ -1599,7 +1847,7 @@ suite('InlineScriptEnvManager', () => { sidecarsByEnvDir.set(normalizePath(envDir.fsPath), meta); }); if (failureMode === 'lock') { - lockStub.onSecondCall().rejects(new Error('merge lock failed')); + lockStub.onCall(3).rejects(new Error('merge lock failed')); } else if (failureMode === 'read') { inspectMetaStub.onFirstCall().rejects(new Error('merge read failed')); } else { @@ -1804,7 +2052,7 @@ suite('InlineScriptEnvManager', () => { assert.ok(environments[0]); assert.ok(environments.every((environment) => environment === environments[0])); - assert.strictEqual(lockStub.callCount, 1); + assert.strictEqual(cacheEntryLockCalls().length, 1); const sourceMetadataIdentityHashes = ( sidecarsByEnvDir.get( normalizePath(cacheLayout.getScriptEnvDir(globalStorageUri, cacheKeyValue).fsPath), @@ -3211,7 +3459,7 @@ suite('InlineScriptEnvManager', () => { telemetryCalls(EventNames.INLINE_SCRIPT_ENV_ERROR).map((call) => call.args), [[EventNames.INLINE_SCRIPT_ENV_ERROR, undefined, { category: 'setup-failure' }]], ); - assert.strictEqual(lockStub.callCount, 0); + assert.strictEqual(cacheEntryLockCalls().length, 0); assert.strictEqual(createWithProgressStub.callCount, 0); }); @@ -5948,6 +6196,34 @@ suite('InlineScriptEnvManager', () => { assert.strictEqual(await fs.pathExists(environment.sysPrefix), true); }); + test('stops before deletion when the cache root is replaced at the same physical path', async () => { + const environment = await createOwnedEnvironment(); + const physicalCacheRootPath = await fs.realpath( + cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath, + ); + const internalManager = manager as unknown as { + acquireCacheEntryLockForClear(envDirPath: string): Promise; + deleteCacheEntryForClear(entryPath: string): Promise; + }; + const acquireEntryLock = internalManager.acquireCacheEntryLockForClear.bind(manager); + sinon.stub(internalManager, 'acquireCacheEntryLockForClear').callsFake(async (entryPath) => { + const entryLock = await acquireEntryLock(entryPath); + await fs.remove(physicalCacheRootPath); + await fs.outputFile(path.join(physicalCacheRootPath, CACHE_KEY, 'replacement.txt'), 'keep'); + return entryLock; + }); + const deleteStub = sinon.stub(internalManager, 'deleteCacheEntryForClear').callThrough(); + + await assert.rejects(manager.clearCache(), /physical root changed/); + + sinon.assert.notCalled(deleteStub); + assert.strictEqual( + await fs.readFile(path.join(physicalCacheRootPath, CACHE_KEY, 'replacement.txt'), 'utf8'), + 'keep', + ); + assert.strictEqual(await fs.pathExists(environment.sysPrefix), true); + }); + test('does not let a pending rehydration restore an association after clear cache', async () => { const uri = scriptUri(); const environment = await createOwnedEnvironment(); @@ -6021,5 +6297,45 @@ suite('InlineScriptEnvManager', () => { assert.ok(await createPromise); assert.ok(readMetadataStub.calledOnce); }); + + test('does not count a create queued between two clear requests as active', async () => { + const uri = scriptUri(); + const calls: string[] = []; + let releaseFirstClear: (() => void) | undefined; + let signalFirstClearStarted: (() => void) | undefined; + const firstClearStarted = new Promise((resolve) => { + signalFirstClearStarted = resolve; + }); + const firstClearGate = new Promise((resolve) => { + releaseFirstClear = resolve; + }); + workspaceState.clear.onFirstCall().callsFake(async () => { + calls.push('firstClear'); + signalFirstClearStarted!(); + await firstClearGate; + persistedAssociations = undefined; + }); + workspaceState.clear.onSecondCall().callsFake(async () => { + calls.push('secondClear'); + persistedAssociations = undefined; + }); + readMetadataStub.callsFake(async () => { + calls.push('create'); + return VALID_METADATA; + }); + + const firstClear = manager.clearCache(); + await firstClearStarted; + const createPromise = manager.create(uri); + const secondClear = manager.clearCache(); + + assert.strictEqual(readMetadataStub.callCount, 0); + releaseFirstClear!(); + await Promise.all([firstClear, secondClear]); + + assert.ok(await createPromise); + assert.ok(readMetadataStub.calledOnce); + assert.deepStrictEqual(calls, ['firstClear', 'secondClear', 'create']); + }); }); }); From 78c8f17a158238a3e747d5645067e426770e2e22 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 25 Aug 2026 15:31:19 -0700 Subject: [PATCH 2/7] refactor: trim PR scope to lock retirement, serialized clears, inline-key preservation Reduce this PR's net diff versus its merge-base to only three logical changes: - #3 atomic canonical lock retirement on release (src/common/lockfile.apis.ts) - #6 serialized persistent-state clears (src/common/persistentState.ts) - #7 generic Clear Cache preserves the inline association key (constants.ts, envCommands.ts, extension.ts, and an import-only change in the inline-script envManager) Revert the other seven changes to the merge-base content: - #1 root-generation nonce, #2 root/entry admission decoupling, #9 create-counting order (inline-script envManager) - #4/#5 reclaim-side lock retirement + generation-specific inspection (keep only the minimal release-side companions inspect/reclaim need to stay correct: .release-* recognition and ENOENT-on-readdir tolerance) - #8 workspace-root protection (settingHelpers) - #10 typed ClearCacheNotSupported (envManagers, NotSupportedError) Also revert the associated test changes for the removed items, keeping the new persistentState suite (#6), the new Clear Environment Caches suite (#7), and the release-side lock-retirement tests (#3). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/common/errors/NotSupportedError.ts | 6 - src/common/lockfile.apis.ts | 53 +-- src/features/envManagers.ts | 20 +- src/features/settings/settingHelpers.ts | 6 +- .../builtin/inlineScript/envManager.ts | 424 ++---------------- src/test/common/lockfile.apis.unit.test.ts | 48 -- src/test/features/envCommands.unit.test.ts | 19 +- src/test/features/envManagers.unit.test.ts | 44 -- .../settings/settingHelpers.unit.test.ts | 25 -- .../inlineScript/envManager.unit.test.ts | 342 +------------- 10 files changed, 76 insertions(+), 911 deletions(-) diff --git a/src/common/errors/NotSupportedError.ts b/src/common/errors/NotSupportedError.ts index a8bb427a6..da1677058 100644 --- a/src/common/errors/NotSupportedError.ts +++ b/src/common/errors/NotSupportedError.ts @@ -11,9 +11,3 @@ export class RemoveEnvironmentNotSupported extends BaseError { super('NotSupported', message); } } - -export class ClearCacheNotSupported extends BaseError { - constructor(message: string) { - super('NotSupported', message); - } -} diff --git a/src/common/lockfile.apis.ts b/src/common/lockfile.apis.ts index 10648cc71..7ae66aa2c 100644 --- a/src/common/lockfile.apis.ts +++ b/src/common/lockfile.apis.ts @@ -19,7 +19,6 @@ export interface AcquiredFileLock { export const FILE_LOCK_DIR_SUFFIX = '.lock'; export const FILE_LOCK_OWNER_MARKER_PREFIX = 'owner-'; export const FILE_LOCK_RETAINED_MARKER_PREFIX = 'retained-'; -export const FILE_LOCK_RECLAIM_MARKER_PREFIX = '.reclaim-'; export const FILE_LOCK_RELEASE_MARKER_PREFIX = '.release-'; export const FILE_LOCK_RETIRED_DIR_INFIX = '.retired-'; /** Legacy retained marker. It remains recognizable but cannot be safely reclaimed. */ @@ -141,7 +140,7 @@ export async function inspectFileLock(filePath: string, options?: InspectFileLoc interface FileLockSnapshot { readonly state: FileLockState; readonly marker?: string; - readonly markerKind?: 'owner' | 'retained' | 'reclaim' | 'release'; + readonly markerKind?: 'owner' | 'retained' | 'release'; readonly generationMarker?: string; } @@ -176,14 +175,12 @@ async function inspectFileLockSnapshot( } const ownerEntries = entries.filter((entry) => entry.startsWith(FILE_LOCK_OWNER_MARKER_PREFIX)); const generationRetainedEntries = entries.filter((entry) => entry.startsWith(FILE_LOCK_RETAINED_MARKER_PREFIX)); - const reclaimEntries = entries.filter((entry) => entry.startsWith(FILE_LOCK_RECLAIM_MARKER_PREFIX)); const releaseEntries = entries.filter((entry) => entry.startsWith(FILE_LOCK_RELEASE_MARKER_PREFIX)); const retainedEntries = entries.filter((entry) => entry === FILE_LOCK_RETAINED_MARKER); const unknownEntries = entries.filter( (entry) => !entry.startsWith(FILE_LOCK_OWNER_MARKER_PREFIX) && !entry.startsWith(FILE_LOCK_RETAINED_MARKER_PREFIX) && - !entry.startsWith(FILE_LOCK_RECLAIM_MARKER_PREFIX) && !entry.startsWith(FILE_LOCK_RELEASE_MARKER_PREFIX) && entry !== FILE_LOCK_RETAINED_MARKER, ); @@ -192,13 +189,11 @@ async function inspectFileLockSnapshot( unknownEntries.length > 0 || ownerEntries.length > 1 || generationRetainedEntries.length > 1 || - reclaimEntries.length > 1 || releaseEntries.length > 1 || retainedEntries.length > 1 || - (retainedEntries.length === 1 && - generationRetainedEntries.length + reclaimEntries.length + releaseEntries.length > 0) || + (retainedEntries.length === 1 && generationRetainedEntries.length + releaseEntries.length > 0) || (retainedEntries.length === 0 && - ownerEntries.length + generationRetainedEntries.length + reclaimEntries.length + releaseEntries.length > 1) + ownerEntries.length + generationRetainedEntries.length + releaseEntries.length > 1) ) { return { state: 'malformed' }; } @@ -212,27 +207,6 @@ async function inspectFileLockSnapshot( } return { state: 'retained', marker: generationRetainedEntries[0], markerKind: 'retained' }; } - if (reclaimEntries.length === 1) { - const reclaimMarker = parseTransitionMarker(reclaimEntries[0], FILE_LOCK_RECLAIM_MARKER_PREFIX); - if (!reclaimMarker) { - return { state: 'malformed' }; - } - const liveness = await (options?.checkProcessLiveness ?? getProcessLiveness)(reclaimMarker.pid); - if (liveness === 'dead') { - return { - state: 'stale', - marker: reclaimEntries[0], - markerKind: 'reclaim', - generationMarker: reclaimMarker.generationMarker, - }; - } - return { - state: liveness === 'live' ? 'held' : 'unavailable', - marker: reclaimEntries[0], - markerKind: 'reclaim', - generationMarker: reclaimMarker.generationMarker, - }; - } if (releaseEntries.length === 1) { const releaseMarker = parseTransitionMarker(releaseEntries[0], FILE_LOCK_RELEASE_MARKER_PREFIX); if (!releaseMarker) { @@ -269,7 +243,7 @@ async function inspectFileLockSnapshot( } /** - * Claim the exact observed stale or retained generation, then atomically retire its canonical directory. + * Claim and remove the exact observed stale or retained generation without releasing the lock directory. */ export async function reclaimFileLock(filePath: string, options?: InspectFileLockOptions): Promise { const lockPath = getFileLockPath(filePath); @@ -282,13 +256,12 @@ export async function reclaimFileLock(filePath: string, options?: InspectFileLoc return false; } - const generationMarker = snapshot.generationMarker ?? snapshot.marker; - const claimedMarkerName = - `${FILE_LOCK_RECLAIM_MARKER_PREFIX}${process.pid}-${crypto.randomBytes(16).toString('hex')}-${generationMarker}`; - const claimedMarker = path.join(lockPath, claimedMarkerName); - const observedMarker = path.join(lockPath, snapshot.marker); + const claimedMarker = path.join( + lockPath, + `.reclaim-${process.pid}-${crypto.randomBytes(16).toString('hex')}-${snapshot.marker}`, + ); try { - await fsapi.rename(observedMarker, claimedMarker); + await fsapi.rename(path.join(lockPath, snapshot.marker), claimedMarker); } catch (error) { if (hasErrorCode(error, 'ENOENT') || hasErrorCode(error, 'EEXIST')) { return false; @@ -296,18 +269,16 @@ export async function reclaimFileLock(filePath: string, options?: InspectFileLoc throw error; } - const retiredPath = getRetiredLockPath(lockPath); try { - await retireCanonicalLockDirectory(lockPath, retiredPath); + await fsapi.unlink(claimedMarker); + await fsapi.rmdir(lockPath); + return true; } catch (error) { - await fsapi.rename(claimedMarker, observedMarker).catch(() => undefined); if (hasErrorCode(error, 'ENOENT') || hasErrorCode(error, 'ENOTEMPTY')) { return false; } throw error; } - await cleanupRetiredLock(retiredPath, claimedMarkerName); - return true; } export async function getProcessLiveness(pid: number): Promise { diff --git a/src/features/envManagers.ts b/src/features/envManagers.ts index 755245a87..a99198e41 100644 --- a/src/features/envManagers.ts +++ b/src/features/envManagers.ts @@ -15,7 +15,6 @@ import { EnvironmentManagerAlreadyRegisteredError, PackageManagerAlreadyRegisteredError, } from '../common/errors/AlreadyRegisteredError'; -import { ClearCacheNotSupported } from '../common/errors/NotSupportedError'; import { InlineScriptRouteabilityChangeEvent, InlineScriptRoutingRegistry, @@ -337,29 +336,12 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { return; } - if ( - scope === INLINE_SCRIPT_MANAGER_ID || - (!(scope instanceof Uri) && - typeof scope !== 'string' && - scope.envId.managerId === INLINE_SCRIPT_MANAGER_ID) - ) { - this.throwInlineClearNotSupported(); - } const manager = this.getEnvironmentManager(scope); - if (manager?.id === INLINE_SCRIPT_MANAGER_ID) { - this.throwInlineClearNotSupported(); - } - if (manager) { + if (manager && manager.id !== INLINE_SCRIPT_MANAGER_ID) { await manager.clearCache(); } } - private throwInlineClearNotSupported(): never { - throw new ClearCacheNotSupported( - `Clear Cache for ${INLINE_SCRIPT_MANAGER_ID} requires the dedicated inline-script cache lifecycle.`, - ); - } - /** * Sets the environment for a single scope, scope of undefined checks 'global'. * If given an array of scopes, delegates to setEnvironments for batch setting. diff --git a/src/features/settings/settingHelpers.ts b/src/features/settings/settingHelpers.ts index c8b3322b3..bbea301db 100644 --- a/src/features/settings/settingHelpers.ts +++ b/src/features/settings/settingHelpers.ts @@ -635,13 +635,9 @@ export async function removeInlineScriptPythonProjectSettings( await Promise.all(promises); - const workspaceRootPaths = new Set(workspaceFolders.map((folder) => normalizePath(folder.uri.fsPath))); return Array.from(removedProjects.values()) .map((project) => currentProjectsByUri.get(project.uri.toString())) - .filter( - (project): project is PythonProject => - project !== undefined && !workspaceRootPaths.has(normalizePath(project.uri.fsPath)), - ); + .filter((project): project is PythonProject => project !== undefined); } export async function addPythonProjectSetting(edits: EditProjectSettings[]): Promise { diff --git a/src/managers/builtin/inlineScript/envManager.ts b/src/managers/builtin/inlineScript/envManager.ts index 865024ae3..6da5b3d07 100644 --- a/src/managers/builtin/inlineScript/envManager.ts +++ b/src/managers/builtin/inlineScript/envManager.ts @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import * as crypto from 'crypto'; import * as fs from 'fs-extra'; import * as path from 'path'; import type { Stats } from 'fs'; @@ -91,12 +90,9 @@ const BASE_INTERPRETER_MANAGER_IDS = new Set([ const CACHE_LOCK_TIMEOUT_MS = 5 * 60 * 1000; const CACHE_LOCK_RETRY_MS = 500; -const CACHE_ROOT_GENERATION_FILENAME = '.root-generation'; -const CACHE_ROOT_GENERATION_ARTIFACT_PATTERN = - /^\.root-generation\.(?:tmp|invalid)-\d+-[0-9a-f]{32}$/; -const CACHE_ROOT_GENERATION_PATTERN = /^[0-9a-f]{32}$/; const CACHED_ASSOCIATION_VALIDATION_INTERVAL_MS = 5_000; const DISCOVERY_RETRY_DELAYS_MS = [1_000, 5_000, 30_000] as const; +/** Workspace-state key for PEP 723 script path to environment executable associations. */ export { INLINE_SCRIPT_ENVS_KEY }; const PERSISTED_ASSOCIATION_SCHEMA_VERSION = 1 as const; @@ -179,11 +175,6 @@ interface SavedMetadataSnapshot { readonly identity?: string; } -interface PhysicalCacheRoot { - readonly path: string; - readonly generation: string; -} - /** Manages extension-owned PEP 723 script environments. */ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { private readonly pendingSetups = new Map>(); @@ -268,9 +259,9 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { scope: CreateEnvironmentScope, options?: CreateEnvironmentOptions, ): Promise { - return this.waitForCacheMaintenance(async () => { - this.activeCreateOperations += 1; - try { + this.activeCreateOperations += 1; + try { + return await this.waitForCacheMaintenance(async () => { try { const scriptUri = this.getScriptUri(scope); if (!scriptUri) { @@ -313,10 +304,10 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { this.log.error(`Failed to set up inline-script environment: ${getErrorMessage(error)}`); return undefined; } - } finally { - this.activeCreateOperations -= 1; - } - }); + }); + } finally { + this.activeCreateOperations -= 1; + } } private async createForScript( @@ -484,11 +475,8 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { async clearCache(): Promise { const activeCreatesAtStart = this.activeCreateOperations; - const cacheRoot = getScriptEnvCacheRoot(this.globalStorageUri); return this.enqueueCacheMaintenance(() => - this.enqueueSelection(() => - this.withCacheRootLifecycleLock(cacheRoot, () => this.clearCacheInternal(activeCreatesAtStart)), - ), + this.enqueueSelection(() => this.clearCacheInternal(activeCreatesAtStart)), ); } @@ -2536,189 +2524,18 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { envDir: Uri, action: (lock: AcquiredFileLock) => Promise, ): Promise { - const lock = await this.acquireCacheEntryLockForMutation(envDir); + const lock = await acquireFileLock(envDir.fsPath, { + timeoutMs: CACHE_LOCK_TIMEOUT_MS, + retryIntervalMs: CACHE_LOCK_RETRY_MS, + }); try { return await action(lock); } finally { - await this.releaseCacheLockOrRetain(lock, 'inline-script cache entry'); - } - } - - private async acquireCacheEntryLockForMutation(envDir: Uri): Promise { - const cacheRoot = getScriptEnvCacheRoot(this.globalStorageUri); - const deadline = Date.now() + CACHE_LOCK_TIMEOUT_MS; - let contentionError: unknown; - while (true) { - if (contentionError && Date.now() >= deadline) { - throw contentionError; - } - const remainingMs = Math.max(0, deadline - Date.now()); - const lifecycleLock = await this.acquireCacheRootLifecycleLock(cacheRoot, remainingMs); - let entryLock: AcquiredFileLock | undefined; - let admissionError: unknown; try { - await fs.ensureDir(cacheRoot.fsPath); - if (!(await this.getPhysicalOwnedCacheRoot(cacheRoot))) { - throw new Error(l10n.t('Unable to establish the script environment cache root generation.')); - } - entryLock = await acquireFileLock(envDir.fsPath, { - timeoutMs: 0, - retryIntervalMs: CACHE_LOCK_RETRY_MS, - }); + await lock.release(); } catch (error) { - admissionError = error; - } - - const lifecycleReleaseError = await this.releaseCacheLockOrRetain( - lifecycleLock, - 'inline-script cache lifecycle', - ); - if (lifecycleReleaseError) { - if (entryLock) { - await this.releaseCacheLockOrRetain(entryLock, 'inline-script cache entry'); - } - throw lifecycleReleaseError; + this.log.warn(`Failed to release inline-script cache lock: ${getErrorMessage(error)}`); } - if (entryLock) { - return entryLock; - } - if (!this.isEntryLockWaitError(admissionError)) { - throw admissionError; - } - contentionError = admissionError; - if (Date.now() >= deadline) { - throw contentionError; - } - await this.waitForCacheEntryRetry(deadline); - } - } - - private isEntryLockWaitError(error: unknown): boolean { - return ( - typeof error === 'object' && - error !== null && - 'code' in error && - (error as NodeJS.ErrnoException).code === 'ELOCKED' - ); - } - - private async waitForCacheEntryRetry(deadline: number): Promise { - const delayMs = Math.min(CACHE_LOCK_RETRY_MS, Math.max(0, deadline - Date.now())); - if (delayMs > 0) { - await new Promise((resolve) => setTimeout(resolve, delayMs)); - } - } - - // Creation holds this only through entry-lock acquisition; clear holds it for the sweep. - private async withCacheRootLifecycleLock(cacheRoot: Uri, operation: () => Promise): Promise { - const lock = await this.acquireCacheRootLifecycleLock(cacheRoot); - let operationFailed = false; - try { - return await operation(); - } catch (error) { - operationFailed = true; - throw error; - } finally { - const releaseError = await this.releaseCacheLockOrRetain(lock, 'inline-script cache lifecycle'); - if (releaseError && !operationFailed) { - throw releaseError; - } - } - } - - private async acquireCacheRootLifecycleLock( - cacheRoot: Uri, - timeoutMs: number = CACHE_LOCK_TIMEOUT_MS, - ): Promise { - await this.prepareCacheRootLifecycleLock(cacheRoot); - let lastError: unknown; - for (let attempt = 0; attempt < 3; attempt += 1) { - try { - return await acquireFileLock(cacheRoot.fsPath, { - timeoutMs: 0, - retryIntervalMs: CACHE_LOCK_RETRY_MS, - }); - } catch (error) { - lastError = error; - if (!this.isLockContentionError(error)) { - throw error; - } - const currentState = await inspectFileLock(cacheRoot.fsPath); - if ( - (currentState === 'stale' || currentState === 'retained') && - (await reclaimFileLock(cacheRoot.fsPath)) - ) { - continue; - } - if (currentState === 'missing') { - continue; - } - if (currentState === 'held') { - try { - return await acquireFileLock(cacheRoot.fsPath, { - timeoutMs, - retryIntervalMs: CACHE_LOCK_RETRY_MS, - }); - } catch (waitError) { - if (!this.isLockContentionError(waitError)) { - throw waitError; - } - const finalState = await inspectFileLock(cacheRoot.fsPath); - if ( - (finalState === 'stale' || finalState === 'retained') && - (await reclaimFileLock(cacheRoot.fsPath)) - ) { - continue; - } - if (finalState === 'missing') { - continue; - } - throw waitError; - } - } - throw error; - } - } - if (lastError) { - throw lastError; - } - throw new Error(l10n.t('Unable to acquire the script environment cache lifecycle lock.')); - } - - private async prepareCacheRootLifecycleLock(cacheRoot: Uri): Promise { - this.validateCacheRootLocation(cacheRoot); - const globalStoragePath = path.resolve(this.globalStorageUri.fsPath); - await fs.ensureDir(globalStoragePath); - const globalStorageStat = await fs.lstat(globalStoragePath); - if (!globalStorageStat.isDirectory() || globalStorageStat.isSymbolicLink()) { - this.log.error( - `Refusing to use inline-script cache lifecycle lock from redirected globalStorage root: ${globalStoragePath}`, - ); - throw new Error( - l10n.t( - 'Refusing to clear the script environment cache because the global storage root is not a normal directory.', - ), - ); - } - } - - private async releaseCacheLockOrRetain( - lock: AcquiredFileLock, - label: string, - ): Promise { - try { - await lock.release(); - return undefined; - } catch (error) { - try { - await lock.retain(); - } catch (retainError) { - this.log.error( - `Failed to retain ${label} after release failed: ${getErrorMessage(retainError)}`, - ); - } - this.log.warn(`Failed to release ${label}: ${getErrorMessage(error)}`); - return error; } } @@ -2781,6 +2598,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { const envDir = getScriptEnvDir(this.globalStorageUri, cacheKey); try { + await fs.ensureDir(cacheRoot.fsPath); return await this.withCacheEntryLock(envDir, async (lock) => { const cached = await this.inspectCacheEntry( cacheRoot, @@ -3022,7 +2840,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { } const cacheRoot = getScriptEnvCacheRoot(this.globalStorageUri); - const physicalCacheRoot = await this.getPhysicalOwnedCacheRoot(cacheRoot); + const physicalCacheRootPath = await this.getPhysicalOwnedCacheRootPath(cacheRoot); const persistedAssociations = await this.getPersistedAssociationSnapshot(); const scriptPaths = new Set([ ...Object.keys(persistedAssociations), @@ -3042,10 +2860,10 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { const removedCacheEntries = new Set(); const deletionErrors: unknown[] = []; - if (physicalCacheRoot) { + if (physicalCacheRootPath) { let entryNames: string[]; try { - entryNames = await fs.readdir(physicalCacheRoot.path); + entryNames = await fs.readdir(physicalCacheRootPath); } catch (error) { if (isFileNotFoundError(error)) { entryNames = []; @@ -3056,16 +2874,13 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { const cacheEntryNames = new Set(); for (const entryName of entryNames) { - if (entryName === CACHE_ROOT_GENERATION_FILENAME) { - continue; - } if (entryName.endsWith(FILE_LOCK_DIR_SUFFIX)) { const envName = entryName.slice(0, -FILE_LOCK_DIR_SUFFIX.length); if (envName.length === 0) { const message = l10n.t( 'Refusing to clear the script environment cache because a lock entry is malformed.', ); - this.log.error(`${message} (${path.join(physicalCacheRoot.path, entryName)})`); + this.log.error(`${message} (${path.join(physicalCacheRootPath, entryName)})`); throw new Error(message); } cacheEntryNames.add(envName); @@ -3078,7 +2893,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { try { const removed = await this.removeCacheEntryForClear( cacheRoot, - physicalCacheRoot, + physicalCacheRootPath, entryName, ); if (removed) { @@ -3087,7 +2902,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { } catch (error) { deletionErrors.push(error); this.log.error( - `Failed to remove inline-script cache entry ${path.join(physicalCacheRoot.path, entryName)}: ${getErrorMessage(error)}`, + `Failed to remove inline-script cache entry ${path.join(physicalCacheRootPath, entryName)}: ${getErrorMessage(error)}`, ); } } @@ -3117,31 +2932,32 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { private async removeCacheEntryForClear( cacheRoot: Uri, - originalPhysicalCacheRoot: PhysicalCacheRoot, + originalPhysicalCacheRootPath: string, entryName: string, ): Promise { - const envDirPath = path.join(originalPhysicalCacheRoot.path, entryName); + const envDirPath = path.join(originalPhysicalCacheRootPath, entryName); let lock: AcquiredFileLock | undefined; try { lock = await this.acquireCacheEntryLockForClear(envDirPath); - const currentPhysicalCacheRoot = await this.getPhysicalOwnedCacheRoot(cacheRoot); + const currentPhysicalCacheRootPath = await this.getPhysicalOwnedCacheRootPath(cacheRoot); + if (!currentPhysicalCacheRootPath) { + return undefined; + } if ( - !currentPhysicalCacheRoot || - normalizePath(currentPhysicalCacheRoot.path) !== normalizePath(originalPhysicalCacheRoot.path) || - currentPhysicalCacheRoot.generation !== originalPhysicalCacheRoot.generation + normalizePath(currentPhysicalCacheRootPath) !== normalizePath(originalPhysicalCacheRootPath) ) { const message = l10n.t( - 'Refusing to clear the script environment cache because its physical root changed during cleanup (a different root generation was observed).', + 'Refusing to clear the script environment cache because its physical root changed during cleanup.', ); this.log.error( - `${message} (${originalPhysicalCacheRoot.path} -> ${currentPhysicalCacheRoot?.path ?? 'missing'})`, + `${message} (${originalPhysicalCacheRootPath} -> ${currentPhysicalCacheRootPath})`, ); throw new Error(message); } const entryPath = await this.getClearableCacheEntryPath( - Uri.file(currentPhysicalCacheRoot.path), - path.join(currentPhysicalCacheRoot.path, entryName), + Uri.file(currentPhysicalCacheRootPath), + path.join(currentPhysicalCacheRootPath, entryName), ); if (!entryPath) { return undefined; @@ -3150,13 +2966,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return entryPath; } finally { if (lock) { - const releaseError = await this.releaseCacheLockOrRetain( - lock, - 'inline-script cache entry during cleanup', - ); - if (releaseError) { - throw releaseError; - } + await lock.release(); } } } @@ -3215,153 +3025,17 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { throw new Error(message); } - private async getPhysicalOwnedCacheRoot(cacheRoot: Uri): Promise { - const physicalPath = await this.getPhysicalOwnedCacheRootPath(cacheRoot); - if (!physicalPath) { - return undefined; - } - try { - const stat = await fs.lstat(physicalPath); - if (!stat.isDirectory() || stat.isSymbolicLink()) { - return undefined; - } - return { - path: physicalPath, - generation: await this.getOrCreateCacheRootGenerationUnderLifecycleLock(physicalPath), - }; - } catch (error) { - if (isFileNotFoundError(error)) { - return undefined; - } - throw error; - } - } - - private async getOrCreateCacheRootGenerationUnderLifecycleLock( - physicalCacheRootPath: string, - ): Promise { - const markerPath = path.join(physicalCacheRootPath, CACHE_ROOT_GENERATION_FILENAME); - await this.cleanupCacheRootGenerationArtifacts(physicalCacheRootPath); - let currentGeneration = await this.inspectCacheRootGeneration(markerPath); - if (currentGeneration.kind === 'valid') { - return currentGeneration.generation; - } - if (currentGeneration.kind === 'malformed') { - const invalidPath = path.join( - physicalCacheRootPath, - `${CACHE_ROOT_GENERATION_FILENAME}.invalid-${process.pid}-${crypto.randomBytes(16).toString('hex')}`, - ); - try { - await fs.rename(markerPath, invalidPath); - } catch (error) { - if (!isFileNotFoundError(error)) { - throw error; - } - } - await fs.unlink(invalidPath).catch((error) => { - if (!isFileNotFoundError(error)) { - throw error; - } - }); - currentGeneration = await this.inspectCacheRootGeneration(markerPath); - if (currentGeneration.kind === 'valid') { - return currentGeneration.generation; - } - if (currentGeneration.kind !== 'missing') { - throw new Error( - l10n.t( - 'Refusing to use the script environment cache because its root generation marker could not be recovered.', - ), - ); - } - } - - const proposedGeneration = crypto.randomBytes(16).toString('hex'); - const tempPath = path.join( - physicalCacheRootPath, - `${CACHE_ROOT_GENERATION_FILENAME}.tmp-${process.pid}-${crypto.randomBytes(16).toString('hex')}`, - ); - try { - await fs.writeFile(tempPath, proposedGeneration, { encoding: 'utf8', flag: 'wx' }); - await fs.rename(tempPath, markerPath); - } finally { - await fs.unlink(tempPath).catch(() => undefined); - } - - const publishedGeneration = await this.inspectCacheRootGeneration(markerPath); - if (publishedGeneration.kind !== 'valid') { - throw new Error( - l10n.t('Refusing to use the script environment cache because its root generation could not be published.'), - ); - } - return publishedGeneration.generation; - } - - private async inspectCacheRootGeneration( - markerPath: string, - ): Promise< - | { readonly kind: 'missing' | 'malformed' } - | { readonly kind: 'valid'; readonly generation: string } - > { - let markerStat; - try { - markerStat = await fs.lstat(markerPath); - } catch (error) { - if (isFileNotFoundError(error)) { - return { kind: 'missing' }; - } - throw error; - } - if (!markerStat.isFile() || markerStat.isSymbolicLink()) { - throw new Error( - l10n.t( - 'Refusing to use the script environment cache because its root generation marker is not a normal file.', - ), - ); - } - let generation: string; - try { - generation = await fs.readFile(markerPath, 'utf8'); - } catch (error) { - if (isFileNotFoundError(error)) { - return { kind: 'missing' }; - } - throw error; - } - if (!CACHE_ROOT_GENERATION_PATTERN.test(generation)) { - return { kind: 'malformed' }; - } - return { kind: 'valid', generation }; - } - - private async cleanupCacheRootGenerationArtifacts(physicalCacheRootPath: string): Promise { - const entries = await fs.readdir(physicalCacheRootPath); - for (const entry of entries.filter((candidate) => CACHE_ROOT_GENERATION_ARTIFACT_PATTERN.test(candidate))) { - const artifactPath = path.join(physicalCacheRootPath, entry); - let stat; - try { - stat = await fs.lstat(artifactPath); - } catch (error) { - if (isFileNotFoundError(error)) { - continue; - } - throw error; - } - if (!stat.isFile() || stat.isSymbolicLink()) { - throw new Error( - l10n.t( - 'Refusing to use the script environment cache because a root generation artifact is not a normal file.', - ), - ); - } - await fs.unlink(artifactPath); - } - } - private async getPhysicalOwnedCacheRootPath(cacheRoot: Uri): Promise { - this.validateCacheRootLocation(cacheRoot); const globalStoragePath = path.resolve(this.globalStorageUri.fsPath); const cacheRootPath = path.resolve(cacheRoot.fsPath); + if (path.basename(cacheRootPath) !== INLINE_SCRIPT_CACHE_DIR_NAME || normalizePath(path.dirname(cacheRootPath)) !== normalizePath(globalStoragePath)) { + this.log.error(`Refusing to clear inline-script cache from unsafe root: ${cacheRootPath}`); + throw new Error(l10n.t('Refusing to clear the script environment cache from an unsafe cache root.')); + } + if (isDriveRoot(globalStoragePath) || !hasMinimumPathDepth(cacheRootPath, 3)) { + this.log.error(`Refusing to clear inline-script cache from unsafe root: ${cacheRootPath}`); + throw new Error(l10n.t('Refusing to clear the script environment cache from an unsafe cache root.')); + } let globalStorageStat; try { @@ -3429,20 +3103,6 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return resolvedCacheRootPath; } - private validateCacheRootLocation(cacheRoot: Uri): void { - const globalStoragePath = path.resolve(this.globalStorageUri.fsPath); - const cacheRootPath = path.resolve(cacheRoot.fsPath); - if ( - path.basename(cacheRootPath) !== INLINE_SCRIPT_CACHE_DIR_NAME || - normalizePath(path.dirname(cacheRootPath)) !== normalizePath(globalStoragePath) || - isDriveRoot(globalStoragePath) || - !hasMinimumPathDepth(cacheRootPath, 3) - ) { - this.log.error(`Refusing to clear inline-script cache from unsafe root: ${cacheRootPath}`); - throw new Error(l10n.t('Refusing to clear the script environment cache from an unsafe cache root.')); - } - } - private async getClearableCacheEntryPath(cacheRoot: Uri, entryPath: string): Promise { let stat; try { diff --git a/src/test/common/lockfile.apis.unit.test.ts b/src/test/common/lockfile.apis.unit.test.ts index 5094b25c7..b2e0772a8 100644 --- a/src/test/common/lockfile.apis.unit.test.ts +++ b/src/test/common/lockfile.apis.unit.test.ts @@ -12,7 +12,6 @@ import { acquireFileLock, AcquireFileLockOptions, FILE_LOCK_OWNER_MARKER_PREFIX, - FILE_LOCK_RECLAIM_MARKER_PREFIX, FILE_LOCK_RELEASE_MARKER_PREFIX, FILE_LOCK_RETAINED_MARKER, FILE_LOCK_RETAINED_MARKER_PREFIX, @@ -322,53 +321,6 @@ suite('lockfile APIs', () => { await replacement.release(); }); - test('recovers an interrupted generation-specific reclaim after its claimant exits', async () => { - const lockPath = getFileLockPath(targetPath); - const generationMarker = `${FILE_LOCK_RETAINED_MARKER_PREFIX}424241-generation`; - const reclaimMarker = - `${FILE_LOCK_RECLAIM_MARKER_PREFIX}424242-${'a'.repeat(32)}-${generationMarker}`; - await fs.ensureDir(lockPath); - await fs.writeFile(path.join(lockPath, reclaimMarker), ''); - const checkProcessLiveness = sinon.stub().withArgs(424242).resolves('dead'); - - assert.strictEqual(await inspectFileLock(targetPath, { checkProcessLiveness }), 'stale'); - assert.strictEqual(await reclaimFileLock(targetPath, { checkProcessLiveness }), true); - assert.strictEqual(await inspectFileLock(targetPath), 'missing'); - }); - - test('does not reclaim an in-progress generation-specific reclaim', async () => { - const lockPath = getFileLockPath(targetPath); - const generationMarker = `${FILE_LOCK_OWNER_MARKER_PREFIX}424241-generation`; - const reclaimMarker = - `${FILE_LOCK_RECLAIM_MARKER_PREFIX}${process.pid}-${'b'.repeat(32)}-${generationMarker}`; - await fs.ensureDir(lockPath); - await fs.writeFile(path.join(lockPath, reclaimMarker), ''); - const checkProcessLiveness = sinon.stub().withArgs(process.pid).resolves('live'); - - assert.strictEqual(await inspectFileLock(targetPath, { checkProcessLiveness }), 'held'); - assert.strictEqual(await reclaimFileLock(targetPath, { checkProcessLiveness }), false); - assert.strictEqual(await fs.pathExists(path.join(lockPath, reclaimMarker)), true); - }); - - test('reclaim keeps the canonical path reusable when retired-directory rmdir fails', async () => { - const lock = await acquireFileLock(targetPath, OPTIONS); - await lock.retain(); - const rmdirStub = sinon - .stub(fsExtra, 'rmdir') - .rejects(Object.assign(new Error('rmdir failed'), { code: 'EACCES' })); - - assert.strictEqual(await reclaimFileLock(targetPath), true); - assert.strictEqual(await inspectFileLock(targetPath), 'missing'); - sinon.assert.called(rmdirStub); - assert.deepStrictEqual( - (await fs.readdir(tempRoot)).filter((entry) => entry.includes(FILE_LOCK_RETIRED_DIR_INFIX)), - [], - ); - rmdirStub.restore(); - const replacement = await acquireFileLock(targetPath, OPTIONS); - await replacement.release(); - }); - test('reclaims an interrupted release transition only after its claimant is dead', async () => { const lockPath = getFileLockPath(targetPath); const generationMarker = `${FILE_LOCK_OWNER_MARKER_PREFIX}424241-generation`; diff --git a/src/test/features/envCommands.unit.test.ts b/src/test/features/envCommands.unit.test.ts index 9d3aef267..cc55f2999 100644 --- a/src/test/features/envCommands.unit.test.ts +++ b/src/test/features/envCommands.unit.test.ts @@ -301,10 +301,10 @@ suite('Clear Script Environment Cache Command Tests', () => { assert.deepStrictEqual(calls, ['clearCache', 'getProjects', 'removeInlineSettings', 'removeProjects']); }); - test('keeps the intrinsic workspace-root project loaded after its inline setting is cleaned', async () => { - const workspaceRootProject: PythonProject = { - uri: Uri.file('/workspace'), - name: 'workspace', + test('keeps loaded projects when inline settings cleanup leaves them configured', async () => { + const inlineProject: PythonProject = { + uri: Uri.file('/workspace/runner'), + name: 'runner', }; const clearCache = sinon.stub().resolves(); const envManagers = { @@ -314,21 +314,16 @@ suite('Clear Script Environment Cache Command Tests', () => { }), } as unknown as EnvironmentManagers; const projectManager = { - getProjects: sinon.stub().returns([workspaceRootProject]), + getProjects: sinon.stub().returns([inlineProject]), remove: sinon.stub(), } as unknown as PythonProjectManager; sinon.stub(windowApis, 'showWarningMessage').resolves('Clear Cache' as never); - const removeInlineSettings = sinon - .stub(settingHelpers, 'removeInlineScriptPythonProjectSettings') - .callsFake(async (projects) => { - assert.deepStrictEqual(projects, [workspaceRootProject]); - return []; - }); + const removeInlineSettings = sinon.stub(settingHelpers, 'removeInlineScriptPythonProjectSettings').resolves([]); await clearScriptEnvironmentCacheCommand(envManagers, projectManager); sinon.assert.calledOnce(clearCache); - sinon.assert.calledOnceWithExactly(removeInlineSettings, [workspaceRootProject]); + sinon.assert.calledOnceWithExactly(removeInlineSettings, [inlineProject]); sinon.assert.notCalled(projectManager.remove as sinon.SinonStub); }); diff --git a/src/test/features/envManagers.unit.test.ts b/src/test/features/envManagers.unit.test.ts index 31ce15f0c..aafe89f27 100644 --- a/src/test/features/envManagers.unit.test.ts +++ b/src/test/features/envManagers.unit.test.ts @@ -7,7 +7,6 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import { Uri } from 'vscode'; import { PythonEnvironment } from '../../api'; -import { ClearCacheNotSupported } from '../../common/errors/NotSupportedError'; import * as frameUtils from '../../common/utils/frameUtils'; import * as workspaceApis from '../../common/workspace.apis'; import { PythonEnvironmentManagers } from '../../features/envManagers'; @@ -394,47 +393,4 @@ suite('PythonEnvironmentManagers - clearCache', () => { sinon.assert.calledOnce(systemClearCache); sinon.assert.notCalled(inlineClearCache); }); - - test('rejects an explicitly scoped inline clear in favor of the dedicated lifecycle', async () => { - const inlineClearCache = sandbox.stub().resolves(); - registerManager('inline-script', inlineClearCache); - - await assert.rejects( - envManagers.clearCache('ms-python.python:inline-script'), - (error: unknown) => - error instanceof ClearCacheNotSupported && - error.category === 'NotSupported' && - /dedicated inline-script cache lifecycle/.test(error.message), - ); - - sinon.assert.notCalled(inlineClearCache); - }); - - test('rejects an explicit inline manager id even when the manager is unregistered', async () => { - await assert.rejects( - envManagers.clearCache('ms-python.python:inline-script'), - (error: unknown) => - error instanceof ClearCacheNotSupported && - error.category === 'NotSupported' && - /dedicated inline-script cache lifecycle/.test(error.message), - ); - }); - - test('rejects an explicit inline environment even when the manager is unregistered', async () => { - const inlineEnvironment = { - envId: { id: 'inline-cache-entry', managerId: 'ms-python.python:inline-script' }, - } as PythonEnvironment; - - await assert.rejects( - envManagers.clearCache(inlineEnvironment), - (error: unknown) => - error instanceof ClearCacheNotSupported && - error.category === 'NotSupported' && - /dedicated inline-script cache lifecycle/.test(error.message), - ); - }); - - test('retains URI routing behavior when no inline manager is registered', async () => { - await envManagers.clearCache(Uri.file('/workspace/script.py')); - }); }); diff --git a/src/test/features/settings/settingHelpers.unit.test.ts b/src/test/features/settings/settingHelpers.unit.test.ts index 34ea41527..b454de792 100644 --- a/src/test/features/settings/settingHelpers.unit.test.ts +++ b/src/test/features/settings/settingHelpers.unit.test.ts @@ -858,31 +858,6 @@ suite('Setting Helpers - Project Removal', () => { ]); }); - test('removes an inline root setting without returning the intrinsic workspace project for unloading', async () => { - const rootProject = new PythonProjectsImpl(firstWorkspace.name, firstWorkspace.uri); - const config = createProjectConfig({ - workspaceName: firstWorkspace.name, - workspaceValue: [ - { path: '.', envManager: INLINE_MANAGER_ID, packageManager: PIP_MANAGER_ID }, - ], - }); - sinon.stub(workspaceApis, 'getWorkspaceFolders').returns([firstWorkspace]); - sinon.stub(workspaceApis, 'getWorkspaceFolder').returns(firstWorkspace); - sinon.stub(workspaceApis, 'getConfiguration').returns(config); - - const removedProjects = await removeInlineScriptPythonProjectSettings([rootProject]); - - assert.deepStrictEqual(removedProjects, []); - assert.deepStrictEqual(updateCalls, [ - { - workspace: firstWorkspace.name, - key: 'pythonProjects', - value: undefined, - target: ConfigurationTarget.Workspace, - }, - ]); - }); - test('removes inline-script settings even when the project is not loaded', async () => { const config = createProjectConfig({ workspaceName: firstWorkspace.name, diff --git a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts index 9302150b1..c95bfc782 100644 --- a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts +++ b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts @@ -122,7 +122,6 @@ suite('InlineScriptEnvManager', () => { let inspectMetaStub: sinon.SinonStub; let retainLockStub: sinon.SinonStub; let releaseLockStub: sinon.SinonStub; - let releaseRootLifecycleLockStub: sinon.SinonStub; let resolveSystemPythonStub: sinon.SinonStub; let resolveVenvStub: sinon.SinonStub; let routingRegistry: InlineScriptRoutingRegistry; @@ -206,12 +205,9 @@ suite('InlineScriptEnvManager', () => { }); retainLockStub = sinon.stub().resolves(); releaseLockStub = sinon.stub().resolves(); - releaseRootLifecycleLockStub = sinon.stub().resolves(); - lockStub = sinon.stub(lockfileApis, 'acquireFileLock').callsFake(async (targetPath: string) => - normalizePath(targetPath) === normalizePath(cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath) - ? { release: releaseRootLifecycleLockStub, retain: sinon.stub().resolves() } - : { release: releaseLockStub, retain: retainLockStub }, - ); + lockStub = sinon + .stub(lockfileApis, 'acquireFileLock') + .resolves({ release: releaseLockStub, retain: retainLockStub }); resolveSystemPythonStub = sinon.stub(builtinUtils, 'resolveSystemPythonEnvironmentPath').resolves(undefined); resolveVenvStub = sinon.stub(venvUtils, 'resolveVenvPythonEnvironmentPath').callsFake(async (environmentPath: string) => { return environmentsByExecutablePath.get(normalizePath(environmentPath)); @@ -272,11 +268,6 @@ suite('InlineScriptEnvManager', () => { return cacheLayout.getScriptEnvDir(globalStorageUri, CACHE_KEY); } - function cacheEntryLockCalls(): readonly sinon.SinonSpyCall[] { - const cacheRootPath = normalizePath(cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath); - return lockStub.getCalls().filter((call) => normalizePath(call.args[0]) !== cacheRootPath); - } - function getCacheKeyInputKey(dependencies: readonly string[], interpreterPath: string): string { return JSON.stringify({ dependencies: Array.from( @@ -1238,254 +1229,15 @@ suite('InlineScriptEnvManager', () => { assert.ok(releaseLockStub.calledOnce); }); - test('uses nonblocking entry admission at the final cache path', async () => { + test('uses a bounded cross-process lock at the final cache path', async () => { await manager.create(scriptUri()); - assert.strictEqual( - lockStub.firstCall.args[0], - cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath, - ); - assert.strictEqual(lockStub.firstCall.args[1].timeoutMs, 0); - const entryLockCall = cacheEntryLockCalls()[0]; - assert.ok(entryLockCall); - assert.strictEqual(entryLockCall.args[0], envDir().fsPath); - const options = entryLockCall.args[1]; - assert.strictEqual(options.timeoutMs, 0); + assert.strictEqual(lockStub.firstCall.args[0], envDir().fsPath); + const options = lockStub.firstCall.args[1]; + assert.ok(options.timeoutMs > 0); assert.ok(options.retryIntervalMs > 0); }); - test('releases the root lifecycle lock after entry admission while the build continues', async () => { - let continueCreation: (() => void) | undefined; - let signalCreationStarted: (() => void) | undefined; - const creationStarted = new Promise((resolve) => { - signalCreationStarted = resolve; - }); - const creationGate = new Promise((resolve) => { - continueCreation = resolve; - }); - createWithProgressStub.callsFake(async (...args: unknown[]) => { - const target = args[6] as string; - await fs.outputFile(venvPythonPath(target), ''); - signalCreationStarted!(); - await creationGate; - return { - environment: makeEnvironment( - 'ms-python.python:inline-script', - '3.12.4', - venvPythonPath(target), - target, - ), - }; - }); - - const createPromise = manager.create(scriptUri()); - await creationStarted; - - sinon.assert.calledOnce(releaseRootLifecycleLockStub); - sinon.assert.notCalled(releaseLockStub); - - continueCreation!(); - assert.ok(await createPromise); - sinon.assert.calledOnce(releaseLockStub); - }); - - test('retains a lifecycle lock handle when terminal release fails', async () => { - const failedRootRelease = sinon - .stub() - .rejects(Object.assign(new Error('retirement failed'), { code: 'ELOCKRELEASEFAILED' })); - const retainedRoot = sinon.stub().resolves(); - const rootPath = normalizePath(cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath); - lockStub.callsFake(async (targetPath: string) => - normalizePath(targetPath) === rootPath - ? { release: failedRootRelease, retain: retainedRoot } - : { release: releaseLockStub, retain: retainLockStub }, - ); - - assert.strictEqual(await manager.create(scriptUri()), undefined); - - sinon.assert.calledOnce(failedRootRelease); - sinon.assert.calledOnce(retainedRoot); - sinon.assert.calledOnce(releaseLockStub); - assert.strictEqual(createWithProgressStub.callCount, 0); - }); - - test('key-A contention does not block independent key-B admission at the root', async () => { - const otherCacheKey = 'fedcba9876543210'; - const firstUri = scriptUri('a.py'); - const secondUri = scriptUri('b.py'); - const secondMetadata = { ...VALID_METADATA, dependencies: ['flask'] }; - readMetadataStub.callsFake(async (uri: Uri) => - normalizePath(uri.fsPath) === normalizePath(secondUri.fsPath) ? secondMetadata : VALID_METADATA, - ); - registerCacheKey(otherCacheKey, secondMetadata.dependencies, baseExecutable); - - const rootPath = normalizePath(cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath); - const firstEntryPath = normalizePath(envDir().fsPath); - const secondEntryPath = normalizePath( - cacheLayout.getScriptEnvDir(globalStorageUri, otherCacheKey).fsPath, - ); - let rootHeld = false; - let allowFirstEntry = false; - const calls: string[] = []; - lockStub.callsFake(async (targetPath: string) => { - const normalizedTarget = normalizePath(targetPath); - if (normalizedTarget === rootPath) { - assert.strictEqual(rootHeld, false, 'root lifecycle lock must be released before another admission'); - rootHeld = true; - return { - retain: sinon.stub().resolves(), - release: async () => { - calls.push('release-root'); - rootHeld = false; - }, - }; - } - assert.strictEqual(rootHeld, true, 'entry admission must follow root acquisition'); - if (normalizedTarget === firstEntryPath && !allowFirstEntry) { - calls.push('contend-a'); - throw Object.assign(new Error('key A is locked'), { code: 'ELOCKED' }); - } - calls.push(normalizedTarget === secondEntryPath ? 'admit-b' : 'admit-a'); - return { release: sinon.stub().resolves(), retain: sinon.stub().resolves() }; - }); - - let resumeFirstRetry: (() => void) | undefined; - let signalFirstWaiting: (() => void) | undefined; - const firstWaiting = new Promise((resolve) => { - signalFirstWaiting = resolve; - }); - const internalManager = manager as unknown as { - waitForCacheEntryRetry(deadline: number): Promise; - }; - sinon.stub(internalManager, 'waitForCacheEntryRetry').callsFake( - () => - new Promise((resolve) => { - signalFirstWaiting!(); - resumeFirstRetry = resolve; - }), - ); - - const firstCreate = manager.create(firstUri); - await firstWaiting; - assert.deepStrictEqual(calls.slice(0, 2), ['contend-a', 'release-root']); - - const secondEnvironment = await manager.create(secondUri); - assert.ok(secondEnvironment, 'independent key B should be admitted while key A waits'); - assert.ok(calls.indexOf('admit-b') > calls.indexOf('release-root')); - - allowFirstEntry = true; - resumeFirstRetry!(); - assert.ok(await firstCreate); - assert.strictEqual(rootHeld, false); - }); - - test('entry admission contention stops at the shared deadline without spinning', async () => { - const rootPath = normalizePath(cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath); - lockStub.callsFake(async (targetPath: string) => { - if (normalizePath(targetPath) === rootPath) { - return { release: releaseRootLifecycleLockStub, retain: sinon.stub().resolves() }; - } - throw Object.assign(new Error('entry remains locked'), { code: 'ELOCKED' }); - }); - const internalManager = manager as unknown as { - waitForCacheEntryRetry(deadline: number): Promise; - }; - const waitStub = sinon.stub(internalManager, 'waitForCacheEntryRetry').callsFake(async (deadline) => { - clock.tick(Math.max(0, deadline - Date.now())); - }); - - assert.strictEqual(await manager.create(scriptUri()), undefined); - - sinon.assert.calledOnce(waitStub); - assert.strictEqual(cacheEntryLockCalls().length, 1); - sinon.assert.calledOnce(releaseRootLifecycleLockStub); - assert.strictEqual(createWithProgressStub.callCount, 0); - }); - - test('root generation remains stable when entry children mutate ctime-backed directory stats', async () => { - const cacheRoot = cacheLayout.getScriptEnvCacheRoot(globalStorageUri); - await fs.ensureDir(cacheRoot.fsPath); - const internalManager = manager as unknown as { - getPhysicalOwnedCacheRoot( - cacheRoot: Uri, - ): Promise<{ path: string; generation: string } | undefined>; - }; - - const before = await internalManager.getPhysicalOwnedCacheRoot(cacheRoot); - await fs.ensureDir(path.join(cacheRoot.fsPath, 'child.lock')); - await fs.writeFile(path.join(cacheRoot.fsPath, 'child.lock', 'marker'), ''); - const after = await internalManager.getPhysicalOwnedCacheRoot(cacheRoot); - - assert.ok(before); - assert.ok(after); - assert.strictEqual(after!.generation, before!.generation); - }); - - test('root generation changes when the same physical path is deleted and recreated', async () => { - const cacheRoot = cacheLayout.getScriptEnvCacheRoot(globalStorageUri); - await fs.ensureDir(cacheRoot.fsPath); - const internalManager = manager as unknown as { - getPhysicalOwnedCacheRoot( - cacheRoot: Uri, - ): Promise<{ path: string; generation: string } | undefined>; - }; - - const before = await internalManager.getPhysicalOwnedCacheRoot(cacheRoot); - await fs.remove(cacheRoot.fsPath); - await fs.ensureDir(cacheRoot.fsPath); - const after = await internalManager.getPhysicalOwnedCacheRoot(cacheRoot); - - assert.ok(before); - assert.ok(after); - assert.strictEqual(normalizePath(after!.path), normalizePath(before!.path)); - assert.notStrictEqual(after!.generation, before!.generation); - }); - - for (const malformedPayload of ['', 'partial', 'g'.repeat(32)]) { - test(`recovers malformed root generation payload ${JSON.stringify(malformedPayload)}`, async () => { - const cacheRoot = cacheLayout.getScriptEnvCacheRoot(globalStorageUri); - const markerPath = path.join(cacheRoot.fsPath, '.root-generation'); - await fs.outputFile(markerPath, malformedPayload); - - assert.ok(await manager.create(scriptUri())); - - assert.match(await fs.readFile(markerPath, 'utf8'), /^[0-9a-f]{32}$/); - assert.deepStrictEqual( - (await fs.readdir(cacheRoot.fsPath)).filter((entry) => - entry.startsWith('.root-generation.'), - ), - [], - ); - }); - } - - test('cleans interrupted generation temp artifacts before atomically publishing the final marker', async () => { - const cacheRoot = cacheLayout.getScriptEnvCacheRoot(globalStorageUri); - const markerPath = path.join(cacheRoot.fsPath, '.root-generation'); - const interruptedTempPath = path.join( - cacheRoot.fsPath, - `.root-generation.tmp-424242-${'a'.repeat(32)}`, - ); - await fs.outputFile(interruptedTempPath, 'partial'); - const renameSpy = sinon.spy(fsExtra, 'rename'); - - assert.ok(await manager.create(scriptUri())); - - assert.strictEqual(await fs.pathExists(interruptedTempPath), false); - assert.match(await fs.readFile(markerPath, 'utf8'), /^[0-9a-f]{32}$/); - assert.ok( - renameSpy.getCalls().some((call) => { - const source = String(call.args[0]); - const destination = String(call.args[1]); - return ( - destination === markerPath && - path.basename(source).startsWith(`.root-generation.tmp-${process.pid}-`) - ); - }), - 'generation must be published by renaming a complete unique temp file', - ); - }); - test('reuses a restart cache entry from an older backup matching the selected base', async () => { const directory = envDir(); const executable = venvPythonPath(directory.fsPath); @@ -1583,7 +1335,7 @@ suite('InlineScriptEnvManager', () => { const [firstResult, secondResult] = await Promise.all([first, second]); assert.strictEqual(firstResult, secondResult); - assert.strictEqual(cacheEntryLockCalls().length, 1); + assert.strictEqual(lockStub.callCount, 1); assert.strictEqual(createWithProgressStub.callCount, 1); }); @@ -1651,7 +1403,7 @@ suite('InlineScriptEnvManager', () => { assert.ok(firstEnvironment); assert.strictEqual(firstEnvironment, secondEnvironment); - assert.strictEqual(cacheEntryLockCalls().length, 1); + assert.strictEqual(lockStub.callCount, 1); assert.strictEqual(createWithProgressStub.callCount, 1); assert.deepStrictEqual( ( @@ -1788,7 +1540,7 @@ suite('InlineScriptEnvManager', () => { assert.ok(firstEnvironment); assert.strictEqual(firstEnvironment, secondEnvironment); assert.strictEqual(createWithProgressStub.callCount, 1); - assert.strictEqual(cacheEntryLockCalls().length, 2); + assert.strictEqual(lockStub.callCount, 2); assert.deepStrictEqual( ( sidecarsByEnvDir.get( @@ -1847,7 +1599,7 @@ suite('InlineScriptEnvManager', () => { sidecarsByEnvDir.set(normalizePath(envDir.fsPath), meta); }); if (failureMode === 'lock') { - lockStub.onCall(3).rejects(new Error('merge lock failed')); + lockStub.onSecondCall().rejects(new Error('merge lock failed')); } else if (failureMode === 'read') { inspectMetaStub.onFirstCall().rejects(new Error('merge read failed')); } else { @@ -2052,7 +1804,7 @@ suite('InlineScriptEnvManager', () => { assert.ok(environments[0]); assert.ok(environments.every((environment) => environment === environments[0])); - assert.strictEqual(cacheEntryLockCalls().length, 1); + assert.strictEqual(lockStub.callCount, 1); const sourceMetadataIdentityHashes = ( sidecarsByEnvDir.get( normalizePath(cacheLayout.getScriptEnvDir(globalStorageUri, cacheKeyValue).fsPath), @@ -3459,7 +3211,7 @@ suite('InlineScriptEnvManager', () => { telemetryCalls(EventNames.INLINE_SCRIPT_ENV_ERROR).map((call) => call.args), [[EventNames.INLINE_SCRIPT_ENV_ERROR, undefined, { category: 'setup-failure' }]], ); - assert.strictEqual(cacheEntryLockCalls().length, 0); + assert.strictEqual(lockStub.callCount, 0); assert.strictEqual(createWithProgressStub.callCount, 0); }); @@ -6196,34 +5948,6 @@ suite('InlineScriptEnvManager', () => { assert.strictEqual(await fs.pathExists(environment.sysPrefix), true); }); - test('stops before deletion when the cache root is replaced at the same physical path', async () => { - const environment = await createOwnedEnvironment(); - const physicalCacheRootPath = await fs.realpath( - cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath, - ); - const internalManager = manager as unknown as { - acquireCacheEntryLockForClear(envDirPath: string): Promise; - deleteCacheEntryForClear(entryPath: string): Promise; - }; - const acquireEntryLock = internalManager.acquireCacheEntryLockForClear.bind(manager); - sinon.stub(internalManager, 'acquireCacheEntryLockForClear').callsFake(async (entryPath) => { - const entryLock = await acquireEntryLock(entryPath); - await fs.remove(physicalCacheRootPath); - await fs.outputFile(path.join(physicalCacheRootPath, CACHE_KEY, 'replacement.txt'), 'keep'); - return entryLock; - }); - const deleteStub = sinon.stub(internalManager, 'deleteCacheEntryForClear').callThrough(); - - await assert.rejects(manager.clearCache(), /physical root changed/); - - sinon.assert.notCalled(deleteStub); - assert.strictEqual( - await fs.readFile(path.join(physicalCacheRootPath, CACHE_KEY, 'replacement.txt'), 'utf8'), - 'keep', - ); - assert.strictEqual(await fs.pathExists(environment.sysPrefix), true); - }); - test('does not let a pending rehydration restore an association after clear cache', async () => { const uri = scriptUri(); const environment = await createOwnedEnvironment(); @@ -6297,45 +6021,5 @@ suite('InlineScriptEnvManager', () => { assert.ok(await createPromise); assert.ok(readMetadataStub.calledOnce); }); - - test('does not count a create queued between two clear requests as active', async () => { - const uri = scriptUri(); - const calls: string[] = []; - let releaseFirstClear: (() => void) | undefined; - let signalFirstClearStarted: (() => void) | undefined; - const firstClearStarted = new Promise((resolve) => { - signalFirstClearStarted = resolve; - }); - const firstClearGate = new Promise((resolve) => { - releaseFirstClear = resolve; - }); - workspaceState.clear.onFirstCall().callsFake(async () => { - calls.push('firstClear'); - signalFirstClearStarted!(); - await firstClearGate; - persistedAssociations = undefined; - }); - workspaceState.clear.onSecondCall().callsFake(async () => { - calls.push('secondClear'); - persistedAssociations = undefined; - }); - readMetadataStub.callsFake(async () => { - calls.push('create'); - return VALID_METADATA; - }); - - const firstClear = manager.clearCache(); - await firstClearStarted; - const createPromise = manager.create(uri); - const secondClear = manager.clearCache(); - - assert.strictEqual(readMetadataStub.callCount, 0); - releaseFirstClear!(); - await Promise.all([firstClear, secondClear]); - - assert.ok(await createPromise); - assert.ok(readMetadataStub.calledOnce); - assert.deepStrictEqual(calls, ['firstClear', 'secondClear', 'create']); - }); }); }); From f03b1afa41f385d2515fd4a91c4ea281bb0b368c Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 25 Aug 2026 16:19:24 -0700 Subject: [PATCH 3/7] fix: make inline-lock release resumable on double-rename failure (review feedback) release() could wedge a lock handle when both the canonical-directory retirement AND the ownership-restoration rename failed: it threw with the handle still 'held' and the owner marker already renamed away, so every later release() hit ENOENT -> ECOMPROMISED forever. Introduce a 'releasing' LockState. release() now transitions the owner marker to the '.release-*' marker, sets state 'releasing', then retires the canonical directory. If retirement fails: - restore succeeds -> revert to 'held' and throw ELOCKRELEASEFAILED (unchanged behavior), or - restore fails -> stay 'releasing' and throw ELOCKRELEASEFAILED so a later release() on the same handle resumes retirement instead of wedging. Retired-directory scavenging is intentionally unchanged (a leaked '.retired-*' artifact still persists, per the existing interrupted-artifact test). Add a unit test that fails both the retirement and the restoration rename, asserts the handle reports 'held' with a live '.release-*' marker, then resumes and completes release() on the same handle. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/common/lockfile.apis.ts | 34 ++++++++++++++------ src/test/common/lockfile.apis.unit.test.ts | 37 ++++++++++++++++++++++ 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/src/common/lockfile.apis.ts b/src/common/lockfile.apis.ts index 7ae66aa2c..ac70aa379 100644 --- a/src/common/lockfile.apis.ts +++ b/src/common/lockfile.apis.ts @@ -31,7 +31,7 @@ export interface InspectFileLockOptions { readonly checkProcessLiveness?: (pid: number) => Promise; } -type LockState = 'held' | 'released' | 'retained'; +type LockState = 'held' | 'releasing' | 'released' | 'retained'; const LOCK_RETIRE_MAX_ATTEMPTS = 3; const LOCK_RETIRE_RETRY_MS = 10; @@ -83,28 +83,37 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock } }, release: async () => { - if (state !== 'held') { + if (state === 'released' || state === 'retained') { return; } - try { - await fsapi.rename(ownerMarker, path.join(lockPath, releaseMarkerName)); - } catch (error) { - if (hasErrorCode(error, 'ENOENT')) { - throw createLockError('Lock ownership was compromised', 'ECOMPROMISED', lockPath); + const releaseMarkerPath = path.join(lockPath, releaseMarkerName); + if (state === 'held') { + try { + await fsapi.rename(ownerMarker, releaseMarkerPath); + } catch (error) { + if (hasErrorCode(error, 'ENOENT')) { + throw createLockError('Lock ownership was compromised', 'ECOMPROMISED', lockPath); + } + throw error; } - throw error; + state = 'releasing'; } + // state === 'releasing': the release marker exists; retire the canonical + // directory. Resumable: if retirement fails and ownership cannot be restored, + // the handle stays 'releasing' so a later release() retries retirement instead + // of leaving the handle unable to make progress. const retiredPath = getRetiredLockPath(lockPath); try { await retireCanonicalLockDirectory(lockPath, retiredPath); } catch (error) { const restored = await fsapi - .rename(path.join(lockPath, releaseMarkerName), ownerMarker) + .rename(releaseMarkerPath, ownerMarker) .then( () => true, () => false, ); if (restored) { + state = 'held'; throw createLockError( 'Failed to retire the lock directory; ownership was restored', 'ELOCKRELEASEFAILED', @@ -112,7 +121,12 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock error, ); } - throw error; + throw createLockError( + 'Failed to retire the lock directory; release can be retried', + 'ELOCKRELEASEFAILED', + lockPath, + error, + ); } state = 'released'; await cleanupRetiredLock(retiredPath, releaseMarkerName); diff --git a/src/test/common/lockfile.apis.unit.test.ts b/src/test/common/lockfile.apis.unit.test.ts index b2e0772a8..2cfbc3f09 100644 --- a/src/test/common/lockfile.apis.unit.test.ts +++ b/src/test/common/lockfile.apis.unit.test.ts @@ -236,6 +236,43 @@ suite('lockfile APIs', () => { sinon.assert.called(renameStub); }); + test('release stays resumable when both retirement and ownership restoration fail', async () => { + const lock = await acquireFileLock(targetPath, OPTIONS); + const lockPath = getFileLockPath(targetPath); + const originalRename = fsExtra.rename; + let blockTransition = true; + sinon.stub(fsExtra, 'rename').callsFake(async (source, destination) => { + const resolvedSource = path.resolve(String(source)); + if (blockTransition) { + // Fail the canonical retirement (lockPath -> .retired-*)... + if (resolvedSource === path.resolve(lockPath)) { + throw Object.assign(new Error('access denied'), { code: 'EACCES' }); + } + // ...and fail the restoration rename (.release-* -> owner-*), while still + // allowing the initial owner-* -> .release-* transition to succeed. + if (path.basename(resolvedSource).startsWith(FILE_LOCK_RELEASE_MARKER_PREFIX)) { + throw Object.assign(new Error('access denied'), { code: 'EACCES' }); + } + } + await originalRename(source, destination); + }); + + await assert.rejects( + lock.release(), + (error: NodeJS.ErrnoException) => error.code === 'ELOCKRELEASEFAILED', + ); + // Ownership was NOT restored: a live .release-* marker remains, no owner marker. + const during = await fs.readdir(lockPath); + assert.strictEqual(during.filter((e) => e.startsWith(FILE_LOCK_OWNER_MARKER_PREFIX)).length, 0); + assert.strictEqual(during.filter((e) => e.startsWith(FILE_LOCK_RELEASE_MARKER_PREFIX)).length, 1); + assert.strictEqual(await inspectFileLock(targetPath), 'held'); + + // Retry on the SAME handle: it resumes retirement from the 'releasing' state. + blockTransition = false; + await lock.release(); + assert.strictEqual(await inspectFileLock(targetPath), 'missing'); + }); + test('retained locks fail fast without waiting for the acquisition timeout', async () => { const lock = await acquireFileLock(targetPath, OPTIONS); await lock.retain(); From bddd428aa630390c809f1c115f5e693672b51bfa Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 25 Aug 2026 16:54:18 -0700 Subject: [PATCH 4/7] fix: wait for all persistent-state deletions to settle before advancing clear queue (review feedback) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b7253a6a-a3c5-4606-b9cf-ec52db1e605f --- src/common/persistentState.ts | 8 +++- src/test/common/persistentState.unit.test.ts | 40 ++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/common/persistentState.ts b/src/common/persistentState.ts index 859e1a43f..a9c8ddcb4 100644 --- a/src/common/persistentState.ts +++ b/src/common/persistentState.ts @@ -41,7 +41,13 @@ class PersistentStateImpl implements PersistentState { const preservedKeys = new Set(options?.preserveKeys ?? []); const operation = this.clearQueue.then(async () => { const keysToClear = (requestedKeys ?? this.momento.keys()).filter((key) => !preservedKeys.has(key)); - await Promise.all(keysToClear.map((key) => this.momento.update(key, undefined))); + const results = await Promise.allSettled(keysToClear.map((key) => this.momento.update(key, undefined))); + const failure = results.find( + (result): result is PromiseRejectedResult => result.status === 'rejected', + ); + if (failure) { + throw failure.reason; + } }); this.clearQueue = operation.catch(() => undefined); return operation; diff --git a/src/test/common/persistentState.unit.test.ts b/src/test/common/persistentState.unit.test.ts index c199ebff3..190318ee1 100644 --- a/src/test/common/persistentState.unit.test.ts +++ b/src/test/common/persistentState.unit.test.ts @@ -136,6 +136,46 @@ suite('persistent state clearing', () => { await workspaceState.clear(['new-key']); assert.strictEqual(await workspaceState.get('new-key'), undefined); }); + + test('holds the queue until every deletion settles when a clear partially fails', async () => { + workspace.reset({ + 'fail-fast': 'unused', + 'slow-delete': 'stale', + }); + const gate = createGate(); + workspace.beforeUpdate = async (key, value) => { + if (key === 'fail-fast' && value === undefined) { + throw new Error('memento update failed'); + } + if (key === 'slow-delete' && value === undefined) { + gate.started.resolve(); + await gate.release.promise; + } + }; + + const failedClear = workspaceState.clear(['fail-fast', 'slow-delete']); + await gate.started.promise; + + // A later write is queued while the failing clear's slow deletion is still pending. + const laterSet = workspaceState.set('slow-delete', 'written-later'); + + // The queue must not advance past the clear until the slow deletion settles, + // so the later write has not been applied yet. + assert.strictEqual(workspace.values.get('slow-delete'), 'stale'); + + gate.release.resolve(); + await assert.rejects(failedClear, /memento update failed/); + await laterSet; + + // The later write wins because it was serialized strictly after the deletion settled; + // it is not clobbered by a late in-flight deletion from the failed clear. + assert.strictEqual(workspace.values.get('slow-delete'), 'written-later'); + assert.strictEqual(await workspaceState.get('slow-delete'), 'written-later'); + assert.deepStrictEqual( + workspace.updates.filter((update) => update.key === 'slow-delete').map((update) => update.value), + [undefined, 'written-later'], + ); + }); }); interface TestMemento { From 5e043b13dac9850d18e95ed9e45ee03756f85d6b Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 25 Aug 2026 19:57:28 -0700 Subject: [PATCH 5/7] fix: serialize persistent-state writes and concurrent lock releases (review feedback) - persistentState.set() now joins the clear queue as the new tail so a later clear() cannot be resurrected by a lagging write (review r3858573315 / r3858651583). - Inline lock release() is serialized through a shared in-flight promise so two concurrent releases no longer race and spuriously report ECOMPROMISED; the handle stays retryable after a failed release (review r3858573008 / r3858434379). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b7253a6a-a3c5-4606-b9cf-ec52db1e605f --- src/common/lockfile.apis.ts | 108 +++++++++++-------- src/common/persistentState.ts | 19 ++-- src/test/common/lockfile.apis.unit.test.ts | 31 ++++++ src/test/common/persistentState.unit.test.ts | 31 ++++++ 4 files changed, 134 insertions(+), 55 deletions(-) diff --git a/src/common/lockfile.apis.ts b/src/common/lockfile.apis.ts index ac70aa379..4fbcc0432 100644 --- a/src/common/lockfile.apis.ts +++ b/src/common/lockfile.apis.ts @@ -70,6 +70,56 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock } let state: LockState = 'held'; + let releaseInFlight: Promise | undefined; + const performRelease = async (): Promise => { + if (state === 'released' || state === 'retained') { + return; + } + const releaseMarkerPath = path.join(lockPath, releaseMarkerName); + if (state === 'held') { + try { + await fsapi.rename(ownerMarker, releaseMarkerPath); + } catch (error) { + if (hasErrorCode(error, 'ENOENT')) { + throw createLockError('Lock ownership was compromised', 'ECOMPROMISED', lockPath); + } + throw error; + } + state = 'releasing'; + } + // state === 'releasing': the release marker exists; retire the canonical + // directory. Resumable: if retirement fails and ownership cannot be restored, + // the handle stays 'releasing' so a later release() retries retirement instead + // of leaving the handle unable to make progress. + const retiredPath = getRetiredLockPath(lockPath); + try { + await retireCanonicalLockDirectory(lockPath, retiredPath); + } catch (error) { + const restored = await fsapi + .rename(releaseMarkerPath, ownerMarker) + .then( + () => true, + () => false, + ); + if (restored) { + state = 'held'; + throw createLockError( + 'Failed to retire the lock directory; ownership was restored', + 'ELOCKRELEASEFAILED', + lockPath, + error, + ); + } + throw createLockError( + 'Failed to retire the lock directory; release can be retried', + 'ELOCKRELEASEFAILED', + lockPath, + error, + ); + } + state = 'released'; + await cleanupRetiredLock(retiredPath, releaseMarkerName); + }; return { retain: async () => { if (state !== 'held') { @@ -82,54 +132,18 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock throw createLockError('Failed to mark the lock as retained', 'ERETAINFAILED', lockPath); } }, - release: async () => { - if (state === 'released' || state === 'retained') { - return; - } - const releaseMarkerPath = path.join(lockPath, releaseMarkerName); - if (state === 'held') { - try { - await fsapi.rename(ownerMarker, releaseMarkerPath); - } catch (error) { - if (hasErrorCode(error, 'ENOENT')) { - throw createLockError('Lock ownership was compromised', 'ECOMPROMISED', lockPath); - } - throw error; - } - state = 'releasing'; - } - // state === 'releasing': the release marker exists; retire the canonical - // directory. Resumable: if retirement fails and ownership cannot be restored, - // the handle stays 'releasing' so a later release() retries retirement instead - // of leaving the handle unable to make progress. - const retiredPath = getRetiredLockPath(lockPath); - try { - await retireCanonicalLockDirectory(lockPath, retiredPath); - } catch (error) { - const restored = await fsapi - .rename(releaseMarkerPath, ownerMarker) - .then( - () => true, - () => false, - ); - if (restored) { - state = 'held'; - throw createLockError( - 'Failed to retire the lock directory; ownership was restored', - 'ELOCKRELEASEFAILED', - lockPath, - error, - ); - } - throw createLockError( - 'Failed to retire the lock directory; release can be retried', - 'ELOCKRELEASEFAILED', - lockPath, - error, - ); + release: () => { + // Serialize concurrent release() calls on the same handle: without this, + // two callers can both observe state === 'held' before either owner-marker + // rename completes, and the loser sees ENOENT and reports ECOMPROMISED even + // though the lock was validly released. Sharing one in-flight promise de-dupes + // concurrent calls; clearing it on settle preserves retry-after-failure. + if (!releaseInFlight) { + releaseInFlight = performRelease().finally(() => { + releaseInFlight = undefined; + }); } - state = 'released'; - await cleanupRetiredLock(retiredPath, releaseMarkerName); + return releaseInFlight; }, }; } catch (error) { diff --git a/src/common/persistentState.ts b/src/common/persistentState.ts index a9c8ddcb4..05e90c9b5 100644 --- a/src/common/persistentState.ts +++ b/src/common/persistentState.ts @@ -26,15 +26,18 @@ class PersistentStateImpl implements PersistentState { return this.momento.get(key, defaultValue); } async set(key: string, value: T): Promise { - await this.clearQueue; - await this.momento.update(key, value); + const operation = this.clearQueue.then(async () => { + await this.momento.update(key, value); - const before = JSON.stringify(value); - const after = JSON.stringify(await this.momento.get(key)); - if (before !== after) { - await this.momento.update(key, undefined); - traceError('Error while updating state for key:', key); - } + const before = JSON.stringify(value); + const after = JSON.stringify(await this.momento.get(key)); + if (before !== after) { + await this.momento.update(key, undefined); + traceError('Error while updating state for key:', key); + } + }); + this.clearQueue = operation.catch(() => undefined); + return operation; } async clear(keys?: string[], options?: { readonly preserveKeys?: readonly string[] }): Promise { const requestedKeys = keys ? [...keys] : undefined; diff --git a/src/test/common/lockfile.apis.unit.test.ts b/src/test/common/lockfile.apis.unit.test.ts index 2cfbc3f09..5a1f1b1e9 100644 --- a/src/test/common/lockfile.apis.unit.test.ts +++ b/src/test/common/lockfile.apis.unit.test.ts @@ -273,6 +273,37 @@ suite('lockfile APIs', () => { assert.strictEqual(await inspectFileLock(targetPath), 'missing'); }); + test('serializes concurrent release() calls through a shared in-flight promise', async () => { + const lock = await acquireFileLock(targetPath, OPTIONS); + const originalRename = fsExtra.rename; + let ownerToReleaseRenames = 0; + let signalStarted!: () => void; + const started = new Promise((resolve) => { + signalStarted = resolve; + }); + let openGate!: () => void; + const gate = new Promise((resolve) => { + openGate = resolve; + }); + sinon.stub(fsExtra, 'rename').callsFake(async (source, destination) => { + if (path.basename(String(destination)).startsWith(FILE_LOCK_RELEASE_MARKER_PREFIX)) { + ownerToReleaseRenames += 1; + signalStarted(); + await gate; + } + await originalRename(source, destination); + }); + + const first = lock.release(); + await started; + const second = lock.release(); + openGate(); + await Promise.all([first, second]); + + assert.strictEqual(ownerToReleaseRenames, 1); + assert.strictEqual(await inspectFileLock(targetPath), 'missing'); + }); + test('retained locks fail fast without waiting for the acquisition timeout', async () => { const lock = await acquireFileLock(targetPath, OPTIONS); await lock.retain(); diff --git a/src/test/common/persistentState.unit.test.ts b/src/test/common/persistentState.unit.test.ts index 190318ee1..dfb4496fb 100644 --- a/src/test/common/persistentState.unit.test.ts +++ b/src/test/common/persistentState.unit.test.ts @@ -176,6 +176,37 @@ suite('persistent state clearing', () => { [undefined, 'written-later'], ); }); + + test('serializes a gated write before a later clear so the clear is not resurrected', async () => { + workspace.reset(); + const gate = createGate(); + workspace.beforeUpdate = async (key, value) => { + if (key === 'race-key' && value === 'written') { + gate.started.resolve(); + await gate.release.promise; + } + }; + + // A write is in flight (gated mid-update)... + const gatedSet = workspaceState.set('race-key', 'written'); + await gate.started.promise; + + // ...and a clear for the same key is requested after it. + const laterClear = workspaceState.clear(['race-key']); + + gate.release.resolve(); + await gatedSet; + await laterClear; + + // The clear was requested after the write, so it must win: the lagging write + // cannot resurrect the key after the clear resolves. + assert.strictEqual(workspace.values.has('race-key'), false); + assert.strictEqual(await workspaceState.get('race-key'), undefined); + assert.deepStrictEqual( + workspace.updates.filter((update) => update.key === 'race-key').map((update) => update.value), + ['written', undefined], + ); + }); }); interface TestMemento { From 7506610dcccd782f988b593d1a45824aae4d60dc Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Wed, 26 Aug 2026 14:57:17 -0700 Subject: [PATCH 6/7] refactor: scope inline cache coordination to inline scripts Revert the broad PersistentState mutation queue so non-inline consumers keep their pre-PR get/set/clear behavior. The general concurrency hardening is replaced by the pre-PR clearing gate; only a no-op `preserveKeys` filter and `clearPersistentState(options)` remain so the generic Clear Cache command can preserve the inline association key without changing any other caller. Move the dedicated inline-cache deletion off the shared `PersistentState.clear([key])` (which coalesces onto an in-flight generic clear and could be silently dropped) and onto the inline-owned persistence queue via `set(key, undefined)`. Generic-preserve and inline-delete stay key-disjoint and the dedicated deletion cannot be lost or resurrected. The atomic-rename lock release hardening and its tests are unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b7253a6a-a3c5-4606-b9cf-ec52db1e605f --- src/common/persistentState.ts | 54 +++---- .../builtin/inlineScript/envManager.ts | 20 ++- src/test/common/persistentState.unit.test.ts | 148 +++--------------- .../inlineScript/envManager.unit.test.ts | 65 +++++++- 4 files changed, 119 insertions(+), 168 deletions(-) diff --git a/src/common/persistentState.ts b/src/common/persistentState.ts index 05e90c9b5..6e763e37d 100644 --- a/src/common/persistentState.ts +++ b/src/common/persistentState.ts @@ -1,6 +1,6 @@ import { ExtensionContext, Memento } from 'vscode'; import { traceError } from './logging'; -import { createDeferred } from './utils/deferred'; +import { createDeferred, Deferred } from './utils/deferred'; export interface PersistentState { get(key: string, defaultValue?: T): Promise; @@ -14,46 +14,38 @@ export interface ClearPersistentStateOptions { } class PersistentStateImpl implements PersistentState { - private clearQueue: Promise = Promise.resolve(); - - constructor(private readonly momento: Memento) {} - + private clearing: Deferred; + constructor(private readonly momento: Memento) { + this.clearing = createDeferred(); + this.clearing.resolve(); + } async get(key: string, defaultValue?: T): Promise { - await this.clearQueue; + await this.clearing.promise; if (defaultValue === undefined) { return this.momento.get(key); } return this.momento.get(key, defaultValue); } async set(key: string, value: T): Promise { - const operation = this.clearQueue.then(async () => { - await this.momento.update(key, value); + await this.clearing.promise; + await this.momento.update(key, value); - const before = JSON.stringify(value); - const after = JSON.stringify(await this.momento.get(key)); - if (before !== after) { - await this.momento.update(key, undefined); - traceError('Error while updating state for key:', key); - } - }); - this.clearQueue = operation.catch(() => undefined); - return operation; + const before = JSON.stringify(value); + const after = JSON.stringify(await this.momento.get(key)); + if (before !== after) { + await this.momento.update(key, undefined); + traceError('Error while updating state for key:', key); + } } async clear(keys?: string[], options?: { readonly preserveKeys?: readonly string[] }): Promise { - const requestedKeys = keys ? [...keys] : undefined; - const preservedKeys = new Set(options?.preserveKeys ?? []); - const operation = this.clearQueue.then(async () => { - const keysToClear = (requestedKeys ?? this.momento.keys()).filter((key) => !preservedKeys.has(key)); - const results = await Promise.allSettled(keysToClear.map((key) => this.momento.update(key, undefined))); - const failure = results.find( - (result): result is PromiseRejectedResult => result.status === 'rejected', - ); - if (failure) { - throw failure.reason; - } - }); - this.clearQueue = operation.catch(() => undefined); - return operation; + if (this.clearing.completed) { + this.clearing = createDeferred(); + const preservedKeys = new Set(options?.preserveKeys ?? []); + const _keys = (keys ?? this.momento.keys()).filter((key) => !preservedKeys.has(key)); + await Promise.all(_keys.map((key) => this.momento.update(key, undefined))); + this.clearing.resolve(); + } + return this.clearing.promise; } } diff --git a/src/managers/builtin/inlineScript/envManager.ts b/src/managers/builtin/inlineScript/envManager.ts index 6403c9d14..0d0069553 100644 --- a/src/managers/builtin/inlineScript/envManager.ts +++ b/src/managers/builtin/inlineScript/envManager.ts @@ -2130,6 +2130,22 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return run; } + /** + * Deletes the entire inline-script association record through the inline-owned + * persistence queue using a direct key update (`set(key, undefined)`) rather than + * `PersistentState.clear([key])`. + * + * `PersistentState.clear()` coalesces onto an already in-flight clear, so a dedicated + * inline cache clear issued while the generic "Clear Cache" command is running (that + * command preserves this key) could be silently dropped. A `set()` is never coalesced + * — it runs strictly after any in-flight clear and then unconditionally writes — so the + * dedicated deletion cannot be lost or resurrected. The generic clear never mutates this + * key, keeping the two operations key-disjoint and deterministic. + */ + private clearPersistedAssociations(): Promise { + return this.enqueuePersistence(async (state) => state.set(INLINE_SCRIPT_ENVS_KEY, undefined)); + } + private async waitForCacheMaintenance(operation: () => Promise): Promise { const barrier = this.cacheMaintenanceBarrier; if (barrier) { @@ -3198,7 +3214,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return undefined; } try { - await this.enqueuePersistence(async (state) => state.clear([INLINE_SCRIPT_ENVS_KEY])); + await this.clearPersistedAssociations(); return undefined; } catch (error) { this.log.error(`Failed to clear inline-script environment associations: ${getErrorMessage(error)}`); @@ -3212,7 +3228,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { ); try { if (persistedPathsToClear.length === Object.keys(persistedAssociations).length) { - await this.enqueuePersistence(async (state) => state.clear([INLINE_SCRIPT_ENVS_KEY])); + await this.clearPersistedAssociations(); } else if (persistedPathsToClear.length > 0) { await this.updatePersistedAssociations( persistedPathsToClear.map((scriptPath) => ({ diff --git a/src/test/common/persistentState.unit.test.ts b/src/test/common/persistentState.unit.test.ts index dfb4496fb..a299e15ee 100644 --- a/src/test/common/persistentState.unit.test.ts +++ b/src/test/common/persistentState.unit.test.ts @@ -51,11 +51,13 @@ suite('persistent state clearing', () => { ); }); - test('serializes generic preserve before a racing dedicated inline clear', async () => { + test('completes a dedicated inline key deletion and a concurrent generic preserve-clear without dropping either', async () => { workspace.reset({ [INLINE_SCRIPT_ENVS_KEY]: { script: 'environment' }, 'other-workspace-key': 'remove', }); + global.reset({ 'other-global-key': 'remove' }); + const gate = createGate(); workspace.beforeUpdate = async (key) => { if (key === 'other-workspace-key') { @@ -64,148 +66,36 @@ suite('persistent state clearing', () => { } }; + // The generic Clear Cache preserves the inline association key while clearing + // everything else. Hold it mid-flight... const genericClear = clearPersistentState({ preserveWorkspaceKeys: [INLINE_SCRIPT_ENVS_KEY] }); await gate.started.promise; - const dedicatedClear = workspaceState.clear([INLINE_SCRIPT_ENVS_KEY]); - assert.strictEqual(workspace.values.has(INLINE_SCRIPT_ENVS_KEY), true); + // ...then issue the dedicated inline deletion (a direct set to undefined, the same + // call the inline manager makes). A clear([key]) here would coalesce onto the + // in-flight generic clear and be dropped; a set is never coalesced. + const dedicatedDelete = workspaceState.set(INLINE_SCRIPT_ENVS_KEY, undefined); + gate.release.resolve(); - await genericClear; - await dedicatedClear; + await Promise.all([genericClear, dedicatedDelete]); + // Neither operation was dropped: the generic clear removed every non-inline key and + // the dedicated deletion removed the inline key. assert.strictEqual(workspace.values.has('other-workspace-key'), false); + assert.strictEqual(global.values.has('other-global-key'), false); assert.strictEqual(workspace.values.has(INLINE_SCRIPT_ENVS_KEY), false); - assert.deepStrictEqual( - workspace.updates.map((update) => update.key), - ['other-workspace-key', INLINE_SCRIPT_ENVS_KEY], - ); }); - test('serializes dedicated inline clear before a racing generic preserve', async () => { + test('clears every key by default, matching the pre-refactor implementation', async () => { workspace.reset({ + 'workspace-key-a': 'a', + 'workspace-key-b': 'b', [INLINE_SCRIPT_ENVS_KEY]: { script: 'environment' }, - 'other-workspace-key': 'remove', - }); - const gate = createGate(); - workspace.beforeUpdate = async (key) => { - if (key === INLINE_SCRIPT_ENVS_KEY) { - gate.started.resolve(); - await gate.release.promise; - } - }; - - const dedicatedClear = workspaceState.clear([INLINE_SCRIPT_ENVS_KEY]); - await gate.started.promise; - const genericClear = clearPersistentState({ preserveWorkspaceKeys: [INLINE_SCRIPT_ENVS_KEY] }); - - gate.release.resolve(); - await dedicatedClear; - await genericClear; - - assert.strictEqual(workspace.values.has(INLINE_SCRIPT_ENVS_KEY), false); - assert.strictEqual(workspace.values.has('other-workspace-key'), false); - assert.deepStrictEqual( - workspace.updates.map((update) => update.key), - [INLINE_SCRIPT_ENVS_KEY, 'other-workspace-key'], - ); - }); - - test('settles the queue tail after update failure so later get, set, and clear succeed', async () => { - workspace.reset({ - 'fail-key': 'keep-after-failure', - 'clear-after-failure': 'remove', - 'read-key': 'readable', - }); - let shouldFail = true; - workspace.beforeUpdate = async (key) => { - if (key === 'fail-key' && shouldFail) { - shouldFail = false; - throw new Error('memento update failed'); - } - }; - - const failedClear = workspaceState.clear(['fail-key']); - const successfulClear = workspaceState.clear(['clear-after-failure']); - - await assert.rejects(failedClear, /memento update failed/); - await successfulClear; - assert.strictEqual(await workspaceState.get('read-key'), 'readable'); - - await workspaceState.set('new-key', 'new-value'); - assert.strictEqual(await workspaceState.get('new-key'), 'new-value'); - await workspaceState.clear(['new-key']); - assert.strictEqual(await workspaceState.get('new-key'), undefined); - }); - - test('holds the queue until every deletion settles when a clear partially fails', async () => { - workspace.reset({ - 'fail-fast': 'unused', - 'slow-delete': 'stale', }); - const gate = createGate(); - workspace.beforeUpdate = async (key, value) => { - if (key === 'fail-fast' && value === undefined) { - throw new Error('memento update failed'); - } - if (key === 'slow-delete' && value === undefined) { - gate.started.resolve(); - await gate.release.promise; - } - }; - - const failedClear = workspaceState.clear(['fail-fast', 'slow-delete']); - await gate.started.promise; - - // A later write is queued while the failing clear's slow deletion is still pending. - const laterSet = workspaceState.set('slow-delete', 'written-later'); - // The queue must not advance past the clear until the slow deletion settles, - // so the later write has not been applied yet. - assert.strictEqual(workspace.values.get('slow-delete'), 'stale'); + await workspaceState.clear(); - gate.release.resolve(); - await assert.rejects(failedClear, /memento update failed/); - await laterSet; - - // The later write wins because it was serialized strictly after the deletion settled; - // it is not clobbered by a late in-flight deletion from the failed clear. - assert.strictEqual(workspace.values.get('slow-delete'), 'written-later'); - assert.strictEqual(await workspaceState.get('slow-delete'), 'written-later'); - assert.deepStrictEqual( - workspace.updates.filter((update) => update.key === 'slow-delete').map((update) => update.value), - [undefined, 'written-later'], - ); - }); - - test('serializes a gated write before a later clear so the clear is not resurrected', async () => { - workspace.reset(); - const gate = createGate(); - workspace.beforeUpdate = async (key, value) => { - if (key === 'race-key' && value === 'written') { - gate.started.resolve(); - await gate.release.promise; - } - }; - - // A write is in flight (gated mid-update)... - const gatedSet = workspaceState.set('race-key', 'written'); - await gate.started.promise; - - // ...and a clear for the same key is requested after it. - const laterClear = workspaceState.clear(['race-key']); - - gate.release.resolve(); - await gatedSet; - await laterClear; - - // The clear was requested after the write, so it must win: the lagging write - // cannot resurrect the key after the clear resolves. - assert.strictEqual(workspace.values.has('race-key'), false); - assert.strictEqual(await workspaceState.get('race-key'), undefined); - assert.deepStrictEqual( - workspace.updates.filter((update) => update.key === 'race-key').map((update) => update.value), - ['written', undefined], - ); + assert.strictEqual(workspace.values.size, 0); }); }); diff --git a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts index 0d19a108a..4a09bdaa7 100644 --- a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts +++ b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts @@ -5944,7 +5944,7 @@ suite('InlineScriptEnvManager', () => { await manager.set(uri, environment); const listener = sinon.spy(); manager.onDidChangeEnvironment(listener); - workspaceState.clear.onFirstCall().rejects(new Error('Memento unavailable')); + workspaceState.set.withArgs(INLINE_SCRIPT_ENVS_KEY, undefined).rejects(new Error('Memento unavailable')); await assert.rejects(manager.clearCache(), /Memento unavailable/); @@ -6056,14 +6056,12 @@ suite('InlineScriptEnvManager', () => { const clearStarted = new Promise((resolve) => { signalClearStarted = resolve; }); - workspaceState.clear.callsFake( - async (keys?: string[]) => + workspaceState.set.withArgs(INLINE_SCRIPT_ENVS_KEY, undefined).callsFake( + async () => new Promise((resolve) => { signalClearStarted!(); releaseClear = () => { - if (!keys || keys.includes(INLINE_SCRIPT_ENVS_KEY)) { - persistedAssociations = undefined; - } + persistedAssociations = undefined; resolve(); }; }), @@ -6080,5 +6078,60 @@ suite('InlineScriptEnvManager', () => { assert.ok(await createPromise); assert.ok(readMetadataStub.calledOnce); }); + + test('serializes a dedicated clear-cache deletion after an in-flight association write so the write cannot resurrect it', async () => { + const uri = scriptUri(); + const environment = await createOwnedEnvironment(); + + let releaseWrite: (() => void) | undefined; + let signalWriteStarted: (() => void) | undefined; + const writeStarted = new Promise((resolve) => { + signalWriteStarted = resolve; + }); + workspaceState.set + .withArgs(INLINE_SCRIPT_ENVS_KEY, sinon.match((value: unknown) => value !== undefined)) + .callsFake( + (_key: string, value: unknown) => + new Promise((resolve) => { + persistedAssociations = value; + signalWriteStarted!(); + releaseWrite = resolve; + }), + ); + + const writePromise = manager.set(uri, environment); + await writeStarted; + + // Request the dedicated clear while the association write is still in flight. + const clearPromise = manager.clearCache(); + releaseWrite!(); + await Promise.all([writePromise, clearPromise]); + + assert.strictEqual(persistedAssociations, undefined); + assert.strictEqual(await manager.get(uri), undefined); + }); + + test('keeps the inline persistence queue usable after a failed dedicated deletion', async () => { + const firstUri = scriptUri('first.py'); + const environment = await createOwnedEnvironment(); + await manager.set(firstUri, environment); + workspaceState.set + .withArgs(INLINE_SCRIPT_ENVS_KEY, undefined) + .onFirstCall() + .rejects(new Error('Memento unavailable')); + + await assert.rejects(manager.clearCache(), /Memento unavailable/); + + // The inline-owned queue recovers: a later association write still persists. + const secondUri = scriptUri('second.py'); + const secondEnvironment = await createOwnedEnvironment('fedcba9876543210'); + await manager.set(secondUri, secondEnvironment); + + assert.strictEqual(await manager.get(secondUri), secondEnvironment); + assert.deepStrictEqual( + (persistedAssociations as Record | undefined)?.[normalizePath(secondUri.fsPath)], + matchedAssociationRecord(secondEnvironment.environmentPath.fsPath), + ); + }); }); }); From 57edd3b57c4c972803e1d8f150accc0193ba5b4b Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Wed, 26 Aug 2026 15:58:53 -0700 Subject: [PATCH 7/7] refactor: move inline script associations to an inline-owned store Stop this PR from modifying or depending on the shared PersistentState implementation. src/common/persistentState.ts is reverted to base (no production diff); its general coalescing/failure defect is left unchanged and out of scope for this inline-only PR. Inline PEP 723 script-to-environment associations now live behind a new inline-owned InlineScriptAssociationStore (Memento-backed). It owns only INLINE_SCRIPT_ENVS_KEY, exposes no arbitrary keys, and serializes every read/mutation/deletion on its own failure-isolated FIFO queue: each caller gets its own operation's result or rejection, and a failed operation still advances the tail so later operations run. Verified writes mirror the prior PersistentState.set read-back semantics. All inline association paths (activation load, reads/rehydration, invalid-entry removal, updates, snapshots, and dedicated deletion) route through the store; the manager no longer depends on PersistentState. The dedicated clear is a queued direct update of only INLINE_SCRIPT_ENVS_KEY to undefined, so it cannot be coalesced onto (and dropped by) an overlapping generic clear. Generic "Clear Cache" preserves the inline key without changing PersistentState: it reads the current workspace keys from the injected Memento, excludes INLINE_SCRIPT_ENVS_KEY, and calls the existing PersistentState.clear(explicitKeys) plus the existing global clear() in parallel, keeping the prior command ordering (persistent state, then managers, then shell profile cache). Generic clear and the inline store are therefore key-disjoint and deterministic, and a wedged shared clear cannot block inline association work. Lock-release hardening and non-inline environment managers are unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b7253a6a-a3c5-4606-b9cf-ec52db1e605f --- src/common/persistentState.ts | 19 +- src/extension.ts | 3 +- src/features/envCommands.ts | 12 +- .../builtin/inlineScript/associationStore.ts | 89 +++++++++ .../builtin/inlineScript/envManager.ts | 55 +++--- src/managers/builtin/inlineScript/main.ts | 13 +- src/test/common/persistentState.unit.test.ts | 161 ----------------- src/test/features/envCommands.unit.test.ts | 102 +++++++++-- .../associationStore.unit.test.ts | 169 ++++++++++++++++++ .../inlineScript/envManager.unit.test.ts | 99 ++++++---- .../builtin/inlineScript/main.unit.test.ts | 14 +- 11 files changed, 472 insertions(+), 264 deletions(-) create mode 100644 src/managers/builtin/inlineScript/associationStore.ts delete mode 100644 src/test/common/persistentState.unit.test.ts create mode 100644 src/test/managers/builtin/inlineScript/associationStore.unit.test.ts diff --git a/src/common/persistentState.ts b/src/common/persistentState.ts index 6e763e37d..c6b0f2632 100644 --- a/src/common/persistentState.ts +++ b/src/common/persistentState.ts @@ -5,12 +5,7 @@ import { createDeferred, Deferred } from './utils/deferred'; export interface PersistentState { get(key: string, defaultValue?: T): Promise; set(key: string, value: T): Promise; - clear(keys?: string[], options?: { readonly preserveKeys?: readonly string[] }): Promise; -} - -export interface ClearPersistentStateOptions { - readonly preserveWorkspaceKeys?: readonly string[]; - readonly preserveGlobalKeys?: readonly string[]; + clear(keys?: string[]): Promise; } class PersistentStateImpl implements PersistentState { @@ -37,11 +32,10 @@ class PersistentStateImpl implements PersistentState { traceError('Error while updating state for key:', key); } } - async clear(keys?: string[], options?: { readonly preserveKeys?: readonly string[] }): Promise { + async clear(keys?: string[]): Promise { if (this.clearing.completed) { this.clearing = createDeferred(); - const preservedKeys = new Set(options?.preserveKeys ?? []); - const _keys = (keys ?? this.momento.keys()).filter((key) => !preservedKeys.has(key)); + const _keys = keys ?? this.momento.keys(); await Promise.all(_keys.map((key) => this.momento.update(key, undefined))); this.clearing.resolve(); } @@ -65,11 +59,8 @@ export function getGlobalPersistentState(): Promise { return _global.promise; } -export async function clearPersistentState(options?: ClearPersistentStateOptions): Promise { +export async function clearPersistentState(): Promise { const [workspace, global] = await Promise.all([_workspace.promise, _global.promise]); - await Promise.all([ - workspace.clear(undefined, { preserveKeys: options?.preserveWorkspaceKeys }), - global.clear(undefined, { preserveKeys: options?.preserveGlobalKeys }), - ]); + await Promise.all([workspace.clear(), global.clear()]); return undefined; } diff --git a/src/extension.ts b/src/extension.ts index c94aa747b..86b1324ad 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -402,7 +402,7 @@ export async function activate(context: ExtensionContext): Promise { - await clearEnvironmentCachesCommand(envManagers, shellStartupProviders); + await clearEnvironmentCachesCommand(envManagers, shellStartupProviders, context.workspaceState); }), ...(isInlineScriptsFeatureEnabled() ? [ @@ -699,6 +699,7 @@ export async function activate(context: ExtensionContext): Promise { - await persistentState.clearPersistentState({ preserveWorkspaceKeys: [INLINE_SCRIPT_ENVS_KEY] }); + // Preserve the inline-script association key without changing the shared PersistentState + // implementation: clear every current workspace key except the inline key by passing an + // explicit filtered list to the existing `clear(keys)`, alongside the existing global clear. + const [workspacePersistentState, globalPersistentState] = await Promise.all([ + persistentState.getWorkspacePersistentState(), + persistentState.getGlobalPersistentState(), + ]); + const workspaceKeys = workspaceState.keys().filter((key) => key !== INLINE_SCRIPT_ENVS_KEY); + await Promise.all([workspacePersistentState.clear(workspaceKeys), globalPersistentState.clear()]); await em.clearCache(undefined); await shellProviders.clearShellProfileCache(startupProviders); } diff --git a/src/managers/builtin/inlineScript/associationStore.ts b/src/managers/builtin/inlineScript/associationStore.ts new file mode 100644 index 000000000..e189ec798 --- /dev/null +++ b/src/managers/builtin/inlineScript/associationStore.ts @@ -0,0 +1,89 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { Memento } from 'vscode'; +import { INLINE_SCRIPT_ENVS_KEY } from '../../../common/constants'; +import { traceError } from '../../../common/logging'; + +/** + * Accessor bound to the single inline-script association key ({@link INLINE_SCRIPT_ENVS_KEY}). + * + * It is handed to callers inside a serialized {@link InlineScriptAssociationStore.runExclusive} + * transaction so a read and its dependent write execute as one atomic queue entry. Callers must + * only use it within the transaction they were given it in — enqueuing another store operation + * from inside a transaction would deadlock the queue on itself. + */ +export interface InlineAssociationAccessor { + /** Raw read of the association key. */ + get(): Promise; + /** + * Verified write of the association key. Mirrors `PersistentState.set`: after the update it + * reads the value back and, on a JSON mismatch, clears the key and logs. A rejected update + * propagates to the caller. + */ + update(value: T): Promise; +} + +/** + * Inline-script-owned persistence for PEP 723 script-to-environment associations. + * + * The store owns exactly one workspace-state key ({@link INLINE_SCRIPT_ENVS_KEY}) and never + * exposes arbitrary keys. Every high-level read/mutation/deletion runs on an internal + * failure-isolated FIFO queue: operations execute in invocation order, each caller receives its + * own operation's success or failure, and a rejected operation still advances the queue so later + * operations run. + * + * The store depends only on the injected {@link Memento}; it never touches the shared + * `PersistentState` clear gate, so a wedged generic "Clear Cache" cannot block inline association + * work, and a dedicated inline deletion cannot be coalesced onto (and dropped by) a generic clear. + */ +export class InlineScriptAssociationStore { + private tail: Promise = Promise.resolve(); + private readonly accessor: InlineAssociationAccessor; + + constructor(private readonly memento: Memento) { + this.accessor = { + get: async (): Promise => this.memento.get(INLINE_SCRIPT_ENVS_KEY), + update: async (value: T): Promise => { + await this.memento.update(INLINE_SCRIPT_ENVS_KEY, value); + const before = JSON.stringify(value); + const after = JSON.stringify(await this.memento.get(INLINE_SCRIPT_ENVS_KEY)); + if (before !== after) { + await this.memento.update(INLINE_SCRIPT_ENVS_KEY, undefined); + traceError('Error while updating state for key:', INLINE_SCRIPT_ENVS_KEY); + } + }, + }; + } + + /** + * Serialize `operation` on the FIFO queue. The operation receives an accessor bound to the + * single association key so it can perform a read-modify-write as one atomic transaction. The + * caller receives the operation's own result or rejection; a rejection still advances the + * queue tail so subsequent operations run. + */ + runExclusive(operation: (state: InlineAssociationAccessor) => Promise): Promise { + const run = this.tail.then(() => operation(this.accessor)); + this.tail = run.then( + () => undefined, + () => undefined, + ); + return run; + } + + /** Queued raw read of the association key. */ + read(): Promise { + return this.runExclusive((state) => state.get()); + } + + /** + * Queued dedicated deletion of the association key via a direct key update to `undefined`. + * + * Because this is an ordinary queued write on the inline-owned queue (never a shared + * `PersistentState.clear`), it cannot be coalesced onto an in-flight generic clear and then + * dropped; it runs strictly in invocation order and always writes. + */ + clear(): Promise { + return this.runExclusive((state) => state.update(undefined)); + } +} diff --git a/src/managers/builtin/inlineScript/envManager.ts b/src/managers/builtin/inlineScript/envManager.ts index 0d0069553..38a8ee641 100644 --- a/src/managers/builtin/inlineScript/envManager.ts +++ b/src/managers/builtin/inlineScript/envManager.ts @@ -5,7 +5,7 @@ import * as fs from 'fs-extra'; import * as path from 'path'; import type { Stats } from 'fs'; import { clean as cleanPep440, satisfies as satisfiesPep440 } from '@renovatebot/pep440'; -import { Disposable, Event, EventEmitter, l10n, LogOutputChannel, MarkdownString, ThemeIcon, Uri } from 'vscode'; +import { Disposable, Event, EventEmitter, l10n, LogOutputChannel, MarkdownString, Memento, ThemeIcon, Uri } from 'vscode'; import { CreateEnvironmentOptions, CreateEnvironmentScope, @@ -49,7 +49,6 @@ import { } from '../../../common/inlineScript/routingRegistry'; import { CONDA_MANAGER_ID, - INLINE_SCRIPT_ENVS_KEY, INLINE_SCRIPT_MANAGER_ID, PYENV_MANAGER_ID, SYSTEM_MANAGER_ID, @@ -62,7 +61,7 @@ import { inspectFileLock, reclaimFileLock, } from '../../../common/lockfile.apis'; -import { getWorkspacePersistentState, PersistentState } from '../../../common/persistentState'; +import { InlineAssociationAccessor, InlineScriptAssociationStore } from './associationStore'; import { EventNames, InlineScriptEnvErrorCategory } from '../../../common/telemetry/constants'; import { sendTelemetryEvent } from '../../../common/telemetry/sender'; import { createDeferred, Deferred } from '../../../common/utils/deferred'; @@ -92,8 +91,6 @@ const CACHE_LOCK_TIMEOUT_MS = 5 * 60 * 1000; const CACHE_LOCK_RETRY_MS = 500; const CACHED_ASSOCIATION_VALIDATION_INTERVAL_MS = 5_000; const DISCOVERY_RETRY_DELAYS_MS = [1_000, 5_000, 30_000] as const; -/** Workspace-state key for PEP 723 script path to environment executable associations. */ -export { INLINE_SCRIPT_ENVS_KEY }; const PERSISTED_ASSOCIATION_SCHEMA_VERSION = 1 as const; interface SelectedBaseInterpreter { @@ -196,7 +193,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { private discoveryRetryAttempt = 0; private discoveryRetryTimer: ReturnType | undefined; private readonly subscriptions: Disposable[] = []; - private persistenceQueue: Promise = Promise.resolve(); + private readonly associationStore: InlineScriptAssociationStore; private readonly persistedAssociationsLoaded: Promise; private selectionQueue: Promise = Promise.resolve(); private cacheMaintenanceQueue: Promise = Promise.resolve(); @@ -228,8 +225,10 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { private readonly baseManager: EnvironmentManager, private readonly globalStorageUri: Uri, public readonly log: LogOutputChannel, + workspaceState: Memento, private readonly routingRegistry: InlineScriptRoutingRegistry = new InlineScriptRoutingRegistry(), ) { + this.associationStore = new InlineScriptAssociationStore(workspaceState); this.subscriptions.push( this.routingRegistry.onDidChangeMetadata((event) => { void this.handleSavedMetadataChange(event).catch((error) => { @@ -1814,7 +1813,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { private loadPersistedAssociations(): Promise { return this.enqueuePersistence(async (state) => { - const rawAssociations = await state.get(INLINE_SCRIPT_ENVS_KEY); + const rawAssociations = await state.get(); const parsed = this.parsePersistedAssociations(rawAssociations); this.applyPersistedAssociations(parsed?.records ?? {}); }); @@ -1840,9 +1839,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { } private async getPersistedAssociation(scriptPath: string): Promise { - await this.persistenceQueue; - const state = await getWorkspacePersistentState(); - const rawAssociations = await state.get(INLINE_SCRIPT_ENVS_KEY); + const rawAssociations = await this.associationStore.read(); if (rawAssociations === undefined) { this.applyPersistedAssociations({}); return undefined; @@ -1905,14 +1902,14 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { private removeInvalidPersistedAssociation(scriptPath: string): Promise { return this.enqueuePersistence(async (state) => { - const rawAssociations = await state.get(INLINE_SCRIPT_ENVS_KEY); + const rawAssociations = await state.get(); if (rawAssociations === undefined) { this.applyPersistedAssociations({}); return; } const parsed = this.parsePersistedAssociations(rawAssociations); if (!parsed) { - await state.set(INLINE_SCRIPT_ENVS_KEY, {}); + await state.update({}); this.applyPersistedAssociations({}); return; } @@ -1920,7 +1917,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { delete parsed.rawEntries[scriptPath]; delete parsed.records[scriptPath]; parsed.invalidKeys.delete(scriptPath); - await state.set(INLINE_SCRIPT_ENVS_KEY, parsed.rawEntries); + await state.update(parsed.rawEntries); } this.applyPersistedAssociations(parsed.records); }); @@ -1928,7 +1925,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { private updatePersistedAssociations(changes: readonly PersistedAssociationChange[]): Promise { return this.enqueuePersistence(async (state) => { - const rawAssociations = await state.get(INLINE_SCRIPT_ENVS_KEY); + const rawAssociations = await state.get(); const parsed = this.parsePersistedAssociations(rawAssociations); const rawEntries = { ...(parsed?.rawEntries ?? {}) }; const associations = { ...(parsed?.records ?? {}) }; @@ -1955,7 +1952,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { delete rawEntries[change.scriptPath]; } } - await state.set(INLINE_SCRIPT_ENVS_KEY, rawEntries); + await state.update(rawEntries); this.applyPersistedAssociations(associations); }); } @@ -2124,26 +2121,22 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { }; } - private enqueuePersistence(operation: (state: PersistentState) => Promise): Promise { - const run = this.persistenceQueue.then(async () => operation(await getWorkspacePersistentState())); - this.persistenceQueue = run.catch(() => undefined); - return run; + private enqueuePersistence(operation: (state: InlineAssociationAccessor) => Promise): Promise { + return this.associationStore.runExclusive(operation); } /** - * Deletes the entire inline-script association record through the inline-owned - * persistence queue using a direct key update (`set(key, undefined)`) rather than - * `PersistentState.clear([key])`. + * Deletes the entire inline-script association record through the inline-owned association + * store's failure-isolated queue. * - * `PersistentState.clear()` coalesces onto an already in-flight clear, so a dedicated - * inline cache clear issued while the generic "Clear Cache" command is running (that - * command preserves this key) could be silently dropped. A `set()` is never coalesced - * — it runs strictly after any in-flight clear and then unconditionally writes — so the - * dedicated deletion cannot be lost or resurrected. The generic clear never mutates this - * key, keeping the two operations key-disjoint and deterministic. + * The store issues a direct key update to `undefined` on the inline-owned queue rather than a + * shared `PersistentState.clear`. A generic "Clear Cache" preserves this key and never mutates + * it, so the two operations are key-disjoint; and because this deletion is an ordinary queued + * write (never coalesced onto an in-flight shared clear) it cannot be silently dropped or + * resurrected. */ private clearPersistedAssociations(): Promise { - return this.enqueuePersistence(async (state) => state.set(INLINE_SCRIPT_ENVS_KEY, undefined)); + return this.associationStore.clear(); } private async waitForCacheMaintenance(operation: () => Promise): Promise { @@ -3264,9 +3257,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { } private async getPersistedAssociationSnapshot(): Promise { - await this.persistenceQueue; - const state = await getWorkspacePersistentState(); - return this.parsePersistedAssociations(await state.get(INLINE_SCRIPT_ENVS_KEY))?.records ?? {}; + return this.parsePersistedAssociations(await this.associationStore.read())?.records ?? {}; } private async removeCacheEntry(envDir: Uri): Promise { diff --git a/src/managers/builtin/inlineScript/main.ts b/src/managers/builtin/inlineScript/main.ts index daf9ad4d6..44cee4d17 100644 --- a/src/managers/builtin/inlineScript/main.ts +++ b/src/managers/builtin/inlineScript/main.ts @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { Disposable, LogOutputChannel, Uri } from 'vscode'; +import { Disposable, LogOutputChannel, Memento, Uri } from 'vscode'; import { EnvironmentManager, PythonEnvironmentApi } from '../../../api'; import { traceInfo, traceVerbose } from '../../../common/logging'; import { InlineScriptFeatureActivation } from '../../../features/inlineScript/activation'; @@ -20,6 +20,7 @@ export async function registerInlineScriptFeatures( baseManager: EnvironmentManager, globalStorageUri: Uri, activation: InlineScriptFeatureActivation, + workspaceState: Memento, ): Promise { if (!activation.enabled) { traceVerbose('Inline-script env manager: skipping registration (internal flag is off)'); @@ -31,7 +32,15 @@ export async function registerInlineScriptFeatures( } const api: PythonEnvironmentApi = await getPythonApi(); - const mgr = new InlineScriptEnvManager(nativeFinder, api, baseManager, globalStorageUri, log, routingRegistry); + const mgr = new InlineScriptEnvManager( + nativeFinder, + api, + baseManager, + globalStorageUri, + log, + workspaceState, + routingRegistry, + ); disposables.push(mgr, api.registerEnvironmentManager(mgr)); setImmediate(() => mgr.startActivationDiscovery()); traceInfo('Inline-script env manager: registered (internal flag is on)'); diff --git a/src/test/common/persistentState.unit.test.ts b/src/test/common/persistentState.unit.test.ts deleted file mode 100644 index a299e15ee..000000000 --- a/src/test/common/persistentState.unit.test.ts +++ /dev/null @@ -1,161 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import assert from 'assert'; -import { ExtensionContext, Memento } from 'vscode'; -import { INLINE_SCRIPT_ENVS_KEY } from '../../common/constants'; -import { - clearPersistentState, - getWorkspacePersistentState, - PersistentState, - setPersistentState, -} from '../../common/persistentState'; - -suite('persistent state clearing', () => { - let workspace: TestMemento; - let global: TestMemento; - let workspaceState: PersistentState; - - suiteSetup(async () => { - workspace = createMemento(); - global = createMemento(); - setPersistentState({ - workspaceState: workspace.memento, - globalState: global.memento, - } as ExtensionContext); - workspaceState = await getWorkspacePersistentState(); - }); - - setup(() => { - workspace.reset(); - global.reset(); - }); - - test('clears selected scopes without snapshotting or rewriting preserved inline associations', async () => { - const inlineAssociations = { 'C:\\workspace\\script.py': 'C:\\cache\\python.exe' }; - workspace.reset({ - [INLINE_SCRIPT_ENVS_KEY]: inlineAssociations, - 'other-workspace-key': 'remove', - }); - global.reset({ 'other-global-key': 'remove' }); - - await clearPersistentState({ preserveWorkspaceKeys: [INLINE_SCRIPT_ENVS_KEY] }); - - assert.deepStrictEqual(workspace.values.get(INLINE_SCRIPT_ENVS_KEY), inlineAssociations); - assert.strictEqual(workspace.values.has('other-workspace-key'), false); - assert.strictEqual(global.values.has('other-global-key'), false); - assert.deepStrictEqual( - workspace.updates.filter((update) => update.key === INLINE_SCRIPT_ENVS_KEY), - [], - 'the preserved value must not be snapshot-restored through Memento.update', - ); - }); - - test('completes a dedicated inline key deletion and a concurrent generic preserve-clear without dropping either', async () => { - workspace.reset({ - [INLINE_SCRIPT_ENVS_KEY]: { script: 'environment' }, - 'other-workspace-key': 'remove', - }); - global.reset({ 'other-global-key': 'remove' }); - - const gate = createGate(); - workspace.beforeUpdate = async (key) => { - if (key === 'other-workspace-key') { - gate.started.resolve(); - await gate.release.promise; - } - }; - - // The generic Clear Cache preserves the inline association key while clearing - // everything else. Hold it mid-flight... - const genericClear = clearPersistentState({ preserveWorkspaceKeys: [INLINE_SCRIPT_ENVS_KEY] }); - await gate.started.promise; - - // ...then issue the dedicated inline deletion (a direct set to undefined, the same - // call the inline manager makes). A clear([key]) here would coalesce onto the - // in-flight generic clear and be dropped; a set is never coalesced. - const dedicatedDelete = workspaceState.set(INLINE_SCRIPT_ENVS_KEY, undefined); - - gate.release.resolve(); - await Promise.all([genericClear, dedicatedDelete]); - - // Neither operation was dropped: the generic clear removed every non-inline key and - // the dedicated deletion removed the inline key. - assert.strictEqual(workspace.values.has('other-workspace-key'), false); - assert.strictEqual(global.values.has('other-global-key'), false); - assert.strictEqual(workspace.values.has(INLINE_SCRIPT_ENVS_KEY), false); - }); - - test('clears every key by default, matching the pre-refactor implementation', async () => { - workspace.reset({ - 'workspace-key-a': 'a', - 'workspace-key-b': 'b', - [INLINE_SCRIPT_ENVS_KEY]: { script: 'environment' }, - }); - - await workspaceState.clear(); - - assert.strictEqual(workspace.values.size, 0); - }); -}); - -interface TestMemento { - readonly memento: Memento; - readonly values: Map; - readonly updates: Array<{ key: string; value: unknown }>; - beforeUpdate?: (key: string, value: unknown) => Promise; - reset(initial?: Record): void; -} - -function createMemento(): TestMemento { - const values = new Map(); - const updates: Array<{ key: string; value: unknown }> = []; - const result: TestMemento = { - memento: undefined as unknown as Memento, - values, - updates, - reset: (initial = {}) => { - values.clear(); - Object.entries(initial).forEach(([key, value]) => values.set(key, value)); - updates.splice(0, updates.length); - result.beforeUpdate = undefined; - }, - }; - const memento = { - keys: () => Array.from(values.keys()), - get: (key: string, defaultValue?: T): T | undefined => - (values.has(key) ? values.get(key) : defaultValue) as T | undefined, - update: async (key: string, value: unknown): Promise => { - await result.beforeUpdate?.(key, value); - updates.push({ key, value }); - if (value === undefined) { - values.delete(key); - } else { - values.set(key, value); - } - }, - } as Memento; - (result as { memento: Memento }).memento = memento; - return result; -} - -function createGate(): { - readonly started: { readonly promise: Promise; resolve(): void }; - readonly release: { readonly promise: Promise; resolve(): void }; -} { - return { - started: createSignal(), - release: createSignal(), - }; -} - -function createSignal(): { readonly promise: Promise; resolve(): void } { - let resolvePromise: (() => void) | undefined; - const promise = new Promise((resolve) => { - resolvePromise = resolve; - }); - return { - promise, - resolve: () => resolvePromise!(), - }; -} diff --git a/src/test/features/envCommands.unit.test.ts b/src/test/features/envCommands.unit.test.ts index e0168bf21..d1159928b 100644 --- a/src/test/features/envCommands.unit.test.ts +++ b/src/test/features/envCommands.unit.test.ts @@ -1,7 +1,7 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import * as typeMoq from 'typemoq'; -import { Terminal, Uri } from 'vscode'; +import { Memento, Terminal, Uri } from 'vscode'; import { PythonEnvironment, PythonEnvironmentApi, PythonProject } from '../../api'; import * as commandApi from '../../common/command.api'; import { INLINE_SCRIPT_ENVS_KEY, INLINE_SCRIPT_MANAGER_ID } from '../../common/constants'; @@ -360,26 +360,54 @@ suite('Clear Environment Caches Command Tests', () => { sinon.restore(); }); - test('generic clear preserves inline association persistence while clearing other state and managers', async () => { + function makeWorkspaceMemento(store: Map): Memento { + return { + get: (key: string) => store.get(key) as T | undefined, + update: async (key: string, value: unknown) => { + if (value === undefined) { + store.delete(key); + } else { + store.set(key, value); + } + }, + keys: () => [...store.keys()], + } as unknown as Memento; + } + + test('generic clear preserves the inline association key while clearing other workspace/global state and managers', async () => { const inlineAssociations = { 'C:\\workspace\\script.py': 'C:\\cache\\python.exe' }; - const workspaceState = new Map([ + const store = new Map([ [INLINE_SCRIPT_ENVS_KEY, inlineAssociations], ['other-workspace-state', { stale: true }], ]); + const workspaceState = makeWorkspaceMemento(store); + const calls: string[] = []; - sinon.stub(persistentState, 'clearPersistentState').callsFake(async (options) => { - calls.push('persistent'); - const preserved = new Set(options?.preserveWorkspaceKeys ?? []); - for (const key of workspaceState.keys()) { - if (!preserved.has(key)) { - workspaceState.delete(key); + let clearedWorkspaceKeys: readonly string[] | undefined; + let globalCleared = false; + const workspacePersistent = { + clear: sinon.stub().callsFake(async (keys?: string[]) => { + calls.push('workspace'); + clearedWorkspaceKeys = keys; + for (const key of keys ?? [...store.keys()]) { + store.delete(key); } - } - }); + }), + } as unknown as persistentState.PersistentState; + const globalPersistent = { + clear: sinon.stub().callsFake(async () => { + calls.push('global'); + globalCleared = true; + }), + } as unknown as persistentState.PersistentState; + sinon.stub(persistentState, 'getWorkspacePersistentState').resolves(workspacePersistent); + sinon.stub(persistentState, 'getGlobalPersistentState').resolves(globalPersistent); + const envManagers = { clearCache: sinon.stub().callsFake(async () => { calls.push('managers'); - assert.deepStrictEqual(workspaceState.get(INLINE_SCRIPT_ENVS_KEY), inlineAssociations); + // The inline association key must still be present when non-inline managers run. + assert.deepStrictEqual(store.get(INLINE_SCRIPT_ENVS_KEY), inlineAssociations); }), } as unknown as EnvironmentManagers; const startupProvider = { @@ -391,13 +419,53 @@ suite('Clear Environment Caches Command Tests', () => { await Promise.all(providers.map((provider) => provider.clearCache())); }); - await clearEnvironmentCachesCommand(envManagers, [startupProvider]); - - assert.deepStrictEqual(calls, ['persistent', 'managers', 'shells']); - assert.deepStrictEqual(workspaceState.get(INLINE_SCRIPT_ENVS_KEY), inlineAssociations); - assert.strictEqual(workspaceState.has('other-workspace-state'), false); + await clearEnvironmentCachesCommand(envManagers, [startupProvider], workspaceState); + + // Only the inline key is excluded from the explicit workspace key list handed to clear(). + assert.deepStrictEqual(clearedWorkspaceKeys, ['other-workspace-state']); + assert.strictEqual(globalCleared, true); + // Persistent state (workspace + global) is cleared before managers, which run before shells. + assert.ok(calls.indexOf('managers') > calls.indexOf('workspace'), 'managers run after workspace clear'); + assert.ok(calls.indexOf('managers') > calls.indexOf('global'), 'managers run after global clear'); + assert.ok(calls.indexOf('shells') > calls.indexOf('managers'), 'shells run after managers'); + assert.strictEqual(calls[calls.length - 1], 'shells'); + // Inline association key preserved; other workspace key cleared. + assert.deepStrictEqual(store.get(INLINE_SCRIPT_ENVS_KEY), inlineAssociations); + assert.strictEqual(store.has('other-workspace-state'), false); sinon.assert.calledOnceWithExactly(envManagers.clearCache as sinon.SinonStub, undefined); }); + + test('generic clear preserves a dormant inline association key when the inline manager is not registered', async () => { + const inlineAssociations = { 'C:\\workspace\\script.py': 'C:\\cache\\python.exe' }; + const store = new Map([[INLINE_SCRIPT_ENVS_KEY, inlineAssociations]]); + const workspaceState = makeWorkspaceMemento(store); + + let clearedWorkspaceKeys: readonly string[] | undefined; + const workspacePersistent = { + clear: sinon.stub().callsFake(async (keys?: string[]) => { + clearedWorkspaceKeys = keys; + for (const key of keys ?? [...store.keys()]) { + store.delete(key); + } + }), + } as unknown as persistentState.PersistentState; + const globalPersistent = { + clear: sinon.stub().resolves(), + } as unknown as persistentState.PersistentState; + sinon.stub(persistentState, 'getWorkspacePersistentState').resolves(workspacePersistent); + sinon.stub(persistentState, 'getGlobalPersistentState').resolves(globalPersistent); + + const envManagers = { + clearCache: sinon.stub().resolves(), + } as unknown as EnvironmentManagers; + sinon.stub(shellProviders, 'clearShellProfileCache').resolves(); + + await clearEnvironmentCachesCommand(envManagers, [], workspaceState); + + // The only workspace key is the inline key, so the explicit list is empty and it is preserved. + assert.deepStrictEqual(clearedWorkspaceKeys, []); + assert.deepStrictEqual(store.get(INLINE_SCRIPT_ENVS_KEY), inlineAssociations); + }); }); suite('Reveal Env In Manager View Command Tests', () => { diff --git a/src/test/managers/builtin/inlineScript/associationStore.unit.test.ts b/src/test/managers/builtin/inlineScript/associationStore.unit.test.ts new file mode 100644 index 000000000..8436f520a --- /dev/null +++ b/src/test/managers/builtin/inlineScript/associationStore.unit.test.ts @@ -0,0 +1,169 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import { Memento } from 'vscode'; +import { INLINE_SCRIPT_ENVS_KEY } from '../../../../common/constants'; +import * as logging from '../../../../common/logging'; +import { InlineScriptAssociationStore } from '../../../../managers/builtin/inlineScript/associationStore'; + +function createMemento(initial?: Record): { memento: Memento; store: Map } { + const store = new Map(initial ? Object.entries(initial) : []); + const memento = { + get: (key: string, defaultValue?: T) => (store.has(key) ? (store.get(key) as T) : defaultValue), + update: async (key: string, value: unknown) => { + if (value === undefined) { + store.delete(key); + } else { + store.set(key, value); + } + }, + keys: () => [...store.keys()], + } as unknown as Memento; + return { memento, store }; +} + +function delay(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +suite('InlineScriptAssociationStore', () => { + teardown(() => { + sinon.restore(); + }); + + test('runs queued operations strictly in invocation order', async () => { + const { memento } = createMemento(); + const store = new InlineScriptAssociationStore(memento); + const order: number[] = []; + + const first = store.runExclusive(async () => { + await delay(10); + order.push(1); + }); + const second = store.runExclusive(async () => { + order.push(2); + }); + const third = store.runExclusive(async () => { + await delay(5); + order.push(3); + }); + + await Promise.all([first, second, third]); + assert.deepStrictEqual(order, [1, 2, 3]); + }); + + test('serializes a read-modify-write transaction as one atomic queue entry', async () => { + const { memento, store: backing } = createMemento(); + const store = new InlineScriptAssociationStore(memento); + + // Two interleaved read-modify-writes must not clobber each other. + const writeA = store.runExclusive(async (state) => { + const current = ((await state.get>()) ?? {}) as Record; + await delay(10); + await state.update({ ...current, 'a.py': 'env-a' }); + }); + const writeB = store.runExclusive(async (state) => { + const current = ((await state.get>()) ?? {}) as Record; + await state.update({ ...current, 'b.py': 'env-b' }); + }); + + await Promise.all([writeA, writeB]); + assert.deepStrictEqual(backing.get(INLINE_SCRIPT_ENVS_KEY), { 'a.py': 'env-a', 'b.py': 'env-b' }); + }); + + test('a dedicated clear queued behind an in-flight write wins and is not resurrected', async () => { + const { memento, store: backing } = createMemento(); + const store = new InlineScriptAssociationStore(memento); + + let releaseWrite!: () => void; + const writeGate = new Promise((resolve) => { + releaseWrite = resolve; + }); + + const writePromise = store.runExclusive(async (state) => { + await writeGate; + await state.update({ 'a.py': 'env-a' }); + }); + // Request the deletion while the write is still in flight. + const clearPromise = store.clear(); + releaseWrite(); + await Promise.all([writePromise, clearPromise]); + + assert.strictEqual(backing.has(INLINE_SCRIPT_ENVS_KEY), false); + assert.strictEqual(await store.read(), undefined); + }); + + test('a failed operation rejects its caller but the queue keeps running later operations', async () => { + const { memento, store: backing } = createMemento(); + const store = new InlineScriptAssociationStore(memento); + + const failing = store.runExclusive(async () => { + throw new Error('boom'); + }); + // Queue a follow-up behind the failing operation before awaiting the rejection. + const later = store.runExclusive((state) => state.update({ 'b.py': 'env-b' })); + + await assert.rejects(failing, /boom/); + await later; + assert.deepStrictEqual(backing.get(INLINE_SCRIPT_ENVS_KEY), { 'b.py': 'env-b' }); + }); + + test('read returns the latest value written through the queue', async () => { + const { memento } = createMemento(); + const store = new InlineScriptAssociationStore(memento); + + assert.strictEqual(await store.read(), undefined); + await store.runExclusive((state) => state.update({ 'c.py': 'env-c' })); + assert.deepStrictEqual(await store.read(), { 'c.py': 'env-c' }); + await store.clear(); + assert.strictEqual(await store.read(), undefined); + }); + + test('a verified write clears the key and logs when the read-back does not match', async () => { + const traceErrorStub = sinon.stub(logging, 'traceError'); + const backing = new Map(); + const memento = { + get: (key: string) => backing.get(key) as T | undefined, + update: async (key: string, value: unknown) => { + if (value === undefined) { + backing.delete(key); + } + // Silently drop non-undefined writes to force a read-back mismatch. + }, + keys: () => [...backing.keys()], + } as unknown as Memento; + const store = new InlineScriptAssociationStore(memento); + + await store.runExclusive((state) => state.update({ 'd.py': 'env-d' })); + + assert.strictEqual(backing.has(INLINE_SCRIPT_ENVS_KEY), false, 'a corrupt write must not be left persisted'); + sinon.assert.calledWithMatch(traceErrorStub, sinon.match.string, INLINE_SCRIPT_ENVS_KEY); + }); + + test('store operations resolve independently of a wedged external clear promise', async () => { + const { memento, store: backing } = createMemento(); + const store = new InlineScriptAssociationStore(memento); + + // Simulate a wedged shared PersistentState.clear() that never settles. The inline store + // holds no reference to it, so its own operations must still complete promptly. + const wedged = new Promise(() => undefined); + void wedged; + + const timeout = delay(1000).then(() => 'timeout' as const); + const write = (async () => { + await store.runExclusive((state) => state.update({ 'e.py': 'env-e' })); + return 'done' as const; + })(); + assert.strictEqual(await Promise.race([write, timeout]), 'done'); + assert.deepStrictEqual(backing.get(INLINE_SCRIPT_ENVS_KEY), { 'e.py': 'env-e' }); + + const clear = (async () => { + await store.clear(); + return 'done' as const; + })(); + assert.strictEqual(await Promise.race([clear, timeout]), 'done'); + assert.strictEqual(backing.has(INLINE_SCRIPT_ENVS_KEY), false); + }); +}); diff --git a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts index 4a09bdaa7..7c6c880cc 100644 --- a/src/test/managers/builtin/inlineScript/envManager.unit.test.ts +++ b/src/test/managers/builtin/inlineScript/envManager.unit.test.ts @@ -7,7 +7,7 @@ import * as fs from 'fs-extra'; import * as os from 'os'; import * as path from 'path'; import * as sinon from 'sinon'; -import { Disposable, LogOutputChannel, TextDocument, Uri } from 'vscode'; +import { Disposable, LogOutputChannel, Memento, TextDocument, Uri } from 'vscode'; import { EnvironmentChangeKind, EnvironmentManager, @@ -19,17 +19,14 @@ import * as cacheLayout from '../../../../common/inlineScript/cacheLayout'; import * as metadataReader from '../../../../common/inlineScript/metadata'; import { InlineScriptRoutingRegistry } from '../../../../common/inlineScript/routingRegistry'; import * as lockfileApis from '../../../../common/lockfile.apis'; -import * as persistentState from '../../../../common/persistentState'; +import { INLINE_SCRIPT_ENVS_KEY } from '../../../../common/constants'; import { EventNames } from '../../../../common/telemetry/constants'; import * as telemetrySender from '../../../../common/telemetry/sender'; import { isWindows } from '../../../../common/utils/platformUtils'; import { normalizePath } from '../../../../common/utils/pathUtils'; import { getVenvPythonPath } from '../../../../common/utils/virtualEnvironment'; import * as workspaceApis from '../../../../common/workspace.apis'; -import { - InlineScriptEnvManager, - INLINE_SCRIPT_ENVS_KEY, -} from '../../../../managers/builtin/inlineScript/envManager'; +import { InlineScriptEnvManager } from '../../../../managers/builtin/inlineScript/envManager'; import * as builtinUtils from '../../../../managers/builtin/utils'; import * as uvPythonInstaller from '../../../../managers/builtin/uvPythonInstaller'; import * as venvUtils from '../../../../managers/builtin/venvUtils'; @@ -135,9 +132,10 @@ suite('InlineScriptEnvManager', () => { let renameFilesListener: ((e: { files: readonly { oldUri: Uri; newUri: Uri }[] }) => unknown) | undefined; let workspaceState: { get: sinon.SinonStub; - set: sinon.SinonStub; - clear: sinon.SinonStub; + update: sinon.SinonStub; + keys: sinon.SinonStub; }; + let workspaceMemento: Memento; let persistedAssociations: unknown; setup(async () => { @@ -166,18 +164,16 @@ suite('InlineScriptEnvManager', () => { get: sinon.stub().callsFake(async (key: string) => { return key === INLINE_SCRIPT_ENVS_KEY ? persistedAssociations : undefined; }), - set: sinon.stub().callsFake(async (key: string, value: unknown) => { + update: sinon.stub().callsFake(async (key: string, value: unknown) => { if (key === INLINE_SCRIPT_ENVS_KEY) { persistedAssociations = value; } }), - clear: sinon.stub().callsFake(async (keys?: string[]) => { - if (!keys || keys.includes(INLINE_SCRIPT_ENVS_KEY)) { - persistedAssociations = undefined; - } - }), + keys: sinon.stub().callsFake(() => + persistedAssociations === undefined ? [] : [INLINE_SCRIPT_ENVS_KEY], + ), }; - sinon.stub(persistentState, 'getWorkspacePersistentState').resolves(workspaceState); + workspaceMemento = workspaceState as unknown as Memento; readMetadataStub = sinon.stub(metadataReader, 'readInlineScriptMetadataFromFile').resolves(VALID_METADATA); computeCacheKeyStub = sinon.stub(cacheKey, 'computeCacheKey').callsFake((inputs) => { @@ -250,6 +246,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, routingRegistry, ); }); @@ -381,7 +378,7 @@ suite('InlineScriptEnvManager', () => { } function workspaceStateSetCalls(key: string): readonly sinon.SinonSpyCall[] { - return workspaceState.set.getCalls().filter((call) => call.args[0] === key); + return workspaceState.update.getCalls().filter((call) => call.args[0] === key); } function matchedAssociationRecord(environmentPath: string, metadataIdentity: string = VALID_METADATA_IDENTITY): unknown { @@ -1495,6 +1492,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); @@ -3410,7 +3408,7 @@ suite('InlineScriptEnvManager', () => { assert.deepStrictEqual(persistedAssociations, { [normalizePath(uri.fsPath)]: matchedAssociationRecord(environment.environmentPath.fsPath), }); - assert.strictEqual(workspaceState.set.firstCall.args[0], INLINE_SCRIPT_ENVS_KEY); + assert.strictEqual(workspaceState.update.firstCall.args[0], INLINE_SCRIPT_ENVS_KEY); assert.strictEqual(listener.callCount, 1); assert.deepStrictEqual(listener.firstCall.args[0], { uri, old: undefined, new: environment }); @@ -3475,6 +3473,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); @@ -3500,6 +3499,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); @@ -3616,6 +3616,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); @@ -3650,6 +3651,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); @@ -3699,6 +3701,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); @@ -3752,11 +3755,12 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); - workspaceState.set.onFirstCall().rejects(new Error('Memento unavailable')); + workspaceState.update.onFirstCall().rejects(new Error('Memento unavailable')); await triggerSavedMetadataChange(restartRoutingRegistry, restarted, uri); await fs.remove(environment.environmentPath.fsPath); clock.tick(5_000 - 1); @@ -3989,6 +3993,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4026,6 +4031,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4074,13 +4080,14 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); ((restarted as unknown as { subscriptions: Disposable[] }).subscriptions[0]).dispose(); - workspaceState.set.onFirstCall().rejects(new Error('Memento unavailable')); - workspaceState.set.onSecondCall().rejects(new Error('Memento unavailable')); + workspaceState.update.onFirstCall().rejects(new Error('Memento unavailable')); + workspaceState.update.onSecondCall().rejects(new Error('Memento unavailable')); await triggerSavedMetadataChange(restartRoutingRegistry, restarted, uri); assert.deepStrictEqual(persistedAssociations, { @@ -4102,6 +4109,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4123,6 +4131,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4151,6 +4160,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4199,6 +4209,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4243,6 +4254,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4277,6 +4289,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4318,6 +4331,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4344,6 +4358,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4370,6 +4385,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4411,6 +4427,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4461,6 +4478,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4492,6 +4510,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4692,6 +4711,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4723,6 +4743,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -4749,6 +4770,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); @@ -4841,6 +4863,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); const listener = sinon.spy(); @@ -4871,6 +4894,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); const listener = sinon.spy(); @@ -4881,7 +4905,7 @@ suite('InlineScriptEnvManager', () => { assert.deepStrictEqual(persistedAssociations, { [normalizePath(uri.fsPath)]: matchedAssociationRecord(environment.environmentPath.fsPath), }); - assert.strictEqual(workspaceState.set.callCount, 0); + assert.strictEqual(workspaceState.update.callCount, 0); assert.strictEqual(listener.callCount, 0); assert.strictEqual(resolveVenvStub.callCount, 0); @@ -4960,7 +4984,7 @@ suite('InlineScriptEnvManager', () => { assert.deepStrictEqual(persistedAssociations, { [normalizePath(uri.fsPath)]: environment.environmentPath.fsPath, }); - assert.strictEqual(workspaceState.set.callCount, 0); + assert.strictEqual(workspaceState.update.callCount, 0); assert.strictEqual(resolveVenvStub.callCount, 0); }); @@ -4977,6 +5001,7 @@ suite('InlineScriptEnvManager', () => { baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, restartRoutingRegistry, ); await nextTurn(); @@ -5033,13 +5058,14 @@ suite('InlineScriptEnvManager', () => { } return persistedAssociations; }); - workspaceState.set.resetHistory(); + workspaceState.update.resetHistory(); manager = new InlineScriptEnvManager( nativeFinder, api, baseManager, globalStorageUri, makeFakeLog(), + workspaceMemento, routingRegistry, ); await initialReadStarted; @@ -5427,7 +5453,7 @@ suite('InlineScriptEnvManager', () => { assert.deepStrictEqual(persistedAssociations, { [scriptPath]: matchedAssociationRecord(environment.environmentPath.fsPath), }); - assert.strictEqual(workspaceState.set.callCount, 0); + assert.strictEqual(workspaceState.update.callCount, 0); }); test('preserves an association when fallback resolution reports another manager', async () => { @@ -5443,7 +5469,7 @@ suite('InlineScriptEnvManager', () => { assert.deepStrictEqual(persistedAssociations, { [normalizePath(uri.fsPath)]: environment.environmentPath.fsPath, }); - assert.strictEqual(workspaceState.set.callCount, 0); + assert.strictEqual(workspaceState.update.callCount, 0); }); test('rejects resolved and selected environments that are outside the owned cache', async () => { @@ -5465,11 +5491,11 @@ suite('InlineScriptEnvManager', () => { assert.strictEqual(await manager.get(uri), undefined); assert.deepStrictEqual(persistedAssociations, {}); - workspaceState.set.resetHistory(); + workspaceState.update.resetHistory(); await assert.rejects(manager.set(uri, unowned), /not an owned cache entry/); assert.deepStrictEqual(persistedAssociations, {}); - assert.strictEqual(workspaceState.set.callCount, 0); + assert.strictEqual(workspaceState.update.callCount, 0); assert.strictEqual(listener.callCount, 0); }); @@ -5503,7 +5529,7 @@ suite('InlineScriptEnvManager', () => { manager.onDidChangeEnvironment(listener); await manager.set(uri, first); - workspaceState.set.onSecondCall().rejects(new Error('Memento unavailable')); + workspaceState.update.onSecondCall().rejects(new Error('Memento unavailable')); await assert.rejects(manager.set(uri, second), /Memento unavailable/); assert.strictEqual(await manager.get(uri), first); @@ -5520,7 +5546,7 @@ suite('InlineScriptEnvManager', () => { manager.onDidChangeEnvironment(listener); await manager.set(uri, environment); - workspaceState.set.onSecondCall().rejects(new Error('Memento unavailable')); + workspaceState.update.onSecondCall().rejects(new Error('Memento unavailable')); await assert.rejects(manager.set(uri, undefined), /Memento unavailable/); assert.strictEqual(await manager.get(uri), environment); @@ -5630,7 +5656,7 @@ suite('InlineScriptEnvManager', () => { const pendingGet = manager.get(uri); await waitForStubCall(resolveVenvStub); - workspaceState.set.onFirstCall().rejects(new Error('Memento unavailable')); + workspaceState.update.onFirstCall().rejects(new Error('Memento unavailable')); await assert.rejects(manager.set(uri, newEnvironment), /Memento unavailable/); resolvePending!(oldEnvironment); @@ -5653,7 +5679,7 @@ suite('InlineScriptEnvManager', () => { /one or more local file URIs/, ); - assert.strictEqual(workspaceState.set.callCount, 0); + assert.strictEqual(workspaceState.update.callCount, 0); assert.strictEqual(await manager.get(valid), undefined); assert.strictEqual(await manager.get(undefined), undefined); assert.strictEqual(await manager.get(Uri.parse('untitled:script.py')), undefined); @@ -5725,6 +5751,7 @@ suite('InlineScriptEnvManager', () => { baseManager, Uri.file(process.platform === 'win32' ? `${process.env.SystemDrive ?? 'C:'}\\` : '/'), makeFakeLog(), + workspaceMemento, ); await assert.rejects( @@ -5743,6 +5770,7 @@ suite('InlineScriptEnvManager', () => { baseManager, symlinkStorageUri, makeFakeLog(), + workspaceMemento, ); const realCacheRoot = cacheLayout.getScriptEnvCacheRoot(symlinkStorageUri).fsPath; const externalCacheRoot = path.join(tempRoot, 'external-cache-root'); @@ -5777,6 +5805,7 @@ suite('InlineScriptEnvManager', () => { baseManager, Uri.file(redirectedStoragePath), makeFakeLog(), + workspaceMemento, ); try { await fs.remove(redirectedStoragePath); @@ -5944,7 +5973,7 @@ suite('InlineScriptEnvManager', () => { await manager.set(uri, environment); const listener = sinon.spy(); manager.onDidChangeEnvironment(listener); - workspaceState.set.withArgs(INLINE_SCRIPT_ENVS_KEY, undefined).rejects(new Error('Memento unavailable')); + workspaceState.update.withArgs(INLINE_SCRIPT_ENVS_KEY, undefined).rejects(new Error('Memento unavailable')); await assert.rejects(manager.clearCache(), /Memento unavailable/); @@ -6056,7 +6085,7 @@ suite('InlineScriptEnvManager', () => { const clearStarted = new Promise((resolve) => { signalClearStarted = resolve; }); - workspaceState.set.withArgs(INLINE_SCRIPT_ENVS_KEY, undefined).callsFake( + workspaceState.update.withArgs(INLINE_SCRIPT_ENVS_KEY, undefined).callsFake( async () => new Promise((resolve) => { signalClearStarted!(); @@ -6088,7 +6117,7 @@ suite('InlineScriptEnvManager', () => { const writeStarted = new Promise((resolve) => { signalWriteStarted = resolve; }); - workspaceState.set + workspaceState.update .withArgs(INLINE_SCRIPT_ENVS_KEY, sinon.match((value: unknown) => value !== undefined)) .callsFake( (_key: string, value: unknown) => @@ -6115,7 +6144,7 @@ suite('InlineScriptEnvManager', () => { const firstUri = scriptUri('first.py'); const environment = await createOwnedEnvironment(); await manager.set(firstUri, environment); - workspaceState.set + workspaceState.update .withArgs(INLINE_SCRIPT_ENVS_KEY, undefined) .onFirstCall() .rejects(new Error('Memento unavailable')); diff --git a/src/test/managers/builtin/inlineScript/main.unit.test.ts b/src/test/managers/builtin/inlineScript/main.unit.test.ts index 69f6d80b5..d62def490 100644 --- a/src/test/managers/builtin/inlineScript/main.unit.test.ts +++ b/src/test/managers/builtin/inlineScript/main.unit.test.ts @@ -3,7 +3,7 @@ import assert from 'assert'; import * as sinon from 'sinon'; -import { Disposable, LogOutputChannel, Uri } from 'vscode'; +import { Disposable, LogOutputChannel, Memento, Uri } from 'vscode'; import { EnvironmentManager, PythonEnvironmentApi } from '../../../../api'; import * as cacheLayout from '../../../../common/inlineScript/cacheLayout'; import { InlineScriptRoutingRegistry } from '../../../../common/inlineScript/routingRegistry'; @@ -49,6 +49,11 @@ suite('registerInlineScriptFeatures (feature-flag gate)', () => { const baseManager = {} as EnvironmentManager; const globalStorageUri = Uri.file('inline-script-global-storage'); const routingRegistry = new InlineScriptRoutingRegistry(); + const workspaceMemento = { + get: () => undefined, + update: async () => undefined, + keys: () => [], + } as unknown as Memento; setup(() => { isEnabledStub = sinon.stub(helpers, 'isInlineScriptsFeatureEnabled'); @@ -79,6 +84,7 @@ suite('registerInlineScriptFeatures (feature-flag gate)', () => { baseManager, globalStorageUri, { enabled: false, routingRegistry: undefined }, + workspaceMemento, ); assert.strictEqual(disposables.length, 0, 'no disposables should be added when flag is off'); @@ -97,6 +103,7 @@ suite('registerInlineScriptFeatures (feature-flag gate)', () => { baseManager, globalStorageUri, { enabled: true, routingRegistry: undefined }, + workspaceMemento, ), /routing registry/i, ); @@ -116,6 +123,7 @@ suite('registerInlineScriptFeatures (feature-flag gate)', () => { baseManager, globalStorageUri, { enabled: true, routingRegistry }, + workspaceMemento, ); assert.strictEqual(getPythonApiStub.callCount, 1); @@ -142,6 +150,7 @@ suite('registerInlineScriptFeatures (feature-flag gate)', () => { baseManager, globalStorageUri, { enabled: true, routingRegistry }, + workspaceMemento, ); assert.strictEqual( @@ -170,6 +179,7 @@ suite('registerInlineScriptFeatures (feature-flag gate)', () => { baseManager, globalStorageUri, activation, + workspaceMemento, ) : Promise.resolve()); @@ -203,6 +213,7 @@ suite('registerInlineScriptFeatures (feature-flag gate)', () => { baseManager, globalStorageUri, activation, + workspaceMemento, ) : Promise.resolve()); await nextTurn(); @@ -236,6 +247,7 @@ suite('registerInlineScriptFeatures (feature-flag gate)', () => { baseManager, globalStorageUri, activation, + workspaceMemento, ) : Promise.resolve());