feat: Delete python virtual environment directory when removing deepnote environment #150
Conversation
…ote environment Signed-off-by: Tomas Kislan <tomas@kislan.sk>
📝 WalkthroughWalkthroughThe DeepnoteEnvironmentManager now deletes virtual environment directories from disk during environment cleanup. An IFileSystem dependency was injected into the class, and the deleteEnvironment method was extended to call fileSystem.delete() on the environment directory after existing cleanup logic, with cancellation checks and logging. Tests were updated to verify disk cleanup behavior. Sequence DiagramsequenceDiagram
participant caller as Caller
participant mgr as DeepnoteEnvironmentManager
participant fs as FileSystem
caller->>mgr: deleteEnvironment(token)
note over mgr: Check cancellation
mgr->>mgr: Cancellation.throwIfCanceled(token)
note over mgr: Existing cleanup logic
mgr->>mgr: persistence.delete()<br/>events fired
note over mgr: NEW: Check cancellation again
mgr->>mgr: Cancellation.throwIfCanceled(token)
rect rgb(200, 230, 201)
note over mgr, fs: NEW: Disk deletion
mgr->>fs: delete(environmentDirectory)
fs-->>mgr: success
mgr->>mgr: log deletion result
end
note over mgr: Catch errors<br/>(log warning, don't fail)
mgr-->>caller: complete
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
=====================================
Coverage 72% 72%
=====================================
Files 568 568
Lines 45226 45256 +30
Branches 5450 5452 +2
=====================================
+ Hits 32827 32855 +28
- Misses 10610 10611 +1
- Partials 1789 1790 +1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts(3 hunks)src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.node.tsfor Desktop-specific implementations that require full file system access and Python environments
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
🧬 Code graph analysis (1)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
🔇 Additional comments (1)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (1)
35-36: IFileSystem injection added correctly.The dependency injection follows coding guidelines by injecting the interface rather than a concrete implementation.
| // Delete the virtual environment directory from disk | ||
| try { | ||
| await this.fileSystem.delete(config.venvPath); | ||
| logger.info(`Deleted virtual environment directory: ${config.venvPath.fsPath}`); | ||
| } catch (error) { | ||
| // Log but don't fail - the directory might not exist or might already be deleted | ||
| logger.warn(`Failed to delete virtual environment directory: ${config.venvPath.fsPath}`, error); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify fileSystem.delete handles recursive deletion.
Virtual environment directories contain nested files and folders. Ensure IFileSystem.delete recursively removes directories by default, or pass explicit options if required.
Run this script to check the IFileSystem interface signature and usage patterns:
🏁 Script executed:
#!/bin/bash
# Check IFileSystem.delete signature and find usage examples
# Find the IFileSystem interface definition
echo "=== IFileSystem interface ==="
ast-grep --pattern $'interface IFileSystem {
$$$
delete($$$) $_
$$$
}'
# Find other delete calls to see if options are passed
echo -e "\n=== Usage examples ==="
rg -nP -A2 'fileSystem\.delete\(' --type=tsLength of output: 450
🏁 Script executed:
#!/bin/bash
# Find where IFileSystem is imported/defined
# Check imports in the file under review
echo "=== IFileSystem import in deepnoteEnvironmentManager ==="
rg -n 'import.*IFileSystem|from.*IFileSystem' src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts
# Search for IFileSystem interface definition
echo -e "\n=== IFileSystem interface definition ==="
rg -nP 'interface\s+IFileSystem|export.*IFileSystem' --type=ts -A 10
# Check type definitions
echo -e "\n=== IFileSystem type/interface ==="
fd -e ts | xargs grep -l 'interface IFileSystem' 2>/dev/null | head -5Length of output: 7386
🏁 Script executed:
#!/bin/bash
# If IFileSystem is from VS Code, check its API
echo "Checking if delete uses recursive flag..."
# Common patterns in file system APIs
echo "- VS Code FileSystem.delete() signature"
echo "- Check if recursive option exists"Length of output: 116
🏁 Script executed:
#!/bin/bash
# Check FileSystem.delete implementation
echo "=== FileSystem delete implementation ==="
rg -nA 5 'delete\s*\(' src/platform/common/platform/fileSystem.ts --type=ts
echo -e "\n=== FileSystemBase delete implementation ==="
rg -nA 5 'delete\s*\(' src/platform/common/platform/fileSystem.ts --type=ts
echo -e "\n=== Check parent class ==="
head -30 src/platform/common/platform/fileSystem.tsLength of output: 1573
🌐 Web query:
VS Code workspace.fs.delete API recursive option default behavior
💡 Result:
Short answer: options.recursive defaults to false — if you omit it, folder deletion is non‑recursive (you must pass { recursive: true } to delete a non‑empty directory). [1][2]
Sources:
- VS Code API / FileSystemProvider (delete signature and options). [1]
- vscode.d.ts showing the FileSystemProvider.delete/watch signatures. [2]
Pass { recursive: true } option to vscode FileSystem API.
VS Code's workspace.fs.delete() defaults to recursive: false. Virtual environment directories contain nested Python files and packages, so deletion at line 198 will fail silently. The catch block logs a warning but doesn't rethrow, leaving the directory on disk and causing a disk space leak.
Update FileSystem.delete() in src/platform/common/platform/fileSystem.ts (line 46) to pass the recursive option:
await this.vscfs.delete(uri, { recursive: true });
Alternatively, expose recursive as an optional parameter in the IFileSystem interface (src/platform/common/platform/types.ts line 42) for callers that need explicit control.
🤖 Prompt for AI Agents
In src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts around
lines 196 to 203, the call to delete the virtual environment directory uses the
FileSystem.delete wrapper which by default calls VS Code's workspace.fs.delete
without recursive: true; update the underlying implementation
(src/platform/common/platform/fileSystem.ts line ~46) so it passes { recursive:
true } to vscfs.delete, or alternatively add an optional recursive parameter to
the IFileSystem.delete signature (src/platform/common/platform/types.ts ~line
42) and propagate that option through the wrapper; then call the delete here
with recursive enabled to ensure nested files are removed.
| } catch (error) { | ||
| // Log but don't fail - the directory might not exist or might already be deleted | ||
| logger.warn(`Failed to delete virtual environment directory: ${config.venvPath.fsPath}`, error); | ||
| } |
There was a problem hiding this comment.
Consider implications of graceful failure.
The catch block logs a warning but doesn't fail, which is reasonable for idempotency. However, if deletion consistently fails (e.g., permissions, locked files), disk space won't be reclaimed while the environment is removed from the manager's state. Consider whether this tradeoff is acceptable or if a more prominent warning to the user is warranted.
🤖 Prompt for AI Agents
In src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts around
lines 200 to 203, the catch currently only logs a warning when venv directory
deletion fails which can hide persistent failures; update the flow so the
manager tracks deletion attempts and does not silently drop the environment
state on repeated failures — on failure increment a retry counter (persisted
in-memory or on the environment record), schedule a backoff retry for deletion,
and if the retry threshold (e.g., 3 attempts) is exceeded emit a higher-severity
log (logger.error) and a user-visible notification/event (or mark the
environment with a "deletion_failed" flag) instead of silently proceeding; keep
the operation non-crashing but ensure the state reflects the failed deletion so
operators can act.
| // Configure mockFileSystem to actually delete directories for testing | ||
| when(mockFileSystem.delete(anything())).thenCall((uri: Uri) => { | ||
| const dirPath = uri.fsPath; | ||
| if (fs.existsSync(dirPath)) { | ||
| fs.rmSync(dirPath, { recursive: true, force: true }); | ||
| } | ||
| return Promise.resolve(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mock performs actual filesystem operations.
This mock executes real filesystem deletion rather than just tracking calls. While this verifies end-to-end behavior, it crosses into integration testing territory. Consider whether this complexity is needed or if verifying the mock was called with the correct Uri is sufficient for a unit test.
| test('should delete virtual environment directory from disk', async () => { | ||
| when(mockStorage.saveEnvironments(anything())).thenResolve(); | ||
|
|
||
| const config = await manager.createEnvironment({ | ||
| name: 'Test', | ||
| pythonInterpreter: testInterpreter | ||
| }); | ||
|
|
||
| // Create the virtual environment directory to simulate it existing | ||
| const venvDirPath = config.venvPath.fsPath; | ||
| fs.mkdirSync(venvDirPath, { recursive: true }); | ||
|
|
||
| // Create a dummy file inside to make it a "real" directory | ||
| fs.writeFileSync(`${venvDirPath}/test.txt`, 'test content'); | ||
|
|
||
| // Verify directory and file exist before deletion | ||
| assert.isTrue(fs.existsSync(venvDirPath), 'Directory should exist before deletion'); | ||
| assert.isTrue(fs.existsSync(`${venvDirPath}/test.txt`), 'File should exist before deletion'); | ||
|
|
||
| // Delete the environment | ||
| await manager.deleteEnvironment(config.id); | ||
|
|
||
| // Verify directory no longer exists (with a small delay to allow async operation) | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
| assert.isFalse(fs.existsSync(venvDirPath), 'Directory should not exist after deletion'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unnecessary setTimeout.
Line 365's 100ms delay is unnecessary. The mock's fs.rmSync is synchronous and wrapped in Promise.resolve(), so await manager.deleteEnvironment(config.id) on line 362 already ensures completion before the assertion.
Apply this diff:
// Delete the environment
await manager.deleteEnvironment(config.id);
- // Verify directory no longer exists (with a small delay to allow async operation)
- await new Promise((resolve) => setTimeout(resolve, 100));
assert.isFalse(fs.existsSync(venvDirPath), 'Directory should not exist after deletion');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should delete virtual environment directory from disk', async () => { | |
| when(mockStorage.saveEnvironments(anything())).thenResolve(); | |
| const config = await manager.createEnvironment({ | |
| name: 'Test', | |
| pythonInterpreter: testInterpreter | |
| }); | |
| // Create the virtual environment directory to simulate it existing | |
| const venvDirPath = config.venvPath.fsPath; | |
| fs.mkdirSync(venvDirPath, { recursive: true }); | |
| // Create a dummy file inside to make it a "real" directory | |
| fs.writeFileSync(`${venvDirPath}/test.txt`, 'test content'); | |
| // Verify directory and file exist before deletion | |
| assert.isTrue(fs.existsSync(venvDirPath), 'Directory should exist before deletion'); | |
| assert.isTrue(fs.existsSync(`${venvDirPath}/test.txt`), 'File should exist before deletion'); | |
| // Delete the environment | |
| await manager.deleteEnvironment(config.id); | |
| // Verify directory no longer exists (with a small delay to allow async operation) | |
| await new Promise((resolve) => setTimeout(resolve, 100)); | |
| assert.isFalse(fs.existsSync(venvDirPath), 'Directory should not exist after deletion'); | |
| }); | |
| test('should delete virtual environment directory from disk', async () => { | |
| when(mockStorage.saveEnvironments(anything())).thenResolve(); | |
| const config = await manager.createEnvironment({ | |
| name: 'Test', | |
| pythonInterpreter: testInterpreter | |
| }); | |
| // Create the virtual environment directory to simulate it existing | |
| const venvDirPath = config.venvPath.fsPath; | |
| fs.mkdirSync(venvDirPath, { recursive: true }); | |
| // Create a dummy file inside to make it a "real" directory | |
| fs.writeFileSync(`${venvDirPath}/test.txt`, 'test content'); | |
| // Verify directory and file exist before deletion | |
| assert.isTrue(fs.existsSync(venvDirPath), 'Directory should exist before deletion'); | |
| assert.isTrue(fs.existsSync(`${venvDirPath}/test.txt`), 'File should exist before deletion'); | |
| // Delete the environment | |
| await manager.deleteEnvironment(config.id); | |
| assert.isFalse(fs.existsSync(venvDirPath), 'Directory should not exist after deletion'); | |
| }); |
🤖 Prompt for AI Agents
In src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
around lines 342 to 367, the test includes an unnecessary await new
Promise(resolve => setTimeout(resolve, 100)) after calling await
manager.deleteEnvironment(config.id); remove that setTimeout and its comment so
the test directly asserts fs.existsSync(venvDirPath) is false immediately after
the awaited deleteEnvironment call, since the mocked fs.rmSync is synchronous
and the awaited call already guarantees completion.
Signed-off-by: Tomas Kislan tomas@kislan.sk
Fixes #
Summary by CodeRabbit
Release Notes