From 5a4b40a3bce27c21019294e2a6dc12e0a5d36e35 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Thu, 23 Jul 2026 16:24:56 -0700 Subject: [PATCH 1/3] Add generic environment-creation utilities Cross-process file lock, venv Python-path helper, cancellation-safe process runner, and createWithProgress tracking options that inline-script environment creation builds on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39dcc6a3-0fbd-4f36-9d0f-68677de49c27 --- src/common/lockfile.apis.ts | 135 +++++++++++ src/common/utils/virtualEnvironment.ts | 11 + src/managers/builtin/helpers.ts | 100 +++++---- src/managers/builtin/venvUtils.ts | 26 ++- src/test/common/lockfile.apis.unit.test.ts | 212 ++++++++++++++++++ .../common/virtualEnvironment.unit.test.ts | 27 +++ .../builtin/helpers.cancellation.unit.test.ts | 85 +++++++ .../venvUtils.createWithProgress.unit.test.ts | 134 +++++++++++ 8 files changed, 683 insertions(+), 47 deletions(-) create mode 100644 src/common/lockfile.apis.ts create mode 100644 src/common/utils/virtualEnvironment.ts create mode 100644 src/test/common/lockfile.apis.unit.test.ts create mode 100644 src/test/common/virtualEnvironment.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/lockfile.apis.ts b/src/common/lockfile.apis.ts new file mode 100644 index 000000000..dd6bc3499 --- /dev/null +++ b/src/common/lockfile.apis.ts @@ -0,0 +1,135 @@ +// 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'; + +/** Acquire an atomic lock released only explicitly; interrupted operations remain locked. */ +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) { + try { + await fsapi.mkdir(lockPath); + try { + await fsapi.writeFile(ownerMarker, '', { flag: 'wx' }); + } catch (error) { + try { + await fsapi.rmdir(lockPath); + } catch { + throw createLockError( + 'Lock initialization failed and left an owner-less lock directory', + 'ELOCKORPHANED', + lockPath, + ); + } + throw error; + } + + let state: LockState = 'held'; + return { + retain: async () => { + if (state !== 'held') { + return; + } + state = 'retained'; + try { + await fsapi.writeFile(retainedMarker, '', { flag: 'wx' }); + } catch (error) { + if (isAlreadyExistsError(error)) { + return; + } + try { + await fsapi.rename(ownerMarker, retainedMarker); + } catch (renameError) { + if (!isAlreadyExistsError(renameError)) { + 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); + } + 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'); +} + +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 + ); +} + +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)); +} + +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/common/utils/virtualEnvironment.ts b/src/common/utils/virtualEnvironment.ts new file mode 100644 index 000000000..c581bf521 --- /dev/null +++ b/src/common/utils/virtualEnvironment.ts @@ -0,0 +1,11 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as path from 'path'; +import { isWindows } from './platformUtils'; + +export function getVenvPythonPath(envPath: string): string { + return isWindows() + ? path.join(envPath, 'Scripts', 'python.exe') + : path.join(envPath, 'bin', 'python'); +} diff --git a/src/managers/builtin/helpers.ts b/src/managers/builtin/helpers.ts index 911bc6031..579892170 100644 --- a/src/managers/builtin/helpers.ts +++ b/src/managers/builtin/helpers.ts @@ -66,40 +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); - token?.onCancellationRequested(() => { - proc.kill(); - reject(new CancellationError()); - }); - - proc.on('error', (err) => { - 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: '', }); } @@ -111,37 +85,75 @@ 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 }); - token?.onCancellationRequested(() => { - proc.kill(); + 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; + options.token?.onCancellationRequested(() => { + cancellationRequested = true; + try { + proc.kill(); + } catch { + // Preserve cancellation when signaling fails. + } reject(new CancellationError()); }); proc.on('error', (err) => { - log?.error(`Error spawning python: ${err}`); - reject(new Error(`Error spawning python: ${err.message}`)); + if (cancellationRequested) { + reject(new CancellationError()); + return; + } + 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/venvUtils.ts b/src/managers/builtin/venvUtils.ts index 4efab305e..887864385 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'; @@ -10,6 +19,7 @@ import { getWorkspacePersistentState } from '../../common/persistentState'; import { EventNames } from '../../common/telemetry/constants'; import { sendTelemetryEvent } from '../../common/telemetry/sender'; import { normalizePath } from '../../common/utils/pathUtils'; +import { getVenvPythonPath } from '../../common/utils/virtualEnvironment'; import { showErrorMessage, showOpenDialog, @@ -52,6 +62,14 @@ export interface CreateEnvironmentResult { * Exists if error occurred while installing packages and includes error description. */ pkgInstallationErr?: string; + + /** Cancellation may leave package processes running. */ + pkgInstallationCancelled?: boolean; +} + +export interface CreateWithProgressOptions { + /** Whether to record uv-created environments in workspace state. Defaults to true. */ + readonly trackUvEnvironment?: boolean; } export async function clearVenvCache(): Promise { @@ -340,9 +358,9 @@ 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'); + const pythonPath = getVenvPythonPath(envPath); return await withProgress( { @@ -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/lockfile.apis.unit.test.ts b/src/test/common/lockfile.apis.unit.test.ts new file mode 100644 index 000000000..a2d343a19 --- /dev/null +++ b/src/test/common/lockfile.apis.unit.test.ts @@ -0,0 +1,212 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +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 = { + 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 () => { + sinon.restore(); + 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((lock) => { + process.stdout.write('locked\\n'); + if (${exitWithoutRelease}) { + process.exit(0); + } + process.stdin.once('data', async () => { + await lock.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 lock = await acquireFileLock(targetPath, OPTIONS); + await assert.rejects(acquireFileLock(targetPath, OPTIONS), (error: NodeJS.ErrnoException) => { + return error.code === 'ELOCKED'; + }); + + await lock.release(); + const lockAfterRetry = await acquireFileLock(targetPath, OPTIONS); + await lockAfterRetry.release(); + 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 lock = await acquireFileLock(targetPath, OPTIONS); + await lock.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 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(lock.release(), (error: NodeJS.ErrnoException) => { + return error.code === 'ECOMPROMISED'; + }); + assert.strictEqual(await fs.pathExists(successorMarker), true); + }); + + 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`; + const retainedEntries = await fs.readdir(lockPath); + assert.ok(retainedEntries.includes('retained')); + assert.strictEqual(retainedEntries.filter((entry) => entry.startsWith('owner-')).length, 1); + + await lock.release(); + assert.deepStrictEqual(await fs.readdir(lockPath), retainedEntries); + }); + + test('falls back to renaming the owner marker when the retained sentinel cannot be written', async () => { + const lock = await acquireFileLock(targetPath, OPTIONS); + sinon.stub(fsExtra, 'writeFile').rejects(Object.assign(new Error('write failed'), { code: 'EACCES' })); + + await lock.retain(); + + const lockPath = `${path.resolve(targetPath)}.lock`; + assert.deepStrictEqual(await fs.readdir(lockPath), ['retained']); + await assert.rejects(acquireFileLock(targetPath, OPTIONS), (error: NodeJS.ErrnoException) => { + return error.code === 'ELOCKRETAINED'; + }); + }); + + test('remains fail-closed when neither retained-marker strategy succeeds', async () => { + const lock = await acquireFileLock(targetPath, OPTIONS); + sinon.stub(fsExtra, 'writeFile').rejects(Object.assign(new Error('write failed'), { code: 'EACCES' })); + sinon.stub(fsExtra, 'rename').rejects(Object.assign(new Error('rename failed'), { code: 'EBUSY' })); + + await assert.rejects(lock.retain(), (error: NodeJS.ErrnoException) => error.code === 'ERETAINFAILED'); + await lock.release(); + + const lockPath = `${path.resolve(targetPath)}.lock`; + const lockEntries = await fs.readdir(lockPath); + assert.strictEqual(lockEntries.filter((entry) => entry.startsWith('owner-')).length, 1); + await assert.rejects(acquireFileLock(targetPath, OPTIONS), (error: NodeJS.ErrnoException) => { + return error.code === 'ELOCKED'; + }); + }); + + 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/common/virtualEnvironment.unit.test.ts b/src/test/common/virtualEnvironment.unit.test.ts new file mode 100644 index 000000000..093ab7104 --- /dev/null +++ b/src/test/common/virtualEnvironment.unit.test.ts @@ -0,0 +1,27 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import assert from 'assert'; +import * as path from 'path'; +import * as sinon from 'sinon'; +import * as platformUtils from '../../common/utils/platformUtils'; +import { getVenvPythonPath } from '../../common/utils/virtualEnvironment'; + +suite('virtual environment utilities', () => { + teardown(() => { + sinon.restore(); + }); + + test('uses Scripts/python.exe on Windows', () => { + sinon.stub(platformUtils, 'isWindows').returns(true); + assert.strictEqual( + getVenvPythonPath(path.join('cache', 'env')), + path.join('cache', 'env', 'Scripts', 'python.exe'), + ); + }); + + test('uses bin/python outside Windows', () => { + sinon.stub(platformUtils, 'isWindows').returns(false); + assert.strictEqual(getVenvPythonPath(path.join('cache', 'env')), path.join('cache', 'env', 'bin', 'python')); + }); +}); 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/venvUtils.createWithProgress.unit.test.ts b/src/test/managers/builtin/venvUtils.createWithProgress.unit.test.ts new file mode 100644 index 000000000..846dc705a --- /dev/null +++ b/src/test/managers/builtin/venvUtils.createWithProgress.unit.test.ts @@ -0,0 +1,134 @@ +// 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 { getVenvPythonPath } from '../../../common/utils/virtualEnvironment'; +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 = getVenvPythonPath(envPath); + 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 25acae1462e04ce50fb15667f4373e26a8354f81 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Fri, 24 Jul 2026 10:13:51 -0700 Subject: [PATCH 2/3] address feedback 1 --- src/managers/builtin/helpers.ts | 98 ++++++++++++++++++--------------- 1 file changed, 53 insertions(+), 45 deletions(-) diff --git a/src/managers/builtin/helpers.ts b/src/managers/builtin/helpers.ts index 579892170..7fb7062a2 100644 --- a/src/managers/builtin/helpers.ts +++ b/src/managers/builtin/helpers.ts @@ -66,14 +66,50 @@ export async function runUV( token?: CancellationToken, timeout?: number, ): Promise { - return runProcess('uv', args, { - cwd, - displayName: 'uv', - log, - token, - timeout, - collectStderr: false, - logPrefix: '', + 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(' ')}`)); + } + }); }); } @@ -85,27 +121,11 @@ export async function runPython( token?: CancellationToken, timeout?: number, ): Promise { - 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(' ')}`); + log?.info(`Running: ${python} ${args.join(' ')}`); return new Promise((resolve, reject) => { - const spawnOptions: { cwd?: string; timeout?: number } = { cwd: options.cwd }; - if (options.timeout !== undefined) { - spawnOptions.timeout = options.timeout; - } - const proc = spawnProcess(executable, args, spawnOptions); + const proc = spawnProcess(python, args, { cwd: cwd, timeout }); let cancellationRequested = false; - options.token?.onCancellationRequested(() => { + token?.onCancellationRequested(() => { cancellationRequested = true; try { proc.kill(); @@ -120,40 +140,28 @@ function runProcess(executable: string, args: string[], options: RunProcessOptio reject(new CancellationError()); return; } - options.log?.error(`Error spawning ${options.displayName}: ${err}`); - reject(new Error(`Error spawning ${options.displayName}: ${err.message}`)); + log?.error(`Error spawning python: ${err}`); + reject(new Error(`Error spawning python: ${err.message}`)); }); let builder = ''; proc.stdout?.on('data', (data) => { const s = data.toString('utf-8'); builder += s; - options.log?.append(`${options.logPrefix}${s}`); + log?.append(`python: ${s}`); }); proc.stderr?.on('data', (data) => { const s = data.toString('utf-8'); - if (options.collectStderr) { - builder += s; - } - options.log?.append(`${options.logPrefix}${s}`); + builder += s; + log?.append(`python: ${s}`); }); proc.on('close', () => { resolve(builder); }); proc.on('exit', (code) => { if (code !== 0) { - reject(new Error(`Failed to run ${options.displayName} ${args.join(' ')}`)); + reject(new Error(`Failed to run python ${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; -} From e78bda6d53df744d1d2573b5803248a98e7fb863 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Fri, 24 Jul 2026 10:27:51 -0700 Subject: [PATCH 3/3] address feedback 2 --- src/common/lockfile.apis.ts | 44 ++++++++----------- src/managers/builtin/venvUtils.ts | 9 +--- .../venvUtils.createWithProgress.unit.test.ts | 6 +-- 3 files changed, 23 insertions(+), 36 deletions(-) diff --git a/src/common/lockfile.apis.ts b/src/common/lockfile.apis.ts index dd6bc3499..1d8409056 100644 --- a/src/common/lockfile.apis.ts +++ b/src/common/lockfile.apis.ts @@ -5,6 +5,19 @@ 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; +} + +export interface AcquiredFileLock { + readonly release: () => Promise; + /** Keep the lock and make later acquisition attempts fail immediately. */ + readonly retain: () => Promise; +} + +type LockState = 'held' | 'released' | 'retained'; + /** Acquire an atomic lock released only explicitly; interrupted operations remain locked. */ export async function acquireFileLock(filePath: string, options: AcquireFileLockOptions): Promise { const lockPath = `${path.resolve(filePath)}.lock`; @@ -40,13 +53,13 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock try { await fsapi.writeFile(retainedMarker, '', { flag: 'wx' }); } catch (error) { - if (isAlreadyExistsError(error)) { + if (hasErrorCode(error, 'EEXIST')) { return; } try { await fsapi.rename(ownerMarker, retainedMarker); } catch (renameError) { - if (!isAlreadyExistsError(renameError)) { + if (!hasErrorCode(renameError, 'EEXIST')) { throw createLockError( 'Failed to mark the lock as retained', 'ERETAINFAILED', @@ -64,7 +77,7 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock try { await fsapi.unlink(ownerMarker); } catch (error) { - if (isFileNotFoundError(error)) { + if (hasErrorCode(error, 'ENOENT')) { throw createLockError('Lock ownership was compromised', 'ECOMPROMISED', lockPath); } throw error; @@ -73,7 +86,7 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock }, }; } catch (error) { - if (!isAlreadyExistsError(error)) { + if (!hasErrorCode(error, 'EEXIST')) { throw error; } if (await isRetainedLock(lockPath)) { @@ -87,20 +100,12 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock } } -function isAlreadyExistsError(error: unknown): boolean { - return hasErrorCode(error, 'EEXIST'); -} - -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)) { + if (hasErrorCode(error, 'ENOENT')) { return false; } throw error; @@ -120,16 +125,3 @@ function createLockError(message: string, code: string, lockPath: string): NodeJ async function delay(milliseconds: number): Promise { return new Promise((resolve) => setTimeout(resolve, milliseconds)); } - -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/venvUtils.ts b/src/managers/builtin/venvUtils.ts index 887864385..c06146999 100644 --- a/src/managers/builtin/venvUtils.ts +++ b/src/managers/builtin/venvUtils.ts @@ -67,11 +67,6 @@ export interface CreateEnvironmentResult { pkgInstallationCancelled?: boolean; } -export interface CreateWithProgressOptions { - /** Whether to record uv-created environments in workspace state. Defaults to true. */ - readonly trackUvEnvironment?: boolean; -} - export async function clearVenvCache(): Promise { const keys = [VENV_WORKSPACE_KEY, VENV_GLOBAL_KEY, UV_ENVS_KEY]; const state = await getWorkspacePersistentState(); @@ -358,7 +353,7 @@ export async function createWithProgress( venvRoot: Uri, envPath: string, packages?: PipPackages, - options?: CreateWithProgressOptions, + trackUvEnvironment = true, ): Promise { const pythonPath = getVenvPythonPath(envPath); @@ -401,7 +396,7 @@ export async function createWithProgress( const env = api.createPythonEnvironmentItem(await getPythonInfo(resolved), manager); if ( - options?.trackUvEnvironment !== false && + trackUvEnvironment && useUv && (resolved.kind === NativePythonEnvironmentKind.venvUv || resolved.kind === NativePythonEnvironmentKind.uvWorkspace) diff --git a/src/test/managers/builtin/venvUtils.createWithProgress.unit.test.ts b/src/test/managers/builtin/venvUtils.createWithProgress.unit.test.ts index 846dc705a..bd8e720e2 100644 --- a/src/test/managers/builtin/venvUtils.createWithProgress.unit.test.ts +++ b/src/test/managers/builtin/venvUtils.createWithProgress.unit.test.ts @@ -16,7 +16,7 @@ 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', () => { +suite('createWithProgress uv tracking', () => { let addUvEnvironmentStub: sinon.SinonStub; let api: PythonEnvironmentApi; let baseEnvironment: PythonEnvironment; @@ -105,7 +105,7 @@ suite('createWithProgress uv tracking options', () => { Uri.file(tempRoot), envPath, undefined, - { trackUvEnvironment: false }, + false, // trackUvEnvironment ); assert.ok(result?.environment); @@ -124,7 +124,7 @@ suite('createWithProgress uv tracking options', () => { Uri.file(tempRoot), envPath, { install: ['requests'], uninstall: [] }, - { trackUvEnvironment: false }, + false, // trackUvEnvironment ); assert.ok(result?.environment);