Skip to content

Commit 8b702c7

Browse files
committed
fix: resolve environment switching issues with stale server connections
Fixed three critical bugs that prevented proper environment switching: 1. Controller disposal race condition: Old controller was disposed before the new controller was ready, causing "DISPOSED" errors during cell execution. Fixed by deferring disposal until after new controller is fully registered. 2. Stale configuration caching: Configuration object wasn't refreshed after startServer(), so we connected with outdated serverInfo. Fixed by always calling getEnvironment() after startServer() to get current server info. 3. Environment manager early return: startServer() had an early return when config.serverInfo was set, preventing verification that the server was actually running. This caused connections to wrong/stale servers when switching TO a previously-used environment. Fixed by always calling serverStarter.startServer() (which is idempotent) to ensure current info. Additional improvements: - Made kernel spec installation idempotent and ensured it runs when reusing existing venvs - Removed legacy auto-create fallback path that's no longer needed - Added proper TypeScript non-null assertions after server info validation
1 parent 7b306d1 commit 8b702c7

File tree

4 files changed

+487
-226
lines changed

4 files changed

+487
-226
lines changed

src/kernels/deepnote/deepnoteToolkitInstaller.node.ts

Lines changed: 49 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,17 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
9595
// Check if venv already exists with toolkit installed
9696
const existingVenv = await this.getVenvInterpreterByPath(venvPath);
9797
if (existingVenv && (await this.isToolkitInstalled(existingVenv))) {
98-
logger.info(`deepnote-toolkit venv already exists and is ready at ${venvPath.fsPath}`);
98+
logger.info(`deepnote-toolkit venv already exists at ${venvPath.fsPath}`);
99+
100+
// Ensure kernel spec is installed (may have been deleted or never installed)
101+
try {
102+
await this.installKernelSpec(existingVenv, venvPath);
103+
} catch (ex) {
104+
logger.warn(`Failed to ensure kernel spec installed: ${ex}`);
105+
// Don't fail - continue with existing venv
106+
}
107+
108+
logger.info(`Venv ready at ${venvPath.fsPath}`);
99109
return existingVenv;
100110
}
101111

@@ -288,30 +298,8 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
288298
logger.info('deepnote-toolkit installed successfully in venv');
289299

290300
// Install kernel spec so the kernel uses this venv's Python
291-
// Install into the venv itself (not --user) so the Deepnote server can discover it
292-
logger.info('Installing kernel spec for venv...');
293301
try {
294-
const kernelSpecName = this.getKernelSpecName(venvPath);
295-
const kernelDisplayName = this.getKernelDisplayName(venvPath);
296-
297-
// Reuse the process service with system environment
298-
await venvProcessService.exec(
299-
venvInterpreter.uri.fsPath,
300-
[
301-
'-m',
302-
'ipykernel',
303-
'install',
304-
'--prefix',
305-
venvPath.fsPath,
306-
'--name',
307-
kernelSpecName,
308-
'--display-name',
309-
kernelDisplayName
310-
],
311-
{ throwOnStdErr: false }
312-
);
313-
const kernelSpecPath = Uri.joinPath(venvPath, 'share', 'jupyter', 'kernels', kernelSpecName);
314-
logger.info(`Kernel spec installed successfully to ${kernelSpecPath.fsPath}`);
302+
await this.installKernelSpec(venvInterpreter, venvPath);
315303
} catch (ex) {
316304
logger.warn(`Failed to install kernel spec: ${ex}`);
317305
// Don't fail the entire installation if kernel spec creation fails
@@ -364,6 +352,43 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
364352
return `Deepnote (${venvDirName})`;
365353
}
366354

355+
/**
356+
* Install ipykernel kernel spec for a venv.
357+
* This is idempotent - safe to call multiple times.
358+
*/
359+
private async installKernelSpec(venvInterpreter: PythonEnvironment, venvPath: Uri): Promise<void> {
360+
const kernelSpecName = this.getKernelSpecName(venvPath);
361+
const kernelSpecPath = Uri.joinPath(venvPath, 'share', 'jupyter', 'kernels', kernelSpecName);
362+
363+
// Check if kernel spec already exists
364+
if (await this.fs.exists(kernelSpecPath)) {
365+
logger.info(`Kernel spec already exists at ${kernelSpecPath.fsPath}`);
366+
return;
367+
}
368+
369+
logger.info(`Installing kernel spec '${kernelSpecName}' for venv at ${venvPath.fsPath}...`);
370+
const kernelDisplayName = this.getKernelDisplayName(venvPath);
371+
372+
const venvProcessService = await this.processServiceFactory.create(undefined);
373+
await venvProcessService.exec(
374+
venvInterpreter.uri.fsPath,
375+
[
376+
'-m',
377+
'ipykernel',
378+
'install',
379+
'--prefix',
380+
venvPath.fsPath,
381+
'--name',
382+
kernelSpecName,
383+
'--display-name',
384+
kernelDisplayName
385+
],
386+
{ throwOnStdErr: false }
387+
);
388+
389+
logger.info(`Kernel spec installed successfully to ${kernelSpecPath.fsPath}`);
390+
}
391+
367392
public getVenvHash(deepnoteFileUri: Uri): string {
368393
// Create a short hash from the file path for kernel naming and venv directory
369394
// This provides better uniqueness and prevents directory structure leakage

src/kernels/deepnote/environments/deepnoteEnvironmentManager.ts

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -191,13 +191,8 @@ export class DeepnoteEnvironmentManager implements IExtensionSyncActivationServi
191191
throw new Error(`Environment not found: ${id}`);
192192
}
193193

194-
if (config.serverInfo) {
195-
logger.info(`Server already running for environment: ${config.name} (${id})`);
196-
return;
197-
}
198-
199194
try {
200-
logger.info(`Starting server for environment: ${config.name} (${id})`);
195+
logger.info(`Ensuring server is running for environment: ${config.name} (${id})`);
201196

202197
// First ensure venv is created and toolkit is installed
203198
await this.toolkitInstaller.ensureVenvAndToolkit(config.pythonInterpreter, config.venvPath, undefined);
@@ -207,7 +202,9 @@ export class DeepnoteEnvironmentManager implements IExtensionSyncActivationServi
207202
await this.toolkitInstaller.installAdditionalPackages(config.venvPath, config.packages, undefined);
208203
}
209204

210-
// Start the Jupyter server
205+
// Start the Jupyter server (serverStarter is idempotent - returns existing if running)
206+
// IMPORTANT: Always call this to ensure we get the current server info
207+
// Don't return early based on config.serverInfo - it may be stale!
211208
const serverInfo = await this.serverStarter.startServer(
212209
config.pythonInterpreter,
213210
config.venvPath,
@@ -221,7 +218,7 @@ export class DeepnoteEnvironmentManager implements IExtensionSyncActivationServi
221218
await this.persistEnvironments();
222219
this._onDidChangeEnvironments.fire();
223220

224-
logger.info(`Server started successfully for environment: ${config.name} (${id})`);
221+
logger.info(`Server running for environment: ${config.name} (${id}) at ${serverInfo.url}`);
225222
} catch (error) {
226223
logger.error(`Failed to start server for environment: ${config.name} (${id})`, error);
227224
throw error;

0 commit comments

Comments
 (0)