diff --git a/cspell.json b/cspell.json index 8aa54f8759..ad6ec1a49b 100644 --- a/cspell.json +++ b/cspell.json @@ -45,6 +45,7 @@ "Dremio", "duckdb", "ename", + "ensurepip", "esmock", "evalue", "findstr", diff --git a/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts b/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts index 2c7ebdadbc..abaa15114a 100644 --- a/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts +++ b/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts @@ -6,10 +6,10 @@ import { CancellationToken, l10n, Uri, workspace } from 'vscode'; import { resolvePythonExecutable } from '@deepnote/runtime-core'; -import { Cancellation } from '../../platform/common/cancellation'; +import { Cancellation, isCancellationError } from '../../platform/common/cancellation'; import { STANDARD_OUTPUT_CHANNEL } from '../../platform/common/constants'; import { IFileSystem } from '../../platform/common/platform/types'; -import { IProcessServiceFactory } from '../../platform/common/process/types.node'; +import { ExecutionResult, IProcessServiceFactory } from '../../platform/common/process/types.node'; import { IExtensionContext, IOutputChannel } from '../../platform/common/types'; import { DeepnoteToolkitInstallError, @@ -116,18 +116,11 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { // Check if venv already exists with toolkit installed const existingVenv = await this.getVenvInterpreterByPath(venvPath); if (existingVenv) { - const toolkitVersion = await this.isToolkitInstalled(existingVenv); + const toolkitVersion = await this.isToolkitInstalled(existingVenv, token); if (toolkitVersion != null) { logger.info(`deepnote-toolkit venv already exists at ${venvPath.fsPath}`); - // Ensure kernel spec is installed (may have been deleted or never installed) - try { - Cancellation.throwIfCanceled(token); - await this.installKernelSpec(existingVenv, venvPath, token); - } catch (ex) { - logger.warn('Failed to ensure kernel spec installed', ex); - // Don't fail - continue with existing venv - } + await this.tryInstallKernelSpec(existingVenv, venvPath, token); logger.info(`Venv ready at ${venvPath.fsPath}`); return { pythonInterpreter: existingVenv, toolkitVersion }; @@ -188,13 +181,10 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { this.outputChannel.appendLine(l10n.t('Installing packages: {0}...', packages.join(', '))); try { - Cancellation.throwIfCanceled(token); - - const venvProcessService = await this.processServiceFactory.create(undefined); - const installResult = await venvProcessService.exec( + const installResult = await this.runPython( venvInterpreter.uri.fsPath, ['-m', 'pip', 'install', '--upgrade', ...packages], - { throwOnStdErr: false } + token ); if (installResult.stdout) { @@ -207,6 +197,13 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { logger.info('Additional packages installed successfully'); this.outputChannel.appendLine(l10n.t('✓ Packages installed successfully')); } catch (ex) { + if (isCancellationError(ex)) { + logger.info('Package installation cancelled'); + this.outputChannel.appendLine(l10n.t('Package installation cancelled')); + + throw ex; + } + logger.error('Failed to install additional packages', ex); this.outputChannel.appendLine(l10n.t('✗ Failed to install packages: {0}', ex)); throw ex; @@ -277,20 +274,13 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { await workspace.fs.delete(venvPath, { recursive: true }); } - // Create new venv - // Use undefined as resource to get full system environment - const processService = await this.processServiceFactory.create(undefined); - const venvResult = await processService.exec(baseInterpreter.uri.fsPath, ['-m', 'venv', venvPath.fsPath], { - throwOnStdErr: false - }); + const venvResult = await this.runPython(baseInterpreter.uri.fsPath, ['-m', 'venv', venvPath.fsPath], token); // Log any stderr output (warnings, etc.) but don't fail on it if (venvResult.stderr) { logger.info('venv creation stderr', venvResult.stderr); } - Cancellation.throwIfCanceled(token); - // Verify venv was created successfully by checking for the Python interpreter const venvInterpreter = await this.getVenvInterpreterByPath(venvPath); if (!venvInterpreter) { @@ -310,6 +300,11 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { // Use the shared helper method to install toolkit packages return await this.installToolkitPackages(venvInterpreter, venvPath, token); } catch (ex) { + // Unwrapped, so the caller's isCancellationError check suppresses the error UI + if (isCancellationError(ex)) { + throw ex; + } + // If this is already a DeepnoteKernelError, rethrow it without wrapping if (ex instanceof DeepnoteVenvCreationError || ex instanceof DeepnoteToolkitInstallError) { throw ex; @@ -339,16 +334,14 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { venvPath: Uri, token?: CancellationToken ): Promise { - // Use undefined as resource to get full system environment (including git in PATH) - const venvProcessService = await this.processServiceFactory.create(undefined); - // Upgrade pip in the venv to the latest version logger.info('Upgrading pip in venv to latest version...'); this.outputChannel.appendLine(l10n.t('Upgrading pip...')); - const pipUpgradeResult = await venvProcessService.exec( + + const pipUpgradeResult = await this.runPython( venvInterpreter.uri.fsPath, ['-m', 'pip', 'install', '--upgrade', 'pip'], - { throwOnStdErr: false } + token ); if (pipUpgradeResult.stdout) { @@ -358,8 +351,6 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { logger.info('pip upgrade stderr', pipUpgradeResult.stderr); } - Cancellation.throwIfCanceled(token); - // Install deepnote-toolkit, ipykernel, python-lsp-server, and deepnote-cli in venv logger.info( `Installing deepnote-toolkit (${DEEPNOTE_TOOLKIT_VERSION}), ipykernel, python-lsp-server, and deepnote-cli in venv from PyPI` @@ -368,7 +359,7 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { l10n.t('Installing deepnote-toolkit, ipykernel, python-lsp-server, and deepnote-cli...') ); - const installResult = await venvProcessService.exec( + const installResult = await this.runPython( venvInterpreter.uri.fsPath, [ '-m', @@ -380,11 +371,9 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { 'python-lsp-server[all]', 'deepnote-cli' ], - { throwOnStdErr: false } + token ); - Cancellation.throwIfCanceled(token); - if (installResult.stdout) { this.outputChannel.appendLine(installResult.stdout); } @@ -393,18 +382,12 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { } // Verify installation - const installedToolkitVersion = await this.isToolkitInstalled(venvInterpreter); + const installedToolkitVersion = await this.isToolkitInstalled(venvInterpreter, token); if (installedToolkitVersion != null) { logger.info('deepnote-toolkit installed successfully in venv'); // Install kernel spec so the kernel uses this venv's Python - try { - Cancellation.throwIfCanceled(token); - await this.installKernelSpec(venvInterpreter, venvPath, token); - } catch (ex) { - logger.warn('Failed to install kernel spec', ex); - // Don't fail the entire installation if kernel spec creation fails - } + await this.tryInstallKernelSpec(venvInterpreter, venvPath, token); this.outputChannel.appendLine(l10n.t('✓ Deepnote toolkit ready')); return { pythonInterpreter: venvInterpreter, toolkitVersion: installedToolkitVersion }; @@ -422,23 +405,79 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { } } - private async isToolkitInstalled(interpreter: PythonEnvironment): Promise { + private async isToolkitInstalled( + interpreter: PythonEnvironment, + token: CancellationToken | undefined + ): Promise { try { - // Use undefined as resource to get full system environment - const processService = await this.processServiceFactory.create(undefined); - const result = await processService.exec(interpreter.uri.fsPath, [ - '-c', - 'import deepnote_toolkit; print(deepnote_toolkit.__version__)' - ]); + const result = await this.runPython( + interpreter.uri.fsPath, + ['-c', 'import deepnote_toolkit; print(deepnote_toolkit.__version__)'], + token + ); + logger.info(`isToolkitInstalled result: ${result.stdout}`); const version = result.stdout.trim(); + return version.length > 0 ? version : undefined; } catch (ex) { + if (isCancellationError(ex)) { + throw ex; + } + logger.debug('deepnote-toolkit not found', ex); + return undefined; } } + /** + * `ProcessService.exec` kills the subprocess when the token fires but still *resolves*, with the + * output captured up to that point. Hence the trailing check: without it a killed `pip install` + * reads as a successful one and a killed version probe reads as "toolkit not installed". + */ + private async runPython( + pythonPath: string, + args: string[], + token: CancellationToken | undefined + ): Promise> { + Cancellation.throwIfCanceled(token); + + // Resource `undefined` gives the full system environment, which pip needs (e.g. git on PATH). + const processService = await this.processServiceFactory.create(undefined); + // Own process group: `python -m venv` runs ensurepip in a subprocess that would otherwise + // outlive cancellation and keep writing into the venv the retry is busy deleting. + const result = await processService.exec(pythonPath, args, { + throwOnStdErr: false, + token, + detached: true + }); + + Cancellation.throwIfCanceled(token); + + return result; + } + + /** + * Failure is tolerated because a venv without a kernel spec still runs and the next + * `ensureVenvAndToolkit` retries. Cancellation is not a failure, so it unwinds the caller. + */ + private async tryInstallKernelSpec( + venvInterpreter: PythonEnvironment, + venvPath: Uri, + token: CancellationToken | undefined + ): Promise { + try { + await this.installKernelSpec(venvInterpreter, venvPath, token); + } catch (ex) { + if (isCancellationError(ex)) { + throw ex; + } + + logger.warn('Failed to install kernel spec', ex); + } + } + /** * Generate a kernel spec name from a venv path. * This is used for both file-based and environment-based venvs. @@ -480,22 +519,16 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { const kernelSpecName = this.getKernelSpecName(venvPath); const kernelSpecPath = Uri.joinPath(venvPath, 'share', 'jupyter', 'kernels', kernelSpecName); - // Check if kernel spec already exists - if (await this.fs.exists(kernelSpecPath)) { + // Keyed on kernel.json, not the directory: a cancelled ipykernel install leaves the + // directory behind, and that must not short-circuit the reinstall. + if (await this.fs.exists(Uri.joinPath(kernelSpecPath, 'kernel.json'))) { logger.info(`Kernel spec already exists at ${kernelSpecPath.fsPath}`); return; } - Cancellation.throwIfCanceled(token); - logger.info(`Installing kernel spec '${kernelSpecName}' for venv at ${venvPath.fsPath}...`); - const kernelDisplayName = this.getKernelDisplayName(venvPath); - - const venvProcessService = await this.processServiceFactory.create(undefined); - - Cancellation.throwIfCanceled(token); - await venvProcessService.exec( + await this.runPython( venvInterpreter.uri.fsPath, [ '-m', @@ -506,9 +539,9 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { '--name', kernelSpecName, '--display-name', - kernelDisplayName + this.getKernelDisplayName(venvPath) ], - { throwOnStdErr: false } + token ); logger.info(`Kernel spec installed successfully to ${kernelSpecPath.fsPath}`); diff --git a/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts b/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts new file mode 100644 index 0000000000..6605306491 --- /dev/null +++ b/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts @@ -0,0 +1,261 @@ +import { assert } from 'chai'; +import { anything, capture, instance, mock, verify, when } from 'ts-mockito'; +import { CancellationError, CancellationTokenSource, Uri } from 'vscode'; + +import { DeepnoteToolkitInstaller } from './deepnoteToolkitInstaller.node'; +import { DeepnoteToolkitInstallError, DeepnoteToolkitMissingError } from '../../platform/errors/deepnoteKernelErrors'; +import { + ExecutionResult, + IProcessService, + IProcessServiceFactory, + SpawnOptions +} from '../../platform/common/process/types.node'; +import { IFileSystem } from '../../platform/common/platform/types'; +import { IExtensionContext, IOutputChannel } from '../../platform/common/types'; +import { PythonEnvironment } from '../../platform/pythonEnvironments/info'; + +/** + * Two invariants, both easy to regress: + * + * 1. Every exec must carry the token — that is what wires Stop to ProcessService.kill(pid) + * (see proc.node.ts), and without it a multi-minute pip install is uninterruptible. + * 2. A killed subprocess still *resolves* exec, so every outcome must be re-checked afterwards. + */ +suite('DeepnoteToolkitInstaller - cancellation', () => { + const globalStorage = Uri.file('/fake/storage'); + const managedVenvPath = Uri.joinPath(globalStorage, 'deepnote-venvs', 'venv_abc123'); + const venvPath = Uri.file('/fake/venv'); + const fakePython = Uri.file('/fake/venv/bin/python'); + const venvInterpreter: PythonEnvironment = { uri: fakePython, id: fakePython.fsPath }; + const baseInterpreter: PythonEnvironment = { uri: Uri.file('/usr/bin/python3'), id: '/usr/bin/python3' }; + + let installer: DeepnoteToolkitInstaller; + let mockProcessService: IProcessService; + let mockProcessServiceFactory: IProcessServiceFactory; + let mockOutputChannel: IOutputChannel; + let mockContext: IExtensionContext; + let mockFs: IFileSystem; + let cts: CancellationTokenSource; + + async function rejection(promise: Promise): Promise { + return promise.then( + () => undefined, + (ex) => ex + ); + } + + function killExecViaToken(): void { + when(mockProcessService.exec(anything(), anything(), anything())).thenCall(async () => { + cts.cancel(); + return { stdout: '', stderr: '' }; + }); + } + + /** Without this the subject calls the real resolvePythonExecutable, which finds no venv on disk. */ + function seedInterpreterCache(path: Uri): void { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (installer as any).venvPythonPaths.set(path.fsPath, fakePython); + } + + setup(() => { + mockProcessService = mock(); + mockProcessServiceFactory = mock(); + mockOutputChannel = mock(); + mockContext = mock(); + mockFs = mock(); + cts = new CancellationTokenSource(); + + const processService = instance(mockProcessService); + // Prevent the ts-mockito instance from being treated as a thenable when + // resolved through a promise (see kernelProcess.node.unit.test.ts). + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (processService as any).then = undefined; + when(mockProcessServiceFactory.create(anything())).thenResolve(processService); + when(mockProcessService.exec(anything(), anything(), anything())).thenResolve({ stdout: '', stderr: '' }); + when(mockContext.globalStorageUri).thenReturn(globalStorage); + + installer = new DeepnoteToolkitInstaller( + instance(mockProcessServiceFactory), + instance(mockOutputChannel), + instance(mockContext), + instance(mockFs) + ); + }); + + teardown(() => cts.dispose()); + + test('installAdditionalPackages forwards the cancellation token to exec', async () => { + seedInterpreterCache(venvPath); + + await installer.installAdditionalPackages(venvPath, ['some-package'], cts.token); + + verify(mockProcessService.exec(anything(), anything(), anything())).once(); + const [file, args, options] = capture(mockProcessService.exec).first(); + assert.deepStrictEqual( + { file, args, options }, + { + file: fakePython.fsPath, + args: ['-m', 'pip', 'install', '--upgrade', 'some-package'], + options: { throwOnStdErr: false, token: cts.token, detached: true } + }, + 'the token must reach exec so Stop can kill pip' + ); + }); + + test('installAdditionalPackages does not call exec when no packages are requested', async () => { + seedInterpreterCache(venvPath); + + await installer.installAdditionalPackages(venvPath, [], cts.token); + + verify(mockProcessService.exec(anything(), anything(), anything())).never(); + }); + + test('installAdditionalPackages reports a killed pip install as cancelled', async () => { + seedInterpreterCache(venvPath); + killExecViaToken(); + + assert.instanceOf( + await rejection(installer.installAdditionalPackages(venvPath, ['some-package'], cts.token)), + CancellationError, + 'pip killed mid-flight must not resolve as installed' + ); + const [lastLine] = capture(mockOutputChannel.appendLine).last(); + assert.strictEqual( + lastLine, + 'Package installation cancelled', + 'the output channel must not accuse a user-initiated Stop of being an install failure' + ); + }); + + test('ensureVenvAndToolkit forwards the cancellation token to the toolkit version probe', async () => { + seedInterpreterCache(venvPath); + when(mockProcessService.exec(anything(), anything(), anything())).thenResolve({ + stdout: '1.2.3\n', + stderr: '' + }); + // Kernel spec already present, so the fast path runs the probe exec and nothing else. + when(mockFs.exists(anything())).thenResolve(true); + + const result = await installer.ensureVenvAndToolkit(venvInterpreter, venvPath, false, cts.token); + + assert.strictEqual(result.toolkitVersion, '1.2.3'); + verify(mockProcessService.exec(anything(), anything(), anything())).once(); + const [file, args, options] = capture(mockProcessService.exec).first(); + assert.deepStrictEqual( + { file, args, options }, + { + file: fakePython.fsPath, + args: ['-c', 'import deepnote_toolkit; print(deepnote_toolkit.__version__)'], + options: { throwOnStdErr: false, token: cts.token, detached: true } + }, + 'the token must reach the isToolkitInstalled probe' + ); + }); + + test('an existing kernel spec is recognized by kernel.json, not by its directory', async () => { + seedInterpreterCache(venvPath); + when(mockProcessService.exec(anything(), anything(), anything())).thenResolve({ + stdout: '1.2.3\n', + stderr: '' + }); + when(mockFs.exists(anything())).thenResolve(true); + + await installer.ensureVenvAndToolkit(venvInterpreter, venvPath, false, cts.token); + + const [checkedPath] = capture(mockFs.exists).first(); + assert.strictEqual( + checkedPath.fsPath, + Uri.joinPath(venvPath, 'share', 'jupyter', 'kernels', 'deepnote-venv', 'kernel.json').fsPath, + 'a cancelled ipykernel install leaves the directory behind, so only kernel.json proves it finished' + ); + }); + + test('a cancelled version probe is not mistaken for a missing toolkit', async () => { + seedInterpreterCache(venvPath); + killExecViaToken(); + + // managedVenv: false means a genuinely missing toolkit surfaces as DeepnoteToolkitMissingError. + const error = await rejection(installer.ensureVenvAndToolkit(venvInterpreter, venvPath, false, cts.token)); + + assert.notInstanceOf(error, DeepnoteToolkitMissingError, 'cancelling must not be read as "toolkit missing"'); + assert.instanceOf(error, CancellationError); + }); + + test('a cancelled kernel spec install does not resolve as a ready venv', async () => { + seedInterpreterCache(venvPath); + when(mockFs.exists(anything())).thenResolve(false); + // Exec 0 is the version probe; the ipykernel install that follows it gets killed. + let execCount = 0; + when(mockProcessService.exec(anything(), anything(), anything())).thenCall( + async (): Promise> => { + if (execCount++ === 0) { + return { stdout: '1.2.3\n', stderr: '' }; + } + + cts.cancel(); + + return { stdout: '', stderr: '' }; + } + ); + + assert.instanceOf( + await rejection(installer.ensureVenvAndToolkit(venvInterpreter, venvPath, false, cts.token)), + CancellationError, + 'a half-written kernel spec must not be reported as a ready venv' + ); + }); + + test('cancelling a managed install rejects with CancellationError, not an install failure', async () => { + when(mockFs.exists(anything())).thenResolve(false); + // `python -m venv` is the first exec of a managed install. + killExecViaToken(); + + const error = await rejection( + installer.ensureVenvAndToolkit(baseInterpreter, managedVenvPath, true, cts.token) + ); + + assert.notInstanceOf( + error, + DeepnoteToolkitInstallError, + 'wrapping cancellation pops the "install failed" UI instead of unwinding quietly' + ); + assert.instanceOf(error, CancellationError); + }); + + test('every subprocess of a managed install carries the cancellation token', async () => { + const execCalls: { file: string; options?: SpawnOptions }[] = []; + when(mockFs.exists(anything())).thenResolve(false); + when(mockProcessService.exec(anything(), anything(), anything())).thenCall( + async (file: string, _args: string[], options?: SpawnOptions): Promise> => { + execCalls.push({ file, options }); + // The venv interpreter only becomes resolvable once `python -m venv` has run. + seedInterpreterCache(managedVenvPath); + + return { stdout: file === fakePython.fsPath ? '1.2.3\n' : '', stderr: '' }; + } + ); + + await installer.ensureVenvAndToolkit(baseInterpreter, managedVenvPath, true, cts.token); + + assert.deepStrictEqual( + execCalls.map(({ file }) => file), + [ + baseInterpreter.uri.fsPath, // python -m venv + fakePython.fsPath, // pip install --upgrade pip + fakePython.fsPath, // pip install deepnote-toolkit ... + fakePython.fsPath, // version probe + fakePython.fsPath // ipykernel install + ] + ); + assert.deepStrictEqual( + execCalls.filter(({ options }) => options?.token !== cts.token), + [], + 'a subprocess without the token cannot be killed by Stop' + ); + assert.deepStrictEqual( + execCalls.filter(({ options }) => options?.detached !== true), + [], + 'a subprocess outside its own group leaves its descendants running after Stop' + ); + }); +}); diff --git a/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts b/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts index fb71649919..b1669cb86e 100644 --- a/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts +++ b/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts @@ -930,6 +930,13 @@ export class DeepnoteKernelAutoSelector implements IDeepnoteKernelAutoSelector, * Handle kernel selection errors with user-friendly messages and actions */ public async handleKernelSelectionError(error: unknown, notebook: NotebookDocument): Promise { + // A user-initiated Stop is not a failure, so it must not raise the error UI. + if (error instanceof Error && isCancellationError(error)) { + logger.info(`Kernel selection cancelled for ${getDisplayPath(notebook.uri)}`); + + return; + } + if (error instanceof DeepnoteToolkitMissingError) { const installAction = l10n.t('Install'); const changeEnvironmentAction = l10n.t('Change Environment'); @@ -1027,6 +1034,12 @@ export class DeepnoteKernelAutoSelector implements IDeepnoteKernelAutoSelector, void window.showInformationMessage(l10n.t('deepnote-toolkit installed successfully')); } catch (installError) { + if (installError instanceof Error && isCancellationError(installError)) { + logger.info('deepnote-toolkit installation cancelled'); + + return; + } + logger.error('Failed to install deepnote-toolkit', installError); const errorMessage = installError instanceof Error ? installError.message : String(installError); diff --git a/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts b/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts index 90ef280a22..8cb56c4aeb 100644 --- a/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts @@ -21,7 +21,8 @@ import { IConfigurationService } from '../../platform/common/types'; import { IDeepnoteNotebookManager } from '../types'; import { IKernelProvider, IKernel, IJupyterKernelSpec } from '../../kernels/types'; import { IDeepnoteRequirementsHelper } from './deepnoteRequirementsHelper.node'; -import { NotebookDocument, Uri, NotebookController, CancellationToken } from 'vscode'; +import { CancellationError, NotebookDocument, Uri, NotebookController, CancellationToken } from 'vscode'; +import { DeepnoteToolkitMissingError } from '../../platform/errors/deepnoteKernelErrors'; import { DeepnoteEnvironment } from '../../kernels/deepnote/environments/deepnoteEnvironment'; import { PythonEnvironment } from '../../platform/pythonEnvironments/info'; import { getNotebookKey } from '../../platform/deepnote/deepnoteProjectUtils'; @@ -1092,6 +1093,55 @@ suite('DeepnoteKernelAutoSelector - rebuildController', () => { }); }); }); + + /** + * Every exec in the installer can now be killed, so cancellation surfaces here immediately + * instead of after pip has finished anyway. A user-initiated Stop is not a failure and + * must not raise the error UI - but a genuine failure still has to. + */ + suite('cancellation is not reported as a failure', () => { + const toolkitMissing = () => new DeepnoteToolkitMissingError('/usr/bin/python3', '/fake/venv'); + + function chooseInstall(): void { + when( + mockedVSCodeNamespaces.window.showWarningMessage(anything(), anything(), anything(), anything()) + ).thenResolve('Install' as never); + } + + test('cancelling the toolkit install does not show an error message', async () => { + chooseInstall(); + when(mockToolkitInstaller.installToolkitInExistingVenv(anything(), anything())).thenReject( + new CancellationError() + ); + + await selector.handleKernelSelectionError(toolkitMissing(), mockNotebook); + + verify(mockedVSCodeNamespaces.window.showErrorMessage(anything())).never(); + }); + + test('a failed toolkit install still shows an error message', async () => { + chooseInstall(); + when(mockToolkitInstaller.installToolkitInExistingVenv(anything(), anything())).thenReject( + new Error('pip exited with code 1') + ); + + await selector.handleKernelSelectionError(toolkitMissing(), mockNotebook); + + verify(mockedVSCodeNamespaces.window.showErrorMessage(anything())).once(); + }); + + test('a cancelled kernel selection does not show an error message', async () => { + await selector.handleKernelSelectionError(new CancellationError(), mockNotebook); + + verify(mockedVSCodeNamespaces.window.showErrorMessage(anything(), anything(), anything())).never(); + }); + + test('a generic kernel selection failure still shows an error message', async () => { + await selector.handleKernelSelectionError(new Error('kernel did not start'), mockNotebook); + + verify(mockedVSCodeNamespaces.window.showErrorMessage(anything(), anything(), anything())).once(); + }); + }); }); /** diff --git a/src/platform/common/process/proc.node.ts b/src/platform/common/process/proc.node.ts index b39165127f..0b677350a8 100644 --- a/src/platform/common/process/proc.node.ts +++ b/src/platform/common/process/proc.node.ts @@ -53,7 +53,7 @@ export class ProcessService implements IProcessService { return false; } } - public static kill(pid?: number): void { + public static kill(pid?: number, killGroup = false): void { try { if (!pid) { return; @@ -62,7 +62,7 @@ export class ProcessService implements IProcessService { // Windows doesn't support SIGTERM, so execute taskkill to kill the process execSync(`taskkill /pid ${pid} /T /F`); } else { - process.kill(pid); + process.kill(killGroup ? -pid : pid); } } catch { // Ignore. @@ -155,12 +155,17 @@ export class ProcessService implements IProcessService { } public exec(file: string, args: string[], options: SpawnOptions = {}): Promise> { const spawnOptions = this.getDefaultOptions(options); + // setsid() is what makes the negative-pid kill below legal: without its own group the child + // shares the extension host's, and kill(-pid) would signal the host. Windows has no + // equivalent - taskkill /T already reaps the tree there, and `detached` pops a console window. + const killGroup = spawnOptions.detached === true && process.platform !== 'win32'; + spawnOptions.detached = killGroup; const proc = spawn(file, args, spawnOptions); const deferred = createDeferred>(); const disposable: IDisposable = { dispose: () => { if (!proc.killed && !deferred.completed) { - ProcessService.kill(proc.pid); + ProcessService.kill(proc.pid, killGroup); } } }; diff --git a/src/platform/common/process/proc.node.unit.test.ts b/src/platform/common/process/proc.node.unit.test.ts new file mode 100644 index 0000000000..4cdc7a8d3e --- /dev/null +++ b/src/platform/common/process/proc.node.unit.test.ts @@ -0,0 +1,92 @@ +import { assert } from 'chai'; +import * as fs from 'fs'; +import * as os from 'os'; +import { CancellationTokenSource } from 'vscode'; + +import * as path from '../../vscode-path/path'; +import { ProcessService } from './proc.node'; + +/** + * Cancelling must reap the whole subprocess tree, not just the process we spawned. + * `python -m venv` runs ensurepip in a subprocess, and pip shells out to build backends; + * a survivor keeps writing into the venv that the retry is busy deleting. + */ +suite('ProcessService - cancellation kills descendants', () => { + const TEST_TIMEOUT_MS = 15_000; + const POLL_TIMEOUT_MS = 10_000; + const POLL_INTERVAL_MS = 20; + + let service: ProcessService; + let cts: CancellationTokenSource; + let pidFile: string; + + // Spawns a grandchild that outlives its parent, and records its pid so the test can watch it. + function parentScript(): string { + return [ + `const cp = require('child_process');`, + `const child = cp.spawn(process.execPath, ['-e', 'setTimeout(() => {}, 60000)'], { stdio: 'ignore' });`, + `require('fs').writeFileSync(${JSON.stringify(pidFile)}, String(child.pid));`, + `setTimeout(() => {}, 60000);` + ].join('\n'); + } + + async function poll(condition: () => boolean, description: string): Promise { + const deadline = Date.now() + POLL_TIMEOUT_MS; + while (Date.now() < deadline) { + if (condition()) { + return; + } + await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); + } + + assert.fail(`Timed out waiting for ${description}`); + } + + setup(() => { + service = new ProcessService(); + cts = new CancellationTokenSource(); + pidFile = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'proc-cancel-')), 'grandchild.pid'); + }); + + teardown(() => { + if (fs.existsSync(pidFile)) { + ProcessService.kill(Number(fs.readFileSync(pidFile, 'utf8'))); + fs.rmSync(path.dirname(pidFile), { recursive: true, force: true }); + } + cts.dispose(); + service.dispose(); + }); + + test('cancelling a detached exec kills the grandchild too', async function () { + this.timeout(TEST_TIMEOUT_MS); + + const execution = service.exec(process.execPath, ['-e', parentScript()], { + token: cts.token, + detached: true + }); + + await poll(() => fs.existsSync(pidFile), 'the grandchild to report its pid'); + const grandchildPid = Number(fs.readFileSync(pidFile, 'utf8')); + assert.isTrue(ProcessService.isAlive(grandchildPid), 'grandchild should be running before cancellation'); + + cts.cancel(); + await execution; + + await poll(() => !ProcessService.isAlive(grandchildPid), 'the grandchild to be killed'); + }); + + test('exec still resolves with the output captured before cancellation', async function () { + this.timeout(TEST_TIMEOUT_MS); + + const execution = service.exec(process.execPath, ['-e', `console.log('before'); ${parentScript()}`], { + token: cts.token, + detached: true + }); + + await poll(() => fs.existsSync(pidFile), 'the grandchild to report its pid'); + cts.cancel(); + + const result = await execution; + assert.strictEqual(result.stdout.trim(), 'before'); + }); +}); diff --git a/src/test/mocks/deepnoteRuntimeCore.ts b/src/test/mocks/deepnoteRuntimeCore.ts index 26e25d6797..1032f55c9e 100644 --- a/src/test/mocks/deepnoteRuntimeCore.ts +++ b/src/test/mocks/deepnoteRuntimeCore.ts @@ -27,6 +27,11 @@ function makeFakeProcess(id: number): ChildProcess { } as unknown as ChildProcess; } +/** The real one probes the filesystem; unit tests have no venv on disk, so it always fails there. */ +export const resolvePythonExecutable: RuntimeCore['resolvePythonExecutable'] = async (pythonPath) => { + throw new Error(`No Python executable found under ${pythonPath}`); +}; + export const startServer: RuntimeCore['startServer'] = async (options) => { startServerCalls.push(options);