From 39a1e419bb6462339755bfd8c2c8b6438312c5d0 Mon Sep 17 00:00:00 2001 From: Marko Paleka Date: Tue, 9 Jun 2026 15:09:19 +0200 Subject: [PATCH 1/2] 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 2c7ebdadb..07cd79c75 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 000000000..04c14c009 --- /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/2] 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 07cd79c75..89ead5f3e 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 04c14c009..a11a8ccd2 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(); }