From d3dd34038f580b0c3c36ce4caa8930f658e2c2fd Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Sat, 22 Aug 2026 18:45:16 -0700 Subject: [PATCH 1/2] fix: force-kill the captured PET child so restart cannot lose or misfire the kill killProcess() sent SIGTERM, scheduled a 500ms callback that re-read the mutable this.proc field, then cleared it synchronously. By the time the callback ran, this.proc was either already undefined (a hung PET was never SIGKILLed) or reassigned by a concurrent restart (the delayed SIGKILL could hit the healthy replacement). Extract killPetProcessWithGrace, which captures the child and clears the holder before signalling, so the delayed SIGKILL only ever targets that captured child. Add focused ownership regression tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/managers/common/nativePythonFinder.ts | 52 +++++-- ...ativePythonFinder.killProcess.unit.test.ts | 140 ++++++++++++++++++ 2 files changed, 177 insertions(+), 15 deletions(-) create mode 100644 src/test/managers/common/nativePythonFinder.killProcess.unit.test.ts diff --git a/src/managers/common/nativePythonFinder.ts b/src/managers/common/nativePythonFinder.ts index a09b83e9e..5e316c229 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) { + try { + outputChannel.info('[pet] Killing hung/crashed PET process'); + proc.kill('SIGTERM'); + setTimeout(() => { + if (proc.exitCode === 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..7fb6f7a05 --- /dev/null +++ b/src/test/managers/common/nativePythonFinder.killProcess.unit.test.ts @@ -0,0 +1,140 @@ +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): { exitCode: number | null; kill: sinon.SinonStub } { + return { exitCode, 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 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'); + }); +}); From 2ad79f9adbbeabf817d911bad98e82c3ea878e67 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Sat, 22 Aug 2026 21:52:43 -0700 Subject: [PATCH 2/2] fix: gate PET force-kill on signalCode so a SIGTERM-terminated child is not SIGKILLed A child that responds to SIGTERM reports exitCode === null with signalCode set, so guarding the escalation on exitCode alone still fired SIGKILL at an already-terminated child. Include signalCode in KillablePetProcess and only signal while both terminal-state fields are null; extend the test double with signalCode and cover the SIGTERM-during-grace case. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/managers/common/nativePythonFinder.ts | 6 ++-- ...ativePythonFinder.killProcess.unit.test.ts | 28 +++++++++++++++++-- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/managers/common/nativePythonFinder.ts b/src/managers/common/nativePythonFinder.ts index 5e316c229..2978d44e1 100644 --- a/src/managers/common/nativePythonFinder.ts +++ b/src/managers/common/nativePythonFinder.ts @@ -343,7 +343,7 @@ async function sendRequestWithTimeout( } } -type KillablePetProcess = Pick; +type KillablePetProcess = Pick; /** * Terminates a PET child process, capturing it before signalling so the delayed SIGKILL only ever @@ -357,12 +357,12 @@ export function killPetProcessWithGrace( ): void { const proc = getProc(); clearProc(); - if (proc && proc.exitCode === null) { + 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) { + if (proc.exitCode === null && proc.signalCode === null) { proc.kill('SIGKILL'); } }, graceMs); diff --git a/src/test/managers/common/nativePythonFinder.killProcess.unit.test.ts b/src/test/managers/common/nativePythonFinder.killProcess.unit.test.ts index 7fb6f7a05..a58f90841 100644 --- a/src/test/managers/common/nativePythonFinder.killProcess.unit.test.ts +++ b/src/test/managers/common/nativePythonFinder.killProcess.unit.test.ts @@ -18,8 +18,11 @@ suite('killPetProcessWithGrace (PET force-kill ownership)', () => { sinon.restore(); }); - function makeProc(exitCode: number | null = null): { exitCode: number | null; kill: sinon.SinonStub } { - return { exitCode, kill: sinon.stub().returns(true) }; + 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', () => { @@ -78,6 +81,27 @@ suite('killPetProcessWithGrace (PET force-kill ownership)', () => { 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;