fix: promptly reject in-flight PET RPC requests on process exit/error

[StellaHuang95]

Context

NativePythonFinderImpl (src/managers/common/nativePythonFinder.ts) runs the PET (Python Environment Tools) locator as a child process and talks to it over a JSON-RPC connection built on StreamMessageReader/StreamMessageWriter. Callers issue configure/refresh/resolve/info requests wrapped in 30-60s timeouts.

Reproduction

1. Start a refresh/resolve that issues an in-flight PET request.
2. PET exits or crashes (or fails to spawn) while the request is pending.
3. Observe: the pending request does not reject when the child dies — the caller blocks for the full 30-60s request timeout before failing/retrying.

Root cause

start() pipes PET stdout into a PassThrough (readable) with { end: false }:

proc.stdout.pipe(readable, { end: false });

The child exit/error handlers only set flags (processExited, processExitReason); they never end readable. So when PET dies, readable never ends, StreamMessageReader never observes EOF, connection.onClose never runs, and the connection is never disposed. In vscode-jsonrpc, pending requests reject only when connection.dispose() runs (a ResponseError with code PendingResponseRejected). With no disposal, pending requests hang until their own timeouts elapse.

Second latent bug: onClose disposed the shared this.startDisposables. After a restart() reassigns that field to the replacement connection's disposables, a stale old-connection close would dispose the replacement.

Fix — per-child ownership & teardown

Each spawned child now owns its own resources, captured in closures so a dead child only ever tears down its own connection:

• const localDisposables: Disposable[] = [] per start(); this.startDisposables points at it (assigned immediately so a spawn/wiring failure still leaves the graceful-kill disposable reachable for cleanup).
• Idempotent endStreams() ends readable/writable exactly once.
• const proc = spawnProcess(...) captured locally; this.proc = proc. The exit/error handlers reference proc, not this.proc.
• handleChildTermination(reason) always calls endStreams() (so the connection disposes and pending requests reject even for a killed/replaced child) and mutates shared processExited/processExitReason only when this.proc === proc (identity guard).
• onClose disposes the captured localDisposables, never this.startDisposables.

Teardown path: child exit/error → endStreams() → readable EOF → reader close → connection.onClose → dispose localDisposables (incl. the connection) → pending requests reject promptly.

Follow-up fix 1 — recoverable connection-loss classification (review-driven)

Prompt rejection means a mid-request PET crash now surfaces as ResponseError(PendingResponseRejected) instead of rpc.ConnectionError. The refresh/resolve recovery blocks and telemetry classifier previously recognized only RpcTimeoutError/rpc.ConnectionError, so the new rejection would have turned a crash that used to time-out-then-restart-and-retry into an immediate, non-retryable failure. Fixed with one precise, shared helper plus a lifecycle-aware gate:

