From c83f7cbf24ecd26a9bde2806e42b7733637eeeac Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Wed, 22 Jul 2026 15:00:10 -0700 Subject: [PATCH 1/3] implement create env --- src/common/inlineScriptCacheKey.ts | 43 +- src/common/inlineScriptCacheLayout.ts | 108 ++- src/common/inlineScriptInterpreter.ts | 3 +- src/common/lockfile.apis.ts | 92 ++ src/extension.ts | 11 +- src/managers/builtin/helpers.ts | 24 +- .../builtin/inlineScriptEnvManager.ts | 453 +++++++++- src/managers/builtin/inlineScriptMain.ts | 15 +- src/managers/builtin/venvUtils.ts | 22 +- .../common/inlineScriptCacheKey.unit.test.ts | 43 + .../inlineScriptCacheLayout.unit.test.ts | 131 ++- .../inlineScriptInterpreter.unit.test.ts | 7 + src/test/common/lockfile.apis.unit.test.ts | 142 ++++ .../builtin/helpers.cancellation.unit.test.ts | 85 ++ .../inlineScriptEnvManager.unit.test.ts | 795 +++++++++++++++--- .../builtin/inlineScriptMain.unit.test.ts | 14 +- .../venvUtils.createWithProgress.unit.test.ts | 137 +++ 17 files changed, 1948 insertions(+), 177 deletions(-) create mode 100644 src/common/lockfile.apis.ts create mode 100644 src/test/common/lockfile.apis.unit.test.ts create mode 100644 src/test/managers/builtin/helpers.cancellation.unit.test.ts create mode 100644 src/test/managers/builtin/venvUtils.createWithProgress.unit.test.ts diff --git a/src/common/inlineScriptCacheKey.ts b/src/common/inlineScriptCacheKey.ts index 13c946462..e701d6612 100644 --- a/src/common/inlineScriptCacheKey.ts +++ b/src/common/inlineScriptCacheKey.ts @@ -31,6 +31,39 @@ function normalizeExtras(inner: string): string { return deduped.length > 0 ? `[${deduped.join(',')}]` : ''; } +function normalizeRequirementTail(value: string): string { + let result = ''; + let unquoted = ''; + let quote: "'" | '"' | undefined; + let escaped = false; + + const flushUnquoted = () => { + result += unquoted.replace(/\s+/g, ' ').replace(/\s*([<>=!~]=?)\s*/g, '$1'); + unquoted = ''; + }; + + for (const character of value) { + if (quote) { + result += character; + if (character === quote && !escaped) { + quote = undefined; + } + escaped = character === '\\' && !escaped; + if (character !== '\\') { + escaped = false; + } + } else if (character === "'" || character === '"') { + flushUnquoted(); + quote = character; + result += character; + } else { + unquoted += character; + } + } + flushUnquoted(); + return result.trim(); +} + /** * Canonicalize a PEP 723 dependency entry so common variants of the same * requirement produce identical strings. Not a full PEP 508 parser — only @@ -70,10 +103,12 @@ export function normalizeDependency(dep: string): string { rest = rest.slice(extrasMatch[0].length); } - const compactedRest = rest - .replace(/\s+/g, ' ') - .replace(/\s*([<>=!~]=?)\s*/g, '$1') - .trim(); + const directReference = rest.trim(); + if (directReference.startsWith('@')) { + return `${name}${extras} ${directReference}`; + } + + const compactedRest = normalizeRequirementTail(rest); return `${name}${extras}${compactedRest}`; } diff --git a/src/common/inlineScriptCacheLayout.ts b/src/common/inlineScriptCacheLayout.ts index 72c83302a..dbcf73a3f 100644 --- a/src/common/inlineScriptCacheLayout.ts +++ b/src/common/inlineScriptCacheLayout.ts @@ -11,9 +11,10 @@ import { isWindows } from './utils/platformUtils'; /** * Versioned name of the cache root under the extension's `globalStorageUri`. * - * Bump the `-v1` suffix together with {@link META_SCHEMA_VERSION} on any - * incompatible on-disk change, so old envs sit unread and TTL out naturally - * instead of being migrated in place. + * PR 5 finalizes the v1 shape before the first production writer. After that + * writer ships, bump the `-v1` suffix together with {@link META_SCHEMA_VERSION} + * on any incompatible on-disk change, so old envs sit unread instead of being + * migrated in place. */ export const INLINE_SCRIPT_CACHE_DIR_NAME = 'script-envs-v1'; @@ -33,14 +34,20 @@ const MAX_META_JSON_BYTES = 1024 * 1024; export interface InlineScriptEnvMeta { /** Version of the serialized metadata schema. */ readonly schemaVersion: typeof META_SCHEMA_VERSION; - /** Filesystem path of the script recorded for lifecycle bookkeeping. */ - readonly scriptFsPath: string; + /** Canonical filesystem path of the base interpreter used to create this environment. */ + readonly baseInterpreterPath: string; + /** Version reported by the base interpreter when this environment was created. */ + readonly baseInterpreterVersion: string; /** Last successful use as a canonical UTC string produced by `Date.toISOString()`. */ readonly lastUsedAt: string; - /** The script's `requires-python` declaration, when present. */ - readonly requiresPython?: string; } +export type InlineScriptMetaReadResult = + | { readonly kind: 'valid'; readonly metadata: InlineScriptEnvMeta } + | { readonly kind: 'missing' | 'invalid' | 'unavailable' }; + +export type BaseInterpreterStatus = 'available' | 'missing' | 'unavailable'; + /** * In-memory summary of one cached entry, populated by the separate disk walk. */ @@ -67,32 +74,43 @@ export function getMetaJsonPath(envDir: Uri): Uri { * Read and validate the extension-owned `.meta.json` sidecar in a cached * environment directory. * - * This function is intentionally non-destructive. Missing, malformed, or - * unreadable sidecars return `undefined` so callers can treat the entry as a - * cache miss. Deleting invalid entries belongs to the dedicated, guarded cache - * cleanup path because read and permission failures may be transient. + * This compatibility wrapper is intentionally non-destructive. Missing, + * malformed, or unreadable sidecars return `undefined`. Callers that decide + * cache lifecycle must use {@link inspectMetaJson} to distinguish conclusive + * `missing` / `invalid` state from transient `unavailable` I/O failures. */ export async function readMetaJson(envDir: Uri): Promise { + const result = await inspectMetaJson(envDir); + return result.kind === 'valid' ? result.metadata : undefined; +} + +/** + * Read and classify an inline-script sidecar without mutating its cache entry. + * `missing` and `invalid` are conclusive under the creator lock; `unavailable` + * represents a transient I/O failure that must not trigger deletion. + */ +export async function inspectMetaJson(envDir: Uri): Promise { const metaPath = getMetaJsonPath(envDir).fsPath; try { - const stat = await fsapi.stat(metaPath); + const stat = await fsapi.lstat(metaPath); if (!stat.isFile()) { traceWarn(`inline-script meta: not a regular file at ${metaPath}`); - return undefined; + return { kind: 'invalid' }; } if (stat.size > MAX_META_JSON_BYTES) { traceWarn(`inline-script meta: refusing to read ${metaPath} (${stat.size} bytes > cap)`); - return undefined; + return { kind: 'invalid' }; } } catch (err) { if (isFileNotFoundError(err)) { traceWarn(`inline-script meta: not found at ${metaPath}`); + return { kind: 'missing' }; } else { const code = (err as NodeJS.ErrnoException | undefined)?.code ?? 'unknown'; traceWarn(`inline-script meta: failed to stat ${metaPath} (code=${code}):`, err); + return { kind: 'unavailable' }; } - return undefined; } let raw: string; @@ -101,7 +119,7 @@ export async function readMetaJson(envDir: Uri): Promise, no * Verify that a cached env's base interpreter still exists on disk. */ export async function verifyBaseInterpreterExists(envDir: Uri): Promise { - return isWindows() ? verifyWindowsBaseInterpreter(envDir) : verifyPosixBaseInterpreter(envDir); + return (await getBaseInterpreterStatus(envDir)) === 'available'; } -async function verifyPosixBaseInterpreter(envDir: Uri): Promise { +/** + * Classify the cached environment's base interpreter without throwing. + * `missing` is definitive stale state; `unavailable` is an uncertain I/O failure + * that callers must preserve rather than delete. + */ +export async function getBaseInterpreterStatus(envDir: Uri): Promise { + return isWindows() ? getWindowsBaseInterpreterStatus(envDir) : getPosixBaseInterpreterStatus(envDir); +} + +async function getPosixBaseInterpreterStatus(envDir: Uri): Promise { const launcherPath = Uri.joinPath(envDir, 'bin', 'python').fsPath; - return isRegularFile(launcherPath, 'base interpreter'); + return getRegularFileStatus(launcherPath, 'base interpreter'); } -async function verifyWindowsBaseInterpreter(envDir: Uri): Promise { +async function getWindowsBaseInterpreterStatus(envDir: Uri): Promise { const pyvenvPath = Uri.joinPath(envDir, 'pyvenv.cfg').fsPath; let raw: string; try { @@ -176,37 +203,39 @@ async function verifyWindowsBaseInterpreter(envDir: Uri): Promise { } catch (err) { if (isFileNotFoundError(err)) { traceWarn(`inline-script env: missing pyvenv.cfg at ${pyvenvPath}`); + return 'missing'; } else { const code = (err as NodeJS.ErrnoException | undefined)?.code ?? 'unknown'; traceWarn(`inline-script env: failed to read ${pyvenvPath} (code=${code}):`, err); + return 'unavailable'; } - return false; } const home = parsePyvenvHome(raw); if (home === undefined) { traceWarn(`inline-script env: no 'home =' line in ${pyvenvPath}`); - return false; + return 'missing'; } const launcherPath = path.join(home, 'python.exe'); - return isRegularFile(launcherPath, 'base interpreter'); + return getRegularFileStatus(launcherPath, 'base interpreter'); } -async function isRegularFile(filePath: string, label: string): Promise { +async function getRegularFileStatus(filePath: string, label: string): Promise { try { const stat = await fsapi.stat(filePath); if (!stat.isFile()) { traceWarn(`inline-script env: ${label} is not a regular file at ${filePath}`); - return false; + return 'missing'; } - return true; + return 'available'; } catch (err) { if (isFileNotFoundError(err)) { traceWarn(`inline-script env: ${label} missing at ${filePath}`); + return 'missing'; } else { const code = (err as NodeJS.ErrnoException | undefined)?.code ?? 'unknown'; traceWarn(`inline-script env: failed to stat ${filePath} (code=${code}):`, err); + return 'unavailable'; } - return false; } } @@ -232,21 +261,30 @@ function validateMeta(value: unknown): InlineScriptEnvMeta | undefined { if (obj.schemaVersion !== META_SCHEMA_VERSION) { return undefined; } - if (typeof obj.scriptFsPath !== 'string' || obj.scriptFsPath.length === 0) { + if ( + typeof obj.baseInterpreterPath !== 'string' || + obj.baseInterpreterPath.length === 0 || + obj.baseInterpreterPath.trim() !== obj.baseInterpreterPath || + !path.isAbsolute(obj.baseInterpreterPath) + ) { return undefined; } - if (!isCanonicalIsoTimestamp(obj.lastUsedAt)) { + if ( + typeof obj.baseInterpreterVersion !== 'string' || + obj.baseInterpreterVersion.trim().length === 0 || + obj.baseInterpreterVersion.trim() !== obj.baseInterpreterVersion + ) { return undefined; } - if (obj.requiresPython !== undefined && typeof obj.requiresPython !== 'string') { + if (!isCanonicalIsoTimestamp(obj.lastUsedAt)) { return undefined; } return { schemaVersion: META_SCHEMA_VERSION, - scriptFsPath: obj.scriptFsPath, + baseInterpreterPath: obj.baseInterpreterPath, + baseInterpreterVersion: obj.baseInterpreterVersion, lastUsedAt: obj.lastUsedAt, - requiresPython: obj.requiresPython, }; } diff --git a/src/common/inlineScriptInterpreter.ts b/src/common/inlineScriptInterpreter.ts index 54754775e..0fba1c5fa 100644 --- a/src/common/inlineScriptInterpreter.ts +++ b/src/common/inlineScriptInterpreter.ts @@ -24,7 +24,8 @@ export function pickCompatibleInterpreter( installed: ReadonlyArray, requiresPython: string | undefined, ): PythonEnvironment | undefined { - const constraint = requiresPython && requiresPython.length > 0 ? requiresPython : undefined; + const trimmedConstraint = requiresPython?.trim(); + const constraint = trimmedConstraint ? trimmedConstraint : undefined; const candidates = installed.filter((env) => isUsableBaseInterpreter(env, constraint)); if (candidates.length === 0) { return undefined; diff --git a/src/common/lockfile.apis.ts b/src/common/lockfile.apis.ts new file mode 100644 index 000000000..bd0143904 --- /dev/null +++ b/src/common/lockfile.apis.ts @@ -0,0 +1,92 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as crypto from 'crypto'; +import * as fsapi from 'fs-extra'; +import * as path from 'path'; + +export interface AcquireFileLockOptions { + /** Maximum time to wait for another live owner. */ + readonly timeoutMs: number; + /** Delay between acquisition attempts. */ + readonly retryIntervalMs: number; +} + +/** + * Acquires a filesystem lock and returns its asynchronous release function. + * + * Acquisition uses atomic directory creation. There is no process-exit cleanup or + * time-based stealing: environment creation cannot be cancelled at every package-manager + * boundary, so a child process may outlive its extension host. An interrupted operation + * therefore fails closed until the cache is explicitly cleared. + */ +export async function acquireFileLock( + filePath: string, + options: AcquireFileLockOptions, +): Promise<() => Promise> { + const lockPath = `${path.resolve(filePath)}.lock`; + const ownerMarker = path.join(lockPath, `owner-${process.pid}-${crypto.randomBytes(16).toString('hex')}`); + const deadline = Date.now() + options.timeoutMs; + + while (true) { + try { + await fsapi.mkdir(lockPath); + try { + await fsapi.writeFile(ownerMarker, '', { flag: 'wx' }); + } catch (error) { + await fsapi.rmdir(lockPath).catch(() => undefined); + throw error; + } + + let released = false; + return async () => { + if (released) { + throw createLockError('Lock has already been released', 'ERELEASED', lockPath); + } + released = true; + try { + await fsapi.unlink(ownerMarker); + } catch (error) { + if (isFileNotFoundError(error)) { + throw createLockError('Lock ownership was compromised', 'ECOMPROMISED', lockPath); + } + throw error; + } + await fsapi.rmdir(lockPath); + }; + } catch (error) { + if (!isAlreadyExistsError(error)) { + throw error; + } + if (Date.now() >= deadline) { + throw createLockError('Lock is already being held', 'ELOCKED', lockPath); + } + await delay(Math.min(options.retryIntervalMs, Math.max(0, deadline - Date.now()))); + } + } +} + +function isAlreadyExistsError(error: unknown): boolean { + return hasErrorCode(error, 'EEXIST'); +} + +function isFileNotFoundError(error: unknown): boolean { + return hasErrorCode(error, 'ENOENT'); +} + +function hasErrorCode(error: unknown, code: string): boolean { + return ( + typeof error === 'object' && + error !== null && + 'code' in error && + (error as NodeJS.ErrnoException).code === code + ); +} + +function createLockError(message: string, code: string, lockPath: string): NodeJS.ErrnoException { + return Object.assign(new Error(message), { code, path: lockPath }); +} + +async function delay(milliseconds: number): Promise { + return new Promise((resolve) => setTimeout(resolve, milliseconds)); +} \ No newline at end of file diff --git a/src/extension.ts b/src/extension.ts index e574eeabb..fad12bb2e 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -657,7 +657,16 @@ export async function activate(context: ExtensionContext): Promise { - proc.kill(); + cancellationRequested = true; + try { + proc.kill(); + } catch { + // Cancellation remains the controlling outcome even when signaling fails. + } reject(new CancellationError()); }); proc.on('error', (err) => { + if (cancellationRequested) { + reject(new CancellationError()); + return; + } log?.error(`Error spawning uv: ${err}`); reject(new Error(`Error spawning uv: ${err.message}`)); }); @@ -114,12 +124,22 @@ export async function runPython( log?.info(`Running: ${python} ${args.join(' ')}`); return new Promise((resolve, reject) => { const proc = spawnProcess(python, args, { cwd: cwd, timeout }); + let cancellationRequested = false; token?.onCancellationRequested(() => { - proc.kill(); + cancellationRequested = true; + try { + proc.kill(); + } catch { + // Cancellation remains the controlling outcome even when signaling fails. + } reject(new CancellationError()); }); proc.on('error', (err) => { + if (cancellationRequested) { + reject(new CancellationError()); + return; + } log?.error(`Error spawning python: ${err}`); reject(new Error(`Error spawning python: ${err.message}`)); }); diff --git a/src/managers/builtin/inlineScriptEnvManager.ts b/src/managers/builtin/inlineScriptEnvManager.ts index 7a1198afb..53f1bb127 100644 --- a/src/managers/builtin/inlineScriptEnvManager.ts +++ b/src/managers/builtin/inlineScriptEnvManager.ts @@ -1,8 +1,12 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { Disposable, Event, EventEmitter, l10n, LogOutputChannel, MarkdownString, ThemeIcon } from 'vscode'; +import * as fs from 'fs-extra'; +import * as path from 'path'; +import { Disposable, Event, EventEmitter, l10n, LogOutputChannel, MarkdownString, ThemeIcon, Uri } from 'vscode'; import { + CreateEnvironmentOptions, + CreateEnvironmentScope, DidChangeEnvironmentEventArgs, DidChangeEnvironmentsEventArgs, EnvironmentManager, @@ -10,18 +14,72 @@ import { GetEnvironmentsScope, IconPath, PythonEnvironment, + PythonEnvironmentApi, RefreshEnvironmentsScope, ResolveEnvironmentContext, SetEnvironmentScope, } from '../../api'; +import { computeCacheKey } from '../../common/inlineScriptCacheKey'; +import { + InlineScriptEnvMeta, + META_SCHEMA_VERSION, + getBaseInterpreterStatus, + getScriptEnvCacheRoot, + getScriptEnvDir, + inspectMetaJson, + writeMetaJson, +} from '../../common/inlineScriptCacheLayout'; +import { pickCompatibleInterpreter } from '../../common/inlineScriptInterpreter'; +import { + InlineScriptMetadata, + matchesPythonVersion, + readInlineScriptMetadataFromFile, +} from '../../common/inlineScriptMetadata'; +import { PYTHON_EXTENSION_ID, SYSTEM_MANAGER_ID } from '../../common/constants'; +import { acquireFileLock } from '../../common/lockfile.apis'; +import { normalizePath } from '../../common/utils/pathUtils'; +import { isWindows } from '../../common/utils/platformUtils'; +import { compareReleaseSegments, parseReleaseSegments } from '../../common/utils/pep440Release'; +import { NativePythonFinder } from '../common/nativePythonFinder'; +import { createWithProgress, resolveVenvPythonEnvironmentPath } from './venvUtils'; + +const BASE_INTERPRETER_MANAGER_IDS = new Set([ + SYSTEM_MANAGER_ID, + `${PYTHON_EXTENSION_ID}:conda`, + `${PYTHON_EXTENSION_ID}:pyenv`, +]); + +// Waiters never steal a lock: interrupted/cancelled creation remains fail-closed. +// The planned PR13 inline-script clear-cache lifecycle flow will own recovery for abandoned locks. +const CACHE_LOCK_TIMEOUT_MS = 5 * 60 * 1000; +const CACHE_LOCK_RETRY_MS = 500; +const INLINE_SCRIPT_MANAGER_ID = `${PYTHON_EXTENSION_ID}:inline-script`; + +interface SelectedBaseInterpreter { + readonly environment: PythonEnvironment; + readonly canonicalPath: string; +} + +interface BuildCacheEntryResult { + readonly environment?: PythonEnvironment; + readonly retainLock?: boolean; +} + +type CacheEntryInspection = + | { readonly kind: 'absent' | 'stale' | 'uncertain' } + | { readonly kind: 'reusable'; readonly environment: PythonEnvironment }; + +type EnvironmentInspection = 'expected' | 'stale' | 'uncertain'; +type PythonReleaseComparison = 'same' | 'different' | 'uncertain'; /** - * Skeleton EnvironmentManager for PEP 723 inline-script envs. Every - * method returns the empty / undefined / no-op equivalent; `create`, - * `remove`, and `quickCreateConfig` are intentionally omitted so the - * picker UI hides their entry points until later PRs land them. + * Environment manager for extension-owned PEP 723 script environments. + * Creation is supported; script associations and discovery are added by + * later rollout phases. */ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { + private readonly pendingCreations = new Map>(); + private readonly _onDidChangeEnvironments = new EventEmitter(); public readonly onDidChangeEnvironments: Event = this._onDidChangeEnvironments.event; @@ -39,7 +97,67 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { ); public readonly iconPath: IconPath = new ThemeIcon('file-code'); - constructor(public readonly log: LogOutputChannel) {} + constructor( + private readonly nativeFinder: NativePythonFinder, + private readonly api: PythonEnvironmentApi, + private readonly baseManager: EnvironmentManager, + private readonly globalStorageUri: Uri, + public readonly log: LogOutputChannel, + ) {} + + async create( + scope: CreateEnvironmentScope, + options?: CreateEnvironmentOptions, + ): Promise { + try { + const scriptUri = this.getScriptUri(scope); + if (!scriptUri) { + this.log.warn('Inline-script environment creation requires exactly one local file URI.'); + return undefined; + } + + const metadata = await readInlineScriptMetadataFromFile(scriptUri); + if (!metadata) { + this.log.warn(`No valid PEP 723 metadata found in ${scriptUri.fsPath}.`); + return undefined; + } + + const packages = [...(metadata.dependencies ?? []), ...(options?.additionalPackages ?? [])].map((value) => + value.trim(), + ); + if (packages.some((value) => value.length === 0)) { + this.log.warn(`Inline-script dependencies must not contain empty entries: ${scriptUri.fsPath}.`); + return undefined; + } + + const selectedBase = await this.selectBaseInterpreter(metadata); + if (!selectedBase) { + this.log.warn(`No installed Python satisfies the inline-script requirements for ${scriptUri.fsPath}.`); + return undefined; + } + const cacheKey = computeCacheKey({ + dependencies: packages, + interpreterPath: selectedBase.canonicalPath, + }); + const pending = this.pendingCreations.get(cacheKey); + if (pending) { + return await pending; + } + + const creation = this.createOrReuseEnvironment(cacheKey, packages, metadata, selectedBase); + this.pendingCreations.set(cacheKey, creation); + try { + return await creation; + } finally { + if (this.pendingCreations.get(cacheKey) === creation) { + this.pendingCreations.delete(cacheKey); + } + } + } catch (error) { + this.log.error(`Failed to set up inline-script environment: ${this.errorMessage(error)}`); + return undefined; + } + } async refresh(_scope: RefreshEnvironmentsScope): Promise { return; @@ -61,6 +179,329 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return undefined; } + private getScriptUri(scope: CreateEnvironmentScope): Uri | undefined { + const uri = scope instanceof Uri ? scope : Array.isArray(scope) && scope.length === 1 ? scope[0] : undefined; + return uri?.scheme === 'file' ? uri : undefined; + } + + private async selectBaseInterpreter(metadata: InlineScriptMetadata): Promise { + const reported = (await this.api.getEnvironments('global')).filter( + (environment) => + BASE_INTERPRETER_MANAGER_IDS.has(environment.envId.managerId) && + (environment.envId.managerId !== `${PYTHON_EXTENSION_ID}:conda` || environment.name === 'base'), + ); + const derivedChecks = await Promise.all( + reported.map(async (environment) => ({ + environment, + derived: await fs.pathExists(path.join(environment.sysPrefix, 'pyvenv.cfg')), + })), + ); + let candidates = derivedChecks.filter((candidate) => !candidate.derived).map((candidate) => candidate.environment); + + while (candidates.length > 0) { + const environment = pickCompatibleInterpreter(candidates, metadata.requiresPython); + if (!environment) { + return undefined; + } + candidates = candidates.filter((candidate) => candidate !== environment); + + const executable = environment.execInfo?.run.executable; + if (!executable) { + continue; + } + try { + return { environment, canonicalPath: await fs.realpath(executable) }; + } catch (error) { + this.log.warn( + `Skipping base interpreter that cannot be resolved at ${executable}: ${this.errorMessage(error)}`, + ); + } + } + + return undefined; + } + + private async createOrReuseEnvironment( + cacheKey: string, + packages: string[], + metadata: InlineScriptMetadata, + selectedBase: SelectedBaseInterpreter, + ): Promise { + const cacheRoot = getScriptEnvCacheRoot(this.globalStorageUri); + const envDir = getScriptEnvDir(this.globalStorageUri, cacheKey); + await fs.ensureDir(cacheRoot.fsPath); + + let release: (() => Promise) | undefined; + try { + release = await acquireFileLock(envDir.fsPath, { + timeoutMs: CACHE_LOCK_TIMEOUT_MS, + retryIntervalMs: CACHE_LOCK_RETRY_MS, + }); + + const cached = await this.inspectCacheEntry(cacheRoot, envDir, metadata, selectedBase); + if (cached.kind === 'reusable') { + return cached.environment; + } + if (cached.kind === 'uncertain') { + this.log.warn(`Preserving an inline-script cache entry that could not be safely inspected: ${envDir.fsPath}`); + return undefined; + } + if (cached.kind === 'stale') { + if (!(await this.removeCacheEntry(envDir))) { + return undefined; + } + } + + const build = await this.buildCacheEntry(envDir, cacheRoot, packages, selectedBase); + if (build.retainLock) { + release = undefined; + } + return build.environment; + } catch (error) { + this.log.error(`Failed to create or reuse inline-script cache entry: ${this.errorMessage(error)}`); + return undefined; + } finally { + if (release) { + try { + await release(); + } catch (error) { + this.log.warn(`Failed to release inline-script cache lock: ${this.errorMessage(error)}`); + } + } + } + } + + private async inspectCacheEntry( + cacheRoot: Uri, + envDir: Uri, + metadata: InlineScriptMetadata, + selectedBase: SelectedBaseInterpreter, + ): Promise { + try { + const stat = await fs.lstat(envDir.fsPath); + if (!stat.isDirectory() || stat.isSymbolicLink()) { + return { kind: 'uncertain' }; + } + } catch (error) { + return this.isFileNotFoundError(error) ? { kind: 'absent' } : { kind: 'uncertain' }; + } + + if (!(await this.isPhysicallyContained(cacheRoot, envDir))) { + return { kind: 'uncertain' }; + } + + let sidecarResult; + try { + sidecarResult = await inspectMetaJson(envDir); + } catch { + return { kind: 'uncertain' }; + } + if (sidecarResult.kind !== 'valid') { + return { kind: sidecarResult.kind === 'unavailable' ? 'uncertain' : 'stale' }; + } + const sidecar = sidecarResult.metadata; + if ( + normalizePath(sidecar.baseInterpreterPath) !== normalizePath(selectedBase.canonicalPath) || + sidecar.baseInterpreterVersion !== selectedBase.environment.version + ) { + return { kind: 'stale' }; + } + + const baseInterpreterStatus = await getBaseInterpreterStatus(envDir); + if (baseInterpreterStatus !== 'available') { + return { kind: baseInterpreterStatus === 'missing' ? 'stale' : 'uncertain' }; + } + + const environment = await resolveVenvPythonEnvironmentPath( + this.getVenvPythonPath(envDir), + this.nativeFinder, + this.api, + this, + this.baseManager, + ); + if (!environment) { + return { kind: 'uncertain' }; + } + const environmentStatus = await this.inspectExpectedCacheEnvironment(environment, cacheRoot, envDir); + if (environmentStatus !== 'expected') { + return { kind: environmentStatus }; + } + const releaseComparison = this.comparePythonReleases( + environment.version, + selectedBase.environment.version, + ); + if (releaseComparison !== 'same') { + return { kind: releaseComparison === 'different' ? 'stale' : 'uncertain' }; + } + const requiresPython = metadata.requiresPython?.trim(); + if (requiresPython && !matchesPythonVersion(requiresPython, environment.version)) { + return { kind: 'stale' }; + } + + try { + await writeMetaJson(envDir, { ...sidecar, lastUsedAt: new Date().toISOString() }); + } catch (error) { + this.log.warn(`Failed to update inline-script cache metadata: ${this.errorMessage(error)}`); + } + return { kind: 'reusable', environment }; + } + + private async buildCacheEntry( + envDir: Uri, + cacheRoot: Uri, + packages: string[], + selectedBase: SelectedBaseInterpreter, + ): Promise { + let result; + try { + result = await createWithProgress( + this.nativeFinder, + this.api, + this.log, + this, + selectedBase.environment, + cacheRoot, + envDir.fsPath, + { install: packages, uninstall: [] }, + { trackUvEnvironment: false }, + ); + } catch (error) { + this.log.error(`Failed to build inline-script environment: ${this.errorMessage(error)}`); + await this.removeCacheEntry(envDir); + return {}; + } + + if (result?.pkgInstallationCancelled) { + this.log.warn( + 'Inline-script package installation was cancelled; retaining the cache lock until explicit cleanup.', + ); + return { retainLock: true }; + } + if (!result?.environment || result.envCreationErr || result.pkgInstallationErr) { + const error = result?.envCreationErr ?? result?.pkgInstallationErr ?? 'environment creation returned no result'; + this.log.error(`Failed to build inline-script environment: ${error}`); + await this.removeCacheEntry(envDir); + return {}; + } + if ( + this.comparePythonReleases(result.environment.version, selectedBase.environment.version) !== 'same' || + (await this.inspectExpectedCacheEnvironment(result.environment, cacheRoot, envDir)) !== 'expected' + ) { + this.log.error('Created inline-script environment does not match the requested cache entry.'); + await this.removeCacheEntry(envDir); + return {}; + } + + const sidecar: InlineScriptEnvMeta = { + schemaVersion: META_SCHEMA_VERSION, + baseInterpreterPath: selectedBase.canonicalPath, + baseInterpreterVersion: selectedBase.environment.version, + lastUsedAt: new Date().toISOString(), + }; + try { + await writeMetaJson(envDir, sidecar); + } catch (error) { + this.log.error(`Failed to record inline-script cache metadata: ${this.errorMessage(error)}`); + await this.removeCacheEntry(envDir); + return {}; + } + + return { environment: result.environment }; + } + + private async removeCacheEntry(envDir: Uri): Promise { + try { + await fs.remove(envDir.fsPath); + return true; + } catch (error) { + this.log.error(`Failed to remove incomplete inline-script environment: ${this.errorMessage(error)}`); + return false; + } + } + + private getVenvPythonPath(envDir: Uri): string { + return isWindows() + ? Uri.joinPath(envDir, 'Scripts', 'python.exe').fsPath + : Uri.joinPath(envDir, 'bin', 'python').fsPath; + } + + private async inspectExpectedCacheEnvironment( + environment: PythonEnvironment, + cacheRoot: Uri, + envDir: Uri, + ): Promise { + if (environment.envId.managerId !== INLINE_SCRIPT_MANAGER_ID) { + return 'uncertain'; + } + try { + const [resolvedRoot, expectedDir, resolvedPrefix, expectedPython, resolvedPython] = await Promise.all([ + fs.realpath(cacheRoot.fsPath), + fs.realpath(envDir.fsPath), + fs.realpath(environment.sysPrefix), + fs.realpath(this.getVenvPythonPath(envDir)), + fs.realpath(environment.environmentPath.fsPath), + ]); + if (!this.isExpectedPhysicalEntry(resolvedRoot, expectedDir, envDir)) { + return 'uncertain'; + } + return ( + normalizePath(expectedDir) === normalizePath(resolvedPrefix) && + normalizePath(expectedPython) === normalizePath(resolvedPython) + ) + ? 'expected' + : 'stale'; + } catch { + return 'uncertain'; + } + } + + private async isPhysicallyContained(cacheRoot: Uri, envDir: Uri): Promise { + try { + const [resolvedRoot, resolvedEntry] = await Promise.all([ + fs.realpath(cacheRoot.fsPath), + fs.realpath(envDir.fsPath), + ]); + return this.isExpectedPhysicalEntry(resolvedRoot, resolvedEntry, envDir); + } catch { + return false; + } + } + + private isExpectedPhysicalEntry(resolvedRoot: string, resolvedEntry: string, envDir: Uri): boolean { + const expectedEntry = path.join(resolvedRoot, path.basename(envDir.fsPath)); + return ( + this.isDescendantPath(resolvedRoot, resolvedEntry) && + normalizePath(path.resolve(resolvedEntry)) === normalizePath(path.resolve(expectedEntry)) + ); + } + + private isDescendantPath(rootPath: string, candidatePath: string): boolean { + const relative = path.relative(rootPath, candidatePath); + return relative.length > 0 && relative !== '..' && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative); + } + + private isFileNotFoundError(error: unknown): boolean { + return ( + typeof error === 'object' && + error !== null && + 'code' in error && + (error as NodeJS.ErrnoException).code === 'ENOENT' + ); + } + + private comparePythonReleases(actual: string, expected: string): PythonReleaseComparison { + const actualRelease = parseReleaseSegments(actual); + const expectedRelease = parseReleaseSegments(expected); + if (actualRelease === undefined || expectedRelease === undefined) { + return 'uncertain'; + } + return compareReleaseSegments(actualRelease, expectedRelease) === 0 ? 'same' : 'different'; + } + + private errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); + } + dispose(): void { this._onDidChangeEnvironments.dispose(); this._onDidChangeEnvironment.dispose(); diff --git a/src/managers/builtin/inlineScriptMain.ts b/src/managers/builtin/inlineScriptMain.ts index fcca5643d..6b53c0589 100644 --- a/src/managers/builtin/inlineScriptMain.ts +++ b/src/managers/builtin/inlineScriptMain.ts @@ -1,11 +1,12 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { Disposable, LogOutputChannel } from 'vscode'; -import { PythonEnvironmentApi } from '../../api'; +import { Disposable, LogOutputChannel, Uri } from 'vscode'; +import { EnvironmentManager, PythonEnvironmentApi } from '../../api'; import { traceInfo, traceVerbose } from '../../common/logging'; import { getPythonApi } from '../../features/pythonApi'; import { isInlineScriptsFeatureEnabled } from '../../helpers'; +import { NativePythonFinder } from '../common/nativePythonFinder'; import { InlineScriptEnvManager } from './inlineScriptEnvManager'; /** @@ -13,14 +14,20 @@ import { InlineScriptEnvManager } from './inlineScriptEnvManager'; * `python-envs.inlineScripts.enabled` flag is true. The flag is * undeclared in `package.json`, so default users see nothing. */ -export async function registerInlineScriptFeatures(disposables: Disposable[], log: LogOutputChannel): Promise { +export async function registerInlineScriptFeatures( + nativeFinder: NativePythonFinder, + disposables: Disposable[], + log: LogOutputChannel, + baseManager: EnvironmentManager, + globalStorageUri: Uri, +): Promise { if (!isInlineScriptsFeatureEnabled()) { traceVerbose('Inline-script env manager: skipping registration (internal flag is off)'); return; } const api: PythonEnvironmentApi = await getPythonApi(); - const mgr = new InlineScriptEnvManager(log); + const mgr = new InlineScriptEnvManager(nativeFinder, api, baseManager, globalStorageUri, log); disposables.push(mgr, api.registerEnvironmentManager(mgr)); traceInfo('Inline-script env manager: registered (internal flag is on)'); } diff --git a/src/managers/builtin/venvUtils.ts b/src/managers/builtin/venvUtils.ts index 4efab305e..d4844fdaf 100644 --- a/src/managers/builtin/venvUtils.ts +++ b/src/managers/builtin/venvUtils.ts @@ -1,7 +1,16 @@ import * as fsapi from 'fs-extra'; import * as os from 'os'; import * as path from 'path'; -import { l10n, LogOutputChannel, ProgressLocation, QuickPickItem, QuickPickItemKind, ThemeIcon, Uri } from 'vscode'; +import { + CancellationError, + l10n, + LogOutputChannel, + ProgressLocation, + QuickPickItem, + QuickPickItemKind, + ThemeIcon, + Uri, +} from 'vscode'; import { EnvironmentManager, PythonEnvironment, PythonEnvironmentApi, PythonEnvironmentInfo } from '../../api'; import { ENVS_EXTENSION_ID } from '../../common/constants'; import { Common, VenvManagerStrings } from '../../common/localize'; @@ -52,6 +61,14 @@ export interface CreateEnvironmentResult { * Exists if error occurred while installing packages and includes error description. */ pkgInstallationErr?: string; + + /** Package installation was cancelled and its process tree may still be terminating. */ + pkgInstallationCancelled?: boolean; +} + +export interface CreateWithProgressOptions { + /** Record uv-created environments in the workspace-scoped uv cache. Defaults to true. */ + readonly trackUvEnvironment?: boolean; } export async function clearVenvCache(): Promise { @@ -340,6 +357,7 @@ export async function createWithProgress( venvRoot: Uri, envPath: string, packages?: PipPackages, + options?: CreateWithProgressOptions, ): Promise { const pythonPath = os.platform() === 'win32' ? path.join(envPath, 'Scripts', 'python.exe') : path.join(envPath, 'bin', 'python'); @@ -383,6 +401,7 @@ export async function createWithProgress( const env = api.createPythonEnvironmentItem(await getPythonInfo(resolved), manager); if ( + options?.trackUvEnvironment !== false && useUv && (resolved.kind === NativePythonEnvironmentKind.venvUv || resolved.kind === NativePythonEnvironmentKind.uvWorkspace) @@ -401,6 +420,7 @@ export async function createWithProgress( } catch (e) { // error occurred while installing packages result.pkgInstallationErr = e instanceof Error ? e.message : String(e); + result.pkgInstallationCancelled = e instanceof CancellationError; } } result.environment = env; diff --git a/src/test/common/inlineScriptCacheKey.unit.test.ts b/src/test/common/inlineScriptCacheKey.unit.test.ts index ce0e72c8b..0f52db3b4 100644 --- a/src/test/common/inlineScriptCacheKey.unit.test.ts +++ b/src/test/common/inlineScriptCacheKey.unit.test.ts @@ -106,6 +106,17 @@ suite('inlineScriptCacheKey', () => { ); }); + test('whitespace inside quoted marker values is preserved exactly', () => { + assert.strictEqual( + normalizeDependency('pkg; platform_version == "X Y"'), + 'pkg; platform_version=="X Y"', + ); + assert.notStrictEqual( + normalizeDependency('pkg; platform_version == "X Y"'), + normalizeDependency('pkg; platform_version == "X Y"'), + ); + }); + test('empty and whitespace-only entries normalize to empty string', () => { assert.strictEqual(normalizeDependency(''), ''); assert.strictEqual(normalizeDependency(' '), ''); @@ -119,6 +130,14 @@ suite('inlineScriptCacheKey', () => { assert.ok(result.includes('requests'), 'leading name should still appear'); }); + test('direct-reference URL and marker boundaries are preserved exactly', () => { + const conditional = "pkg @ https://example.invalid/pkg! ;python_version == '0'"; + const unconditional = "pkg @ https://example.invalid/pkg!;python_version=='0'"; + assert.strictEqual(normalizeDependency(conditional), conditional); + assert.strictEqual(normalizeDependency(unconditional), unconditional); + assert.notStrictEqual(normalizeDependency(conditional), normalizeDependency(unconditional)); + }); + test('leading and trailing whitespace is stripped', () => { assert.strictEqual(normalizeDependency(' requests '), 'requests'); assert.strictEqual(normalizeDependency('\trequests<3\n'), 'requests<3'); @@ -240,6 +259,30 @@ suite('inlineScriptCacheKey', () => { assert.notStrictEqual(a, b); }); + test('meaningful whitespace inside quoted marker values changes the key', () => { + const a = computeCacheKey({ + dependencies: ['pkg; platform_version == "X Y"'], + interpreterPath: interpreter, + }); + const b = computeCacheKey({ + dependencies: ['pkg; platform_version == "X Y"'], + interpreterPath: interpreter, + }); + assert.notStrictEqual(a, b); + }); + + test('direct-reference URL terminator whitespace changes the key', () => { + const conditional = computeCacheKey({ + dependencies: ["pkg @ https://example.invalid/pkg! ;python_version == '0'"], + interpreterPath: interpreter, + }); + const unconditional = computeCacheKey({ + dependencies: ["pkg @ https://example.invalid/pkg!;python_version=='0'"], + interpreterPath: interpreter, + }); + assert.notStrictEqual(conditional, unconditional); + }); + test('changing the interpreter path changes the key', () => { const a = computeCacheKey({ dependencies: ['requests'], interpreterPath: '/usr/bin/python3' }); const b = computeCacheKey({ dependencies: ['requests'], interpreterPath: '/opt/python/python3' }); diff --git a/src/test/common/inlineScriptCacheLayout.unit.test.ts b/src/test/common/inlineScriptCacheLayout.unit.test.ts index dfc0517b0..604e35b0b 100644 --- a/src/test/common/inlineScriptCacheLayout.unit.test.ts +++ b/src/test/common/inlineScriptCacheLayout.unit.test.ts @@ -2,6 +2,7 @@ // Licensed under the MIT License. import assert from 'assert'; +import fsExtra from 'fs-extra'; import * as fs from 'fs-extra'; import * as os from 'os'; import * as path from 'path'; @@ -13,9 +14,11 @@ import { InlineScriptEnvMeta, META_JSON_FILENAME, META_SCHEMA_VERSION, + getBaseInterpreterStatus, getMetaJsonPath, getScriptEnvCacheRoot, getScriptEnvDir, + inspectMetaJson, readMetaJson, selectStaleEntries, verifyBaseInterpreterExists, @@ -27,9 +30,9 @@ import * as platformUtils from '../../common/utils/platformUtils'; function makeMeta(overrides: Partial = {}): InlineScriptEnvMeta { return { schemaVersion: META_SCHEMA_VERSION, - scriptFsPath: '/tmp/script.py', + baseInterpreterPath: Uri.file(path.join(os.tmpdir(), 'base-python')).fsPath, + baseInterpreterVersion: '3.12.4', lastUsedAt: '2026-06-18T22:45:12.000Z', - requiresPython: '>=3.11', ...overrides, }; } @@ -133,14 +136,13 @@ suite('inlineScriptCacheLayout', () => { ); }); - test('writeMetaJson serializes optional requiresPython as undefined-erased', async () => { - const meta = makeMeta({ requiresPython: undefined }); + test('writeMetaJson only serializes environment-level metadata', async () => { + const meta = makeMeta(); await writeMetaJson(envDir, meta); const onDisk = JSON.parse(await fs.readFile(getMetaJsonPath(envDir).fsPath, 'utf8')); + assert.strictEqual(onDisk.baseInterpreterPath, meta.baseInterpreterPath); + assert.strictEqual('scriptFsPath' in onDisk, false); assert.strictEqual('requiresPython' in onDisk, false); - const read = await readMetaJson(envDir); - assert.ok(read); - assert.strictEqual(read.requiresPython, undefined); }); test('writeMetaJson produces human-readable, indented JSON', async () => { @@ -175,6 +177,46 @@ suite('inlineScriptCacheLayout', () => { assert.ok(traceWarnStub.called, 'expected a traceWarn'); }); + test('classifies missing, malformed, and valid metadata distinctly', async () => { + assert.deepStrictEqual(await inspectMetaJson(envDir), { kind: 'missing' }); + await writeRaw('this is not json'); + assert.deepStrictEqual(await inspectMetaJson(envDir), { kind: 'invalid' }); + const metadata = makeMeta(); + await writeRaw(JSON.stringify(metadata)); + assert.deepStrictEqual(await inspectMetaJson(envDir), { kind: 'valid', metadata }); + }); + + test('classifies non-ENOENT sidecar stat failures as unavailable', async () => { + sinon.stub(fsExtra, 'lstat').rejects(Object.assign(new Error('permission denied'), { code: 'EACCES' })); + assert.deepStrictEqual(await inspectMetaJson(envDir), { kind: 'unavailable' }); + }); + + test('classifies non-ENOENT sidecar read failures as unavailable', async () => { + await writeRaw(JSON.stringify(makeMeta())); + sinon.stub(fsExtra, 'readFile').rejects(Object.assign(new Error('I/O error'), { code: 'EIO' })); + assert.deepStrictEqual(await inspectMetaJson(envDir), { kind: 'unavailable' }); + }); + + test('does not follow a sidecar symlink outside the environment', async function () { + const externalMetaPath = path.join(tmpDir, 'external-meta.json'); + const sidecarPath = getMetaJsonPath(envDir).fsPath; + await fs.writeJson(externalMetaPath, makeMeta()); + try { + await fs.symlink(externalMetaPath, sidecarPath, 'file'); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'EPERM' || code === 'EACCES') { + this.skip(); + return; + } + throw error; + } + + assert.deepStrictEqual(await inspectMetaJson(envDir), { kind: 'invalid' }); + assert.strictEqual(await readMetaJson(envDir), undefined); + assert.deepStrictEqual(await fs.readJson(externalMetaPath), makeMeta()); + }); + test('returns undefined for malformed JSON', async () => { await writeRaw('this is not json'); const result = await readMetaJson(envDir); @@ -194,37 +236,63 @@ suite('inlineScriptCacheLayout', () => { assert.ok(traceWarnStub.called); }); - test('returns undefined when scriptFsPath is missing', async () => { - const { scriptFsPath: _omit, ...partial } = makeMeta(); + test('returns undefined when baseInterpreterPath is missing', async () => { + const { baseInterpreterPath: _omit, ...partial } = makeMeta(); await writeRaw(JSON.stringify(partial)); const result = await readMetaJson(envDir); assert.strictEqual(result, undefined); }); - test('returns undefined when scriptFsPath is an empty string', async () => { - await writeRaw(JSON.stringify({ ...makeMeta(), scriptFsPath: '' })); + test('returns undefined when baseInterpreterPath is an empty string', async () => { + await writeRaw(JSON.stringify({ ...makeMeta(), baseInterpreterPath: '' })); const result = await readMetaJson(envDir); assert.strictEqual(result, undefined); }); - test('returns undefined when lastUsedAt is not parseable', async () => { - await writeRaw(JSON.stringify({ ...makeMeta(), lastUsedAt: 'not-a-date' })); + test('returns undefined when baseInterpreterPath is relative', async () => { + await writeRaw(JSON.stringify({ ...makeMeta(), baseInterpreterPath: path.join('relative', 'python') })); const result = await readMetaJson(envDir); assert.strictEqual(result, undefined); }); - test('returns undefined when requiresPython is present but not a string', async () => { - await writeRaw(JSON.stringify({ ...makeMeta(), requiresPython: 311 })); + test('returns undefined when baseInterpreterPath is not a string', async () => { + await writeRaw(JSON.stringify({ ...makeMeta(), baseInterpreterPath: 312 })); const result = await readMetaJson(envDir); assert.strictEqual(result, undefined); }); - test('accepts a meta with requiresPython explicitly omitted', async () => { - const { requiresPython: _omit, ...partial } = makeMeta(); + test('returns undefined when baseInterpreterVersion is missing or blank', async () => { + const { baseInterpreterVersion: _omit, ...partial } = makeMeta(); await writeRaw(JSON.stringify(partial)); + assert.strictEqual(await readMetaJson(envDir), undefined); + await writeRaw(JSON.stringify({ ...makeMeta(), baseInterpreterVersion: ' ' })); + assert.strictEqual(await readMetaJson(envDir), undefined); + }); + + test('returns undefined when lastUsedAt is not parseable', async () => { + await writeRaw(JSON.stringify({ ...makeMeta(), lastUsedAt: 'not-a-date' })); + const result = await readMetaJson(envDir); + assert.strictEqual(result, undefined); + }); + + test('drops legacy script-specific fields', async () => { + await writeRaw(JSON.stringify({ ...makeMeta(), scriptFsPath: 'script.py', requiresPython: '>=3.11' })); const result = await readMetaJson(envDir); assert.ok(result); - assert.strictEqual(result.requiresPython, undefined); + assert.strictEqual('scriptFsPath' in result, false); + assert.strictEqual('requiresPython' in result, false); + }); + + test('rejects the provisional pre-writer v1 sidecar shape', async () => { + await writeRaw( + JSON.stringify({ + schemaVersion: 1, + scriptFsPath: Uri.file(path.join(os.tmpdir(), 'script.py')).fsPath, + lastUsedAt: '2026-06-18T22:45:12.000Z', + requiresPython: '>=3.11', + }), + ); + assert.strictEqual(await readMetaJson(envDir), undefined); }); test('returns undefined for a top-level non-object payload', async () => { @@ -245,11 +313,6 @@ suite('inlineScriptCacheLayout', () => { assert.ok(await readMetaJson(envDir)); }); - test('returns undefined when requiresPython is explicitly null', async () => { - await writeRaw(JSON.stringify({ ...makeMeta(), requiresPython: null })); - assert.strictEqual(await readMetaJson(envDir), undefined); - }); - test('returns undefined for a non-canonical ISO timestamp (e.g. "2026")', async () => { await writeRaw(JSON.stringify({ ...makeMeta(), lastUsedAt: '2026' })); assert.strictEqual(await readMetaJson(envDir), undefined); @@ -295,6 +358,28 @@ suite('inlineScriptCacheLayout', () => { }); }); + suite('getBaseInterpreterStatus classification', () => { + let tmpDir: string; + let envDir: Uri; + + setup(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'isclayout-base-status-')); + envDir = Uri.file(path.join(tmpDir, 'env')); + await fs.ensureDir(envDir.fsPath); + sinon.stub(platformUtils, 'isWindows').returns(false); + }); + + teardown(async () => { + await fs.remove(tmpDir); + }); + + test('distinguishes a missing base interpreter from an unavailable stat', async () => { + assert.strictEqual(await getBaseInterpreterStatus(envDir), 'missing'); + sinon.stub(fsExtra, 'stat').rejects(Object.assign(new Error('I/O error'), { code: 'EIO' })); + assert.strictEqual(await getBaseInterpreterStatus(envDir), 'unavailable'); + }); + }); + suite('selectStaleEntries', () => { const TTL_MS = 14 * 24 * 60 * 60 * 1000; const now = new Date('2026-06-22T12:00:00.000Z'); diff --git a/src/test/common/inlineScriptInterpreter.unit.test.ts b/src/test/common/inlineScriptInterpreter.unit.test.ts index b0ec8c979..6a58680c1 100644 --- a/src/test/common/inlineScriptInterpreter.unit.test.ts +++ b/src/test/common/inlineScriptInterpreter.unit.test.ts @@ -156,6 +156,13 @@ suite('inlineScriptInterpreter', () => { assert.strictEqual(picked.version, '3.12.4'); }); + test('whitespace-only requiresPython is treated as no constraint', () => { + const envs = [makeEnv('3.10.0'), makeEnv('3.12.4')]; + const picked = pickCompatibleInterpreter(envs, ' '); + assert.ok(picked, 'whitespace-only constraint must not silently reject all envs'); + assert.strictEqual(picked.version, '3.12.4'); + }); + test('ranks versions with pre-release / dev / local suffixes by release segments only', () => { // 3.12.0a1 and 3.12.0.dev1 both parse to [3,12,0]; stable sort // means the first-listed 3.12 entry wins. diff --git a/src/test/common/lockfile.apis.unit.test.ts b/src/test/common/lockfile.apis.unit.test.ts new file mode 100644 index 000000000..b0d1b3fc0 --- /dev/null +++ b/src/test/common/lockfile.apis.unit.test.ts @@ -0,0 +1,142 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import assert from 'assert'; +import { ChildProcessWithoutNullStreams, spawn } from 'child_process'; +import * as fs from 'fs-extra'; +import * as os from 'os'; +import * as path from 'path'; +import { acquireFileLock, AcquireFileLockOptions } from '../../common/lockfile.apis'; + +const OPTIONS: AcquireFileLockOptions = { + timeoutMs: 40, + retryIntervalMs: 5, +}; + +const LOCK_MODULE_PATH = path.resolve(__dirname, '..', '..', 'common', 'lockfile.apis.js'); + +suite('lockfile APIs', () => { + let tempRoot: string; + let targetPath: string; + + setup(async () => { + tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'python-envs-lock-')); + targetPath = path.join(tempRoot, 'cache-entry'); + }); + + teardown(async () => { + await fs.remove(tempRoot); + }); + + function startLockingChild(exitWithoutRelease: boolean): ChildProcessWithoutNullStreams { + const script = ` + const { acquireFileLock } = require(process.argv[1]); + acquireFileLock(process.argv[2], { timeoutMs: 1000, retryIntervalMs: 10 }) + .then((release) => { + process.stdout.write('locked\\n'); + if (${exitWithoutRelease}) { + process.exit(0); + } + process.stdin.once('data', async () => { + await release(); + process.exit(0); + }); + }) + .catch((error) => { + process.stderr.write(String(error && error.stack ? error.stack : error)); + process.exit(1); + }); + `; + return spawn(process.execPath, ['-e', script, LOCK_MODULE_PATH, targetPath]); + } + + async function waitForLocked(child: ChildProcessWithoutNullStreams): Promise { + await new Promise((resolve, reject) => { + let stdout = ''; + let stderr = ''; + child.stdout.on('data', (data) => { + stdout += data.toString(); + if (stdout.includes('locked')) { + resolve(); + } + }); + child.stderr.on('data', (data) => { + stderr += data.toString(); + }); + child.once('error', reject); + child.once('exit', (code) => { + if (!stdout.includes('locked')) { + reject(new Error(`locking child exited with code ${code}: ${stderr}`)); + } + }); + }); + } + + async function waitForExit(child: ChildProcessWithoutNullStreams): Promise { + if (child.exitCode !== null) { + assert.strictEqual(child.exitCode, 0); + return; + } + await new Promise((resolve, reject) => { + child.once('error', reject); + child.once('exit', (code) => { + if (code === 0) { + resolve(); + } else { + reject(new Error(`locking child exited with code ${code}`)); + } + }); + }); + } + + test('excludes a second owner until the first releases the lock', async () => { + const release = await acquireFileLock(targetPath, OPTIONS); + await assert.rejects(acquireFileLock(targetPath, OPTIONS), (error: NodeJS.ErrnoException) => { + return error.code === 'ELOCKED'; + }); + + await release(); + const releaseAfterRetry = await acquireFileLock(targetPath, OPTIONS); + await releaseAfterRetry(); + assert.strictEqual(await fs.pathExists(`${path.resolve(targetPath)}.lock`), false); + }); + + test('excludes another process until the owner explicitly releases', async () => { + const child = startLockingChild(false); + await waitForLocked(child); + + await assert.rejects(acquireFileLock(targetPath, OPTIONS), (error: NodeJS.ErrnoException) => { + return error.code === 'ELOCKED'; + }); + + child.stdin.write('release\n'); + await waitForExit(child); + const release = await acquireFileLock(targetPath, OPTIONS); + await release(); + }); + + test('leaves the lock fail-closed when the owner process exits without release', async () => { + const child = startLockingChild(true); + await waitForLocked(child); + await waitForExit(child); + + await assert.rejects(acquireFileLock(targetPath, OPTIONS), (error: NodeJS.ErrnoException) => { + return error.code === 'ELOCKED'; + }); + assert.strictEqual(await fs.pathExists(`${path.resolve(targetPath)}.lock`), true); + }); + + test('an old release cannot remove a successor lock generation', async () => { + const release = await acquireFileLock(targetPath, OPTIONS); + const lockPath = `${path.resolve(targetPath)}.lock`; + await fs.remove(lockPath); + await fs.ensureDir(lockPath); + const successorMarker = path.join(lockPath, 'successor-owner'); + await fs.writeFile(successorMarker, ''); + + await assert.rejects(release(), (error: NodeJS.ErrnoException) => { + return error.code === 'ECOMPROMISED'; + }); + assert.strictEqual(await fs.pathExists(successorMarker), true); + }); +}); \ No newline at end of file diff --git a/src/test/managers/builtin/helpers.cancellation.unit.test.ts b/src/test/managers/builtin/helpers.cancellation.unit.test.ts new file mode 100644 index 000000000..5a4583066 --- /dev/null +++ b/src/test/managers/builtin/helpers.cancellation.unit.test.ts @@ -0,0 +1,85 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import assert from 'assert'; +import * as sinon from 'sinon'; +import { CancellationError, CancellationToken, CancellationTokenSource } from 'vscode'; +import * as childProcessApis from '../../../common/childProcess.apis'; +import { runPython, runUV } from '../../../managers/builtin/helpers'; +import { MockChildProcess } from '../../mocks/mockChildProcess'; + +suite('process helper cancellation safety', () => { + let spawnStub: sinon.SinonStub; + + setup(() => { + spawnStub = sinon.stub(childProcessApis, 'spawnProcess'); + }); + + teardown(() => { + sinon.restore(); + }); + + async function expectCancellation( + process: MockChildProcess, + run: (token: CancellationToken) => Promise, + killBehavior: 'emitError' | 'throw', + ): Promise { + spawnStub.returns(process); + const killStub = sinon.stub(process, 'kill'); + if (killBehavior === 'emitError') { + killStub.callsFake(() => { + process.emit('error', new Error('kill EPERM')); + return false; + }); + } else { + killStub.throws(new Error('kill EPERM')); + } + + const tokenSource = new CancellationTokenSource(); + const result = run(tokenSource.token); + tokenSource.cancel(); + + await assert.rejects(result, (error: Error) => { + assert.ok(error instanceof CancellationError); + return true; + }); + assert.ok(killStub.calledOnce); + tokenSource.dispose(); + } + + test('runUV remains cancelled when kill emits an error synchronously', async () => { + const process = new MockChildProcess('uv', ['pip', 'install', 'requests']); + await expectCancellation( + process, + (token) => runUV(['pip', 'install', 'requests'], undefined, undefined, token), + 'emitError', + ); + }); + + test('runUV remains cancelled when kill throws', async () => { + const process = new MockChildProcess('uv', ['pip', 'install', 'requests']); + await expectCancellation( + process, + (token) => runUV(['pip', 'install', 'requests'], undefined, undefined, token), + 'throw', + ); + }); + + test('runPython remains cancelled when kill emits an error synchronously', async () => { + const process = new MockChildProcess('python', ['-m', 'pip', 'install', 'requests']); + await expectCancellation( + process, + (token) => runPython('python', ['-m', 'pip', 'install', 'requests'], undefined, undefined, token), + 'emitError', + ); + }); + + test('runPython remains cancelled when kill throws', async () => { + const process = new MockChildProcess('python', ['-m', 'pip', 'install', 'requests']); + await expectCancellation( + process, + (token) => runPython('python', ['-m', 'pip', 'install', 'requests'], undefined, undefined, token), + 'throw', + ); + }); +}); diff --git a/src/test/managers/builtin/inlineScriptEnvManager.unit.test.ts b/src/test/managers/builtin/inlineScriptEnvManager.unit.test.ts index 91e1bd84e..dd42c3898 100644 --- a/src/test/managers/builtin/inlineScriptEnvManager.unit.test.ts +++ b/src/test/managers/builtin/inlineScriptEnvManager.unit.test.ts @@ -2,160 +2,763 @@ // Licensed under the MIT License. import assert from 'assert'; +import * as fs from 'fs-extra'; +import * as os from 'os'; +import * as path from 'path'; import * as sinon from 'sinon'; import { LogOutputChannel, Uri } from 'vscode'; -import { EnvironmentManager, PythonEnvironment } from '../../../api'; +import { EnvironmentManager, PythonEnvironment, PythonEnvironmentApi } from '../../../api'; +import * as cacheKey from '../../../common/inlineScriptCacheKey'; +import * as cacheLayout from '../../../common/inlineScriptCacheLayout'; +import * as lockfileApis from '../../../common/lockfile.apis'; +import * as metadataReader from '../../../common/inlineScriptMetadata'; import { InlineScriptEnvManager } from '../../../managers/builtin/inlineScriptEnvManager'; +import * as venvUtils from '../../../managers/builtin/venvUtils'; +import { NativePythonFinder } from '../../../managers/common/nativePythonFinder'; + +const CACHE_KEY = '0123456789abcdef'; +const NOW = new Date('2026-07-21T12:00:00.000Z'); +const VALID_METADATA: metadataReader.InlineScriptMetadata = { + requiresPython: '>=3.11', + dependencies: ['requests'], + range: { start: 0, end: 40 }, +}; function makeFakeLog(): LogOutputChannel { - return sinon.createStubInstance( - class { - info() {} - warn() {} - error() {} - debug() {} - trace() {} - show() {} - dispose() {} - append() {} - appendLine() {} - replace() {} - clear() {} - hide() {} - }, - ) as unknown as LogOutputChannel; + return { + info: sinon.stub(), + warn: sinon.stub(), + error: sinon.stub(), + debug: sinon.stub(), + trace: sinon.stub(), + show: sinon.stub(), + dispose: sinon.stub(), + append: sinon.stub(), + appendLine: sinon.stub(), + replace: sinon.stub(), + clear: sinon.stub(), + hide: sinon.stub(), + } as unknown as LogOutputChannel; } -function makeEnv(): PythonEnvironment { +function makeEnvironment( + managerId: string, + version: string, + executable: string, + sysPrefix: string = path.dirname(executable), + name: string = `Python ${version}`, +): PythonEnvironment { return { - envId: { id: 'fake', managerId: 'ms-python.python:inline-script' }, - name: 'fake', - displayName: 'fake', - displayPath: '/fake', - version: '3.12.0', - environmentPath: Uri.file('/fake'), - execInfo: { run: { executable: '/fake' } }, - sysPrefix: '/fake', + envId: { id: `${managerId}-${version}-${executable}`, managerId }, + name, + displayName: `Python ${version}`, + displayPath: executable, + version, + environmentPath: Uri.file(executable), + execInfo: { run: { executable } }, + sysPrefix, }; } -suite('InlineScriptEnvManager (skeleton)', () => { - let mgr: InlineScriptEnvManager; +function venvPythonPath(envDir: string): string { + return path.join(envDir, process.platform === 'win32' ? 'Scripts' : 'bin', process.platform === 'win32' ? 'python.exe' : 'python'); +} + +suite('InlineScriptEnvManager', () => { + let api: PythonEnvironmentApi; + let apiGetEnvironmentsStub: sinon.SinonStub; + let baseEnvironment: PythonEnvironment; + let baseExecutable: string; + let baseManager: EnvironmentManager; + let computeCacheKeyStub: sinon.SinonStub; + let createWithProgressStub: sinon.SinonStub; + let globalStorageUri: Uri; + let lockStub: sinon.SinonStub; + let manager: InlineScriptEnvManager; + let nativeFinder: NativePythonFinder; + let readMetadataStub: sinon.SinonStub; + let inspectMetaStub: sinon.SinonStub; + let releaseLockStub: sinon.SinonStub; + let resolveVenvStub: sinon.SinonStub; + let tempRoot: string; + let baseInterpreterStatusStub: sinon.SinonStub; + let writeMetaStub: sinon.SinonStub; + + setup(async () => { + tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'inline-script-manager-')); + globalStorageUri = Uri.file(path.join(tempRoot, 'global-storage')); + baseExecutable = path.join(tempRoot, 'base-python', process.platform === 'win32' ? 'python.exe' : 'python'); + await fs.outputFile(baseExecutable, ''); + baseEnvironment = makeEnvironment('ms-python.python:system', '3.12.4', baseExecutable); + + apiGetEnvironmentsStub = sinon.stub().resolves([baseEnvironment]); + api = { getEnvironments: apiGetEnvironmentsStub } as unknown as PythonEnvironmentApi; + nativeFinder = {} as NativePythonFinder; + baseManager = {} as EnvironmentManager; - setup(() => { - mgr = new InlineScriptEnvManager(makeFakeLog()); + readMetadataStub = sinon.stub(metadataReader, 'readInlineScriptMetadataFromFile').resolves(VALID_METADATA); + computeCacheKeyStub = sinon.stub(cacheKey, 'computeCacheKey').returns(CACHE_KEY); + inspectMetaStub = sinon.stub(cacheLayout, 'inspectMetaJson').resolves({ kind: 'missing' }); + baseInterpreterStatusStub = sinon.stub(cacheLayout, 'getBaseInterpreterStatus').resolves('available'); + writeMetaStub = sinon.stub(cacheLayout, 'writeMetaJson').resolves(); + releaseLockStub = sinon.stub().resolves(); + lockStub = sinon.stub(lockfileApis, 'acquireFileLock').resolves(releaseLockStub); + resolveVenvStub = sinon.stub(venvUtils, 'resolveVenvPythonEnvironmentPath').resolves(undefined); + createWithProgressStub = sinon.stub(venvUtils, 'createWithProgress').callsFake(async (...args: unknown[]) => { + const envDir = args[6] as string; + await fs.outputFile(venvPythonPath(envDir), ''); + return { + environment: makeEnvironment('ms-python.python:inline-script', '3.12.4', venvPythonPath(envDir), envDir), + }; + }); + + sinon.useFakeTimers({ now: NOW, toFake: ['Date'] }); + manager = new InlineScriptEnvManager(nativeFinder, api, baseManager, globalStorageUri, makeFakeLog()); }); - teardown(() => { - mgr.dispose(); + teardown(async () => { + manager.dispose(); sinon.restore(); + await fs.remove(tempRoot); }); - suite('static metadata', () => { - test('name is "inline-script"', () => { - assert.strictEqual(mgr.name, 'inline-script'); + function scriptUri(name = 'script.py'): Uri { + return Uri.file(path.join(tempRoot, name)); + } + + function envDir(): Uri { + return cacheLayout.getScriptEnvDir(globalStorageUri, CACHE_KEY); + } + + function setSidecar(metadata: cacheLayout.InlineScriptEnvMeta): void { + inspectMetaStub.resolves({ kind: 'valid', metadata }); + } + + suite('static metadata and deferred methods', () => { + test('exposes creation but leaves later-phase methods empty', async () => { + const asInterface: EnvironmentManager = manager; + assert.strictEqual(typeof asInterface.create, 'function'); + assert.strictEqual(asInterface.remove, undefined); + assert.strictEqual(asInterface.quickCreateConfig, undefined); + assert.deepStrictEqual(await manager.getEnvironments('all'), []); + assert.strictEqual(await manager.get(scriptUri()), undefined); + assert.strictEqual(await manager.resolve(scriptUri()), undefined); + }); + + test('retains inline-script manager presentation metadata', () => { + assert.strictEqual(manager.name, 'inline-script'); + assert.ok(manager.displayName); + assert.strictEqual(manager.preferredPackageManagerId, 'ms-python.python:pip'); + assert.ok(manager.iconPath); + assert.ok(manager.tooltip); }); + }); - test('displayName is set (for the picker section header)', () => { - assert.ok(mgr.displayName); - assert.ok(mgr.displayName.length > 0); + suite('scope and metadata validation', () => { + test('rejects global, empty, multiple, and non-file scopes without reading metadata', async () => { + assert.strictEqual(await manager.create('global'), undefined); + assert.strictEqual(await manager.create([]), undefined); + assert.strictEqual(await manager.create([scriptUri('a.py'), scriptUri('b.py')]), undefined); + assert.strictEqual(await manager.create(Uri.parse('untitled:script.py')), undefined); + assert.strictEqual(readMetadataStub.callCount, 0); + assert.strictEqual(lockStub.callCount, 0); }); - test('preferredPackageManagerId is the standard pip manager id', () => { - assert.strictEqual(mgr.preferredPackageManagerId, 'ms-python.python:pip'); + test('accepts a singleton URI array', async () => { + const uri = scriptUri(); + const result = await manager.create([uri]); + assert.ok(result); + assert.ok(readMetadataStub.calledOnceWithExactly(uri)); }); - test('iconPath is defined (renders in the picker)', () => { - assert.ok(mgr.iconPath); + test('returns undefined without cache mutation when metadata is absent', async () => { + readMetadataStub.resolves(undefined); + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(apiGetEnvironmentsStub.callCount, 0); + assert.strictEqual(lockStub.callCount, 0); }); - test('tooltip is defined (shown on hover in the picker)', () => { - assert.ok(mgr.tooltip); + test('rejects empty dependency entries before selecting or locking', async () => { + readMetadataStub.resolves({ ...VALID_METADATA, dependencies: ['requests', ' '] }); + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(apiGetEnvironmentsStub.callCount, 0); + assert.strictEqual(lockStub.callCount, 0); }); }); - suite('skeleton method behavior', () => { - test('getEnvironments("all") returns []', async () => { - assert.deepStrictEqual(await mgr.getEnvironments('all'), []); + suite('base interpreter selection', () => { + test('excludes derived managers even when they report newer global environments', async () => { + const pipenv = makeEnvironment('ms-python.python:pipenv', '3.14.0', baseExecutable); + apiGetEnvironmentsStub.resolves([pipenv, baseEnvironment]); + + await manager.create(scriptUri()); + + assert.strictEqual(createWithProgressStub.firstCall.args[4], baseEnvironment); }); - test('getEnvironments("global") returns []', async () => { - assert.deepStrictEqual(await mgr.getEnvironments('global'), []); + test('excludes named conda environments even when they are newer than conda base', async () => { + const condaNamed = makeEnvironment( + 'ms-python.python:conda', + '3.14.0', + baseExecutable, + undefined, + 'project-env', + ); + const condaBase = makeEnvironment( + 'ms-python.python:conda', + '3.11.9', + baseExecutable, + undefined, + 'base', + ); + apiGetEnvironmentsStub.resolves([condaNamed, condaBase]); + + await manager.create(scriptUri()); + + assert.strictEqual(createWithProgressStub.firstCall.args[4], condaBase); }); - test('getEnvironments(Uri) returns []', async () => { - assert.deepStrictEqual(await mgr.getEnvironments(Uri.file('/tmp/script.py')), []); + test('falls back when the newest compatible interpreter cannot be canonicalized', async () => { + const missingExecutable = path.join(tempRoot, 'missing', 'python'); + const newest = makeEnvironment('ms-python.python:system', '3.13.0', missingExecutable); + apiGetEnvironmentsStub.resolves([baseEnvironment, newest]); + + await manager.create(scriptUri()); + + assert.strictEqual(createWithProgressStub.firstCall.args[4], baseEnvironment); }); - test('get(undefined) returns undefined', async () => { - assert.strictEqual(await mgr.get(undefined), undefined); + test('excludes pyenv virtual environments reported in global scope', async () => { + const pyenvVenvRoot = path.join(tempRoot, 'pyenv', 'versions', 'project-env'); + const pyenvVenvExecutable = venvPythonPath(pyenvVenvRoot); + await fs.outputFile(pyenvVenvExecutable, ''); + await fs.outputFile(path.join(pyenvVenvRoot, 'pyvenv.cfg'), 'home = base'); + const pyenvVenv = makeEnvironment( + 'ms-python.python:pyenv', + '3.13.0', + pyenvVenvExecutable, + pyenvVenvRoot, + ); + apiGetEnvironmentsStub.resolves([pyenvVenv, baseEnvironment]); + + await manager.create(scriptUri()); + + assert.strictEqual(createWithProgressStub.firstCall.args[4], baseEnvironment); }); - test('get(Uri) returns undefined', async () => { - assert.strictEqual(await mgr.get(Uri.file('/tmp/script.py')), undefined); + test('does not invoke creation when no installed base satisfies requires-python', async () => { + readMetadataStub.resolves({ ...VALID_METADATA, requiresPython: '>=3.13' }); + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(lockStub.callCount, 0); + assert.strictEqual(createWithProgressStub.callCount, 0); }); + }); + + suite('cache creation', () => { + test('hashes and installs metadata plus additional packages, then writes the sidecar', async () => { + const result = await manager.create(scriptUri(), { additionalPackages: ['pytest'] }); - test('set(scope, env) is a no-op and does not throw', async () => { - await assert.doesNotReject(mgr.set(Uri.file('/tmp/script.py'), makeEnv())); - await assert.doesNotReject(mgr.set(undefined, undefined)); + assert.ok(result); + assert.deepStrictEqual(computeCacheKeyStub.firstCall.args[0], { + dependencies: ['requests', 'pytest'], + interpreterPath: baseExecutable, + }); + assert.strictEqual(createWithProgressStub.firstCall.args[0], nativeFinder); + assert.strictEqual(createWithProgressStub.firstCall.args[1], api); + assert.strictEqual(createWithProgressStub.firstCall.args[3], manager); + assert.strictEqual(createWithProgressStub.firstCall.args[4], baseEnvironment); + assert.strictEqual(createWithProgressStub.firstCall.args[5].fsPath, cacheLayout.getScriptEnvCacheRoot(globalStorageUri).fsPath); + assert.strictEqual(createWithProgressStub.firstCall.args[6], envDir().fsPath); + assert.deepStrictEqual(createWithProgressStub.firstCall.args[7], { + install: ['requests', 'pytest'], + uninstall: [], + }); + assert.deepStrictEqual(writeMetaStub.firstCall.args, [ + envDir(), + { + schemaVersion: cacheLayout.META_SCHEMA_VERSION, + baseInterpreterPath: baseExecutable, + baseInterpreterVersion: baseEnvironment.version, + lastUsedAt: NOW.toISOString(), + }, + ]); + assert.deepStrictEqual(createWithProgressStub.firstCall.args[8], { trackUvEnvironment: false }); + assert.ok(releaseLockStub.calledOnce); }); - test('refresh(scope) is a no-op and does not throw', async () => { - await assert.doesNotReject(mgr.refresh(undefined)); - await assert.doesNotReject(mgr.refresh(Uri.file('/tmp/script.py'))); + test('uses a bounded cross-process lock 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.ok(options.retryIntervalMs > 0); }); - test('resolve(Uri) returns undefined', async () => { - assert.strictEqual(await mgr.resolve(Uri.file('/tmp/script.py')), undefined); + test('coalesces simultaneous same-key creation within one extension host', async () => { + let continueCreation: (() => void) | undefined; + let creationStarted: (() => void) | undefined; + let secondCallHashed: (() => void) | undefined; + const started = new Promise((resolve) => { + creationStarted = resolve; + }); + const secondHashed = new Promise((resolve) => { + secondCallHashed = resolve; + }); + const gate = new Promise((resolve) => { + continueCreation = resolve; + }); + computeCacheKeyStub.callsFake(() => { + if (computeCacheKeyStub.callCount === 2) { + secondCallHashed!(); + } + return CACHE_KEY; + }); + createWithProgressStub.callsFake(async (...args: unknown[]) => { + const target = args[6] as string; + await fs.outputFile(venvPythonPath(target), ''); + creationStarted!(); + await gate; + return { + environment: makeEnvironment( + 'ms-python.python:inline-script', + '3.12.4', + venvPythonPath(target), + target, + ), + }; + }); + + const first = manager.create(scriptUri('a.py')); + await started; + const second = manager.create(scriptUri('b.py')); + await secondHashed; + continueCreation!(); + const [firstResult, secondResult] = await Promise.all([first, second]); + + assert.strictEqual(firstResult, secondResult); + assert.strictEqual(lockStub.callCount, 1); + assert.strictEqual(createWithProgressStub.callCount, 1); }); - test('does not implement optional create / remove / quickCreateConfig', () => { - // Cast via the interface to probe optional methods (the concrete class type doesn't declare them). - const asInterface: EnvironmentManager = mgr; - assert.strictEqual(asInterface.create, undefined); - assert.strictEqual(asInterface.remove, undefined); - assert.strictEqual(asInterface.quickCreateConfig, undefined); + test('returns undefined without building when the cache lock cannot be acquired', async () => { + lockStub.rejects(Object.assign(new Error('already locked'), { code: 'ELOCKED' })); + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(createWithProgressStub.callCount, 0); }); }); - suite('events', () => { - test('onDidChangeEnvironments is exposed and subscribable', () => { - const disposable = mgr.onDidChangeEnvironments(() => undefined); - assert.ok(disposable); - disposable.dispose(); + suite('cache reuse', () => { + test('returns a valid cached environment and refreshes lastUsedAt', async () => { + await fs.ensureDir(envDir().fsPath); + const sidecar: cacheLayout.InlineScriptEnvMeta = { + schemaVersion: cacheLayout.META_SCHEMA_VERSION, + baseInterpreterPath: baseExecutable, + baseInterpreterVersion: baseEnvironment.version, + lastUsedAt: '2026-07-01T00:00:00.000Z', + }; + const cached = makeEnvironment( + 'ms-python.python:inline-script', + '3.12.4', + venvPythonPath(envDir().fsPath), + envDir().fsPath, + ); + await fs.outputFile(venvPythonPath(envDir().fsPath), ''); + setSidecar(sidecar); + resolveVenvStub.resolves(cached); + + const result = await manager.create(scriptUri()); + + assert.strictEqual(result, cached); + assert.strictEqual(createWithProgressStub.callCount, 0); + assert.ok(baseInterpreterStatusStub.calledOnceWithExactly(envDir())); + assert.strictEqual(resolveVenvStub.firstCall.args[0], venvPythonPath(envDir().fsPath)); + assert.deepStrictEqual(writeMetaStub.firstCall.args, [ + envDir(), + { ...sidecar, lastUsedAt: NOW.toISOString() }, + ]); + }); + + test('returns a valid hit even when the last-used timestamp cannot be updated', async () => { + await fs.ensureDir(envDir().fsPath); + setSidecar({ + schemaVersion: cacheLayout.META_SCHEMA_VERSION, + baseInterpreterPath: baseExecutable, + baseInterpreterVersion: baseEnvironment.version, + lastUsedAt: '2026-07-01T00:00:00.000Z', + }); + const cached = makeEnvironment( + 'ms-python.python:inline-script', + '3.12.4', + venvPythonPath(envDir().fsPath), + envDir().fsPath, + ); + await fs.outputFile(venvPythonPath(envDir().fsPath), ''); + resolveVenvStub.resolves(cached); + writeMetaStub.rejects(new Error('read-only filesystem')); + + assert.strictEqual(await manager.create(scriptUri()), cached); + assert.strictEqual(createWithProgressStub.callCount, 0); }); - test('onDidChangeEnvironment is exposed and subscribable', () => { - const disposable = mgr.onDidChangeEnvironment(() => undefined); - assert.ok(disposable); - disposable.dispose(); + test('removes and rebuilds a cache entry whose sidecar names another base', async () => { + await fs.ensureDir(envDir().fsPath); + setSidecar({ + schemaVersion: cacheLayout.META_SCHEMA_VERSION, + baseInterpreterPath: path.join(tempRoot, 'different-python'), + baseInterpreterVersion: baseEnvironment.version, + lastUsedAt: NOW.toISOString(), + }); + + const result = await manager.create(scriptUri()); + + assert.ok(result); + assert.strictEqual(resolveVenvStub.callCount, 0); + assert.strictEqual(createWithProgressStub.callCount, 1); }); - test('skeleton methods do not fire any events', async () => { - const envsListener = sinon.spy(); - const envListener = sinon.spy(); - mgr.onDidChangeEnvironments(envsListener); - mgr.onDidChangeEnvironment(envListener); + test('rebuilds when the base version changed at the same canonical path', async () => { + await fs.ensureDir(envDir().fsPath); + setSidecar({ + schemaVersion: cacheLayout.META_SCHEMA_VERSION, + baseInterpreterPath: baseExecutable, + baseInterpreterVersion: '3.11.9', + lastUsedAt: NOW.toISOString(), + }); + + assert.ok(await manager.create(scriptUri())); + assert.strictEqual(resolveVenvStub.callCount, 0); + assert.strictEqual(createWithProgressStub.callCount, 1); + }); - await mgr.getEnvironments('all'); - await mgr.get(undefined); - await mgr.set(Uri.file('/tmp/script.py'), makeEnv()); - await mgr.refresh(undefined); - await mgr.resolve(Uri.file('/tmp/script.py')); + test('preserves the entry when resolution returns an environment owned by another manager', async () => { + await fs.outputFile(venvPythonPath(envDir().fsPath), ''); + const markerPath = path.join(envDir().fsPath, 'keep.txt'); + await fs.outputFile(markerPath, 'keep'); + setSidecar({ + schemaVersion: cacheLayout.META_SCHEMA_VERSION, + baseInterpreterPath: baseExecutable, + baseInterpreterVersion: baseEnvironment.version, + lastUsedAt: NOW.toISOString(), + }); + resolveVenvStub.resolves( + makeEnvironment( + 'ms-python.python:system', + '3.11.9', + venvPythonPath(envDir().fsPath), + envDir().fsPath, + ), + ); - assert.strictEqual(envsListener.callCount, 0, 'getEnvironments/refresh must not fire envs event'); - assert.strictEqual(envListener.callCount, 0, 'set must not fire env event in the skeleton'); + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(await fs.readFile(markerPath, 'utf8'), 'keep'); + assert.strictEqual(createWithProgressStub.callCount, 0); + assert.strictEqual(writeMetaStub.callCount, 0); + }); + + test('preserves an inline-owned entry whose resolved version is unparseable', async () => { + await fs.outputFile(venvPythonPath(envDir().fsPath), ''); + const markerPath = path.join(envDir().fsPath, 'keep.txt'); + await fs.outputFile(markerPath, 'keep'); + setSidecar({ + schemaVersion: cacheLayout.META_SCHEMA_VERSION, + baseInterpreterPath: baseExecutable, + baseInterpreterVersion: baseEnvironment.version, + lastUsedAt: NOW.toISOString(), + }); + resolveVenvStub.resolves( + makeEnvironment( + 'ms-python.python:inline-script', + 'Unknown', + venvPythonPath(envDir().fsPath), + envDir().fsPath, + ), + ); + + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(await fs.readFile(markerPath, 'utf8'), 'keep'); + assert.strictEqual(createWithProgressStub.callCount, 0); + }); + + test('rebuilds when the resolved environment no longer satisfies requires-python', async () => { + await fs.ensureDir(envDir().fsPath); + setSidecar({ + schemaVersion: cacheLayout.META_SCHEMA_VERSION, + baseInterpreterPath: baseExecutable, + baseInterpreterVersion: baseEnvironment.version, + lastUsedAt: NOW.toISOString(), + }); + await fs.outputFile(venvPythonPath(envDir().fsPath), ''); + resolveVenvStub.resolves( + makeEnvironment( + 'ms-python.python:inline-script', + '3.10.0', + venvPythonPath(envDir().fsPath), + envDir().fsPath, + ), + ); + + assert.ok(await manager.create(scriptUri())); + assert.strictEqual(createWithProgressStub.callCount, 1); + }); + + test('rebuilds when the cached Python differs from the selected base but still satisfies the script', async () => { + await fs.outputFile(venvPythonPath(envDir().fsPath), ''); + setSidecar({ + schemaVersion: cacheLayout.META_SCHEMA_VERSION, + baseInterpreterPath: baseExecutable, + baseInterpreterVersion: baseEnvironment.version, + lastUsedAt: NOW.toISOString(), + }); + resolveVenvStub.resolves( + makeEnvironment( + 'ms-python.python:inline-script', + '3.11.9', + venvPythonPath(envDir().fsPath), + envDir().fsPath, + ), + ); + + assert.ok(await manager.create(scriptUri())); + assert.strictEqual(createWithProgressStub.callCount, 1); + }); + + for (const metadataKind of ['missing', 'invalid'] as const) { + test(`rebuilds an existing cache entry when metadata is ${metadataKind}`, async () => { + const markerPath = path.join(envDir().fsPath, 'keep.txt'); + await fs.outputFile(markerPath, 'keep'); + inspectMetaStub.resolves({ kind: metadataKind }); + + assert.ok(await manager.create(scriptUri())); + assert.strictEqual(await fs.pathExists(markerPath), false); + assert.strictEqual(createWithProgressStub.callCount, 1); + assert.strictEqual(writeMetaStub.callCount, 1); + }); + } + + test('preserves an existing cache entry when metadata is unavailable', async () => { + const markerPath = path.join(envDir().fsPath, 'keep.txt'); + await fs.outputFile(markerPath, 'keep'); + inspectMetaStub.resolves({ kind: 'unavailable' }); + + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(await fs.readFile(markerPath, 'utf8'), 'keep'); + assert.strictEqual(createWithProgressStub.callCount, 0); + assert.strictEqual(writeMetaStub.callCount, 0); + }); + + test('preserves an existing cache entry when metadata inspection rejects', async () => { + const markerPath = path.join(envDir().fsPath, 'keep.txt'); + await fs.outputFile(markerPath, 'keep'); + inspectMetaStub.rejects(new Error('transient read failure')); + + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(await fs.readFile(markerPath, 'utf8'), 'keep'); + assert.strictEqual(createWithProgressStub.callCount, 0); + }); + + test('preserves an existing cache entry when its base interpreter cannot be inspected', async () => { + const markerPath = path.join(envDir().fsPath, 'keep.txt'); + await fs.outputFile(markerPath, 'keep'); + setSidecar({ + schemaVersion: cacheLayout.META_SCHEMA_VERSION, + baseInterpreterPath: baseExecutable, + baseInterpreterVersion: baseEnvironment.version, + lastUsedAt: NOW.toISOString(), + }); + baseInterpreterStatusStub.resolves('unavailable'); + + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(await fs.readFile(markerPath, 'utf8'), 'keep'); + assert.strictEqual(createWithProgressStub.callCount, 0); + }); + + test('rebuilds an existing cache entry when its base interpreter is definitively missing', async () => { + await fs.ensureDir(envDir().fsPath); + setSidecar({ + schemaVersion: cacheLayout.META_SCHEMA_VERSION, + baseInterpreterPath: baseExecutable, + baseInterpreterVersion: baseEnvironment.version, + lastUsedAt: NOW.toISOString(), + }); + baseInterpreterStatusStub.resolves('missing'); + + assert.ok(await manager.create(scriptUri())); + assert.strictEqual(createWithProgressStub.callCount, 1); + }); + + test('does not inspect or modify an entry resolving outside the physical cache root', async function () { + const cacheRoot = cacheLayout.getScriptEnvCacheRoot(globalStorageUri); + const externalEnv = path.join(tempRoot, 'external-env'); + const markerPath = path.join(externalEnv, 'keep.txt'); + await fs.ensureDir(cacheRoot.fsPath); + await fs.outputFile(markerPath, 'keep'); + try { + await fs.symlink(externalEnv, envDir().fsPath, process.platform === 'win32' ? 'junction' : 'dir'); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'EPERM' || code === 'EACCES') { + this.skip(); + return; + } + throw error; + } + + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(await fs.readFile(markerPath, 'utf8'), 'keep'); + assert.strictEqual((await fs.lstat(envDir().fsPath)).isSymbolicLink(), true); + assert.strictEqual(inspectMetaStub.callCount, 0); + assert.strictEqual(writeMetaStub.callCount, 0); + assert.strictEqual(createWithProgressStub.callCount, 0); + }); + + test('does not inspect or modify an entry aliasing another hash directory', async function () { + const cacheRoot = cacheLayout.getScriptEnvCacheRoot(globalStorageUri); + const otherEnv = path.join(cacheRoot.fsPath, 'fedcba9876543210'); + const markerPath = path.join(otherEnv, 'keep.txt'); + await fs.outputFile(markerPath, 'keep'); + try { + await fs.symlink(otherEnv, envDir().fsPath, process.platform === 'win32' ? 'junction' : 'dir'); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'EPERM' || code === 'EACCES') { + this.skip(); + return; + } + throw error; + } + + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(await fs.readFile(markerPath, 'utf8'), 'keep'); + assert.strictEqual((await fs.lstat(envDir().fsPath)).isSymbolicLink(), true); + assert.strictEqual(inspectMetaStub.callCount, 0); + assert.strictEqual(writeMetaStub.callCount, 0); + assert.strictEqual(createWithProgressStub.callCount, 0); + }); + }); + + suite('transaction rollback', () => { + test('retains the partial environment and lock when package installation is cancelled', async () => { + createWithProgressStub.callsFake(async (...args: unknown[]) => { + const target = args[6] as string; + await fs.outputFile(venvPythonPath(target), ''); + return { + environment: makeEnvironment( + 'ms-python.python:inline-script', + '3.12.4', + venvPythonPath(target), + target, + ), + pkgInstallationErr: 'Canceled', + pkgInstallationCancelled: true, + }; + }); + + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(await fs.pathExists(envDir().fsPath), true); + assert.strictEqual(writeMetaStub.callCount, 0); + assert.strictEqual(releaseLockStub.callCount, 0); + }); + + test('removes the partial environment when package installation fails', async () => { + createWithProgressStub.callsFake(async (...args: unknown[]) => { + const target = args[6] as string; + await fs.ensureDir(target); + return { + environment: makeEnvironment( + 'ms-python.python:inline-script', + '3.12.4', + venvPythonPath(target), + target, + ), + pkgInstallationErr: 'network failure', + }; + }); + + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(await fs.pathExists(envDir().fsPath), false); + assert.strictEqual(writeMetaStub.callCount, 0); + assert.ok(releaseLockStub.calledOnce); + }); + + test('removes the new environment when sidecar writing fails', async () => { + writeMetaStub.rejects(new Error('disk full')); + + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(await fs.pathExists(envDir().fsPath), false); + assert.ok(releaseLockStub.calledOnce); + }); + + test('removes a partial environment when createWithProgress throws', async () => { + createWithProgressStub.callsFake(async (...args: unknown[]) => { + await fs.ensureDir(args[6] as string); + throw new Error('unexpected create failure'); + }); + + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(await fs.pathExists(envDir().fsPath), false); + assert.ok(releaseLockStub.calledOnce); + }); + + test('rejects and removes a created environment with a different Python release', async () => { + createWithProgressStub.callsFake(async (...args: unknown[]) => { + const target = args[6] as string; + await fs.outputFile(venvPythonPath(target), ''); + return { + environment: makeEnvironment( + 'ms-python.python:inline-script', + '3.11.9', + venvPythonPath(target), + target, + ), + }; + }); + + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(await fs.pathExists(envDir().fsPath), false); + assert.strictEqual(writeMetaStub.callCount, 0); + }); + + test('rejects and removes a created environment outside the expected cache directory', async () => { + createWithProgressStub.callsFake(async (...args: unknown[]) => { + const target = args[6] as string; + const otherRoot = path.join(tempRoot, 'unexpected-env'); + const otherPython = venvPythonPath(otherRoot); + await fs.outputFile(venvPythonPath(target), ''); + await fs.outputFile(otherPython, ''); + return { + environment: makeEnvironment( + 'ms-python.python:inline-script', + '3.12.4', + otherPython, + otherRoot, + ), + }; + }); + + assert.strictEqual(await manager.create(scriptUri()), undefined); + assert.strictEqual(await fs.pathExists(envDir().fsPath), false); + assert.strictEqual(writeMetaStub.callCount, 0); }); }); - suite('disposal', () => { - test('dispose() does not throw', () => { - assert.doesNotThrow(() => mgr.dispose()); + suite('events and disposal', () => { + test('create does not establish an association or fire later-phase events', async () => { + const environmentsListener = sinon.spy(); + const environmentListener = sinon.spy(); + manager.onDidChangeEnvironments(environmentsListener); + manager.onDidChangeEnvironment(environmentListener); + + assert.ok(await manager.create(scriptUri())); + assert.deepStrictEqual(await manager.getEnvironments('all'), []); + assert.strictEqual(await manager.get(scriptUri()), undefined); + assert.strictEqual(environmentsListener.callCount, 0); + assert.strictEqual(environmentListener.callCount, 0); }); - test('dispose() is idempotent', () => { - mgr.dispose(); - assert.doesNotThrow(() => mgr.dispose()); + test('dispose is idempotent', () => { + manager.dispose(); + assert.doesNotThrow(() => manager.dispose()); }); }); }); diff --git a/src/test/managers/builtin/inlineScriptMain.unit.test.ts b/src/test/managers/builtin/inlineScriptMain.unit.test.ts index 4d5ac3a37..7b25b1ad0 100644 --- a/src/test/managers/builtin/inlineScriptMain.unit.test.ts +++ b/src/test/managers/builtin/inlineScriptMain.unit.test.ts @@ -3,11 +3,12 @@ import assert from 'assert'; import * as sinon from 'sinon'; -import { Disposable, LogOutputChannel } from 'vscode'; -import { PythonEnvironmentApi } from '../../../api'; +import { Disposable, LogOutputChannel, Uri } from 'vscode'; +import { EnvironmentManager, PythonEnvironmentApi } from '../../../api'; import * as pythonApi from '../../../features/pythonApi'; import * as helpers from '../../../helpers'; import { registerInlineScriptFeatures } from '../../../managers/builtin/inlineScriptMain'; +import { NativePythonFinder } from '../../../managers/common/nativePythonFinder'; function makeFakeLog(): LogOutputChannel { return { @@ -30,6 +31,9 @@ suite('registerInlineScriptFeatures (feature-flag gate)', () => { let isEnabledStub: sinon.SinonStub; let getPythonApiStub: sinon.SinonStub; let registerEnvironmentManagerStub: sinon.SinonStub; + const nativeFinder = {} as NativePythonFinder; + const baseManager = {} as EnvironmentManager; + const globalStorageUri = Uri.file('inline-script-global-storage'); setup(() => { isEnabledStub = sinon.stub(helpers, 'isInlineScriptsFeatureEnabled'); @@ -47,7 +51,7 @@ suite('registerInlineScriptFeatures (feature-flag gate)', () => { isEnabledStub.returns(false); const disposables: Disposable[] = []; - await registerInlineScriptFeatures(disposables, makeFakeLog()); + await registerInlineScriptFeatures(nativeFinder, disposables, makeFakeLog(), baseManager, globalStorageUri); assert.strictEqual(disposables.length, 0, 'no disposables should be added when flag is off'); assert.strictEqual(getPythonApiStub.called, false, 'should not even call getPythonApi when gated off'); @@ -58,10 +62,12 @@ suite('registerInlineScriptFeatures (feature-flag gate)', () => { isEnabledStub.returns(true); const disposables: Disposable[] = []; - await registerInlineScriptFeatures(disposables, makeFakeLog()); + await registerInlineScriptFeatures(nativeFinder, disposables, makeFakeLog(), baseManager, globalStorageUri); assert.strictEqual(getPythonApiStub.callCount, 1); assert.strictEqual(registerEnvironmentManagerStub.callCount, 1); assert.strictEqual(disposables.length, 2, 'expected manager + registration disposable'); + const manager = registerEnvironmentManagerStub.firstCall.args[0]; + assert.strictEqual(typeof manager.create, 'function'); }); }); diff --git a/src/test/managers/builtin/venvUtils.createWithProgress.unit.test.ts b/src/test/managers/builtin/venvUtils.createWithProgress.unit.test.ts new file mode 100644 index 000000000..c7293f266 --- /dev/null +++ b/src/test/managers/builtin/venvUtils.createWithProgress.unit.test.ts @@ -0,0 +1,137 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import assert from 'assert'; +import * as fs from 'fs-extra'; +import * as os from 'os'; +import * as path from 'path'; +import * as sinon from 'sinon'; +import { CancellationError, LogOutputChannel, Uri } from 'vscode'; +import { EnvironmentManager, PythonEnvironment, PythonEnvironmentApi } from '../../../api'; +import * as windowApis from '../../../common/window.apis'; +import * as builtinHelpers from '../../../managers/builtin/helpers'; +import * as uvEnvironments from '../../../managers/builtin/uvEnvironments'; +import { createWithProgress } from '../../../managers/builtin/venvUtils'; +import { NativePythonEnvironmentKind, NativePythonFinder } from '../../../managers/common/nativePythonFinder'; +import * as managerUtils from '../../../managers/common/utils'; + +suite('createWithProgress uv tracking options', () => { + let addUvEnvironmentStub: sinon.SinonStub; + let api: PythonEnvironmentApi; + let baseEnvironment: PythonEnvironment; + let envPath: string; + let log: LogOutputChannel; + let manager: EnvironmentManager; + let nativeFinder: NativePythonFinder; + let tempRoot: string; + + setup(async () => { + tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'create-with-progress-')); + envPath = path.join(tempRoot, 'env'); + const pythonPath = path.join( + envPath, + process.platform === 'win32' ? 'Scripts' : 'bin', + process.platform === 'win32' ? 'python.exe' : 'python', + ); + await fs.outputFile(pythonPath, ''); + + baseEnvironment = { + envId: { id: 'base', managerId: 'ms-python.python:system' }, + name: 'base', + displayName: 'base', + displayPath: pythonPath, + version: '3.12.4', + environmentPath: Uri.file(pythonPath), + execInfo: { run: { executable: pythonPath } }, + sysPrefix: tempRoot, + }; + const createdEnvironment = { + ...baseEnvironment, + envId: { id: 'created', managerId: 'ms-python.python:inline-script' }, + }; + api = { + createPythonEnvironmentItem: sinon.stub().returns(createdEnvironment), + managePackages: sinon.stub().resolves(), + } as unknown as PythonEnvironmentApi; + nativeFinder = { + resolve: sinon.stub().resolves({ + executable: pythonPath, + prefix: envPath, + version: '3.12.4', + kind: NativePythonEnvironmentKind.venvUv, + }), + } as unknown as NativePythonFinder; + log = { + error: sinon.stub(), + info: sinon.stub(), + append: sinon.stub(), + } as unknown as LogOutputChannel; + manager = { log } as EnvironmentManager; + + sinon.stub(windowApis, 'withProgress').callsFake(async (_options, task) => task({} as never, {} as never)); + sinon.stub(builtinHelpers, 'shouldUseUv').resolves(true); + sinon.stub(builtinHelpers, 'runUV').resolves(''); + sinon.stub(managerUtils, 'getShellActivationCommands').resolves({ + shellActivation: new Map(), + shellDeactivation: new Map(), + }); + addUvEnvironmentStub = sinon.stub(uvEnvironments, 'addUvEnvironment').resolves(); + }); + + teardown(async () => { + sinon.restore(); + await fs.remove(tempRoot); + }); + + test('tracks uv environments by default for existing callers', async () => { + const result = await createWithProgress( + nativeFinder, + api, + log, + manager, + baseEnvironment, + Uri.file(tempRoot), + envPath, + ); + + assert.ok(result?.environment); + assert.ok(addUvEnvironmentStub.calledOnce); + }); + + test('skips workspace-scoped uv tracking when explicitly disabled', async () => { + const result = await createWithProgress( + nativeFinder, + api, + log, + manager, + baseEnvironment, + Uri.file(tempRoot), + envPath, + undefined, + { trackUvEnvironment: false }, + ); + + assert.ok(result?.environment); + assert.strictEqual(addUvEnvironmentStub.callCount, 0); + }); + + test('marks cancelled package installation as potentially still mutating', async () => { + (api.managePackages as sinon.SinonStub).rejects(new CancellationError()); + + const result = await createWithProgress( + nativeFinder, + api, + log, + manager, + baseEnvironment, + Uri.file(tempRoot), + envPath, + { install: ['requests'], uninstall: [] }, + { trackUvEnvironment: false }, + ); + + assert.ok(result?.environment); + assert.strictEqual(typeof result.pkgInstallationErr, 'string'); + assert.strictEqual(result.pkgInstallationCancelled, true); + }); +}); From f85b91fb5b6f4bed04809ba4791849dd9d56cc9b Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Wed, 22 Jul 2026 17:00:20 -0700 Subject: [PATCH 2/3] Trim inline-script env comments to essentials Remove verbose narrative comments from the PEP 723 inline-script changes, keeping only must-have documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39dcc6a3-0fbd-4f36-9d0f-68677de49c27 --- src/common/inlineScriptCacheLayout.ts | 35 ++++--------------- src/common/lockfile.apis.ts | 11 +----- src/managers/builtin/helpers.ts | 4 +-- .../builtin/inlineScriptEnvManager.ts | 8 +---- src/managers/builtin/venvUtils.ts | 4 +-- 5 files changed, 12 insertions(+), 50 deletions(-) diff --git a/src/common/inlineScriptCacheLayout.ts b/src/common/inlineScriptCacheLayout.ts index dbcf73a3f..4fa063b6d 100644 --- a/src/common/inlineScriptCacheLayout.ts +++ b/src/common/inlineScriptCacheLayout.ts @@ -8,14 +8,7 @@ import { Uri } from 'vscode'; import { traceWarn } from './logging'; import { isWindows } from './utils/platformUtils'; -/** - * Versioned name of the cache root under the extension's `globalStorageUri`. - * - * PR 5 finalizes the v1 shape before the first production writer. After that - * writer ships, bump the `-v1` suffix together with {@link META_SCHEMA_VERSION} - * on any incompatible on-disk change, so old envs sit unread instead of being - * migrated in place. - */ +/** Bump with {@link META_SCHEMA_VERSION} for incompatible cache formats. */ export const INLINE_SCRIPT_CACHE_DIR_NAME = 'script-envs-v1'; export const META_JSON_FILENAME = '.meta.json'; @@ -34,9 +27,9 @@ const MAX_META_JSON_BYTES = 1024 * 1024; export interface InlineScriptEnvMeta { /** Version of the serialized metadata schema. */ readonly schemaVersion: typeof META_SCHEMA_VERSION; - /** Canonical filesystem path of the base interpreter used to create this environment. */ + /** Canonical base-interpreter path. */ readonly baseInterpreterPath: string; - /** Version reported by the base interpreter when this environment was created. */ + /** Base-interpreter version. */ readonly baseInterpreterVersion: string; /** Last successful use as a canonical UTC string produced by `Date.toISOString()`. */ readonly lastUsedAt: string; @@ -70,25 +63,13 @@ export function getMetaJsonPath(envDir: Uri): Uri { return Uri.joinPath(envDir, META_JSON_FILENAME); } -/** - * Read and validate the extension-owned `.meta.json` sidecar in a cached - * environment directory. - * - * This compatibility wrapper is intentionally non-destructive. Missing, - * malformed, or unreadable sidecars return `undefined`. Callers that decide - * cache lifecycle must use {@link inspectMetaJson} to distinguish conclusive - * `missing` / `invalid` state from transient `unavailable` I/O failures. - */ +/** Read validated sidecar metadata, returning `undefined` for non-valid state. */ export async function readMetaJson(envDir: Uri): Promise { const result = await inspectMetaJson(envDir); return result.kind === 'valid' ? result.metadata : undefined; } -/** - * Read and classify an inline-script sidecar without mutating its cache entry. - * `missing` and `invalid` are conclusive under the creator lock; `unavailable` - * represents a transient I/O failure that must not trigger deletion. - */ +/** Classify sidecar state; only `unavailable` denotes transient I/O. */ export async function inspectMetaJson(envDir: Uri): Promise { const metaPath = getMetaJsonPath(envDir).fsPath; @@ -181,11 +162,7 @@ export async function verifyBaseInterpreterExists(envDir: Uri): Promise return (await getBaseInterpreterStatus(envDir)) === 'available'; } -/** - * Classify the cached environment's base interpreter without throwing. - * `missing` is definitive stale state; `unavailable` is an uncertain I/O failure - * that callers must preserve rather than delete. - */ +/** Classify the base interpreter; `unavailable` denotes transient I/O. */ export async function getBaseInterpreterStatus(envDir: Uri): Promise { return isWindows() ? getWindowsBaseInterpreterStatus(envDir) : getPosixBaseInterpreterStatus(envDir); } diff --git a/src/common/lockfile.apis.ts b/src/common/lockfile.apis.ts index bd0143904..02fbc0661 100644 --- a/src/common/lockfile.apis.ts +++ b/src/common/lockfile.apis.ts @@ -6,20 +6,11 @@ import * as fsapi from 'fs-extra'; import * as path from 'path'; export interface AcquireFileLockOptions { - /** Maximum time to wait for another live owner. */ readonly timeoutMs: number; - /** Delay between acquisition attempts. */ readonly retryIntervalMs: number; } -/** - * Acquires a filesystem lock and returns its asynchronous release function. - * - * Acquisition uses atomic directory creation. There is no process-exit cleanup or - * time-based stealing: environment creation cannot be cancelled at every package-manager - * boundary, so a child process may outlive its extension host. An interrupted operation - * therefore fails closed until the cache is explicitly cleared. - */ +/** Acquire an atomic lock released only explicitly; interrupted operations remain locked. */ export async function acquireFileLock( filePath: string, options: AcquireFileLockOptions, diff --git a/src/managers/builtin/helpers.ts b/src/managers/builtin/helpers.ts index b6f62414c..7fb7062a2 100644 --- a/src/managers/builtin/helpers.ts +++ b/src/managers/builtin/helpers.ts @@ -79,7 +79,7 @@ export async function runUV( try { proc.kill(); } catch { - // Cancellation remains the controlling outcome even when signaling fails. + // Preserve cancellation when signaling fails. } reject(new CancellationError()); }); @@ -130,7 +130,7 @@ export async function runPython( try { proc.kill(); } catch { - // Cancellation remains the controlling outcome even when signaling fails. + // Preserve cancellation when signaling fails. } reject(new CancellationError()); }); diff --git a/src/managers/builtin/inlineScriptEnvManager.ts b/src/managers/builtin/inlineScriptEnvManager.ts index 53f1bb127..71b203585 100644 --- a/src/managers/builtin/inlineScriptEnvManager.ts +++ b/src/managers/builtin/inlineScriptEnvManager.ts @@ -49,8 +49,6 @@ const BASE_INTERPRETER_MANAGER_IDS = new Set([ `${PYTHON_EXTENSION_ID}:pyenv`, ]); -// Waiters never steal a lock: interrupted/cancelled creation remains fail-closed. -// The planned PR13 inline-script clear-cache lifecycle flow will own recovery for abandoned locks. const CACHE_LOCK_TIMEOUT_MS = 5 * 60 * 1000; const CACHE_LOCK_RETRY_MS = 500; const INLINE_SCRIPT_MANAGER_ID = `${PYTHON_EXTENSION_ID}:inline-script`; @@ -72,11 +70,7 @@ type CacheEntryInspection = type EnvironmentInspection = 'expected' | 'stale' | 'uncertain'; type PythonReleaseComparison = 'same' | 'different' | 'uncertain'; -/** - * Environment manager for extension-owned PEP 723 script environments. - * Creation is supported; script associations and discovery are added by - * later rollout phases. - */ +/** Manages extension-owned PEP 723 script environments. */ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { private readonly pendingCreations = new Map>(); diff --git a/src/managers/builtin/venvUtils.ts b/src/managers/builtin/venvUtils.ts index d4844fdaf..e01242b96 100644 --- a/src/managers/builtin/venvUtils.ts +++ b/src/managers/builtin/venvUtils.ts @@ -62,12 +62,12 @@ export interface CreateEnvironmentResult { */ pkgInstallationErr?: string; - /** Package installation was cancelled and its process tree may still be terminating. */ + /** Cancellation may leave package processes running. */ pkgInstallationCancelled?: boolean; } export interface CreateWithProgressOptions { - /** Record uv-created environments in the workspace-scoped uv cache. Defaults to true. */ + /** Whether to record uv-created environments in workspace state. Defaults to true. */ readonly trackUvEnvironment?: boolean; } From dcce173dd6b84aad3866c0014094e6fd69bf284c Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Wed, 22 Jul 2026 17:37:04 -0700 Subject: [PATCH 3/3] Address PR 1646 review feedback Apply the reasonable automated review fixes while preserving the PR5 happy-path scope. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88b2e063-b52a-4743-bf2b-9ee639ec7a40 --- src/common/inlineScriptCacheKey.ts | 1 + src/common/inlineScriptCacheLayout.ts | 21 +++ src/common/lockfile.apis.ts | 99 ++++++++---- src/managers/builtin/helpers.ts | 98 ++++++------ .../builtin/inlineScriptEnvManager.ts | 150 +++++++++--------- src/managers/builtin/venvUtils.ts | 9 +- .../inlineScriptCacheLayout.unit.test.ts | 38 +++++ src/test/common/lockfile.apis.unit.test.ts | 58 +++++-- .../inlineScriptEnvManager.unit.test.ts | 50 ++++-- 9 files changed, 338 insertions(+), 186 deletions(-) diff --git a/src/common/inlineScriptCacheKey.ts b/src/common/inlineScriptCacheKey.ts index e701d6612..2ef4c964a 100644 --- a/src/common/inlineScriptCacheKey.ts +++ b/src/common/inlineScriptCacheKey.ts @@ -42,6 +42,7 @@ function normalizeRequirementTail(value: string): string { unquoted = ''; }; + // Preserve PEP 508 marker literals verbatim; a backslash escapes the next character. for (const character of value) { if (quote) { result += character; diff --git a/src/common/inlineScriptCacheLayout.ts b/src/common/inlineScriptCacheLayout.ts index 4fa063b6d..9600674a3 100644 --- a/src/common/inlineScriptCacheLayout.ts +++ b/src/common/inlineScriptCacheLayout.ts @@ -6,6 +6,7 @@ import * as fsapi from 'fs-extra'; import * as path from 'path'; import { Uri } from 'vscode'; import { traceWarn } from './logging'; +import { normalizePath } from './utils/pathUtils'; import { isWindows } from './utils/platformUtils'; /** Bump with {@link META_SCHEMA_VERSION} for incompatible cache formats. */ @@ -63,6 +64,19 @@ export function getMetaJsonPath(envDir: Uri): Uri { return Uri.joinPath(envDir, META_JSON_FILENAME); } +/** Resolve a cache entry only when it is the requested direct child of the physical cache root. */ +export async function resolveCacheEntryPath(cacheRoot: Uri, envDir: Uri): Promise { + const [resolvedRoot, resolvedEntry] = await Promise.all([ + fsapi.realpath(cacheRoot.fsPath), + fsapi.realpath(envDir.fsPath), + ]); + const expectedEntry = path.join(resolvedRoot, path.basename(envDir.fsPath)); + return isDescendantPath(resolvedRoot, resolvedEntry) && + normalizePath(path.resolve(resolvedEntry)) === normalizePath(path.resolve(expectedEntry)) + ? resolvedEntry + : undefined; +} + /** Read validated sidecar metadata, returning `undefined` for non-valid state. */ export async function readMetaJson(envDir: Uri): Promise { const result = await inspectMetaJson(envDir); @@ -230,6 +244,13 @@ function isFileNotFoundError(err: unknown): boolean { return typeof err === 'object' && err !== null && 'code' in err && (err as NodeJS.ErrnoException).code === 'ENOENT'; } +function isDescendantPath(rootPath: string, candidatePath: string): boolean { + const relative = path.relative(rootPath, candidatePath); + return ( + relative.length > 0 && relative !== '..' && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative) + ); +} + function validateMeta(value: unknown): InlineScriptEnvMeta | undefined { if (typeof value !== 'object' || value === null || Array.isArray(value)) { return undefined; diff --git a/src/common/lockfile.apis.ts b/src/common/lockfile.apis.ts index 02fbc0661..e6f8b601f 100644 --- a/src/common/lockfile.apis.ts +++ b/src/common/lockfile.apis.ts @@ -5,18 +5,11 @@ import * as crypto from 'crypto'; import * as fsapi from 'fs-extra'; import * as path from 'path'; -export interface AcquireFileLockOptions { - readonly timeoutMs: number; - readonly retryIntervalMs: number; -} - /** Acquire an atomic lock released only explicitly; interrupted operations remain locked. */ -export async function acquireFileLock( - filePath: string, - options: AcquireFileLockOptions, -): Promise<() => Promise> { +export async function acquireFileLock(filePath: string, options: AcquireFileLockOptions): Promise { const lockPath = `${path.resolve(filePath)}.lock`; const ownerMarker = path.join(lockPath, `owner-${process.pid}-${crypto.randomBytes(16).toString('hex')}`); + const retainedMarker = path.join(lockPath, 'retained'); const deadline = Date.now() + options.timeoutMs; while (true) { @@ -25,30 +18,56 @@ export async function acquireFileLock( try { await fsapi.writeFile(ownerMarker, '', { flag: 'wx' }); } catch (error) { - await fsapi.rmdir(lockPath).catch(() => undefined); + try { + await fsapi.rmdir(lockPath); + } catch { + throw createLockError( + 'Lock initialization failed and left an owner-less lock directory', + 'ELOCKORPHANED', + lockPath, + ); + } throw error; } - let released = false; - return async () => { - if (released) { - throw createLockError('Lock has already been released', 'ERELEASED', lockPath); - } - released = true; - try { - await fsapi.unlink(ownerMarker); - } catch (error) { - if (isFileNotFoundError(error)) { - throw createLockError('Lock ownership was compromised', 'ECOMPROMISED', lockPath); + let state: LockState = 'held'; + return { + retain: async () => { + if (state !== 'held') { + return; } - throw error; - } - await fsapi.rmdir(lockPath); + state = 'retained'; + try { + await fsapi.rename(ownerMarker, retainedMarker); + } catch (error) { + if (!isAlreadyExistsError(error)) { + throw createLockError('Failed to mark the lock as retained', 'ERETAINFAILED', lockPath); + } + } + }, + release: async () => { + if (state !== 'held') { + return; + } + state = 'released'; + try { + await fsapi.unlink(ownerMarker); + } catch (error) { + if (isFileNotFoundError(error)) { + throw createLockError('Lock ownership was compromised', 'ECOMPROMISED', lockPath); + } + throw error; + } + await fsapi.rmdir(lockPath); + }, }; } catch (error) { if (!isAlreadyExistsError(error)) { throw error; } + if (await isRetainedLock(lockPath)) { + throw createLockError('Lock was retained after an interrupted operation', 'ELOCKRETAINED', lockPath); + } if (Date.now() >= deadline) { throw createLockError('Lock is already being held', 'ELOCKED', lockPath); } @@ -65,12 +84,21 @@ function isFileNotFoundError(error: unknown): boolean { return hasErrorCode(error, 'ENOENT'); } +async function isRetainedLock(lockPath: string): Promise { + try { + await fsapi.lstat(path.join(lockPath, 'retained')); + return true; + } catch (error) { + if (isFileNotFoundError(error)) { + return false; + } + throw error; + } +} + function hasErrorCode(error: unknown, code: string): boolean { return ( - typeof error === 'object' && - error !== null && - 'code' in error && - (error as NodeJS.ErrnoException).code === code + typeof error === 'object' && error !== null && 'code' in error && (error as NodeJS.ErrnoException).code === code ); } @@ -80,4 +108,17 @@ function createLockError(message: string, code: string, lockPath: string): NodeJ async function delay(milliseconds: number): Promise { return new Promise((resolve) => setTimeout(resolve, milliseconds)); -} \ No newline at end of file +} + +export interface AcquireFileLockOptions { + readonly timeoutMs: number; + readonly retryIntervalMs: number; +} + +export interface AcquiredFileLock { + readonly release: () => Promise; + /** Keep the lock and make later acquisition attempts fail immediately. */ + readonly retain: () => Promise; +} + +type LockState = 'held' | 'released' | 'retained'; diff --git a/src/managers/builtin/helpers.ts b/src/managers/builtin/helpers.ts index 7fb7062a2..579892170 100644 --- a/src/managers/builtin/helpers.ts +++ b/src/managers/builtin/helpers.ts @@ -66,50 +66,14 @@ export async function runUV( token?: CancellationToken, timeout?: number, ): Promise { - log?.info(`Running: uv ${args.join(' ')}`); - return new Promise((resolve, reject) => { - const spawnOptions: { cwd?: string; timeout?: number } = { cwd }; - if (timeout !== undefined) { - spawnOptions.timeout = timeout; - } - const proc = spawnProcess('uv', args, spawnOptions); - let cancellationRequested = false; - token?.onCancellationRequested(() => { - cancellationRequested = true; - try { - proc.kill(); - } catch { - // Preserve cancellation when signaling fails. - } - reject(new CancellationError()); - }); - - proc.on('error', (err) => { - if (cancellationRequested) { - reject(new CancellationError()); - return; - } - log?.error(`Error spawning uv: ${err}`); - reject(new Error(`Error spawning uv: ${err.message}`)); - }); - - let builder = ''; - proc.stdout?.on('data', (data) => { - const s = data.toString('utf-8'); - builder += s; - log?.append(s); - }); - proc.stderr?.on('data', (data) => { - log?.append(data.toString('utf-8')); - }); - proc.on('close', () => { - resolve(builder); - }); - proc.on('exit', (code) => { - if (code !== 0) { - reject(new Error(`Failed to run uv ${args.join(' ')}`)); - } - }); + return runProcess('uv', args, { + cwd, + displayName: 'uv', + log, + token, + timeout, + collectStderr: false, + logPrefix: '', }); } @@ -121,11 +85,27 @@ export async function runPython( token?: CancellationToken, timeout?: number, ): Promise { - log?.info(`Running: ${python} ${args.join(' ')}`); + return runProcess(python, args, { + cwd, + displayName: 'python', + log, + token, + timeout, + collectStderr: true, + logPrefix: 'python: ', + }); +} + +function runProcess(executable: string, args: string[], options: RunProcessOptions): Promise { + options.log?.info(`Running: ${executable} ${args.join(' ')}`); return new Promise((resolve, reject) => { - const proc = spawnProcess(python, args, { cwd: cwd, timeout }); + const spawnOptions: { cwd?: string; timeout?: number } = { cwd: options.cwd }; + if (options.timeout !== undefined) { + spawnOptions.timeout = options.timeout; + } + const proc = spawnProcess(executable, args, spawnOptions); let cancellationRequested = false; - token?.onCancellationRequested(() => { + options.token?.onCancellationRequested(() => { cancellationRequested = true; try { proc.kill(); @@ -140,28 +120,40 @@ export async function runPython( reject(new CancellationError()); return; } - log?.error(`Error spawning python: ${err}`); - reject(new Error(`Error spawning python: ${err.message}`)); + options.log?.error(`Error spawning ${options.displayName}: ${err}`); + reject(new Error(`Error spawning ${options.displayName}: ${err.message}`)); }); let builder = ''; proc.stdout?.on('data', (data) => { const s = data.toString('utf-8'); builder += s; - log?.append(`python: ${s}`); + options.log?.append(`${options.logPrefix}${s}`); }); proc.stderr?.on('data', (data) => { const s = data.toString('utf-8'); - builder += s; - log?.append(`python: ${s}`); + if (options.collectStderr) { + builder += s; + } + options.log?.append(`${options.logPrefix}${s}`); }); proc.on('close', () => { resolve(builder); }); proc.on('exit', (code) => { if (code !== 0) { - reject(new Error(`Failed to run python ${args.join(' ')}`)); + reject(new Error(`Failed to run ${options.displayName} ${args.join(' ')}`)); } }); }); } + +interface RunProcessOptions { + readonly cwd?: string; + readonly displayName: 'python' | 'uv'; + readonly log?: LogOutputChannel; + readonly token?: CancellationToken; + readonly timeout?: number; + readonly collectStderr: boolean; + readonly logPrefix: string; +} diff --git a/src/managers/builtin/inlineScriptEnvManager.ts b/src/managers/builtin/inlineScriptEnvManager.ts index 71b203585..7a6c4f8f3 100644 --- a/src/managers/builtin/inlineScriptEnvManager.ts +++ b/src/managers/builtin/inlineScriptEnvManager.ts @@ -27,6 +27,7 @@ import { getScriptEnvCacheRoot, getScriptEnvDir, inspectMetaJson, + resolveCacheEntryPath, writeMetaJson, } from '../../common/inlineScriptCacheLayout'; import { pickCompatibleInterpreter } from '../../common/inlineScriptInterpreter'; @@ -36,12 +37,11 @@ import { readInlineScriptMetadataFromFile, } from '../../common/inlineScriptMetadata'; import { PYTHON_EXTENSION_ID, SYSTEM_MANAGER_ID } from '../../common/constants'; -import { acquireFileLock } from '../../common/lockfile.apis'; +import { acquireFileLock, AcquiredFileLock } from '../../common/lockfile.apis'; import { normalizePath } from '../../common/utils/pathUtils'; -import { isWindows } from '../../common/utils/platformUtils'; import { compareReleaseSegments, parseReleaseSegments } from '../../common/utils/pep440Release'; import { NativePythonFinder } from '../common/nativePythonFinder'; -import { createWithProgress, resolveVenvPythonEnvironmentPath } from './venvUtils'; +import { createWithProgress, getVenvPythonPath, resolveVenvPythonEnvironmentPath } from './venvUtils'; const BASE_INTERPRETER_MANAGER_IDS = new Set([ SYSTEM_MANAGER_ID, @@ -53,23 +53,6 @@ const CACHE_LOCK_TIMEOUT_MS = 5 * 60 * 1000; const CACHE_LOCK_RETRY_MS = 500; const INLINE_SCRIPT_MANAGER_ID = `${PYTHON_EXTENSION_ID}:inline-script`; -interface SelectedBaseInterpreter { - readonly environment: PythonEnvironment; - readonly canonicalPath: string; -} - -interface BuildCacheEntryResult { - readonly environment?: PythonEnvironment; - readonly retainLock?: boolean; -} - -type CacheEntryInspection = - | { readonly kind: 'absent' | 'stale' | 'uncertain' } - | { readonly kind: 'reusable'; readonly environment: PythonEnvironment }; - -type EnvironmentInspection = 'expected' | 'stale' | 'uncertain'; -type PythonReleaseComparison = 'same' | 'different' | 'uncertain'; - /** Manages extension-owned PEP 723 script environments. */ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { private readonly pendingCreations = new Map>(); @@ -185,12 +168,22 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { (environment.envId.managerId !== `${PYTHON_EXTENSION_ID}:conda` || environment.name === 'base'), ); const derivedChecks = await Promise.all( - reported.map(async (environment) => ({ - environment, - derived: await fs.pathExists(path.join(environment.sysPrefix, 'pyvenv.cfg')), - })), + reported.map(async (environment) => { + if (!path.isAbsolute(environment.sysPrefix)) { + this.log.warn( + `Skipping base interpreter with a non-absolute sysPrefix: ${environment.sysPrefix || ''}.`, + ); + return { environment, derived: true }; + } + return { + environment, + derived: await fs.pathExists(path.join(environment.sysPrefix, 'pyvenv.cfg')), + }; + }), ); - let candidates = derivedChecks.filter((candidate) => !candidate.derived).map((candidate) => candidate.environment); + let candidates = derivedChecks + .filter((candidate) => !candidate.derived) + .map((candidate) => candidate.environment); while (candidates.length > 0) { const environment = pickCompatibleInterpreter(candidates, metadata.requiresPython); @@ -225,9 +218,9 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { const envDir = getScriptEnvDir(this.globalStorageUri, cacheKey); await fs.ensureDir(cacheRoot.fsPath); - let release: (() => Promise) | undefined; + let lock: AcquiredFileLock | undefined; try { - release = await acquireFileLock(envDir.fsPath, { + lock = await acquireFileLock(envDir.fsPath, { timeoutMs: CACHE_LOCK_TIMEOUT_MS, retryIntervalMs: CACHE_LOCK_RETRY_MS, }); @@ -237,7 +230,9 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return cached.environment; } if (cached.kind === 'uncertain') { - this.log.warn(`Preserving an inline-script cache entry that could not be safely inspected: ${envDir.fsPath}`); + this.log.warn( + `Preserving an inline-script cache entry that could not be safely inspected: ${envDir.fsPath}`, + ); return undefined; } if (cached.kind === 'stale') { @@ -248,16 +243,24 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { const build = await this.buildCacheEntry(envDir, cacheRoot, packages, selectedBase); if (build.retainLock) { - release = undefined; + const retainedLock = lock; + lock = undefined; + try { + await retainedLock.retain(); + } catch (error) { + this.log.error( + `Failed to mark the inline-script cache lock as retained: ${this.errorMessage(error)}`, + ); + } } return build.environment; } catch (error) { this.log.error(`Failed to create or reuse inline-script cache entry: ${this.errorMessage(error)}`); return undefined; } finally { - if (release) { + if (lock) { try { - await release(); + await lock.release(); } catch (error) { this.log.warn(`Failed to release inline-script cache lock: ${this.errorMessage(error)}`); } @@ -280,7 +283,14 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return this.isFileNotFoundError(error) ? { kind: 'absent' } : { kind: 'uncertain' }; } - if (!(await this.isPhysicallyContained(cacheRoot, envDir))) { + let resolvedEntry: string | undefined; + try { + resolvedEntry = await resolveCacheEntryPath(cacheRoot, envDir); + } catch (error) { + this.log.warn(`Failed to resolve inline-script cache entry: ${this.errorMessage(error)}`); + return { kind: 'uncertain' }; + } + if (!resolvedEntry) { return { kind: 'uncertain' }; } @@ -307,7 +317,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { } const environment = await resolveVenvPythonEnvironmentPath( - this.getVenvPythonPath(envDir), + getVenvPythonPath(envDir.fsPath), this.nativeFinder, this.api, this, @@ -320,12 +330,9 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { if (environmentStatus !== 'expected') { return { kind: environmentStatus }; } - const releaseComparison = this.comparePythonReleases( - environment.version, - selectedBase.environment.version, - ); + const releaseComparison = this.comparePythonReleases(environment.version, selectedBase.environment.version); if (releaseComparison !== 'same') { - return { kind: releaseComparison === 'different' ? 'stale' : 'uncertain' }; + return { kind: 'stale' }; } const requiresPython = metadata.requiresPython?.trim(); if (requiresPython && !matchesPythonVersion(requiresPython, environment.version)) { @@ -372,7 +379,8 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return { retainLock: true }; } if (!result?.environment || result.envCreationErr || result.pkgInstallationErr) { - const error = result?.envCreationErr ?? result?.pkgInstallationErr ?? 'environment creation returned no result'; + const error = + result?.envCreationErr ?? result?.pkgInstallationErr ?? 'environment creation returned no result'; this.log.error(`Failed to build inline-script environment: ${error}`); await this.removeCacheEntry(envDir); return {}; @@ -413,12 +421,6 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { } } - private getVenvPythonPath(envDir: Uri): string { - return isWindows() - ? Uri.joinPath(envDir, 'Scripts', 'python.exe').fsPath - : Uri.joinPath(envDir, 'bin', 'python').fsPath; - } - private async inspectExpectedCacheEnvironment( environment: PythonEnvironment, cacheRoot: Uri, @@ -428,52 +430,25 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { return 'uncertain'; } try { - const [resolvedRoot, expectedDir, resolvedPrefix, expectedPython, resolvedPython] = await Promise.all([ - fs.realpath(cacheRoot.fsPath), - fs.realpath(envDir.fsPath), + const [expectedDir, resolvedPrefix, expectedPython, resolvedPython] = await Promise.all([ + resolveCacheEntryPath(cacheRoot, envDir), fs.realpath(environment.sysPrefix), - fs.realpath(this.getVenvPythonPath(envDir)), + fs.realpath(getVenvPythonPath(envDir.fsPath)), fs.realpath(environment.environmentPath.fsPath), ]); - if (!this.isExpectedPhysicalEntry(resolvedRoot, expectedDir, envDir)) { + if (!expectedDir) { return 'uncertain'; } - return ( - normalizePath(expectedDir) === normalizePath(resolvedPrefix) && + return normalizePath(expectedDir) === normalizePath(resolvedPrefix) && normalizePath(expectedPython) === normalizePath(resolvedPython) - ) ? 'expected' : 'stale'; - } catch { + } catch (error) { + this.log.warn(`Failed to inspect inline-script cache environment: ${this.errorMessage(error)}`); return 'uncertain'; } } - private async isPhysicallyContained(cacheRoot: Uri, envDir: Uri): Promise { - try { - const [resolvedRoot, resolvedEntry] = await Promise.all([ - fs.realpath(cacheRoot.fsPath), - fs.realpath(envDir.fsPath), - ]); - return this.isExpectedPhysicalEntry(resolvedRoot, resolvedEntry, envDir); - } catch { - return false; - } - } - - private isExpectedPhysicalEntry(resolvedRoot: string, resolvedEntry: string, envDir: Uri): boolean { - const expectedEntry = path.join(resolvedRoot, path.basename(envDir.fsPath)); - return ( - this.isDescendantPath(resolvedRoot, resolvedEntry) && - normalizePath(path.resolve(resolvedEntry)) === normalizePath(path.resolve(expectedEntry)) - ); - } - - private isDescendantPath(rootPath: string, candidatePath: string): boolean { - const relative = path.relative(rootPath, candidatePath); - return relative.length > 0 && relative !== '..' && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative); - } - private isFileNotFoundError(error: unknown): boolean { return ( typeof error === 'object' && @@ -501,3 +476,20 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable { this._onDidChangeEnvironment.dispose(); } } + +interface SelectedBaseInterpreter { + readonly environment: PythonEnvironment; + readonly canonicalPath: string; +} + +interface BuildCacheEntryResult { + readonly environment?: PythonEnvironment; + readonly retainLock?: boolean; +} + +type CacheEntryInspection = + | { readonly kind: 'absent' | 'stale' | 'uncertain' } + | { readonly kind: 'reusable'; readonly environment: PythonEnvironment }; + +type EnvironmentInspection = 'expected' | 'stale' | 'uncertain'; +type PythonReleaseComparison = 'same' | 'different' | 'uncertain'; diff --git a/src/managers/builtin/venvUtils.ts b/src/managers/builtin/venvUtils.ts index e01242b96..00706f594 100644 --- a/src/managers/builtin/venvUtils.ts +++ b/src/managers/builtin/venvUtils.ts @@ -359,8 +359,7 @@ export async function createWithProgress( packages?: PipPackages, options?: CreateWithProgressOptions, ): Promise { - const pythonPath = - os.platform() === 'win32' ? path.join(envPath, 'Scripts', 'python.exe') : path.join(envPath, 'bin', 'python'); + const pythonPath = getVenvPythonPath(envPath); return await withProgress( { @@ -433,6 +432,12 @@ export async function createWithProgress( ); } +export function getVenvPythonPath(envPath: string): string { + return os.platform() === 'win32' + ? path.join(envPath, 'Scripts', 'python.exe') + : path.join(envPath, 'bin', 'python'); +} + export function ensureGlobalEnv(basePythons: PythonEnvironment[], log: LogOutputChannel): PythonEnvironment[] { if (basePythons.length === 0) { log.error('No base python found'); diff --git a/src/test/common/inlineScriptCacheLayout.unit.test.ts b/src/test/common/inlineScriptCacheLayout.unit.test.ts index 604e35b0b..2f70c199e 100644 --- a/src/test/common/inlineScriptCacheLayout.unit.test.ts +++ b/src/test/common/inlineScriptCacheLayout.unit.test.ts @@ -20,6 +20,7 @@ import { getScriptEnvDir, inspectMetaJson, readMetaJson, + resolveCacheEntryPath, selectStaleEntries, verifyBaseInterpreterExists, writeMetaJson, @@ -438,6 +439,43 @@ suite('inlineScriptCacheLayout', () => { }); }); + suite('resolveCacheEntryPath', () => { + let tmpDir: string; + let cacheRoot: Uri; + + setup(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'isclayout-containment-')); + cacheRoot = Uri.file(path.join(tmpDir, 'script-envs-v1')); + await fs.ensureDir(cacheRoot.fsPath); + }); + + teardown(async () => { + await fs.remove(tmpDir); + }); + + test('resolves a direct cache entry', async () => { + const envDir = Uri.joinPath(cacheRoot, '0123456789abcdef'); + await fs.ensureDir(envDir.fsPath); + + assert.strictEqual(await resolveCacheEntryPath(cacheRoot, envDir), await fs.realpath(envDir.fsPath)); + }); + + test('rejects the cache root and paths outside it', async () => { + const externalEnv = Uri.file(path.join(tmpDir, '0123456789abcdef')); + await fs.ensureDir(externalEnv.fsPath); + + assert.strictEqual(await resolveCacheEntryPath(cacheRoot, cacheRoot), undefined); + assert.strictEqual(await resolveCacheEntryPath(cacheRoot, externalEnv), undefined); + }); + + test('surfaces a missing entry', async () => { + await assert.rejects( + resolveCacheEntryPath(cacheRoot, Uri.joinPath(cacheRoot, '0123456789abcdef')), + (error: NodeJS.ErrnoException) => error.code === 'ENOENT', + ); + }); + }); + suite('verifyBaseInterpreterExists', () => { let tmpDir: string; let envDir: Uri; diff --git a/src/test/common/lockfile.apis.unit.test.ts b/src/test/common/lockfile.apis.unit.test.ts index b0d1b3fc0..fdcca123b 100644 --- a/src/test/common/lockfile.apis.unit.test.ts +++ b/src/test/common/lockfile.apis.unit.test.ts @@ -3,9 +3,11 @@ import assert from 'assert'; import { ChildProcessWithoutNullStreams, spawn } from 'child_process'; +import fsExtra from 'fs-extra'; import * as fs from 'fs-extra'; import * as os from 'os'; import * as path from 'path'; +import * as sinon from 'sinon'; import { acquireFileLock, AcquireFileLockOptions } from '../../common/lockfile.apis'; const OPTIONS: AcquireFileLockOptions = { @@ -25,6 +27,7 @@ suite('lockfile APIs', () => { }); teardown(async () => { + sinon.restore(); await fs.remove(tempRoot); }); @@ -32,13 +35,13 @@ suite('lockfile APIs', () => { const script = ` const { acquireFileLock } = require(process.argv[1]); acquireFileLock(process.argv[2], { timeoutMs: 1000, retryIntervalMs: 10 }) - .then((release) => { + .then((lock) => { process.stdout.write('locked\\n'); if (${exitWithoutRelease}) { process.exit(0); } process.stdin.once('data', async () => { - await release(); + await lock.release(); process.exit(0); }); }) @@ -90,14 +93,14 @@ suite('lockfile APIs', () => { } test('excludes a second owner until the first releases the lock', async () => { - const release = await acquireFileLock(targetPath, OPTIONS); + const lock = await acquireFileLock(targetPath, OPTIONS); await assert.rejects(acquireFileLock(targetPath, OPTIONS), (error: NodeJS.ErrnoException) => { return error.code === 'ELOCKED'; }); - await release(); - const releaseAfterRetry = await acquireFileLock(targetPath, OPTIONS); - await releaseAfterRetry(); + await lock.release(); + const lockAfterRetry = await acquireFileLock(targetPath, OPTIONS); + await lockAfterRetry.release(); assert.strictEqual(await fs.pathExists(`${path.resolve(targetPath)}.lock`), false); }); @@ -111,8 +114,8 @@ suite('lockfile APIs', () => { child.stdin.write('release\n'); await waitForExit(child); - const release = await acquireFileLock(targetPath, OPTIONS); - await release(); + const lock = await acquireFileLock(targetPath, OPTIONS); + await lock.release(); }); test('leaves the lock fail-closed when the owner process exits without release', async () => { @@ -127,16 +130,49 @@ suite('lockfile APIs', () => { }); test('an old release cannot remove a successor lock generation', async () => { - const release = await acquireFileLock(targetPath, OPTIONS); + const lock = await acquireFileLock(targetPath, OPTIONS); const lockPath = `${path.resolve(targetPath)}.lock`; await fs.remove(lockPath); await fs.ensureDir(lockPath); const successorMarker = path.join(lockPath, 'successor-owner'); await fs.writeFile(successorMarker, ''); - await assert.rejects(release(), (error: NodeJS.ErrnoException) => { + await assert.rejects(lock.release(), (error: NodeJS.ErrnoException) => { return error.code === 'ECOMPROMISED'; }); assert.strictEqual(await fs.pathExists(successorMarker), true); }); -}); \ No newline at end of file + + test('release is idempotent', async () => { + const lock = await acquireFileLock(targetPath, OPTIONS); + + await lock.release(); + await lock.release(); + + assert.strictEqual(await fs.pathExists(`${path.resolve(targetPath)}.lock`), false); + }); + + test('retained locks fail fast without waiting for the acquisition timeout', async () => { + const lock = await acquireFileLock(targetPath, OPTIONS); + await lock.retain(); + const startedAt = Date.now(); + + await assert.rejects( + acquireFileLock(targetPath, { timeoutMs: 10_000, retryIntervalMs: 1_000 }), + (error: NodeJS.ErrnoException) => error.code === 'ELOCKRETAINED', + ); + + assert.ok(Date.now() - startedAt < 1_000); + const lockPath = `${path.resolve(targetPath)}.lock`; + assert.deepStrictEqual(await fs.readdir(lockPath), ['retained']); + }); + + test('reports an owner-less lock when initialization cleanup fails', async () => { + sinon.stub(fsExtra, 'writeFile').rejects(Object.assign(new Error('write failed'), { code: 'EIO' })); + sinon.stub(fsExtra, 'rmdir').rejects(Object.assign(new Error('cleanup failed'), { code: 'EACCES' })); + + await assert.rejects(acquireFileLock(targetPath, OPTIONS), (error: NodeJS.ErrnoException) => { + return error.code === 'ELOCKORPHANED' && error.path === `${path.resolve(targetPath)}.lock`; + }); + }); +}); diff --git a/src/test/managers/builtin/inlineScriptEnvManager.unit.test.ts b/src/test/managers/builtin/inlineScriptEnvManager.unit.test.ts index dd42c3898..0289b803f 100644 --- a/src/test/managers/builtin/inlineScriptEnvManager.unit.test.ts +++ b/src/test/managers/builtin/inlineScriptEnvManager.unit.test.ts @@ -60,9 +60,7 @@ function makeEnvironment( }; } -function venvPythonPath(envDir: string): string { - return path.join(envDir, process.platform === 'win32' ? 'Scripts' : 'bin', process.platform === 'win32' ? 'python.exe' : 'python'); -} +const venvPythonPath = venvUtils.getVenvPythonPath; suite('InlineScriptEnvManager', () => { let api: PythonEnvironmentApi; @@ -78,6 +76,7 @@ suite('InlineScriptEnvManager', () => { let nativeFinder: NativePythonFinder; let readMetadataStub: sinon.SinonStub; let inspectMetaStub: sinon.SinonStub; + let retainLockStub: sinon.SinonStub; let releaseLockStub: sinon.SinonStub; let resolveVenvStub: sinon.SinonStub; let tempRoot: string; @@ -101,14 +100,22 @@ suite('InlineScriptEnvManager', () => { inspectMetaStub = sinon.stub(cacheLayout, 'inspectMetaJson').resolves({ kind: 'missing' }); baseInterpreterStatusStub = sinon.stub(cacheLayout, 'getBaseInterpreterStatus').resolves('available'); writeMetaStub = sinon.stub(cacheLayout, 'writeMetaJson').resolves(); + retainLockStub = sinon.stub().resolves(); releaseLockStub = sinon.stub().resolves(); - lockStub = sinon.stub(lockfileApis, 'acquireFileLock').resolves(releaseLockStub); + lockStub = sinon + .stub(lockfileApis, 'acquireFileLock') + .resolves({ release: releaseLockStub, retain: retainLockStub }); resolveVenvStub = sinon.stub(venvUtils, 'resolveVenvPythonEnvironmentPath').resolves(undefined); createWithProgressStub = sinon.stub(venvUtils, 'createWithProgress').callsFake(async (...args: unknown[]) => { const envDir = args[6] as string; - await fs.outputFile(venvPythonPath(envDir), ''); + await fs.outputFile(venvUtils.getVenvPythonPath(envDir), ''); return { - environment: makeEnvironment('ms-python.python:inline-script', '3.12.4', venvPythonPath(envDir), envDir), + environment: makeEnvironment( + 'ms-python.python:inline-script', + '3.12.4', + venvUtils.getVenvPythonPath(envDir), + envDir, + ), }; }); @@ -252,6 +259,24 @@ suite('InlineScriptEnvManager', () => { assert.strictEqual(lockStub.callCount, 0); assert.strictEqual(createWithProgressStub.callCount, 0); }); + + test('skips base records with empty or relative sysPrefix values', async () => { + const emptyPrefixExecutable = path.join(tempRoot, 'empty-prefix-python'); + const relativePrefixExecutable = path.join(tempRoot, 'relative-prefix-python'); + await fs.outputFile(emptyPrefixExecutable, ''); + await fs.outputFile(relativePrefixExecutable, ''); + apiGetEnvironmentsStub.resolves([ + makeEnvironment('ms-python.python:system', '3.14.0', emptyPrefixExecutable, ''), + makeEnvironment('ms-python.python:system', '3.13.0', relativePrefixExecutable, 'relative-prefix'), + baseEnvironment, + ]); + + assert.ok(await manager.create(scriptUri())); + sinon.assert.calledWith(computeCacheKeyStub, { + dependencies: ['requests'], + interpreterPath: await fs.realpath(baseExecutable), + }); + }); }); suite('cache creation', () => { @@ -456,8 +481,8 @@ suite('InlineScriptEnvManager', () => { assert.strictEqual(writeMetaStub.callCount, 0); }); - test('preserves an inline-owned entry whose resolved version is unparseable', async () => { - await fs.outputFile(venvPythonPath(envDir().fsPath), ''); + test('rebuilds an inline-owned entry whose resolved version is unparseable', async () => { + await fs.outputFile(venvUtils.getVenvPythonPath(envDir().fsPath), ''); const markerPath = path.join(envDir().fsPath, 'keep.txt'); await fs.outputFile(markerPath, 'keep'); setSidecar({ @@ -470,14 +495,14 @@ suite('InlineScriptEnvManager', () => { makeEnvironment( 'ms-python.python:inline-script', 'Unknown', - venvPythonPath(envDir().fsPath), + venvUtils.getVenvPythonPath(envDir().fsPath), envDir().fsPath, ), ); - assert.strictEqual(await manager.create(scriptUri()), undefined); - assert.strictEqual(await fs.readFile(markerPath, 'utf8'), 'keep'); - assert.strictEqual(createWithProgressStub.callCount, 0); + assert.ok(await manager.create(scriptUri())); + assert.strictEqual(await fs.pathExists(markerPath), false); + assert.strictEqual(createWithProgressStub.callCount, 1); }); test('rebuilds when the resolved environment no longer satisfies requires-python', async () => { @@ -657,6 +682,7 @@ suite('InlineScriptEnvManager', () => { assert.strictEqual(await manager.create(scriptUri()), undefined); assert.strictEqual(await fs.pathExists(envDir().fsPath), true); assert.strictEqual(writeMetaStub.callCount, 0); + assert.ok(retainLockStub.calledOnce); assert.strictEqual(releaseLockStub.callCount, 0); });