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 3 commits into
base: main
Choose a base branch
from
+72 −3
Draft

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aa8334c
feat: Delete python virtual environment directory when removing deepn…
tkislan Oct 31, 2025
b79d0ef
Merge remote-tracking branch 'origin/main' into tk/delete-venv-directory
tkislan Nov 24, 2025
60cab8d
Enhance error logging for virtual environment deletion in DeepnoteEnv…
tkislan Nov 24, 2025
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 208 to 215
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();
});

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');
});
});

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