diff --git a/src/managers/common/nativePythonFinder.ts b/src/managers/common/nativePythonFinder.ts index a09b83e9e..2978d44e1 100644 --- a/src/managers/common/nativePythonFinder.ts +++ b/src/managers/common/nativePythonFinder.ts @@ -42,6 +42,7 @@ const MAX_RESTART_ATTEMPTS = 3; const RESTART_BACKOFF_BASE_MS = 1_000; // 1 second base, exponential: 1s, 2s, 4s const MAX_CONFIGURE_TIMEOUTS_BEFORE_KILL = 2; // Kill on the 2nd consecutive timeout const MAX_REFRESH_RETRIES = 1; // Retry refresh once after timeout +const KILL_PROCESS_GRACE_PERIOD_MS = 500; // Grace after SIGTERM before escalating to SIGKILL /** * Computes the configure timeout with exponential backoff. @@ -342,6 +343,35 @@ async function sendRequestWithTimeout( } } +type KillablePetProcess = Pick; + +/** + * Terminates a PET child process, capturing it before signalling so the delayed SIGKILL only ever + * targets that captured child and never a replacement a concurrent restart may install. + */ +export function killPetProcessWithGrace( + getProc: () => KillablePetProcess | undefined, + clearProc: () => void, + outputChannel: Pick, + graceMs: number = KILL_PROCESS_GRACE_PERIOD_MS, +): void { + const proc = getProc(); + clearProc(); + if (proc && proc.exitCode === null && proc.signalCode === null) { + try { + outputChannel.info('[pet] Killing hung/crashed PET process'); + proc.kill('SIGTERM'); + setTimeout(() => { + if (proc.exitCode === null && proc.signalCode === null) { + proc.kill('SIGKILL'); + } + }, graceMs); + } catch (ex) { + outputChannel.error('[pet] Error killing process:', ex); + } + } +} + class NativePythonFinderImpl implements NativePythonFinder { private connection: rpc.MessageConnection; private readonly pool: WorkerPool; @@ -542,21 +572,13 @@ class NativePythonFinderImpl implements NativePythonFinder { * Attempts to kill the PET process. Used during restart and timeout recovery. */ private killProcess(): void { - if (this.proc && this.proc.exitCode === null) { - try { - this.outputChannel.info('[pet] Killing hung/crashed PET process'); - this.proc.kill('SIGTERM'); - // Give it a moment to terminate gracefully, then force kill - setTimeout(() => { - if (this.proc && this.proc.exitCode === null) { - this.proc.kill('SIGKILL'); - } - }, 500); - } catch (ex) { - this.outputChannel.error('[pet] Error killing process:', ex); - } - } - this.proc = undefined; + killPetProcessWithGrace( + () => this.proc, + () => { + this.proc = undefined; + }, + this.outputChannel, + ); } public async refresh(hardRefresh: boolean, options?: NativePythonEnvironmentKind | Uri[]): Promise { diff --git a/src/test/managers/common/nativePythonFinder.killProcess.unit.test.ts b/src/test/managers/common/nativePythonFinder.killProcess.unit.test.ts new file mode 100644 index 000000000..a58f90841 --- /dev/null +++ b/src/test/managers/common/nativePythonFinder.killProcess.unit.test.ts @@ -0,0 +1,164 @@ +import assert from 'node:assert'; +import * as sinon from 'sinon'; +import { killPetProcessWithGrace } from '../../../managers/common/nativePythonFinder'; + +suite('killPetProcessWithGrace (PET force-kill ownership)', () => { + const GRACE_MS = 500; + + let clock: sinon.SinonFakeTimers; + let outputChannel: { info: sinon.SinonStub; error: sinon.SinonStub }; + + setup(() => { + clock = sinon.useFakeTimers(); + outputChannel = { info: sinon.stub(), error: sinon.stub() }; + }); + + teardown(() => { + clock.restore(); + sinon.restore(); + }); + + function makeProc( + exitCode: number | null = null, + signalCode: NodeJS.Signals | null = null, + ): { exitCode: number | null; signalCode: NodeJS.Signals | null; kill: sinon.SinonStub } { + return { exitCode, signalCode, kill: sinon.stub().returns(true) }; + } + + test('sends SIGTERM to the running child and relinquishes ownership synchronously', () => { + const original = makeProc(null); + let holder: typeof original | undefined = original; + + killPetProcessWithGrace( + () => holder, + () => { + holder = undefined; + }, + outputChannel, + ); + + assert.strictEqual(holder, undefined, 'ownership should be relinquished before any async work'); + assert.ok(original.kill.calledOnceWithExactly('SIGTERM'), 'original should receive exactly one SIGTERM'); + assert.ok(outputChannel.info.called, 'kill should be logged'); + }); + + test('escalates to SIGKILL on the captured child after the grace period', () => { + const original = makeProc(null); + let holder: typeof original | undefined = original; + + killPetProcessWithGrace( + () => holder, + () => { + holder = undefined; + }, + outputChannel, + ); + + assert.ok(original.kill.calledWith('SIGTERM'), 'SIGTERM sent immediately'); + assert.ok(!original.kill.calledWith('SIGKILL'), 'SIGKILL not sent before grace elapses'); + + clock.tick(GRACE_MS); + + assert.ok(original.kill.calledWith('SIGKILL'), 'SIGKILL sent after grace period'); + assert.strictEqual(original.kill.callCount, 2, 'exactly SIGTERM then SIGKILL'); + }); + + test('does not force-kill a child that exits during the grace period', () => { + const original = makeProc(null); + let holder: typeof original | undefined = original; + + killPetProcessWithGrace( + () => holder, + () => { + holder = undefined; + }, + outputChannel, + ); + + original.exitCode = 0; + clock.tick(GRACE_MS); + + assert.ok(original.kill.calledOnceWithExactly('SIGTERM'), 'only SIGTERM, no SIGKILL for an exited child'); + }); + + test('does not force-kill a child terminated by SIGTERM during the grace period', () => { + const original = makeProc(null); + let holder: typeof original | undefined = original; + + killPetProcessWithGrace( + () => holder, + () => { + holder = undefined; + }, + outputChannel, + ); + + original.signalCode = 'SIGTERM'; + clock.tick(GRACE_MS); + + assert.ok( + original.kill.calledOnceWithExactly('SIGTERM'), + 'only SIGTERM, no SIGKILL for a signal-terminated child', + ); + }); + + test('does not signal a child that had already exited, but still clears ownership', () => { + const original = makeProc(143); + let holder: typeof original | undefined = original; + + killPetProcessWithGrace( + () => holder, + () => { + holder = undefined; + }, + outputChannel, + ); + clock.tick(GRACE_MS); + + assert.strictEqual(holder, undefined, 'ownership cleared even when no signal is needed'); + assert.strictEqual(original.kill.callCount, 0, 'no signals sent to an already-exited child'); + assert.ok(outputChannel.info.notCalled, 'no kill message logged when nothing is killed'); + }); + + test('never kills a replacement child assigned during the grace period', () => { + const original = makeProc(null); + const replacement = makeProc(null); + let holder: typeof original | undefined = original; + + killPetProcessWithGrace( + () => holder, + () => { + holder = undefined; + }, + outputChannel, + ); + + holder = replacement; + clock.tick(GRACE_MS); + + assert.ok(original.kill.calledWith('SIGKILL'), 'the captured original is force-killed'); + assert.strictEqual(replacement.kill.callCount, 0, 'the replacement child is never signalled'); + }); + + test('catches and logs SIGTERM errors without throwing, and still clears ownership', () => { + const original = makeProc(null); + original.kill = sinon.stub().throws(new Error('kill failed')); + let holder: typeof original | undefined = original; + + assert.doesNotThrow(() => + killPetProcessWithGrace( + () => holder, + () => { + holder = undefined; + }, + outputChannel, + ), + ); + + assert.strictEqual(holder, undefined, 'ownership relinquished even when the kill throws'); + assert.ok(outputChannel.error.called, 'kill error is logged'); + + clock.tick(GRACE_MS); + assert.strictEqual(original.kill.callCount, 1, 'no SIGKILL scheduled after a SIGTERM failure'); + }); +});