From f01b5728b5fe57ba0a95ec5998f36e1bdafac852 Mon Sep 17 00:00:00 2001 From: Lukas Saltenas Date: Wed, 5 Nov 2025 16:59:48 +0200 Subject: [PATCH 1/6] fix(deepnote-server): ensure consecutive port allocation skips bound ports --- .../deepnote/deepnoteServerStarter.node.ts | 107 ++++-- .../deepnoteServerStarter.unit.test.ts | 359 ++++++++++++++++++ 2 files changed, 442 insertions(+), 24 deletions(-) create mode 100644 src/kernels/deepnote/deepnoteServerStarter.unit.test.ts diff --git a/src/kernels/deepnote/deepnoteServerStarter.node.ts b/src/kernels/deepnote/deepnoteServerStarter.node.ts index 8c97dc84d3..effdc7f47a 100644 --- a/src/kernels/deepnote/deepnoteServerStarter.node.ts +++ b/src/kernels/deepnote/deepnoteServerStarter.node.ts @@ -2,7 +2,6 @@ // Licensed under the MIT License. import * as fs from 'fs-extra'; -import getPort from 'get-port'; import { inject, injectable, named, optional } from 'inversify'; import * as os from 'os'; import { CancellationToken, l10n, Uri } from 'vscode'; @@ -19,6 +18,7 @@ import { ISqlIntegrationEnvVarsProvider } from '../../platform/notebooks/deepnot import { PythonEnvironment } from '../../platform/pythonEnvironments/info'; import * as path from '../../platform/vscode-path/path'; import { DEEPNOTE_DEFAULT_PORT, DeepnoteServerInfo, IDeepnoteServerStarter, IDeepnoteToolkitInstaller } from './types'; +import tcpPortUsed from 'tcp-port-used'; /** * Lock file data structure for tracking server ownership @@ -520,13 +520,11 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension } } - // Allocate Jupyter port first - const jupyterPort = await this.findAvailablePort(DEEPNOTE_DEFAULT_PORT, portsInUse); - portsInUse.add(jupyterPort); // Reserve it immediately - - // Allocate LSP port (starting from jupyterPort + 1 to avoid conflicts) - const lspPort = await this.findAvailablePort(jupyterPort + 1, portsInUse); - portsInUse.add(lspPort); // Reserve it immediately + // Find a pair of consecutive available ports + const { jupyterPort, lspPort } = await this.findConsecutiveAvailablePorts( + DEEPNOTE_DEFAULT_PORT, + portsInUse + ); // Reserve both ports by adding to serverInfos // This prevents other concurrent allocations from getting the same ports @@ -538,7 +536,7 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension this.serverInfos.set(key, serverInfo); logger.info( - `Allocated ports for ${key}: jupyter=${jupyterPort}, lsp=${lspPort} (excluded: ${ + `Allocated consecutive ports for ${key}: jupyter=${jupyterPort}, lsp=${lspPort} (excluded: ${ portsInUse.size > 2 ? Array.from(portsInUse) .filter((p) => p !== jupyterPort && p !== lspPort) @@ -554,9 +552,81 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension } } + /** + * Find a pair of consecutive available ports (port and port+1). + * This is critical for the deepnote-toolkit server which expects consecutive ports. + * + * @param startPort The port number to start searching from + * @param portsInUse Set of ports already allocated to other servers + * @returns A pair of consecutive ports { jupyterPort, lspPort } where lspPort = jupyterPort + 1 + * @throws DeepnoteServerStartupError if no consecutive ports can be found after maxAttempts + */ + private async findConsecutiveAvailablePorts( + startPort: number, + portsInUse: Set + ): Promise<{ jupyterPort: number; lspPort: number }> { + const maxAttempts = 100; + + for (let attempt = 0; attempt < maxAttempts; attempt++) { + // Try to find an available Jupyter port + const candidatePort = await this.findAvailablePort( + attempt === 0 ? startPort : startPort + attempt, + portsInUse + ); + + const nextPort = candidatePort + 1; + + // Check if the consecutive port (candidatePort + 1) is also available + const isNextPortInUse = portsInUse.has(nextPort); + const isNextPortAvailable = !isNextPortInUse && (await this.isPortAvailable(nextPort)); + + if (isNextPortAvailable) { + // Found a consecutive pair! + return { jupyterPort: candidatePort, lspPort: nextPort }; + } + + // Consecutive port not available - mark both as unavailable and try next + portsInUse.add(candidatePort); + portsInUse.add(nextPort); + } + + // Failed to find consecutive ports after max attempts + throw new DeepnoteServerStartupError( + 'python', + startPort, + 'process_failed', + '', + l10n.t( + 'Failed to find consecutive available ports after {0} attempts starting from port {1}. Please close some applications using network ports and try again.', + maxAttempts, + startPort + ) + ); + } + + /** + * Check if a specific port is available on the system by actually trying to bind to it. + * This is more reliable than get-port which doesn't test the exact port. + */ + private async isPortAvailable(port: number): Promise { + try { + const inUse = await tcpPortUsed.check(port, '127.0.0.1'); + if (inUse) { + return false; + } + + // Also check IPv6 loopback to be safe + const inUseIpv6 = await tcpPortUsed.check(port, '::1'); + return !inUseIpv6; + } catch (error) { + logger.warn(`Failed to check port availability for ${port}:`, error); + return false; + } + } + /** * Find an available port starting from the given port number. - * Checks both our internal portsInUse set and system availability. + * Checks both our internal portsInUse set and system availability by actually binding to test. */ private async findAvailablePort(startPort: number, portsInUse: Set): Promise { let port = startPort; @@ -566,23 +636,12 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension while (attempts < maxAttempts) { // Skip ports already in use by our servers if (!portsInUse.has(port)) { - // Check if this port is available on the system - const availablePort = await getPort({ - host: 'localhost', - port - }); + // Check if this port is actually available on the system by binding to it + const available = await this.isPortAvailable(port); - // If get-port returned the same port, it's available - if (availablePort === port) { + if (available) { return port; } - - // get-port returned a different port - check if that one is in use - if (!portsInUse.has(availablePort)) { - return availablePort; - } - - // Both our requested port and get-port's suggestion are in use, try next } // Try next port diff --git a/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts b/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts new file mode 100644 index 0000000000..5ab9570a58 --- /dev/null +++ b/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts @@ -0,0 +1,359 @@ +import { assert } from 'chai'; +import { anything, instance, mock, when } from 'ts-mockito'; +import { DeepnoteServerStarter } from './deepnoteServerStarter.node'; +import { IProcessServiceFactory } from '../../platform/common/process/types.node'; +import { IAsyncDisposableRegistry, IHttpClient, IOutputChannel } from '../../platform/common/types'; +import { IDeepnoteToolkitInstaller } from './types'; +import { ISqlIntegrationEnvVarsProvider } from '../../platform/notebooks/deepnote/types'; + +/** + * Integration tests for DeepnoteServerStarter port allocation logic. + * These tests use real port checking to ensure consecutive ports are allocated. + * + * Note: These are integration tests that actually check port availability on the system. + * They test the critical fix where consecutive ports must be available. + */ +suite('DeepnoteServerStarter - Port Allocation Integration Tests', () => { + let serverStarter: DeepnoteServerStarter; + let mockProcessServiceFactory: IProcessServiceFactory; + let mockToolkitInstaller: IDeepnoteToolkitInstaller; + let mockOutputChannel: IOutputChannel; + let mockHttpClient: IHttpClient; + let mockAsyncRegistry: IAsyncDisposableRegistry; + let mockSqlIntegrationEnvVars: ISqlIntegrationEnvVarsProvider; + + // Helper to access private methods for testing + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const getPrivateMethod = (obj: any, methodName: string) => { + return obj[methodName].bind(obj); + }; + + setup(() => { + // Create mocks + mockProcessServiceFactory = mock(); + mockToolkitInstaller = mock(); + mockOutputChannel = mock(); + mockHttpClient = mock(); + mockAsyncRegistry = mock(); + mockSqlIntegrationEnvVars = mock(); + + when(mockAsyncRegistry.push(anything())).thenReturn(); + when(mockOutputChannel.appendLine(anything())).thenReturn(); + + serverStarter = new DeepnoteServerStarter( + instance(mockProcessServiceFactory), + instance(mockToolkitInstaller), + instance(mockOutputChannel), + instance(mockHttpClient), + instance(mockAsyncRegistry), + instance(mockSqlIntegrationEnvVars) + ); + }); + + suite('isPortAvailable', () => { + test('should return true when port is available', async () => { + // Use a high unlikely-to-be-used port + const port = 54321; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const isPortAvailable = getPrivateMethod(serverStarter as any, 'isPortAvailable'); + const result = await isPortAvailable(port); + + // Port should be available (or test might be flaky if something uses this port) + assert.isTrue(result, 'Port 54321 should be available'); + }); + + test('CRITICAL: should return false when port is actually in use', async () => { + // This is the test that would have caught the bug! + // We actually bind to a port and verify isPortAvailable detects it + const port = 54323; + const net = require('net'); + const server = net.createServer(); + + // Bind to the port + await new Promise((resolve) => { + server.listen(port, 'localhost', () => { + resolve(); + }); + }); + + try { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const isPortAvailable = getPrivateMethod(serverStarter as any, 'isPortAvailable'); + const result = await isPortAvailable(port); + + // CRITICAL: Should return false because we're holding the port + assert.isFalse(result, 'Port should be detected as in use when actually bound'); + } finally { + // Clean up: close the server + await new Promise((resolve) => { + server.close(() => resolve()); + }); + } + }); + + test('should detect when checking port availability', async () => { + // This tests that isPortAvailable returns a boolean + const port = 54322; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const isPortAvailable = getPrivateMethod(serverStarter as any, 'isPortAvailable'); + const result = await isPortAvailable(port); + + // Result should be a boolean + assert.isBoolean(result); + }); + }); + + suite('findAvailablePort', () => { + test('should find an available port starting from given port', async () => { + const portsInUse = new Set(); + const startPort = 54400; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const findAvailablePort = getPrivateMethod(serverStarter as any, 'findAvailablePort'); + const result = await findAvailablePort(startPort, portsInUse); + + // Should find a port at or after the start port + assert.isAtLeast(result, startPort); + }); + + test('should skip ports in portsInUse set', async () => { + const portsInUse = new Set([54500, 54501, 54502]); + const startPort = 54500; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const findAvailablePort = getPrivateMethod(serverStarter as any, 'findAvailablePort'); + const result = await findAvailablePort(startPort, portsInUse); + + // Should skip the ports in use + assert.isFalse(portsInUse.has(result), 'Should not return a port from portsInUse'); + assert.isAtLeast(result, 54503); + }); + + test('should find available port within reasonable attempts', async () => { + const portsInUse = new Set(); + const startPort = 54600; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const findAvailablePort = getPrivateMethod(serverStarter as any, 'findAvailablePort'); + const result = await findAvailablePort(startPort, portsInUse); + + // Should find a port without error + assert.isNumber(result); + assert.isAtLeast(result, startPort); + }); + }); + + suite('allocatePorts - Consecutive Port Allocation (Critical Bug Fix)', () => { + test('should allocate consecutive ports (LSP = Jupyter + 1)', async () => { + const key = 'test-consecutive-1'; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const allocatePorts = getPrivateMethod(serverStarter as any, 'allocatePorts'); + const result = await allocatePorts(key); + + // THIS IS THE CRITICAL ASSERTION: LSP port must be exactly Jupyter + 1 + assert.strictEqual( + result.lspPort, + result.jupyterPort + 1, + 'LSP port must be consecutive (Jupyter port + 1)' + ); + }); + + test('should allocate different consecutive port pairs for multiple servers', async () => { + const key1 = 'test-server-1'; + const key2 = 'test-server-2'; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const allocatePorts = getPrivateMethod(serverStarter as any, 'allocatePorts'); + + const result1 = await allocatePorts(key1); + const result2 = await allocatePorts(key2); + + // Both should have consecutive ports + assert.strictEqual(result1.lspPort, result1.jupyterPort + 1); + assert.strictEqual(result2.lspPort, result2.jupyterPort + 1); + + // Ports should not overlap + assert.notEqual(result1.jupyterPort, result2.jupyterPort); + assert.notEqual(result1.lspPort, result2.lspPort); + assert.notEqual(result1.jupyterPort, result2.lspPort); + assert.notEqual(result1.lspPort, result2.jupyterPort); + }); + + test('CRITICAL REGRESSION TEST: should skip non-consecutive ports when LSP port is taken', async () => { + // This test simulates the EXACT bug scenario that was reported: + // - Port 8888 is available + // - Port 8889 (8888+1) is TAKEN by another process + // - System should NOT allocate 8888+8890 (non-consecutive) + // - System SHOULD find a different consecutive pair like 8890+8891 + + const net = require('net'); + const blockingServer = net.createServer(); + const blockedPort = 54701; // We'll block this port to simulate 8889 being taken + + // Bind to port 54701 to block it + await new Promise((resolve) => { + blockingServer.listen(blockedPort, 'localhost', () => { + resolve(); + }); + }); + + try { + const key = 'test-blocked-lsp-port'; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const allocatePorts = getPrivateMethod(serverStarter as any, 'allocatePorts'); + + // Try to allocate ports - it should skip 54700 because 54701 is taken + const result = await allocatePorts(key); + + // CRITICAL: Ports must be consecutive + assert.strictEqual( + result.lspPort, + result.jupyterPort + 1, + 'Even when some ports are blocked, allocated ports MUST be consecutive' + ); + + // Should not have allocated the blocked port or its predecessor + assert.notEqual(result.jupyterPort, blockedPort); + assert.notEqual(result.lspPort, blockedPort); + assert.isFalse( + result.jupyterPort === blockedPort - 1 && result.lspPort === blockedPort, + 'Should not allocate pair where second port is blocked' + ); + } finally { + // Clean up: close the blocking server + await new Promise((resolve) => { + blockingServer.close(() => resolve()); + }); + } + }); + + test('should handle rapid sequential allocations', async () => { + const keys = ['seq-1', 'seq-2', 'seq-3', 'seq-4', 'seq-5']; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const allocatePorts = getPrivateMethod(serverStarter as any, 'allocatePorts'); + + const results = []; + for (const key of keys) { + const result = await allocatePorts(key); + results.push(result); + } + + // All should have unique, consecutive port pairs + const allPorts = results.flatMap((r) => [r.jupyterPort, r.lspPort]); + const uniquePorts = new Set(allPorts); + assert.strictEqual(uniquePorts.size, results.length * 2, 'All ports should be unique'); + + // Each result should have consecutive ports + for (const result of results) { + assert.strictEqual(result.lspPort, result.jupyterPort + 1); + } + }); + + test('should update serverInfos map with allocated ports', async () => { + const key = 'test-server-info'; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const allocatePorts = getPrivateMethod(serverStarter as any, 'allocatePorts'); + const result = await allocatePorts(key); + + // Check that serverInfos was updated + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const serverInfos = (serverStarter as any).serverInfos as Map; + assert.isTrue(serverInfos.has(key)); + + const info = serverInfos.get(key); + assert.strictEqual(info.jupyterPort, result.jupyterPort); + assert.strictEqual(info.lspPort, result.lspPort); + assert.strictEqual(info.url, `http://localhost:${result.jupyterPort}`); + }); + + test('should respect already allocated ports', async () => { + // First allocation + const key1 = 'first-server'; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const allocatePorts = getPrivateMethod(serverStarter as any, 'allocatePorts'); + const result1 = await allocatePorts(key1); + + // Second allocation should get different ports + const key2 = 'second-server'; + const result2 = await allocatePorts(key2); + + // Verify no overlap + const ports1 = new Set([result1.jupyterPort, result1.lspPort]); + assert.isFalse(ports1.has(result2.jupyterPort), 'Second Jupyter port should not overlap'); + assert.isFalse(ports1.has(result2.lspPort), 'Second LSP port should not overlap'); + }); + }); + + suite('Port Allocation Edge Cases', () => { + test('should allocate ports successfully even after multiple allocations', async () => { + // Allocate many port pairs to test robustness + const count = 10; + const keys = Array.from({ length: count }, (_, i) => `stress-test-${i}`); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const allocatePorts = getPrivateMethod(serverStarter as any, 'allocatePorts'); + + const results = []; + for (const key of keys) { + const result = await allocatePorts(key); + results.push(result); + } + + // All should be successful and consecutive + assert.strictEqual(results.length, count); + for (const result of results) { + assert.strictEqual(result.lspPort, result.jupyterPort + 1); + } + + // All ports should be unique + const allPorts = results.flatMap((r) => [r.jupyterPort, r.lspPort]); + const uniquePorts = new Set(allPorts); + assert.strictEqual(uniquePorts.size, count * 2); + }); + + test('should return valid port numbers', async () => { + const key = 'valid-ports'; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const allocatePorts = getPrivateMethod(serverStarter as any, 'allocatePorts'); + const result = await allocatePorts(key); + + // Ports should be in valid range + assert.isAtLeast(result.jupyterPort, 1024, 'Port should be above well-known ports'); + assert.isBelow(result.jupyterPort, 65536, 'Port should be below max port number'); + assert.isAtLeast(result.lspPort, 1024); + assert.isBelow(result.lspPort, 65536); + }); + }); + + suite('Critical Bug Fix Verification', () => { + test('REGRESSION TEST: should never allocate non-consecutive ports', async () => { + // This is the critical regression test for the bug where + // if Jupyter port was available but LSP port (Jupyter+1) was not, + // the system would allocate non-consecutive ports causing server hangs + + const keys = ['regression-1', 'regression-2', 'regression-3']; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const allocatePorts = getPrivateMethod(serverStarter as any, 'allocatePorts'); + + const results = await Promise.all(keys.map((key) => allocatePorts(key))); + + for (let i = 0; i < results.length; i++) { + const result = results[i]; + assert.strictEqual( + result.lspPort, + result.jupyterPort + 1, + `Server ${i + 1} (${keys[i]}): LSP port MUST be Jupyter port + 1. ` + + `This prevents server startup hangs when toolkit expects consecutive ports.` + ); + } + }); + }); +}); From d11e00e90318efcb0a05bed26371fa723216b961 Mon Sep 17 00:00:00 2001 From: Lukas Saltenas Date: Wed, 5 Nov 2025 17:18:58 +0200 Subject: [PATCH 2/6] fix: orphan process kill logic is unnecessary --- src/kernels/deepnote/deepnoteServerStarter.node.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/kernels/deepnote/deepnoteServerStarter.node.ts b/src/kernels/deepnote/deepnoteServerStarter.node.ts index effdc7f47a..acd2d2845f 100644 --- a/src/kernels/deepnote/deepnoteServerStarter.node.ts +++ b/src/kernels/deepnote/deepnoteServerStarter.node.ts @@ -579,6 +579,9 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension // Check if the consecutive port (candidatePort + 1) is also available const isNextPortInUse = portsInUse.has(nextPort); const isNextPortAvailable = !isNextPortInUse && (await this.isPortAvailable(nextPort)); + logger.info( + `Consecutive port check for base ${candidatePort}: next=${nextPort}, inUseSet=${isNextPortInUse}, available=${isNextPortAvailable}` + ); if (isNextPortAvailable) { // Found a consecutive pair! @@ -979,14 +982,8 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension pidsToSkip.push({ pid, reason: 'belongs to current session' }); } } else { - // No lock file - check if orphaned before killing - const isOrphaned = await this.isProcessOrphaned(pid); - if (isOrphaned) { - logger.info(`PID ${pid} has no lock file and is orphaned - will kill`); - pidsToKill.push(pid); - } else { - pidsToSkip.push({ pid, reason: 'no lock file but has active parent process' }); - } + // No lock file - assume it's an external/non-managed process and skip it + pidsToSkip.push({ pid, reason: 'no lock file (assuming external process)' }); } } From 868d109f29842fc0c11801f46d82b3252a4ba5f7 Mon Sep 17 00:00:00 2001 From: Lukas Saltenas Date: Wed, 5 Nov 2025 17:28:15 +0200 Subject: [PATCH 3/6] chore: improve tests --- .../deepnoteServerStarter.unit.test.ts | 72 ++++++++++--------- 1 file changed, 39 insertions(+), 33 deletions(-) diff --git a/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts b/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts index 5ab9570a58..ae5d82d6f5 100644 --- a/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts +++ b/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts @@ -1,10 +1,13 @@ import { assert } from 'chai'; +import * as sinon from 'sinon'; +import tcpPortUsed from 'tcp-port-used'; import { anything, instance, mock, when } from 'ts-mockito'; import { DeepnoteServerStarter } from './deepnoteServerStarter.node'; import { IProcessServiceFactory } from '../../platform/common/process/types.node'; import { IAsyncDisposableRegistry, IHttpClient, IOutputChannel } from '../../platform/common/types'; import { IDeepnoteToolkitInstaller } from './types'; import { ISqlIntegrationEnvVarsProvider } from '../../platform/notebooks/deepnote/types'; +import { logger } from '../../platform/logging'; /** * Integration tests for DeepnoteServerStarter port allocation logic. @@ -51,58 +54,61 @@ suite('DeepnoteServerStarter - Port Allocation Integration Tests', () => { }); suite('isPortAvailable', () => { - test('should return true when port is available', async () => { - // Use a high unlikely-to-be-used port + let checkStub: sinon.SinonStub; + + setup(() => { + checkStub = sinon.stub(tcpPortUsed, 'check'); + }); + + teardown(() => { + checkStub.restore(); + }); + + test('should return true when both IPv4 and IPv6 loopbacks are free', async () => { const port = 54321; + checkStub.onFirstCall().resolves(false); // IPv4 + checkStub.onSecondCall().resolves(false); // IPv6 + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const isPortAvailable = getPrivateMethod(serverStarter as any, 'isPortAvailable'); + const result = await isPortAvailable(port); + + assert.isTrue(result, 'Expected port to be reported as available'); + assert.strictEqual(checkStub.callCount, 2, 'Should check both IPv4 and IPv6 loopbacks'); + assert.deepEqual(checkStub.getCall(0).args, [port, '127.0.0.1']); + assert.deepEqual(checkStub.getCall(1).args, [port, '::1']); + }); + + test('should return false when IPv4 loopback is already in use', async () => { + const port = 54322; + checkStub.onFirstCall().resolves(true); // eslint-disable-next-line @typescript-eslint/no-explicit-any const isPortAvailable = getPrivateMethod(serverStarter as any, 'isPortAvailable'); const result = await isPortAvailable(port); - // Port should be available (or test might be flaky if something uses this port) - assert.isTrue(result, 'Port 54321 should be available'); + assert.isFalse(result, 'Expected port to be reported as in use'); + assert.strictEqual(checkStub.callCount, 1, 'IPv6 check should be skipped when IPv4 is busy'); }); - test('CRITICAL: should return false when port is actually in use', async () => { - // This is the test that would have caught the bug! - // We actually bind to a port and verify isPortAvailable detects it + test('should return false and log when port checks throw', async () => { const port = 54323; - const net = require('net'); - const server = net.createServer(); + const error = new Error('check failed'); + checkStub.rejects(error); - // Bind to the port - await new Promise((resolve) => { - server.listen(port, 'localhost', () => { - resolve(); - }); - }); + const warnStub = sinon.stub(logger, 'warn'); try { // eslint-disable-next-line @typescript-eslint/no-explicit-any const isPortAvailable = getPrivateMethod(serverStarter as any, 'isPortAvailable'); const result = await isPortAvailable(port); - // CRITICAL: Should return false because we're holding the port - assert.isFalse(result, 'Port should be detected as in use when actually bound'); + assert.isFalse(result, 'Expected port check to fail closed when an error occurs'); + assert.isTrue(warnStub.called, 'Expected warning to be logged when check fails'); } finally { - // Clean up: close the server - await new Promise((resolve) => { - server.close(() => resolve()); - }); + warnStub.restore(); } }); - - test('should detect when checking port availability', async () => { - // This tests that isPortAvailable returns a boolean - const port = 54322; - - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const isPortAvailable = getPrivateMethod(serverStarter as any, 'isPortAvailable'); - const result = await isPortAvailable(port); - - // Result should be a boolean - assert.isBoolean(result); - }); }); suite('findAvailablePort', () => { From f9ebd069586d7d1d3798ebfa5ed3c6f371e480ec Mon Sep 17 00:00:00 2001 From: Lukas Saltenas Date: Wed, 5 Nov 2025 17:38:49 +0200 Subject: [PATCH 4/6] chore: better test coverage --- cspell.json | 3 +- .../deepnoteServerStarter.unit.test.ts | 108 ++++++++++++++++++ 2 files changed, 110 insertions(+), 1 deletion(-) diff --git a/cspell.json b/cspell.json index 8d6abf871b..0b4cf5d0d1 100644 --- a/cspell.json +++ b/cspell.json @@ -45,6 +45,7 @@ "jupyter", "jupyterlab", "JVSC", + "loopbacks", "matplotlib", "millis", "mindsdb", @@ -67,9 +68,9 @@ "Unconfigured", "unittests", "vegalite", - "venv's", "venv", "Venv", + "venv's", "venvs", "vscode", "xanchor", diff --git a/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts b/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts index ae5d82d6f5..41cc6c04d9 100644 --- a/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts +++ b/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts @@ -362,4 +362,112 @@ suite('DeepnoteServerStarter - Port Allocation Integration Tests', () => { } }); }); + + suite('findConsecutiveAvailablePorts - Edge Cases', () => { + test('should mark both ports unavailable and continue when consecutive port is taken', async () => { + // This test covers the scenario where a candidate port is available + // but the next port (candidate + 1) is not available. + // The system should mark BOTH ports as unavailable in portsInUse and continue searching. + + const net = require('net'); + const server1 = net.createServer(); + const server2 = net.createServer(); + const blockedPort1 = 54801; + const blockedPort2 = 54803; + + // Block ports 54801 and 54803 (leaving 54800 and 54802 available but not consecutive) + await new Promise((resolve) => { + server1.listen(blockedPort1, 'localhost', () => { + server2.listen(blockedPort2, 'localhost', () => { + resolve(); + }); + }); + }); + + try { + const portsInUse = new Set(); + const startPort = 54800; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const findConsecutiveAvailablePorts = getPrivateMethod( + serverStarter as any, + 'findConsecutiveAvailablePorts' + ); + + // Should skip 54800 (since 54801 is blocked) and 54802 (since 54803 is blocked) + // and find the next consecutive pair like 54804+54805 + const result = await findConsecutiveAvailablePorts(startPort, portsInUse); + + // Verify ports are consecutive + assert.strictEqual(result.lspPort, result.jupyterPort + 1); + + // Should have found ports after the blocked ones + assert.isTrue( + result.jupyterPort > blockedPort2 || result.jupyterPort < blockedPort1 - 1, + 'Should skip blocked port ranges' + ); + } finally { + // Clean up + await new Promise((resolve) => { + server1.close(() => { + server2.close(() => resolve()); + }); + }); + } + }); + + test('should throw DeepnoteServerStartupError when max attempts exhausted', async () => { + // This test covers the scenario where we cannot find consecutive ports + // after maxAttempts (100 attempts). This should throw a DeepnoteServerStartupError. + + const net = require('net'); + const servers: any[] = []; + + try { + // Block a large range of ports to make it impossible to find consecutive pairs + const startPort = 55000; + const portsToBlock = 210; // More than maxAttempts (100) * 2 + + // Create servers blocking many ports + for (let i = 0; i < portsToBlock; i++) { + const server = net.createServer(); + servers.push(server); + await new Promise((resolve) => { + server.listen(startPort + i, 'localhost', () => resolve()); + }); + } + + const portsInUse = new Set(); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const findConsecutiveAvailablePorts = getPrivateMethod( + serverStarter as any, + 'findConsecutiveAvailablePorts' + ); + + // Should throw DeepnoteServerStartupError after maxAttempts + let errorThrown = false; + try { + await findConsecutiveAvailablePorts(startPort, portsInUse); + } catch (error: any) { + errorThrown = true; + assert.strictEqual(error.constructor.name, 'DeepnoteServerStartupError'); + assert.include(error.stderr, 'Failed to find consecutive available ports'); + assert.include(error.stderr, '100 attempts'); + } + + assert.isTrue(errorThrown, 'Expected DeepnoteServerStartupError to be thrown'); + } finally { + // Clean up all servers + await Promise.all( + servers.map( + (server) => + new Promise((resolve) => { + server.close(() => resolve()); + }) + ) + ); + } + }); + }); }); From d7b17907ecc4a2fd11af997831a8a6d1e1faa5f9 Mon Sep 17 00:00:00 2001 From: Lukas Saltenas Date: Wed, 5 Nov 2025 17:40:50 +0200 Subject: [PATCH 5/6] chore: check pairs dont collide --- .../deepnoteServerStarter.unit.test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts b/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts index 41cc6c04d9..4aee7afc40 100644 --- a/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts +++ b/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts @@ -351,6 +351,7 @@ suite('DeepnoteServerStarter - Port Allocation Integration Tests', () => { const results = await Promise.all(keys.map((key) => allocatePorts(key))); + // Verify each result has consecutive ports for (let i = 0; i < results.length; i++) { const result = results[i]; assert.strictEqual( @@ -360,6 +361,23 @@ suite('DeepnoteServerStarter - Port Allocation Integration Tests', () => { `This prevents server startup hangs when toolkit expects consecutive ports.` ); } + + // Verify uniqueness: no two concurrent calls received the same port pair + const portPairs = new Set(results.map((r) => `${r.jupyterPort}:${r.lspPort}`)); + assert.strictEqual( + portPairs.size, + results.length, + 'All concurrent allocations must receive unique port pairs' + ); + + // Verify uniqueness of individual ports + const allPorts = results.flatMap((r) => [r.jupyterPort, r.lspPort]); + const uniquePorts = new Set(allPorts); + assert.strictEqual( + uniquePorts.size, + allPorts.length, + 'All allocated ports (both Jupyter and LSP) must be unique across concurrent calls' + ); }); }); From f6c0c503e6d3a33addb5fe44c289448e9b660e13 Mon Sep 17 00:00:00 2001 From: Lukas Saltenas Date: Wed, 5 Nov 2025 17:50:12 +0200 Subject: [PATCH 6/6] chore: concurrent logic and improved tests --- .../deepnote/deepnoteServerStarter.node.ts | 14 +++++----- .../deepnoteServerStarter.unit.test.ts | 27 +++++++++++++++---- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/kernels/deepnote/deepnoteServerStarter.node.ts b/src/kernels/deepnote/deepnoteServerStarter.node.ts index acd2d2845f..04b03485e9 100644 --- a/src/kernels/deepnote/deepnoteServerStarter.node.ts +++ b/src/kernels/deepnote/deepnoteServerStarter.node.ts @@ -499,14 +499,16 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension * @returns Object with jupyterPort and lspPort */ private async allocatePorts(key: string): Promise<{ jupyterPort: number; lspPort: number }> { - // Wait for any ongoing port allocation to complete - await this.portAllocationLock; - - // Create new allocation promise and update the lock + // Chain onto the existing lock promise to serialize allocations even when multiple calls start concurrently + const previousLock = this.portAllocationLock; let releaseLock: () => void; - this.portAllocationLock = new Promise((resolve) => { + const currentLock = new Promise((resolve) => { releaseLock = resolve; }); + this.portAllocationLock = previousLock.then(() => currentLock); + + // Wait until all prior allocations have completed before proceeding + await previousLock; try { // Get all ports currently in use by our managed servers @@ -547,7 +549,7 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension return { jupyterPort, lspPort }; } finally { - // Release the lock to allow next allocation + // Release the lock to allow next allocation in the chain to proceed releaseLock!(); } } diff --git a/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts b/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts index 4aee7afc40..dda362b526 100644 --- a/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts +++ b/src/kernels/deepnote/deepnoteServerStarter.unit.test.ts @@ -53,6 +53,13 @@ suite('DeepnoteServerStarter - Port Allocation Integration Tests', () => { ); }); + teardown(async () => { + // Dispose the serverStarter to clean up any allocated ports and state + if (serverStarter) { + await serverStarter.dispose(); + } + }); + suite('isPortAvailable', () => { let checkStub: sinon.SinonStub; @@ -344,7 +351,13 @@ suite('DeepnoteServerStarter - Port Allocation Integration Tests', () => { // if Jupyter port was available but LSP port (Jupyter+1) was not, // the system would allocate non-consecutive ports causing server hangs - const keys = ['regression-1', 'regression-2', 'regression-3']; + // Use unique keys with timestamp to avoid conflicts with other tests + const timestamp = Date.now(); + const keys = [ + `concurrent-test-${timestamp}-1`, + `concurrent-test-${timestamp}-2`, + `concurrent-test-${timestamp}-3` + ]; // eslint-disable-next-line @typescript-eslint/no-explicit-any const allocatePorts = getPrivateMethod(serverStarter as any, 'allocatePorts'); @@ -437,21 +450,25 @@ suite('DeepnoteServerStarter - Port Allocation Integration Tests', () => { test('should throw DeepnoteServerStartupError when max attempts exhausted', async () => { // This test covers the scenario where we cannot find consecutive ports // after maxAttempts (100 attempts). This should throw a DeepnoteServerStartupError. + // Strategy: Block every other port so individual ports are available, + // but no consecutive pairs exist (blocking the +1 port for each available port) const net = require('net'); const servers: any[] = []; try { - // Block a large range of ports to make it impossible to find consecutive pairs + // Block every other port starting from 55001 (leaving 55000, 55002, 55004, etc. available) + // This ensures findAvailablePort succeeds, but the consecutive port is always blocked const startPort = 55000; - const portsToBlock = 210; // More than maxAttempts (100) * 2 + const portsToBlock = 200; // Block 200 odd-numbered ports - // Create servers blocking many ports + // Create servers blocking every other port (the +1 ports) for (let i = 0; i < portsToBlock; i++) { + const portToBlock = startPort + i * 2 + 1; // Block 55001, 55003, 55005, etc. const server = net.createServer(); servers.push(server); await new Promise((resolve) => { - server.listen(startPort + i, 'localhost', () => resolve()); + server.listen(portToBlock, 'localhost', () => resolve()); }); }