Skip to content

Commit 38f3a8e

Browse files
jankucajankuca
committed
fix: Windows process detection now resolves command lines properly
- Windows tasklist CSV output never includes command lines - Changed to two-step approach: collect all python.exe/pythonw.exe PIDs first - Then resolve each PID's command line using isDeepnoteRelatedProcess() - Only verified deepnote-related processes are added to candidate list - Kill logic remains gated by isDeepnoteRelatedProcess + orphan checks - Fixes false negatives where deepnote processes were missed on Windows
1 parent 0b22d4d commit 38f3a8e

File tree

1 file changed

+142
-108
lines changed

1 file changed

+142
-108
lines changed

‎src/kernels/deepnote/deepnoteServerStarter.node.ts‎

Lines changed: 142 additions & 108 deletions
Original file line numberDiff line numberDiff line change
@@ -858,146 +858,180 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension
858858
const processService = await this.processServiceFactory.create(undefined);
859859

860860
// Find all deepnote-toolkit server processes and related child processes
861-
let command: string;
862-
let args: string[];
861+
const candidatePids: number[] = [];
863862

864863
if (process.platform === 'win32') {
865-
// Windows: use tasklist and findstr
866-
command = 'tasklist';
867-
args = ['/FI', 'IMAGENAME eq python.exe', '/FO', 'CSV', '/NH'];
868-
} else {
869-
// Unix-like: use ps and grep
870-
command = 'ps';
871-
args = ['aux'];
872-
}
864+
// Windows: tasklist CSV doesn't include command lines, so we need a two-step approach:
865+
// 1. Get all python.exe and pythonw.exe PIDs
866+
// 2. Check each PID's command line to see if it's deepnote-related
873867

874-
const result = await processService.exec(command, args, { throwOnStdErr: false });
868+
// Step 1: Get all Python process PIDs
869+
const pythonPids: number[] = [];
875870

