Skip to content

feat: Delete python virtual environment directory when removing deepnote environment #150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: Delete python virtual environment directory when removing deepnote environment #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { generateUuid as uuid } from '../../../platform/common/uuid';
import { IExtensionContext, IOutputChannel } from '../../../platform/common/types';
import { IExtensionSyncActivationService } from '../../../platform/activation/types';
import { logger } from '../../../platform/logging';
import { IFileSystem } from '../../../platform/common/platform/types';
import { DeepnoteEnvironmentStorage } from './deepnoteEnvironmentStorage.node';
import {
CreateDeepnoteEnvironmentOptions,
Expand Down Expand Up @@ -31,7 +32,8 @@ export class DeepnoteEnvironmentManager implements IExtensionSyncActivationServi
@inject(DeepnoteEnvironmentStorage) private readonly storage: DeepnoteEnvironmentStorage,
@inject(IDeepnoteToolkitInstaller) private readonly toolkitInstaller: IDeepnoteToolkitInstaller,
@inject(IDeepnoteServerStarter) private readonly serverStarter: IDeepnoteServerStarter,
@inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly outputChannel: IOutputChannel
@inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly outputChannel: IOutputChannel,
@inject(IFileSystem) private readonly fileSystem: IFileSystem
) {}

/**
Expand Down Expand Up @@ -191,6 +193,17 @@ export class DeepnoteEnvironmentManager implements IExtensionSyncActivationServi

Cancellation.throwIfCanceled(token);

// 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);
}
Comment on lines +196 to +203
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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=ts

Length 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 -5

Length 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.ts

Length 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.

Comment on lines +200 to +203
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


Cancellation.throwIfCanceled(token);

this.environments.delete(id);
await this.persistEnvironments();
this._onDidChangeEnvironments.fire();
Expand Down
57 changes: 55 additions & 2 deletions src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ import { assert, use } from 'chai';
import chaiAsPromised from 'chai-as-promised';
import { anything, instance, mock, when, verify, deepEqual } from 'ts-mockito';
import { Uri } from 'vscode';
import * as fs from 'fs';
import * as os from 'os';
import { DeepnoteEnvironmentManager } from './deepnoteEnvironmentManager.node';
import { DeepnoteEnvironmentStorage } from './deepnoteEnvironmentStorage.node';
import { IExtensionContext, IOutputChannel } from '../../../platform/common/types';
import { IFileSystem } from '../../../platform/common/platform/types';
import {
IDeepnoteServerStarter,
IDeepnoteToolkitInstaller,
Expand All @@ -24,6 +27,8 @@ suite('DeepnoteEnvironmentManager', () => {
let mockToolkitInstaller: IDeepnoteToolkitInstaller;
let mockServerStarter: IDeepnoteServerStarter;
let mockOutputChannel: IOutputChannel;
let mockFileSystem: IFileSystem;
let testGlobalStoragePath: string;

const testInterpreter: PythonEnvironment = {
id: 'test-python-id',
Expand All @@ -49,19 +54,40 @@ suite('DeepnoteEnvironmentManager', () => {
mockToolkitInstaller = mock<IDeepnoteToolkitInstaller>();
mockServerStarter = mock<IDeepnoteServerStarter>();
mockOutputChannel = mock<IOutputChannel>();
mockFileSystem = mock<IFileSystem>();

when(mockContext.globalStorageUri).thenReturn(Uri.file('/global/storage'));
// Create a temporary directory for test storage
testGlobalStoragePath = fs.mkdtempSync(`${os.tmpdir()}/deepnote-test-`);

when(mockContext.globalStorageUri).thenReturn(Uri.file(testGlobalStoragePath));
when(mockStorage.loadEnvironments()).thenResolve([]);

// 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();
});
Comment on lines +65 to +72
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


manager = new DeepnoteEnvironmentManager(
instance(mockContext),
instance(mockStorage),
instance(mockToolkitInstaller),
instance(mockServerStarter),
instance(mockOutputChannel)
instance(mockOutputChannel),
instance(mockFileSystem)
);
});

teardown(() => {
// Clean up the temporary directory after each test
if (testGlobalStoragePath && fs.existsSync(testGlobalStoragePath)) {
fs.rmSync(testGlobalStoragePath, { recursive: true, force: true });
}
});

suite('activate', () => {
test('should load environments on activation', async () => {
const existingConfigs = [
Expand Down Expand Up @@ -312,6 +338,33 @@ suite('DeepnoteEnvironmentManager', () => {
test('should throw error for non-existent environment', async () => {
await assert.isRejected(manager.deleteEnvironment('non-existent'), 'Environment not found: non-existent');
});

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');
});
Comment on lines +342 to +367
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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.

});

suite('startServer', () => {
Expand Down