• isPetConnectionLostError(ex) (src/common/telemetry/errorClassifier.ts) — the single source of truth for "PET connection loss": rpc.ConnectionError or rpc.ResponseError with ErrorCodes.PendingResponseRejected. Used in classifyError() (mapped to connection_error, ordered before the generic ResponseError → rpc_error branch so a dispose-driven rejection isn't mislabeled).
• isRecoverableConnectionLoss(ex) — the lifecycle-aware wrapper used at the recovery call sites: !disposed && !isRestarting && isPetConnectionLostError(ex). The !isRestarting && !disposed guard is essential: restart() and dispose() also dispose the connection and reject in-flight requests with PendingResponseRejected; without the guard a self-inflicted disposal would be mistaken for a fresh crash and trigger a double-restart or kill the freshly-started replacement child. A private disposed flag is set at the very top of dispose().
• The three recovery blocks (resolve catch, doRefresh retry loop, doRefreshAttempt catch) now gate on isRecoverableConnectionLoss(ex) and key their reason/processExitReason strings off ex instanceof RpcTimeoutError (timeout) vs. else (crash) — correct for all three shapes (RpcTimeoutError, ConnectionError, PendingResponseRejected). Configure timeouts remain excluded (configure handles its own retry); a configure-time connection loss correctly still recovers because the child is genuinely gone.

Follow-up fix 2 — avoid ERR_STREAM_WRITE_AFTER_END on exit (review-driven)

exit can fire while proc.stdout still has buffered bytes queued for readable. With stdout still piped, readable.end() could receive those late bytes as a write to an already-ended stream (ERR_STREAM_WRITE_AFTER_END), which escapes once the reader's error handler is torn down. In the per-child endStreams(), the child's stdout is now unpiped from the local readable (and the local writable detached from stdin) before ending/destroying them:

childStdout?.unpipe(readable);
writable.unpipe();
readable.end();
writable.end();

EOF still propagates because readable.end() is unchanged — unpipe only stops future source bytes (already-buffered data flushes before EOF). Both unpipes are no-ops when the pipe was never established/already removed, so they stay safe under duplicate exit/error/dispose, guarded by the existing streamsEnded idempotency flag.

Follow-up fix 3 — a disposed finder must never spawn / restart / retry / CLI-fall-back (review-driven)

Both prior reviews flagged a pre-existing race left out of scope: if dispose() runs while restart() is parked on its exponential-backoff await, the restart resumes after the timer fires and calls start() — spawning a replacement PET child that nothing will ever dispose (its startDisposables/connection were already torn down). A post-dispose resolve() could likewise still reach the CLI fallback. Now that the disposed flag exists, this is closed by using it consistently:

• NativePythonFinderDisposedError — a precise, exported error so an in-flight or post-dispose caller settles exactly once with a recognizable error instead of leaking a process/timer.
• throwIfDisposed() is invoked at every spawn/restart/retry/CLI chokepoint: resolve() and refresh() entry (public in-flight paths fail fast), ensureProcessRunning() (covers in-flight resolve/refresh), restart() before the backoff and immediately after it (before any start()), start() itself (immediately before spawn), and both resolveViaJsonCli()/refreshViaJsonCli() (hard stop before a CLI process).
• Cancelable backoff: restart()'s backoff wait is now cancelable via a cancelRestartBackoff hook. dispose() (after setting disposed = true) calls it, which clears the backoff timer (no leak) and unblocks the wait immediately — restart() then hits the post-backoff throwIfDisposed() and aborts without spawning. The abort is recognized in restart()'s catch, which skips error telemetry/logs (intentional teardown, not a failure) and rethrows the precise error; finally clears the hook and isRestarting.
• CLI fallback: isServerExhausted() returns false when disposed, so neither resolve() nor doRefresh() falls through to the JSON CLI after teardown; resolve()'s outer catch also rethrows the disposed error before any telemetry/CLI. A disposed finder therefore never spawns a server or CLI process, restarts, or retries.

Race & idempotency safety

• Duplicate exit + error: endStreams() is guarded by streamsEnded; connection.dispose() is idempotent; the first processExitReason wins. Harmless.
• Stale child after restart(): the this.proc === proc identity guard prevents a dead child from flipping shared flags on a live replacement; onClose disposing only localDisposables prevents a stale close from tearing down the replacement connection. restart() disposes the old connection before its backoff await, and this.proc isn't reassigned to the replacement until inside start() while isRestarting is still true — so every crash-driven rejection microtask drains while isRecoverableConnectionLoss() returns false, and no stale rejection can restart twice or kill the replacement.
• Dispose during restart backoff: the cancelable backoff + post-backoff disposed re-check means a disposal during the wait aborts the restart promptly (no clock advance needed), spawns no replacement child, and leaks no timer.
• Intentional disposal ≠ crash: disposed is set at the top of dispose() and never reset, so post-dispose PendingResponseRejected rejections are permanently classified non-recoverable, and every lifecycle chokepoint fails fast with NativePythonFinderDisposedError.
• No reentrancy: PassThrough.end() fires close asynchronously; connection.dispose() fires a separate dispose emitter; closeHandler is state-guarded — so onClose → forEach → endStreams cannot recurse.
• restart()/dispose() behavior preserved for the non-disposed path; PET itself is untouched.

Tests

New deterministic unit tests drive the real vscode-jsonrpc connection against a fake child (EventEmitter + PassThrough), stubbing only spawnProcess.

src/test/managers/common/nativePythonFinder.petExit.unit.test.ts (12 tests):

1. Pending request rejects promptly on exit (asserts the specific dispose-driven ResponseError/PendingResponseRejected).
2. Pending request rejects promptly on error.
3. Duplicate error + exit is harmless (idempotent; first reason wins).
4. A stale old-child exit cannot close a replacement connection.
5. restart() produces a usable connection and resets exit state.
6. A current-child crash during refresh promptly restarts and the retry succeeds (end-to-end via a FakePetServer).
7. A connection loss during disposal is NOT treated as a recoverable crash.
8. The refresh retry limit is preserved for connection-loss errors (exactly 2 attempts).
9. Buffered stdout after exit raises no late write/error and does not affect a replacement (asserts stdout.listenerCount('data') === 0 post-exit).
10. Dispose during restart backoff aborts the restart with the disposed error, spawns no replacement child, and leaves no pending timer (clock.countTimers() === 0) — proven without advancing the clock.
11. start() after dispose throws NativePythonFinderDisposedError and spawns no child.
12. resolve()/refresh() after dispose reject with the disposed error and spawn no process.

src/test/common/telemetry/errorClassifier.unit.test.ts: PendingResponseRejected → connection_error, plus an isPetConnectionLostError suite.

Determinism: kickoffInfoFetch is stubbed to avoid the pre-existing uncleared 2s info timer; timing-sensitive tests fake only setTimeout/clearTimeout and restore them; killProcess/doRefreshAttempt are stubbed where a real kill timer or precise attempt count is needed. No uncancelled real timers; the suite exits promptly with no leaked-timer or unhandled-rejection warnings.

npm run lint, npm run compile-tests, and the targeted suites (35 tests) all pass. The full npm run unittest is green (1707 passing); an occasional pre-existing, environment-specific Windows EPERM flake (inlineScriptCacheLayout concurrent rename) is unrelated to these changes.

To expose the production wiring to tests (there is no other injection seam for the spawned PET server), NativePythonFinderImpl and NativePythonFinderDisposedError are exported as @internal test seams only — they are not referenced by src/api.ts and remain out of the public extension API; production still constructs the finder via createNativePythonFinder().

Overlap with #11 — required companion (merge ordering)

PR #11 (fix-pet-force-kill, make PET child-process teardown restart-safe) edits the same nativePythonFinder.ts lifecycle/kill handlers (start()'s exit/error handling and killProcess()). Both branches are cut from main; this PR does not pull in #11's changes, to keep the issue separation.

#11 is the required companion for the pre-existing force-kill escalation (killProcess()'s delayed SIGKILL reads this.proc at fire time). That hardening is intentionally left to #11 and not duplicated here. The two are complementary (force-kill ownership + late-lifecycle events vs. prompt RPC rejection + recoverable-connection-loss classification + dispose-race safety) but will textually conflict in start()/killProcess. Whichever PR merges second must rebase and re-run the combined PET-exit/kill/lifecycle unit suites; no logic from the two fixes is mutually exclusive.

Limitations

• Telemetry label shift (intended). Dispose/restart-driven PendingResponseRejected on an in-flight request now classifies as connection_error rather than rpc_error (result:'error' unchanged) — the more accurate categorization.
• Pre-existing and out of scope (left untouched): sendRequestWithTimeout doesn't clearTimeout on success. The killProcess() delayed-SIGKILL this.proc read is addressed by companion PR fix: make PET child-process teardown restart-safe (force-kill ownership + late lifecycle events) #11 (see above), not here.

[Copilot]

🟢 Approval recommended

The fix is well-scoped, addresses the documented root cause directly, and is backed by deterministic unit tests; the remaining feedback is a minor test-mocking convention issue.

Pull request overview

This PR fixes a hang in NativePythonFinderImpl where in-flight PET JSON-RPC requests could wait out their full 30–60s timeout when the PET child process exits/errors, by ensuring the underlying JSON-RPC streams are ended and the per-child connection is disposed promptly (rejecting pending requests immediately). It also adds focused unit tests that exercise the real vscode-jsonrpc wiring against a fake child process to verify correct teardown and restart behavior.

Changes:

• Refactors PET process lifecycle management so each spawned child owns its own disposables, and termination always ends the RPC streams to trigger connection disposal.
• Guards shared “process exited” state updates with an identity check to prevent stale children from impacting a replacement connection after restart.
• Adds deterministic unit tests covering exit/error teardown, idempotency, stale-child safety, and restart usability.

File summaries

File	Description
src/managers/common/nativePythonFinder.ts	Implements per-child resource ownership and guaranteed stream shutdown on PET exit/error to promptly reject pending RPC requests and avoid stale-close teardown of replacements.
src/test/managers/common/nativePythonFinder.petExit.unit.test.ts	Adds unit tests that drive the real jsonrpc connection against a fake child to validate prompt rejection and restart/stale-child race safety.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[StellaHuang95]

🔒 Automated review in progress — @StellaHuang95 is auto-reviewing this PR.

[StellaHuang95]

Issue · Please address or respond

📍 src/test/managers/common/nativePythonFinder.petExit.unit.test.ts:12

This violates tests-helper-placement.md: all file- and suite-scoped helpers are used only by this suite but precede its tests. Move FakeChild, flush, isPendingResponseRejected, makeOutputChannel, createFinder, getConnection, and getState to the end of the suite block.

[verified]

[StellaHuang95]

Warning · Non-blocking recommendation

📍 src/test/managers/common/nativePythonFinder.petExit.unit.test.ts:118

The tests cast through several private fields and methods (connection, process state, start, and restart), tightly coupling coverage to class layout. Prefer a narrow process/session injection or observable lifecycle seam so internal refactors do not break behavior-focused tests.

[verified]

[StellaHuang95]

Issue · Please address or respond

The file- and suite-local helpers precede the tests. Move them to the end of the smallest suite/file scope that uses them, as required by the test helper-placement convention.

[StellaHuang95]

Issue · Please address or respond

makeOutputChannel() duplicates the existing typed createMockLogOutputChannel() utility and requires an as never cast. Use the shared helper instead.

[StellaHuang95]

Warning · Non-blocking recommendation

Unpiping stdout on exit can prevent a complete response already buffered in the child stream from reaching readable, converting a successful request into PendingResponseRejected. Drain stdout through its close event, with a bounded fallback, before ending the reader; add coverage for delayed delivery of a response written before exit.

[StellaHuang95]

Warning · Non-blocking recommendation

If spawning succeeds but a required stdio stream is absent, this throws before registering the child cleanup disposable, leaving a live child without teardown ownership. Register cleanup immediately after spawning or explicitly terminate the child on this failure path, and cover it with a live fake child missing stdio.

[StellaHuang95]

Minor follow-up: the lifecycle tests access several private fields and methods through casts; a narrow test seam would reduce coupling to the class layout.

[StellaHuang95]

Warning · Non-blocking recommendation

The lifecycle tests cast through numerous private fields and methods. Introduce a narrow process/session test seam so this behavior coverage does not couple to the class's private layout.

[StellaHuang95]

Warning · Non-blocking recommendation

Recovery policy now depends on a telemetry module that imports RpcTimeoutError from this manager, reinforcing a circular dependency. Move the PET error types and predicates into a dependency-neutral common module consumed by both layers.