fix: force-kill the captured PET child so restart cannot lose or misfire the kill - #11
fix: force-kill the captured PET child so restart cannot lose or misfire the kill#11StellaHuang95 wants to merge 1 commit into
Conversation
StellaHuang95
changed the title
|
🔒 Automated review in progress — @StellaHuang95 is auto-reviewing this PR. |
| 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 |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
The restart teardown now centralizes the grace period and checks both exitCode and signalCode, while the disposable teardown still uses a hard-coded 500 and exitCode alone. Consider sharing the constant and running predicate to prevent these sibling lifecycle policies from drifting.
[verified]
StellaHuang95
added
the
review-auto:approved
…LI fallback (PR #14) Closes the pre-existing dispose-during-restart-backoff race flagged by both reviews: if dispose() ran while restart() was parked on its backoff, the restart resumed and spawned a replacement PET child that nothing would ever dispose. Use the disposed state consistently via a precise NativePythonFinderDisposedError and a throwIfDisposed() guard at every spawn/restart/retry/CLI chokepoint (resolve/refresh entry, ensureProcessRunning, restart before+after backoff, start, resolve/refreshViaJsonCli). Make the restart backoff cancelable so dispose() unblocks it immediately (clearing the timer so none leaks) and the post-backoff guard aborts before any spawn; skip restart error telemetry for an intentional-disposal abort. isServerExhausted() is false once disposed so neither resolve nor refresh falls through to the JSON CLI. Non-disposed restart/recovery behavior is unchanged, and PR #11's force-kill escalation is intentionally not duplicated here. Adds 3 deterministic fake-timer tests: dispose during backoff (no replacement child, no leaked timer via countTimers()===0, settles without ticking the clock), start() after dispose throws and never spawns, and resolve()/refresh() after dispose reject with the disposed error and never spawn. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
db25074 to
7b8d21d
Compare
StellaHuang95
changed the title
StellaHuang95
removed
the
review-auto:approved
…ire 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>
7b8d21d to
d3dd340
Compare
| if (proc.exitCode === null) { | ||
| proc.kill('SIGKILL'); | ||
| } | ||
| }, graceMs); |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
The captured-process predicate only checks exitCode. A child that exits from SIGTERM can retain exitCode === null while reporting signalCode === 'SIGTERM', so the grace callback may still attempt SIGKILL against an already-exited PID. Include signalCode in the running predicate, and reuse that predicate plus KILL_PROCESS_GRACE_PERIOD_MS in the disposable teardown path to keep the sibling lifecycle policies aligned.
|
Non-blocking follow-ups: intentional exits from a superseded PET child should not be logged as unexpected errors, and a narrow production-wiring test around |
StellaHuang95
added
the
review-auto:approved
| if (proc && proc.exitCode === null) { | ||
| try { | ||
| outputChannel.info('[pet] Killing hung/crashed PET process'); | ||
| proc.kill('SIGTERM'); |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
A child terminated by SIGTERM can retain exitCode === null while setting signalCode, so the delayed callback can send a redundant SIGKILL. Include signalCode in the running-process predicate and add coverage for a signal-terminated child.
1 participant
Fixes a force-kill ownership bug in the PET (
python-environment-tools) child-process teardown insrc/managers/common/nativePythonFinder.ts.Problem
PET runs as a long-lived child process managed by
NativePythonFinderImpl. When a request times out or the RPC connection breaks, recovery callskillProcess()to terminate the hung child, thenrestart()spawns a fresh one intothis.proc. The teardown could act on the wrong process across that restart boundary.Root cause
killProcess()sentSIGTERM, scheduled a 500 mssetTimeoutthat re-read the mutablethis.procfield to decide whether toSIGKILL, then synchronously setthis.proc = undefined. When the callback ran,this.procwas either alreadyundefined(so a hung PET was neverSIGKILLed) or had been reassigned by a concurrent restart (so the callback couldSIGKILLthe healthy replacement).Fix
Extract
killPetProcessWithGrace(getProc, clearProc, outputChannel, graceMs), which captures the current child and clears the holder before signalling, then sendsSIGTERMand — after the grace period —SIGKILLs only that captured child.killProcess()delegates via() => this.proc/() => { this.proc = undefined; }. Because the delayedSIGKILLcloses over the captured local child rather than the mutable field, a concurrent restart's replacement is never targeted, and a hung child is reliably force-killed. The grace period is unchanged (KILL_PROCESS_GRACE_PERIOD_MS = 500). No PET source is touched and there is no extension-API change.Tests
nativePythonFinder.killProcess.unit.test.ts(6 cases, sinon fake timers):SIGTERM→SIGKILLon the captured child, synchronous ownership release, exit-during-grace not force-killed, already-exited child not signalled (holder still cleared), a replacement assigned during the grace period is never killed (key regression), andSIGTERMerrors are caught/logged without throwing. All 6 pass;npm run lintandnpm run compile-testsare clean.