fix: forward cancellation token to toolkit installer subprocesses - #409
Conversation
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #409 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
|
Warning Review limit reached
Next review available in: 55 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR centralizes Python subprocess execution in Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant KernelSelector
participant ToolkitInstaller
participant ProcessService
User->>KernelSelector: cancel kernel or toolkit operation
KernelSelector->>ToolkitInstaller: start cancellation-aware installation
ToolkitInstaller->>ProcessService: execute detached Python process with token
ProcessService-->>ToolkitInstaller: return cancellation result or error
ToolkitInstaller-->>KernelSelector: propagate CancellationError
KernelSelector-->>User: suppress error notification
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts (1)
57-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated
CancellationTokenSourcecreate/dispose boilerplate.All three tests wrap the body in identical
const cts = new CancellationTokenSource(); try {...} finally { cts.dispose(); }. Could extract a small helper/fixture, but with only 3 tests the payoff is marginal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts` around lines 57 - 124, The three tests in installAdditionalPackages and ensureVenvAndToolkit repeat the same CancellationTokenSource setup/teardown boilerplate, so factor that pattern into a small helper or fixture in deepnoteToolkitInstaller.unit.test.ts. Keep the helper focused on creating the token, running the async test body, and disposing the source afterward, then update the tests to use it while preserving the existing assertions and token forwarding checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts`:
- Around line 57-124: The three tests in installAdditionalPackages and
ensureVenvAndToolkit repeat the same CancellationTokenSource setup/teardown
boilerplate, so factor that pattern into a small helper or fixture in
deepnoteToolkitInstaller.unit.test.ts. Keep the helper focused on creating the
token, running the async test body, and disposing the source afterward, then
update the tests to use it while preserving the existing assertions and token
forwarding checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9686885-8346-41c5-abba-80b1b0da61b5
📒 Files selected for processing (2)
src/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
…-kernel-cant-be-cancelled
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k
cspell.json sets "language": "en", so the en-US dictionary rejected "recognised" in a test title. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k
1d28cb4
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (1)
522-526: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate the cached kernel specification before returning.
If cancellation or a previous installation leaves a truncated or stale
kernel.json, this branch returns success and skips repair. File existence does not prove that the kernel specification is valid or points tovenvInterpreter.Read and validate
kernel.jsonbefore returning. Reinstall it when required fields or the interpreter path are invalid.As per coding guidelines, “Validate cached state before early returns — before returning ‘already configured’, verify the state is still valid.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kernels/deepnote/deepnoteToolkitInstaller.node.ts` around lines 522 - 526, Update the cached-kernel branch around the kernel.json existence check to read and parse the specification before returning. Validate the required fields and ensure its interpreter path points to venvInterpreter; only log and return when valid, otherwise continue through the reinstall flow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/platform/common/process/proc.node.unit.test.ts`:
- Around line 1-10: Update proc.node.unit.test.ts to remove the Microsoft
copyright and MIT license header, and move the ../../vscode-path/path import
into the local-import group after the third-party imports.
---
Outside diff comments:
In `@src/kernels/deepnote/deepnoteToolkitInstaller.node.ts`:
- Around line 522-526: Update the cached-kernel branch around the kernel.json
existence check to read and parse the specification before returning. Validate
the required fields and ensure its interpreter path points to venvInterpreter;
only log and return when valid, otherwise continue through the reinstall flow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c753ed4-f092-4b34-9927-c8f158bdb9c0
📒 Files selected for processing (6)
src/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/deepnoteToolkitInstaller.unit.test.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.tssrc/platform/common/process/proc.node.tssrc/platform/common/process/proc.node.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/kernels/deepnote/deepnoteToolkitInstaller.unit.test.ts
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeHawfLMtvXn252EkCip2k
|
@coderabbitai approve last commit was just a spell check fix |
|
✅ Action performedComments resolved and changes approved. |
4 participants
Problem
The pip/venv
processService.execcalls in the Deepnote toolkit installer were started without theCancellationToken. Cancellation was only checked between calls, so cancelling during a multi-minutepip installdid nothing until the install finished — the Stop button appeared dead.Fix
Pass the token into every
execcall indeepnoteToolkitInstaller.node.ts(and thread it throughisToolkitInstalled). The process layer already wirestoken.onCancellationRequestedto kill the subprocess, so cancelling now terminates the running install immediately.Testing
exectsccleanOpened as draft pending review.
Summary by CodeRabbit
Bug Fixes
Tests