876-
if (result.stdout) {
877-
const lines = result.stdout.split('\n');
878-
const candidatePids: number[] = [];
871+
// Check python.exe
872+
const pythonResult = await processService.exec(
873+
'tasklist',
874+
['/FI', 'IMAGENAME eq python.exe', '/FO', 'CSV', '/NH'],
875+
{ throwOnStdErr: false }
876+
);
877+
if (pythonResult.stdout) {
878+
const lines = pythonResult.stdout.split('\n');
879+
for (const line of lines) {
880+
// Windows CSV format: "python.exe","12345",...
881+
const match = line.match(/"python\.exe","(\d+)"/);
882+
if (match) {
883+
const pid = parseInt(match[1], 10);
884+
if (!isNaN(pid)) {
885+
pythonPids.push(pid);
886+
}
887+
}
888+
}
889+
}
890+
891+
// Check pythonw.exe
892+
const pythonwResult = await processService.exec(
893+
'tasklist',
894+
['/FI', 'IMAGENAME eq pythonw.exe', '/FO', 'CSV', '/NH'],
895+
{ throwOnStdErr: false }
896+
);
897+
if (pythonwResult.stdout) {
898+
const lines = pythonwResult.stdout.split('\n');
899+
for (const line of lines) {
900+
// Windows CSV format: "pythonw.exe","12345",...
901+
const match = line.match(/"pythonw\.exe","(\d+)"/);
902+
if (match) {
903+
const pid = parseInt(match[1], 10);
904+
if (!isNaN(pid)) {
905+
pythonPids.push(pid);
906+
}
907+
}
908+
}
909+
}
879910

880-
for (const line of lines) {
881-
// Look for processes running deepnote_toolkit server or related child processes
882-
// This includes:
883-
// - deepnote_toolkit server (main server process)
884-
// - pylsp (Python LSP server child process)
885-
// - jupyter (Jupyter server child process)
886-
const isDeepnoteRelated =
887-
(line.includes('deepnote_toolkit') && line.includes('server')) ||
888-
(line.includes('pylsp') && line.includes('2087')) || // LSP server on port 2087
889-
(line.includes('jupyter') && line.includes('deepnote'));
911+
logger.debug(`Found ${pythonPids.length} Python process(es) on Windows`);
890912

913+
// Step 2: Check each Python PID to see if it's deepnote-related
914+
for (const pid of pythonPids) {
915+
const isDeepnoteRelated = await this.isDeepnoteRelatedProcess(pid);
891916
if (isDeepnoteRelated) {
892-
// Extract PID based on platform
893-
let pid: number | undefined;
894-
895-
if (process.platform === 'win32') {
896-
// Windows CSV format: "python.exe","12345",...
897-
const match = line.match(/"python\.exe","(\d+)"/);
898-
if (match) {
899-
pid = parseInt(match[1], 10);
900-
}
901-
} else {
917+
candidatePids.push(pid);
918+
}
919+
}
920+
} else {
921+
// Unix-like: use ps with full command line
922+
const result = await processService.exec('ps', ['aux'], { throwOnStdErr: false });
923+
924+
if (result.stdout) {
925+
const lines = result.stdout.split('\n');
926+
927+
for (const line of lines) {
928+
// Look for processes running deepnote_toolkit server or related child processes
929+
// This includes:
930+
// - deepnote_toolkit server (main server process)
931+
// - pylsp (Python LSP server child process)
932+
// - jupyter (Jupyter server child process)
933+
const isDeepnoteRelated =
934+
(line.includes('deepnote_toolkit') && line.includes('server')) ||
935+
(line.includes('pylsp') && line.includes('2087')) || // LSP server on port 2087
936+
(line.includes('jupyter') && line.includes('deepnote'));
937+
938+
if (isDeepnoteRelated) {
902939
// Unix format: user PID ...
903940
const parts = line.trim().split(/\s+/);
904941
if (parts.length > 1) {
905-
pid = parseInt(parts[1], 10);
942+
const pid = parseInt(parts[1], 10);
943+
if (!isNaN(pid)) {
944+
candidatePids.push(pid);
945+
}
906946
}
907947
}
908-
909-
if (pid && !isNaN(pid)) {
910-
candidatePids.push(pid);
911-
}
912948
}
913949
}
950+
}
914951

915-
if (candidatePids.length > 0) {
916-
logger.info(
917-
`Found ${candidatePids.length} deepnote-related process(es): ${candidatePids.join(', ')}`
918-
);
952+
if (candidatePids.length > 0) {
953+
logger.info(`Found ${candidatePids.length} deepnote-related process(es): ${candidatePids.join(', ')}`);
919954

920-
const pidsToKill: number[] = [];
921-
const pidsToSkip: Array<{ pid: number; reason: string }> = [];
922-
923-
// Check each process to determine if it should be killed
924-
for (const pid of candidatePids) {
925-
// Check if there's a lock file for this PID (only main server processes have lock files)
926-
const lockData = await this.readLockFile(pid);
927-
928-
if (lockData) {
929-
// Lock file exists - this is a main server process
930-
// Check if it belongs to a different session
931-
if (lockData.sessionId !== this.sessionId) {
932-
// Different session - check if the process is actually orphaned
933-
const isOrphaned = await this.isProcessOrphaned(pid);
934-
if (isOrphaned) {
935-
logger.info(
936-
`PID ${pid} belongs to session ${lockData.sessionId} and is orphaned - will kill`
937-
);
938-
pidsToKill.push(pid);
939-
} else {
940-
pidsToSkip.push({
941-
pid,
942-
reason: `belongs to active session ${lockData.sessionId.substring(0, 8)}...`
943-
});
944-
}
945-
} else {
946-
// Same session - this shouldn't happen during startup, but skip it
947-
pidsToSkip.push({ pid, reason: 'belongs to current session' });
948-
}
949-
} else {
950-
// No lock file - could be a child process (LSP, Jupyter) or orphaned main process
951-
// Check if orphaned before killing
955+
const pidsToKill: number[] = [];
956+
const pidsToSkip: Array<{ pid: number; reason: string }> = [];
957+
958+
// Check each process to determine if it should be killed
959+
for (const pid of candidatePids) {
960+
// Check if there's a lock file for this PID (only main server processes have lock files)
961+
const lockData = await this.readLockFile(pid);
962+
963+
if (lockData) {
964+
// Lock file exists - this is a main server process
965+
// Check if it belongs to a different session
966+
if (lockData.sessionId !== this.sessionId) {
967+
// Different session - check if the process is actually orphaned
952968
const isOrphaned = await this.isProcessOrphaned(pid);
953969
if (isOrphaned) {
954-
logger.info(`PID ${pid} has no lock file and is orphaned - will kill`);
970+
logger.info(
971+
`PID ${pid} belongs to session ${lockData.sessionId} and is orphaned - will kill`
972+
);
955973
pidsToKill.push(pid);
956974
} else {
957-
pidsToSkip.push({ pid, reason: 'no lock file but has active parent process' });
975+
pidsToSkip.push({
976+
pid,
977+
reason: `belongs to active session ${lockData.sessionId.substring(0, 8)}...`
978+
});
958979
}
980+
} else {
981+
// Same session - this shouldn't happen during startup, but skip it
982+
pidsToSkip.push({ pid, reason: 'belongs to current session' });
959983
}
960-
}
961-
962-
// Log skipped processes
963-
if (pidsToSkip.length > 0) {
964-
for (const { pid, reason } of pidsToSkip) {
965-
logger.info(`Skipping PID ${pid}: ${reason}`);
984+
} else {
985+
// No lock file - could be a child process (LSP, Jupyter) or orphaned main process
986+
// Check if orphaned before killing
987+
const isOrphaned = await this.isProcessOrphaned(pid);
988+
if (isOrphaned) {
989+
logger.info(`PID ${pid} has no lock file and is orphaned - will kill`);
990+
pidsToKill.push(pid);
991+
} else {
992+
pidsToSkip.push({ pid, reason: 'no lock file but has active parent process' });
966993
}
967994
}
995+
}
968996

969-
// Kill orphaned processes
970-
if (pidsToKill.length > 0) {
971-
logger.info(`Killing ${pidsToKill.length} orphaned process(es): ${pidsToKill.join(', ')}`);
972-
this.outputChannel.appendLine(
973-
`Cleaning up ${pidsToKill.length} orphaned deepnote-related process(es)...`
974-
);
997+
// Log skipped processes
998+
if (pidsToSkip.length > 0) {
999+
for (const { pid, reason } of pidsToSkip) {
1000+
logger.info(`Skipping PID ${pid}: ${reason}`);
1001+
}
1002+
}
9751003

976-
for (const pid of pidsToKill) {
977-
try {
978-
if (process.platform === 'win32') {
979-
await processService.exec('taskkill', ['/F', '/T', '/PID', pid.toString()], {
980-
throwOnStdErr: false
981-
});
982-
} else {
983-
await processService.exec('kill', ['-9', pid.toString()], { throwOnStdErr: false });
984-
}
985-
logger.info(`Killed orphaned process ${pid}`);
1004+
// Kill orphaned processes
1005+
if (pidsToKill.length > 0) {
1006+
logger.info(`Killing ${pidsToKill.length} orphaned process(es): ${pidsToKill.join(', ')}`);
1007+
this.outputChannel.appendLine(
1008+
`Cleaning up ${pidsToKill.length} orphaned deepnote-related process(es)...`
1009+
);
9861010

987-
// Clean up the lock file after killing (if it exists)
988-
await this.deleteLockFile(pid);
989-
} catch (ex) {
990-
logger.warn(`Failed to kill process ${pid}: ${ex}`);
1011+
for (const pid of pidsToKill) {
1012+
try {
1013+
if (process.platform === 'win32') {
1014+
await processService.exec('taskkill', ['/F', '/T', '/PID', pid.toString()], {
1015+
throwOnStdErr: false
1016+
});
1017+
} else {
1018+
await processService.exec('kill', ['-9', pid.toString()], { throwOnStdErr: false });
9911019
}
992-
}
1020+
logger.info(`Killed orphaned process ${pid}`);
9931021

994-
this.outputChannel.appendLine('✓ Cleanup complete');
995-
} else {
996-
logger.info('No orphaned deepnote-related processes found (all processes are active)');
1022+
// Clean up the lock file after killing (if it exists)
1023+
await this.deleteLockFile(pid);
1024+
} catch (ex) {
1025+
logger.warn(`Failed to kill process ${pid}: ${ex}`);
1026+
}
9971027
}
1028+
1029+
this.outputChannel.appendLine('✓ Cleanup complete');
9981030
} else {
999-
logger.info('No deepnote-related processes found');
1031+
logger.info('No orphaned deepnote-related processes found (all processes are active)');
10001032
}
1033+
} else {
1034+
logger.info('No deepnote-related processes found');
10011035
}
10021036
} catch (ex) {
10031037
// Don't fail startup if cleanup fails

0 commit comments

Comments
 (0)