fix: force-kill the captured PET child so restart cannot lose or misfire the kill

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<T>(
     }
 }
 
+type KillablePetProcess = Pick<ChildProcess, 'kill' | 'exitCode' | 'signalCode'>;
+
+/**
+ * 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<LogOutputChannel, 'info' | 'error'>,
+    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<NativePythonEnvironmentKind | Uri[] | undefined, NativeInfo[]>;
@@ -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<NativeInfo[]> {

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');
+    });
+});