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

* feat: Delete python virtual environment directory when removing deepnote environment

Signed-off-by: Tomas Kislan <tomas@kislan.sk>

* Enhance error logging for virtual environment deletion in DeepnoteEnvironmentManager. Added detailed message output when deletion fails, improving debugging capabilities. Updated unit test comment for clarity.

Signed-off-by: Tomas Kislan <tomas@kislan.sk>

---------

Signed-off-by: Tomas Kislan <tomas@kislan.sk>

Author: tkislan

src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts

@@ -10,6 +10,7 @@ import { logger } from '../../../platform/logging';
 import { IDeepnoteEnvironmentManager, IDeepnoteServerStarter } from '../types';
 import { CreateDeepnoteEnvironmentOptions, DeepnoteEnvironment } from './deepnoteEnvironment';
 import { DeepnoteEnvironmentStorage } from './deepnoteEnvironmentStorage.node';
+import { IFileSystem } from '../../../platform/common/platform/types';
 
 /**
  * Manager for Deepnote kernel environments.
@@ -30,7 +31,8 @@ export class DeepnoteEnvironmentManager implements IExtensionSyncActivationServi
         @inject(IExtensionContext) private readonly context: IExtensionContext,
         @inject(DeepnoteEnvironmentStorage) private readonly storage: DeepnoteEnvironmentStorage,
         @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
     ) {}
 
     /**
@@ -199,6 +201,21 @@ 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);
+            const msg = error instanceof Error ? error.message : String(error);
+            this.outputChannel.appendLine(
+                l10n.t('Failed to delete Deepnote virtual environment directory for "{0}": {1}', config.name, msg)
+            );
+        }
+
+        Cancellation.throwIfCanceled(token);
+
         this.environments.delete(id);
         await this.persistEnvironments();
         this._onDidChangeEnvironments.fire();

src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts

@@ -2,11 +2,14 @@ import { assert, use } from 'chai';
 import chaiAsPromised from 'chai-as-promised';
 import { anything, instance, mock, when, verify } 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 { IDeepnoteServerStarter } from '../types';
 import { PythonEnvironment } from '../../../platform/pythonEnvironments/info';
+import { IFileSystem } from '../../../platform/common/platform/types';
 
 use(chaiAsPromised);
 
@@ -16,6 +19,8 @@ suite('DeepnoteEnvironmentManager', () => {
     let mockStorage: DeepnoteEnvironmentStorage;
     let mockServerStarter: IDeepnoteServerStarter;
     let mockOutputChannel: IOutputChannel;
+    let mockFileSystem: IFileSystem;
+    let testGlobalStoragePath: string;
 
     const testInterpreter: PythonEnvironment = {
         id: 'test-python-id',
@@ -28,18 +33,39 @@ suite('DeepnoteEnvironmentManager', () => {
         mockStorage = mock<DeepnoteEnvironmentStorage>();
         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(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 = [
@@ -235,6 +261,32 @@ 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
+            assert.isFalse(fs.existsSync(venvDirPath), 'Directory should not exist after deletion');
+        });
     });
 
     suite('updateLastUsed', () => {