From 39a1e419bb6462339755bfd8c2c8b6438312c5d0 Mon Sep 17 00:00:00 2001 From: Marko Paleka Date: Tue, 9 Jun 2026 15:09:19 +0200 Subject: [PATCH 1/8] fix: forward cancellation token to toolkit installer subprocesses The pip/venv exec calls in the Deepnote toolkit installer were started without the CancellationToken, so cancellation was only checked between calls. Cancelling during a multi-minute pip install did nothing until the install finished, leaving the Stop button unresponsive. Pass the token into every processService.exec call (and thread it through isToolkitInstalled) so cancelling now terminates the running subprocess immediately. --- .../deepnote/deepnoteToolkitInstaller.node.ts | 29 +++-- .../deepnoteToolkitInstaller.unit.test.ts | 104 ++++++++++++++++++ 2 files changed, 121 insertions(+), 12 deletions(-) create mode 100644 src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts diff --git a/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts b/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts index 2c7ebdadbc..07cd79c758 100644 --- a/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts +++ b/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts @@ -116,7 +116,7 @@ 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}`); @@ -194,7 +194,7 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { const installResult = await venvProcessService.exec( venvInterpreter.uri.fsPath, ['-m', 'pip', 'install', '--upgrade', ...packages], - { throwOnStdErr: false } + { throwOnStdErr: false, token } ); if (installResult.stdout) { @@ -281,7 +281,8 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { // 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 + throwOnStdErr: false, + token }); // Log any stderr output (warnings, etc.) but don't fail on it @@ -348,7 +349,7 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { const pipUpgradeResult = await venvProcessService.exec( venvInterpreter.uri.fsPath, ['-m', 'pip', 'install', '--upgrade', 'pip'], - { throwOnStdErr: false } + { throwOnStdErr: false, token } ); if (pipUpgradeResult.stdout) { @@ -380,7 +381,7 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { 'python-lsp-server[all]', 'deepnote-cli' ], - { throwOnStdErr: false } + { throwOnStdErr: false, token } ); Cancellation.throwIfCanceled(token); @@ -393,7 +394,7 @@ 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'); @@ -422,14 +423,18 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { } } - private async isToolkitInstalled(interpreter: PythonEnvironment): Promise { + private async isToolkitInstalled( + interpreter: PythonEnvironment, + token?: CancellationToken + ): 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 processService.exec( + 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; @@ -508,7 +513,7 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { '--display-name', kernelDisplayName ], - { throwOnStdErr: false } + { 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..04c14c0098 --- /dev/null +++ b/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts @@ -0,0 +1,104 @@ +import { assert } from 'chai'; +import { anything, instance, mock, when } from 'ts-mockito'; +import { CancellationToken, CancellationTokenSource, Uri } from 'vscode'; + +import { DeepnoteToolkitInstaller } from './deepnoteToolkitInstaller.node'; +import { IFileSystem } from '../../platform/common/platform/types'; +import { ExecutionResult, IProcessService, IProcessServiceFactory } from '../../platform/common/process/types.node'; +import { IExtensionContext, IOutputChannel } from '../../platform/common/types'; + +/** + * Regression test for SAL-105: "Hanging kernel can't be cancelled". + * + * Every processService.exec(...) in the toolkit installer must forward the + * CancellationToken it was given. The token is what wires VS Code's Stop / + * Cancel button to ProcessService.kill(pid) (see proc.node.ts), so omitting it + * makes long-running pip installs uninterruptible. + * + * The process layer is hand-rolled (rather than ts-mockito) because the real + * code calls create(undefined) / exec(...) and ts-mockito argument matchers + * behave unreliably for interface mocks here, returning never-resolving stubs. + */ +suite('DeepnoteToolkitInstaller - cancellation token propagation (SAL-105)', () => { + type ExecCall = { file: string; args: string[]; options?: { token?: CancellationToken } }; + + let installer: DeepnoteToolkitInstaller; + let execCalls: ExecCall[]; + let mockOutputChannel: IOutputChannel; + let mockContext: IExtensionContext; + let mockFs: IFileSystem; + + const venvPath = Uri.file('/fake/venv'); + const fakePython = Uri.file('/fake/venv/bin/python'); + + setup(() => { + execCalls = []; + mockOutputChannel = mock(); + mockContext = mock(); + mockFs = mock(); + + when(mockOutputChannel.appendLine(anything())).thenReturn(); + + const fakeProcessService = { + exec: async ( + file: string, + args: string[], + options?: { token?: CancellationToken } + ): Promise> => { + execCalls.push({ file, args, options }); + + return { stdout: '', stderr: '' }; + } + } as unknown as IProcessService; + + const fakeProcessServiceFactory = { + create: async () => fakeProcessService + } as unknown as IProcessServiceFactory; + + installer = new DeepnoteToolkitInstaller( + fakeProcessServiceFactory, + instance(mockOutputChannel), + instance(mockContext), + instance(mockFs) + ); + + // Seed the interpreter cache so getVenvInterpreterByPath() resolves + // without touching the real filesystem / resolvePythonExecutable. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (installer as any).venvPythonPaths.set(venvPath.fsPath, fakePython); + }); + + test('installAdditionalPackages forwards the cancellation token to processService.exec', async () => { + const cts = new CancellationTokenSource(); + + try { + await installer.installAdditionalPackages(venvPath, ['some-package'], cts.token); + + assert.strictEqual(execCalls.length, 1, 'exec should be called exactly once'); + + const call = execCalls[0]; + assert.strictEqual(call.file, fakePython.fsPath); + assert.include(call.args, 'pip', 'should run a pip install'); + assert.isDefined(call.options, 'exec options should be provided'); + assert.strictEqual( + call.options!.token, + cts.token, + 'the cancellation token must be forwarded to exec so Stop can kill the process' + ); + } finally { + cts.dispose(); + } + }); + + test('installAdditionalPackages does not call exec when no packages are requested', async () => { + const cts = new CancellationTokenSource(); + + try { + await installer.installAdditionalPackages(venvPath, [], cts.token); + + assert.strictEqual(execCalls.length, 0, 'exec should not be called for an empty package list'); + } finally { + cts.dispose(); + } + }); +}); From 66a0ded1ae8b96b3fca36b08cbad0f9d1a3e9cf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jurov=C3=BDch?= Date: Wed, 1 Jul 2026 17:25:09 -0700 Subject: [PATCH 2/8] fix: handle cancellation outcomes now that execs are killable ProcessService.exec resolves with partial output when the token kills the process (only shellExec rejects), so forwarding the token exposed several paths that misread a cancelled exec as a domain result: - rethrow CancellationError unwrapped from installVenvAndToolkit's catch so upstream isCancellationError checks suppress the error UI instead of showing an install failure - make isToolkitInstalled cancellation-aware: throw on cancel after the probe exec instead of returning undefined, which misdiagnosed healthy venvs as toolkit-missing and successful installs as failed verification - require the token parameter on isToolkitInstalled so future callers cannot silently reintroduce an uncancellable probe - check for kernel.json rather than the kernelspec directory and re-check the token after the ipykernel exec, so a cancelled install cannot leave a permanently trusted partial kernelspec - re-check the token in installAdditionalPackages before reporting success, and log cancellation instead of a failure message Rewrite the unit test on the repo's ts-mockito pattern (capture/verify, deepStrictEqual per CLAUDE.md) and cover the ensureVenvAndToolkit probe. Co-Authored-By: Claude Fable 5 --- .../deepnote/deepnoteToolkitInstaller.node.ts | 41 ++++++- .../deepnoteToolkitInstaller.unit.test.ts | 101 +++++++++++------- 2 files changed, 98 insertions(+), 44 deletions(-) diff --git a/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts b/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts index 07cd79c758..89ead5f3ee 100644 --- a/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts +++ b/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts @@ -6,7 +6,7 @@ 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'; @@ -125,6 +125,9 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { Cancellation.throwIfCanceled(token); await this.installKernelSpec(existingVenv, venvPath, token); } catch (ex) { + if (isCancellationError(ex as Error)) { + throw ex; + } logger.warn('Failed to ensure kernel spec installed', ex); // Don't fail - continue with existing venv } @@ -197,6 +200,10 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { { throwOnStdErr: false, token } ); + // exec resolves with partial output when the token kills pip, + // so re-check before reporting success + Cancellation.throwIfCanceled(token); + if (installResult.stdout) { this.outputChannel.appendLine(installResult.stdout); } @@ -207,6 +214,11 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { logger.info('Additional packages installed successfully'); this.outputChannel.appendLine(l10n.t('✓ Packages installed successfully')); } catch (ex) { + if (isCancellationError(ex as Error)) { + 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; @@ -311,6 +323,12 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { // Use the shared helper method to install toolkit packages return await this.installToolkitPackages(venvInterpreter, venvPath, token); } catch (ex) { + // Rethrow cancellation unwrapped so upstream isCancellationError checks + // can suppress the error UI instead of reporting an install failure + if (isCancellationError(ex as Error)) { + throw ex; + } + // If this is already a DeepnoteKernelError, rethrow it without wrapping if (ex instanceof DeepnoteVenvCreationError || ex instanceof DeepnoteToolkitInstallError) { throw ex; @@ -403,6 +421,9 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { Cancellation.throwIfCanceled(token); await this.installKernelSpec(venvInterpreter, venvPath, token); } catch (ex) { + if (isCancellationError(ex as Error)) { + throw ex; + } logger.warn('Failed to install kernel spec', ex); // Don't fail the entire installation if kernel spec creation fails } @@ -425,7 +446,7 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { private async isToolkitInstalled( interpreter: PythonEnvironment, - token?: CancellationToken + token: CancellationToken | undefined ): Promise { try { // Use undefined as resource to get full system environment @@ -435,10 +456,16 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { ['-c', 'import deepnote_toolkit; print(deepnote_toolkit.__version__)'], { token } ); + // exec resolves with partial output when the token kills the process, + // so a cancelled probe must not be reported as "toolkit missing" + Cancellation.throwIfCanceled(token); logger.info(`isToolkitInstalled result: ${result.stdout}`); const version = result.stdout.trim(); return version.length > 0 ? version : undefined; } catch (ex) { + if (isCancellationError(ex as Error)) { + throw ex; + } logger.debug('deepnote-toolkit not found', ex); return undefined; } @@ -485,8 +512,10 @@ 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)) { + // Check if kernel spec already exists. Check for kernel.json rather than the + // directory: a cancelled ipykernel install can leave a partially written + // directory, which 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; } @@ -516,6 +545,10 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { { throwOnStdErr: false, token } ); + // exec resolves even when the token killed the process mid-write, + // so re-check before declaring the kernel spec installed + Cancellation.throwIfCanceled(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 index 04c14c0098..a11a8ccd20 100644 --- a/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts +++ b/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts @@ -1,29 +1,24 @@ import { assert } from 'chai'; -import { anything, instance, mock, when } from 'ts-mockito'; -import { CancellationToken, CancellationTokenSource, Uri } from 'vscode'; +import { anything, capture, instance, mock, verify, when } from 'ts-mockito'; +import { CancellationTokenSource, Uri } from 'vscode'; import { DeepnoteToolkitInstaller } from './deepnoteToolkitInstaller.node'; import { IFileSystem } from '../../platform/common/platform/types'; -import { ExecutionResult, IProcessService, IProcessServiceFactory } from '../../platform/common/process/types.node'; +import { IProcessService, IProcessServiceFactory } from '../../platform/common/process/types.node'; import { IExtensionContext, IOutputChannel } from '../../platform/common/types'; /** - * Regression test for SAL-105: "Hanging kernel can't be cancelled". + * Regression tests for SAL-105: "Hanging kernel can't be cancelled". * * Every processService.exec(...) in the toolkit installer must forward the * CancellationToken it was given. The token is what wires VS Code's Stop / * Cancel button to ProcessService.kill(pid) (see proc.node.ts), so omitting it * makes long-running pip installs uninterruptible. - * - * The process layer is hand-rolled (rather than ts-mockito) because the real - * code calls create(undefined) / exec(...) and ts-mockito argument matchers - * behave unreliably for interface mocks here, returning never-resolving stubs. */ suite('DeepnoteToolkitInstaller - cancellation token propagation (SAL-105)', () => { - type ExecCall = { file: string; args: string[]; options?: { token?: CancellationToken } }; - let installer: DeepnoteToolkitInstaller; - let execCalls: ExecCall[]; + let mockProcessService: IProcessService; + let mockProcessServiceFactory: IProcessServiceFactory; let mockOutputChannel: IOutputChannel; let mockContext: IExtensionContext; let mockFs: IFileSystem; @@ -32,31 +27,22 @@ suite('DeepnoteToolkitInstaller - cancellation token propagation (SAL-105)', () const fakePython = Uri.file('/fake/venv/bin/python'); setup(() => { - execCalls = []; + mockProcessService = mock(); + mockProcessServiceFactory = mock(); mockOutputChannel = mock(); mockContext = mock(); mockFs = mock(); - when(mockOutputChannel.appendLine(anything())).thenReturn(); - - const fakeProcessService = { - exec: async ( - file: string, - args: string[], - options?: { token?: CancellationToken } - ): Promise> => { - execCalls.push({ file, args, options }); - - return { stdout: '', stderr: '' }; - } - } as unknown as IProcessService; - - const fakeProcessServiceFactory = { - create: async () => fakeProcessService - } as unknown as IProcessServiceFactory; + 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: '' }); installer = new DeepnoteToolkitInstaller( - fakeProcessServiceFactory, + instance(mockProcessServiceFactory), instance(mockOutputChannel), instance(mockContext), instance(mockFs) @@ -74,15 +60,15 @@ suite('DeepnoteToolkitInstaller - cancellation token propagation (SAL-105)', () try { await installer.installAdditionalPackages(venvPath, ['some-package'], cts.token); - assert.strictEqual(execCalls.length, 1, 'exec should be called exactly once'); - - const call = execCalls[0]; - assert.strictEqual(call.file, fakePython.fsPath); - assert.include(call.args, 'pip', 'should run a pip install'); - assert.isDefined(call.options, 'exec options should be provided'); - assert.strictEqual( - call.options!.token, - 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 } + }, 'the cancellation token must be forwarded to exec so Stop can kill the process' ); } finally { @@ -96,7 +82,42 @@ suite('DeepnoteToolkitInstaller - cancellation token propagation (SAL-105)', () try { await installer.installAdditionalPackages(venvPath, [], cts.token); - assert.strictEqual(execCalls.length, 0, 'exec should not be called for an empty package list'); + verify(mockProcessService.exec(anything(), anything(), anything())).never(); + } finally { + cts.dispose(); + } + }); + + test('ensureVenvAndToolkit forwards the cancellation token to the toolkit version probe', async () => { + const cts = new CancellationTokenSource(); + + try { + when(mockProcessService.exec(anything(), anything(), anything())).thenResolve({ + stdout: '1.2.3\n', + stderr: '' + }); + // Kernel spec already installed, so the fast path runs the probe exec only + when(mockFs.exists(anything())).thenResolve(true); + + const result = await installer.ensureVenvAndToolkit( + { uri: fakePython, id: fakePython.fsPath }, + 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: { token: cts.token } + }, + 'the cancellation token must be forwarded to the isToolkitInstalled probe' + ); } finally { cts.dispose(); } From 0c06a43967a1189fa820bf61b7ef120a35c94a91 Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 11 Aug 2026 08:07:18 +0000 Subject: [PATCH 3/8] refactor: route toolkit installer execs through one cancellable helper Collapse the six processService.exec call sites into a single runPython helper that forwards the token and re-checks it afterwards, so a new exec cannot silently omit either. Removes seven now-redundant throwIfCanceled calls and three restatements of the same rationale comment. Deduplicate the two identical kernel-spec try/catch blocks into tryInstallKernelSpec; mutation testing showed one of the two cancellation guards was covered by no test at all. Add resolvePythonExecutable to the runtime-core test mock. Merging main brought in the ESM loader interception from #429, which lacks that export, so loading the installer aborted the whole unit suite at import time. Extend the cancellation tests from 3 to 9: the existing ones only asserted token forwarding, leaving the cancellation outcomes untested. Each new test was verified to fail when its fix is reverted. Drop the `ex as Error` casts (useUnknownInCatchVariables is off, so they were no-ops) and trim comments that restated the code. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k --- .../deepnote/deepnoteToolkitInstaller.node.ts | 149 +++++----- .../deepnoteToolkitInstaller.unit.test.ts | 279 +++++++++++++----- src/test/mocks/deepnoteRuntimeCore.ts | 5 + 3 files changed, 279 insertions(+), 154 deletions(-) diff --git a/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts b/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts index 89ead5f3ee..a973711201 100644 --- a/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts +++ b/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts @@ -9,7 +9,7 @@ import { resolvePythonExecutable } from '@deepnote/runtime-core'; 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, @@ -120,17 +120,7 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { 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) { - if (isCancellationError(ex as Error)) { - throw 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 }; @@ -191,19 +181,12 @@ 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 } + token ); - // exec resolves with partial output when the token kills pip, - // so re-check before reporting success - Cancellation.throwIfCanceled(token); - if (installResult.stdout) { this.outputChannel.appendLine(installResult.stdout); } @@ -214,11 +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 as Error)) { + 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; @@ -289,21 +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, - token - }); + 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) { @@ -323,9 +300,8 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { // Use the shared helper method to install toolkit packages return await this.installToolkitPackages(venvInterpreter, venvPath, token); } catch (ex) { - // Rethrow cancellation unwrapped so upstream isCancellationError checks - // can suppress the error UI instead of reporting an install failure - if (isCancellationError(ex as Error)) { + // Unwrapped, so the caller's isCancellationError check suppresses the error UI + if (isCancellationError(ex)) { throw ex; } @@ -358,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 } + token ); if (pipUpgradeResult.stdout) { @@ -377,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` @@ -387,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', @@ -399,11 +371,9 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { 'python-lsp-server[all]', 'deepnote-cli' ], - { throwOnStdErr: false, token } + token ); - Cancellation.throwIfCanceled(token); - if (installResult.stdout) { this.outputChannel.appendLine(installResult.stdout); } @@ -417,16 +387,7 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { 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) { - if (isCancellationError(ex as Error)) { - throw 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 }; @@ -449,28 +410,68 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { 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( + const result = await this.runPython( interpreter.uri.fsPath, ['-c', 'import deepnote_toolkit; print(deepnote_toolkit.__version__)'], - { token } + token ); - // exec resolves with partial output when the token kills the process, - // so a cancelled probe must not be reported as "toolkit missing" - Cancellation.throwIfCanceled(token); + logger.info(`isToolkitInstalled result: ${result.stdout}`); const version = result.stdout.trim(); + return version.length > 0 ? version : undefined; } catch (ex) { - if (isCancellationError(ex as Error)) { + 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); + const result = await processService.exec(pythonPath, args, { throwOnStdErr: false, token }); + + 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. @@ -512,24 +513,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. Check for kernel.json rather than the - // directory: a cancelled ipykernel install can leave a partially written - // directory, which must not short-circuit the reinstall. + // 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', @@ -540,15 +533,11 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { '--name', kernelSpecName, '--display-name', - kernelDisplayName + this.getKernelDisplayName(venvPath) ], - { throwOnStdErr: false, token } + token ); - // exec resolves even when the token killed the process mid-write, - // so re-check before declaring the kernel spec installed - Cancellation.throwIfCanceled(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 index a11a8ccd20..e1c4787501 100644 --- a/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts +++ b/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts @@ -1,30 +1,61 @@ import { assert } from 'chai'; import { anything, capture, instance, mock, verify, when } from 'ts-mockito'; -import { CancellationTokenSource, Uri } from 'vscode'; +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 { IProcessService, IProcessServiceFactory } from '../../platform/common/process/types.node'; import { IExtensionContext, IOutputChannel } from '../../platform/common/types'; +import { PythonEnvironment } from '../../platform/pythonEnvironments/info'; /** - * Regression tests for SAL-105: "Hanging kernel can't be cancelled". + * Two invariants, both easy to regress: * - * Every processService.exec(...) in the toolkit installer must forward the - * CancellationToken it was given. The token is what wires VS Code's Stop / - * Cancel button to ProcessService.kill(pid) (see proc.node.ts), so omitting it - * makes long-running pip installs uninterruptible. + * 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 token propagation (SAL-105)', () => { +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; - const venvPath = Uri.file('/fake/venv'); - const fakePython = Uri.file('/fake/venv/bin/python'); + 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(); @@ -32,6 +63,7 @@ suite('DeepnoteToolkitInstaller - cancellation token propagation (SAL-105)', () 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 @@ -40,6 +72,7 @@ suite('DeepnoteToolkitInstaller - cancellation token propagation (SAL-105)', () (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), @@ -47,79 +80,177 @@ suite('DeepnoteToolkitInstaller - cancellation token propagation (SAL-105)', () instance(mockContext), instance(mockFs) ); - - // Seed the interpreter cache so getVenvInterpreterByPath() resolves - // without touching the real filesystem / resolvePythonExecutable. - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (installer as any).venvPythonPaths.set(venvPath.fsPath, fakePython); }); - test('installAdditionalPackages forwards the cancellation token to processService.exec', async () => { - const cts = new CancellationTokenSource(); - - try { - 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 } - }, - 'the cancellation token must be forwarded to exec so Stop can kill the process' - ); - } finally { - cts.dispose(); - } + 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 } + }, + 'the token must reach exec so Stop can kill pip' + ); }); test('installAdditionalPackages does not call exec when no packages are requested', async () => { - const cts = new CancellationTokenSource(); + seedInterpreterCache(venvPath); - try { - await installer.installAdditionalPackages(venvPath, [], cts.token); + await installer.installAdditionalPackages(venvPath, [], cts.token); - verify(mockProcessService.exec(anything(), anything(), anything())).never(); - } finally { - cts.dispose(); - } + 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 () => { - const cts = new CancellationTokenSource(); - - try { - when(mockProcessService.exec(anything(), anything(), anything())).thenResolve({ - stdout: '1.2.3\n', - stderr: '' - }); - // Kernel spec already installed, so the fast path runs the probe exec only - when(mockFs.exists(anything())).thenResolve(true); - - const result = await installer.ensureVenvAndToolkit( - { uri: fakePython, id: fakePython.fsPath }, - 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: { token: cts.token } - }, - 'the cancellation token must be forwarded to the isToolkitInstalled probe' - ); - } finally { - cts.dispose(); - } + 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 } + }, + 'the token must reach the isToolkitInstalled probe' + ); + }); + + test('an existing kernel spec is recognised 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' + ); }); }); 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); From b3e397493c8867a01402307a693a478559c91b0f Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 11 Aug 2026 08:36:02 +0000 Subject: [PATCH 4/8] test: use US spelling in a test name to satisfy cspell cspell.json sets "language": "en", so the en-US dictionary rejected "recognised" in a test title. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k --- src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts b/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts index e1c4787501..d6630d6cd7 100644 --- a/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts +++ b/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts @@ -152,7 +152,7 @@ suite('DeepnoteToolkitInstaller - cancellation', () => { ); }); - test('an existing kernel spec is recognised by kernel.json, not by its directory', async () => { + 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', From 487b98013120662c56cec40d058e8e777597b89b Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 11 Aug 2026 18:14:36 +0000 Subject: [PATCH 5/8] fix: reap the whole subprocess tree when an installer exec is cancelled Forwarding the token made the installer's subprocesses killable, but ProcessService.kill only signals the process we spawned. `python -m venv` runs ensurepip in a subprocess and pip shells out to build backends, so cancelling left survivors writing into the venv that the retry path is busy deleting recursively. exec now opts into its own process group when the caller asks for `detached`, and kills that group instead of the single pid. Windows is unchanged by construction: taskkill /T already reaps the tree, and `detached` there pops a console window. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k --- .../deepnote/deepnoteToolkitInstaller.node.ts | 8 +- .../deepnoteToolkitInstaller.unit.test.ts | 9 +- src/platform/common/process/proc.node.ts | 11 ++- .../common/process/proc.node.unit.test.ts | 95 +++++++++++++++++++ 4 files changed, 117 insertions(+), 6 deletions(-) create mode 100644 src/platform/common/process/proc.node.unit.test.ts diff --git a/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts b/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts index a973711201..abaa15114a 100644 --- a/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts +++ b/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts @@ -445,7 +445,13 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller { // Resource `undefined` gives the full system environment, which pip needs (e.g. git on PATH). const processService = await this.processServiceFactory.create(undefined); - const result = await processService.exec(pythonPath, args, { throwOnStdErr: false, token }); + // 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); diff --git a/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts b/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts index d6630d6cd7..6605306491 100644 --- a/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts +++ b/src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts @@ -96,7 +96,7 @@ suite('DeepnoteToolkitInstaller - cancellation', () => { { file: fakePython.fsPath, args: ['-m', 'pip', 'install', '--upgrade', 'some-package'], - options: { throwOnStdErr: false, token: cts.token } + options: { throwOnStdErr: false, token: cts.token, detached: true } }, 'the token must reach exec so Stop can kill pip' ); @@ -146,7 +146,7 @@ suite('DeepnoteToolkitInstaller - cancellation', () => { { file: fakePython.fsPath, args: ['-c', 'import deepnote_toolkit; print(deepnote_toolkit.__version__)'], - options: { throwOnStdErr: false, token: cts.token } + options: { throwOnStdErr: false, token: cts.token, detached: true } }, 'the token must reach the isToolkitInstalled probe' ); @@ -252,5 +252,10 @@ suite('DeepnoteToolkitInstaller - cancellation', () => { [], '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/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..ea31ed1818 --- /dev/null +++ b/src/platform/common/process/proc.node.unit.test.ts @@ -0,0 +1,95 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import { assert } from 'chai'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from '../../vscode-path/path'; +import { CancellationTokenSource } from 'vscode'; + +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'); + }); +}); From 1d28cb423afa05244e9f6b81a2151b5a8c7e567c Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 11 Aug 2026 18:14:42 +0000 Subject: [PATCH 6/8] fix: stop reporting a cancelled toolkit install as a failure Now that the execs are actually killable, cancelling surfaces immediately instead of after pip has finished anyway, so the unguarded catches around the install and kernel-selection flows raise "Failed to install deepnote-toolkit: Canceled" and "Failed to load Deepnote kernel: Canceled" on every Stop. Neither is a failure the user needs to see. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k --- .../deepnoteKernelAutoSelector.node.ts | 13 +++++ ...epnoteKernelAutoSelector.node.unit.test.ts | 52 ++++++++++++++++++- 2 files changed, 64 insertions(+), 1 deletion(-) 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..89f124d672 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 is killable now, 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(); + }); + }); }); /** From 4491513f5ab3872ccab89d4f4b477104370f0ab7 Mon Sep 17 00:00:00 2001 From: tomas Date: Wed, 12 Aug 2026 06:05:15 +0000 Subject: [PATCH 7/8] style: apply new-file conventions to the proc cancellation test Drop the Microsoft copyright header and move the vscode-path import into the local-import group, matching every other file this branch adds. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k --- src/platform/common/process/proc.node.unit.test.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/platform/common/process/proc.node.unit.test.ts b/src/platform/common/process/proc.node.unit.test.ts index ea31ed1818..4cdc7a8d3e 100644 --- a/src/platform/common/process/proc.node.unit.test.ts +++ b/src/platform/common/process/proc.node.unit.test.ts @@ -1,12 +1,9 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - import { assert } from 'chai'; import * as fs from 'fs'; import * as os from 'os'; -import * as path from '../../vscode-path/path'; import { CancellationTokenSource } from 'vscode'; +import * as path from '../../vscode-path/path'; import { ProcessService } from './proc.node'; /** From e265115fb1f74968e0d235f3d3329e23847ee5d8 Mon Sep 17 00:00:00 2001 From: tomas Date: Wed, 12 Aug 2026 06:10:42 +0000 Subject: [PATCH 8/8] Fix spell check --- cspell.json | 1 + .../deepnote/deepnoteKernelAutoSelector.node.unit.test.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) 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/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts b/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts index 89f124d672..8cb56c4aeb 100644 --- a/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts @@ -1095,7 +1095,7 @@ suite('DeepnoteKernelAutoSelector - rebuildController', () => { }); /** - * Every exec in the installer is killable now, so cancellation surfaces here immediately + * 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. */