fix: bound end-to-end refresh discovery latency

[StellaHuang95]

Summary

Bounds the end-to-end latency of a NativePythonFinder refresh so a single stuck/slow PET run can no longer stall discovery indefinitely. One monotonic operation budget is captured at enqueue time and used two ways: the WorkerPool rejects the item if it is still queued when the budget elapses, and the same deadline clamps every extension-controlled running stage (configure / refresh / resolve / restart backoff / CLI fallback). This caps both (1) distinct-key queue waits behind a stuck refresh in the 1-worker pool and (2) CLI-fallback enrichment that otherwise scales with the environment count — without ever truncating the set of discovered environments and without changing resolve() or any non-refresh caller.

This revision incorporates two rounds of review feedback: the queue expiry is now absolute (not timer-only), restart() rechecks the deadline after its backoff and before spawn, a deadline-clamped stage timeout is reclassified to a budget error before ordinary stage telemetry/counters mutate, the operation budget now reserves the full CLI enumeration timeout so a valid CLI fallback is never cut to a small remainder, and an enrichment-incomplete refresh is returned to the caller but not cached so a later refresh retries enrichment.

Operation budget — exact timing sequence / arithmetic

The budget is derived from existing stage constants, not a magic number, and is the maximum of the two reachable successful paths so it can never truncate either valid flow:

computeRefreshOperationBudgetMs() = max(
    computeServerRefreshBudgetMs()      = 184_000,   // full successful server retry path
    computeCliFallbackPathBudgetMs()    = 214_000    // worst server attempt + transition + full CLI scan
) = 214_000 ms

Server retry path (184s). A successful doRefresh runs at most MAX_REFRESH_RETRIES + 1 = 2 attempts. The attained worst-case is reachable when the operation enters with an elevated restart state (restartAttempts = 2) and a pending configure-timeout backoff (configureRetry.timeoutCount = 1) while the process is healthy:

Attempt 0 (fails with a retryable refresh-RPC timeout → triggers the single retry;
            NO restart precedes it, so the extended configure timeout is NOT reset):
    configure (extended after a prior timeout)  MAX_CONFIGURE_TIMEOUT_MS   = 60_000
    refresh RPC timeout                          REFRESH_TIMEOUT_MS         = 30_000
                                                                          = 90_000

Attempt 1 (restarts the process killed by attempt 0, then succeeds;
            the restart resets configure back to its base timeout):
    restart backoff (highest reachable attempt)
        RESTART_BACKOFF_BASE_MS * 2^(MAX_RESTART_ATTEMPTS-1) = 1_000 * 2^2 =  4_000
    configure (reset to base by the restart)     CONFIGURE_TIMEOUT_MS       = 30_000
    refresh RPC                                  REFRESH_TIMEOUT_MS         = 30_000
    parallel resolve enrichment (bounded)        RESOLVE_TIMEOUT_MS         = 30_000
                                                                          = 94_000

server path = 90_000 + 94_000 = 184_000 ms

Key coupling that makes this an attained maximum, not a loose over-approximation: restart() calls configureRetry.reset(), so a single attempt can never have both a restart and a 60s extended configure. The failing attempt hits the 60s configure maximum (no restart); the succeeding attempt hits the restart-backoff maximum (configure reset to 30s). Their maxima occur on different attempts under the same reachable entry state, so summing them is legitimate. The formula is expressed in terms of MAX_REFRESH_RETRIES so it scales if the retry count changes.

CLI-fallback path (214s). A valid CLI fallback is itself an existing successful path and must keep its established enumeration timeout instead of being cut to whatever the server path leaves behind. It is bounded as one worst failing server attempt + the restart/kill transition into the fallback + a full CLI scan:

computeCliFallbackPathBudgetMs():
    worst failing server attempt   MAX_CONFIGURE_TIMEOUT_MS + REFRESH_TIMEOUT_MS = 90_000
    transition (one max restart backoff)                                        =  4_000
    full CLI enumeration           CLI_FALLBACK_TIMEOUT_MS                       = 120_000
                                                                               = 214_000 ms

To keep the server path from consuming this reserve, doRefresh skips the second server retry whenever another attempt would leave less than a full CLI scan:

retryWouldStarveCliFallbackMs(remaining) =
    remaining < worstRetryCost (4_000 + 30_000 + 30_000 = 64_000)
              + cliReserve     (CLI_FALLBACK_TIMEOUT_MS 120_000 + MIN_STAGE_BUDGET_MS 1_000 = 121_000)
    = remaining < 185_000

So after a 90s failing server attempt (≈124s of a fresh 214s budget remaining), the guard trips (124_000 < 185_000) and we fall back to the CLI immediately with the full ~124s — a 60s valid CLI scan is always allowed after a server failure while total latency stays ≤ 214s. This deliberately avoids stacking every pathological timeout into an arbitrary cap: the budget is exactly the larger of two successful paths.

Floor: MIN_STAGE_BUDGET_MS = 1_000. Below 1s remaining we fail fast instead of handing a stage a near-zero timeout that cannot complete (the fastest PET JSON-RPC round-trip, info, is already budgeted a "generous" 2s).

Concrete reproductions this fixes

• Queue wait behind a stuck refresh: PET hangs on a refresh for cache key A (the single worker is occupied). A refresh for a different key B is enqueued behind it. Before: B waits unbounded for A. After: B carries its own 214s deadline; if it is still queued when the budget elapses, the pool removes it and rejects with QueueTaskExpiredError — it never executes, and later refreshes keep working. The expiry is absolute: even if the expiry timer callback is delayed (event-loop stall), next() rechecks now() >= expiresAt before dequeuing, so a past-deadline item can never start.
• Large-environment CLI enrichment: server mode is exhausted, so the CLI fallback runs pet find --json and then resolves each incomplete environment (batched by CLI_RESOLVE_CONCURRENCY). On a machine with hundreds of incomplete envs, enrichment time scales with the count. After: once the shared budget is spent, enrichment stops and every already-discovered environment is still returned (unresolved records retained as-is). The result is additionally flagged complete: false so it is returned to the caller but not written to the soft cache, and a later refresh retries the enrichment.

Deadline & state-machine semantics

• Deadline is an absolute, monotonic deadline (performance.now(), injectable for tests). remainingMs() counts down; isExhausted(floor) is remaining < floor (strict).
• clampTimeoutToRemaining(base, deadline?, stage, floor?): returns base unchanged when deadline is undefined (preserves non-refresh callers), throws RefreshBudgetExceededError when remaining < floor, else returns min(base, remaining).
• WorkerPool absolute pending-task expiration (addToQueue(item, position?, expiresInMs?)): the queued wrapper stores an absolute expiresAt (from an injectable now clock, default Date.now) in addition to arming a setTimeout. Transitions are queued → running | expired → settled, exactly once:
  • expire() (timer wins while queued): guarded by running/expired flags and an indexOf check; removes the item from the queue and settles it once via settleExpired().
  • next() (dequeue): rechecks now() >= expiresAt before transitioning to running, so a delayed timer / event-loop stall can never let a past-deadline item run — it is settled as expired and the loop continues to the next queued item. On a genuine dequeue it sets running = true and clears the timer before the worker starts (one-way transition; a running item can never expire).
  • Timers are cleared on dequeue, on completed() (settle), and on clear() (stop). No Promise.race, no abandoned task. Behavior is unchanged when expiresInMs is omitted (original unbounded queueing), keeping the pool generic even though only the finder uses it today.
• restart() deadline recheck: the (clamped) exponential backoff now runs through backoffThenCheckBudget(waitMs, deadline), which waits and then rechecks the deadline immediately before teardown / state-reset / spawn — PET is never started after the budget is spent. If the budget expires during the wait, it throws RefreshBudgetExceededError; the catch undoes the speculative restartAttempts++ (no process was spawned) and skips restart-error telemetry. The pre-existing entry-level fail-fast check (before any state mutation) is retained.
• Deadline provenance (toBudgetError(ex, deadline, stage, wasClamped)): only a stage whose timeout was actually clamped to the remaining budget — i.e. the deadline, not the stage's own base timeout, was the binding constraint (effectiveTimeout < baseTimeout, computed at clamp time by each caller) — is reclassified. Such a clamped stage that times out necessarily consumed the whole budget, so its RpcTimeoutError is converted to RefreshBudgetExceededError before configure()/doRefreshAttempt() mutate ordinary stage retry counters (configureRetry.onTimeout()) or emit PET_CONFIGURE/PET_REFRESH stage-timeout telemetry — so a budget cap is never misattributed to a slow PET. A stage that ran on its unclamped base timeout and merely finished with little budget left (e.g. a 30s timeout that started with 30.5s remaining) is a genuine PET timeout and keeps normal timeout/retry/recovery handling. The reclassification uses this clamp-time signal, not a post-hoc deadline.isExhausted() check, so a real slow-PET timeout at a budget boundary is never swallowed by the budget path.

Uncancellable PET boundary (explicitly out of scope)

This change bounds only extension-controlled waits. It reuses the existing sendRequestWithTimeout cancellation (CancellationTokenSource + timer) and the existing process-termination paths (killProcess) to stop waiting and, where applicable, kill a hung PET process. It does not add cooperative cancellation inside PET (the Rust locator).

Decision on terminating in-flight PET for the deadline (requested in review): enforcing the operation deadline does not terminate the PET process. Exhausting our budget does not mean PET is unhealthy (it may simply be enumerating a very large environment set), and sendRequestWithTimeout already cancels the in-flight RPC via its CancellationTokenSource. So a budget-clamped configure/refresh timeout flows through the budget path without killing the process or incrementing normal configure-timeout state; the warm process is preserved for the next operation (which starts with a fresh deadline). Ordinary (non-budget) refresh/connection timeouts retain their existing kill-for-restart behavior. PET-side cooperative cancellation remains future work.

Safety & compatibility

• resolve() and every other non-refresh caller pass no deadline, so clampTimeoutToRemaining returns the base timeout, restart backoff is unclamped (backoffThenCheckBudget just waits), and resolveViaJsonCli's default timeout is unchanged — existing behavior preserved. resolve() does not use the pool and still returns NativeEnvInfo.
• The internal RefreshResult { info, complete } is confined to the refresh worker path (doRefresh/doRefreshAttempt/refreshViaJsonCli → handleHardRefresh). Consumers of refresh() still receive NativeInfo[]; handleHardRefresh always returns the full enumerated list and only decides whether to cache it based on complete. Because it cache.delete(key) up front, an uncached partial leaves no entry, so the next soft refresh re-hard-refreshes and retries enrichment.
• A find enumeration that times out still rejects (surfaced as a budget error when clamped) — it never returns a truncated enumeration. Only enrichment (per-env resolve) after a successful find degrades to "retain all + mark incomplete".
• The budget error never retries and never falls back to the CLI once the whole budget is spent (the CLI shares the same deadline); it propagates cleanly and the in-flight cache slot is cleared on both success and failure paths, exactly as today.
• QueueTaskExpiredError and RefreshBudgetExceededError are both time-budget exhaustions, classified via the existing telemetry patterns as rpc_timeout (so isTimeoutErrorType records timeout). The instanceof branch is placed before message-pattern matching (the budget error message can contain the word "restart", which would otherwise mis-classify as process_crash).
• No PET changes, no unrelated flows touched, no cross-platform path assumptions (ReturnType<typeof setTimeout>, perf_hooks).

Tests

• WorkerPool (workerPool.unit.test.ts, deterministic fake timers):
  • Existing timer-driven suite: queued-behind-never-resolving expires and never runs; dequeue clears the timer; expiry/dequeue boundary settles exactly once (both directions); stop clears the timer; later tasks still run; omitting expiresInMs preserves unbounded queueing.
  • New absolute-deadline suite (injected clock decoupled from the faked setTimeout): an event-loop stall where the clock jumps past the deadline but the timer has not fired — next()'s absolute recheck expires the item, it never runs, and later work still runs; a boundary case where now == expiresAt expires (>=); next() skips a stalled-expired item and continues to the next valid queued item; and an already-expired enqueue (expiresInMs <= 0) rejects immediately without stranding the single parked worker (later work still runs, proving the worker is re-parked rather than lost).
• Finder budget (nativePythonFinder.budget.unit.test.ts, injected clock, pure/injected-clock seams):
  • computeServerRefreshBudgetMs = 184000, computeCliFallbackPathBudgetMs = 214000, computeRefreshOperationBudgetMs = max = 214000, and a boundedness assertion (not an arbitrary stack).
  • retryWouldStarveCliFallbackMs: 185000 threshold; true when a retry would starve the CLI (e.g. 124s remaining); false with ample budget; and an explicit proof that a 60s valid CLI scan remains fundable after a 90s server failure while total latency stays bounded.
  • decideRefreshRetry (composed retry/CLI-fallback decision): the post-failure control flow of doRefresh is extracted into a pure, exported decision function so the composition — not just the arithmetic — is unit-tested without spawning a real PET. Cases: a budget error stops immediately; a retryable failure with ample budget retries; a regression test proving server exhaustion → CLI fallback when another server attempt would starve the reserved CLI budget (this is the exact path the earlier 184s budget bug got wrong); a sub-floor budget yields a budget error; the final attempt falls back iff server mode is exhausted else rethrows; and the no-deadline (resolve-like) path always retries a mid-attempt failure.
  • toBudgetError provenance: undefined with no deadline / for a non-timeout error / for an unclamped stage while the budget is healthy; a regression proving an unclamped 30s stage that started with 30.5s remaining and times out is not reclassified as a budget error even though the deadline is now exhausted (the wasClamped clamp-time signal, not a post-hoc isExhausted(), gates reclassification); reclassifies a genuinely deadline-clamped RpcTimeoutError; and does not reclassify a clamped non-timeout error. The RefreshBudgetExceededError message is asserted exactly.
  • backoffThenCheckBudget: no-throw with no deadline (still waits); rejects when the budget expires during the wait; resolves when budget remains.
  • Deadline, clampTimeoutToRemaining passthrough/clamp/floor-throw, and stage-to-stage propagation shrinking the clamp then failing fast.
• Classifier (errorClassifier.unit.test.ts): both new errors classify as rpc_timeout and register as timeouts.
• Targeted run: 67 passing. Full suite: 1738 passing / 5 pending (1739 tests total — the single failure is the pre-existing, unrelated inlineScriptCacheLayout concurrent writeMetaJson calls Windows fs.rename EPERM flake, which passes in isolation and is outside this diff). Lint clean, compile-tests clean.

Limitations

• Contention trade-off (by design): because one deadline bounds enqueue-to-settle, a distinct-key refresh queued behind a slow (but not stuck) refresh can fail fast instead of waiting. This is the intended bound; startup fan-out coalesces (all managers refresh key 'all', de-duplicated via inFlightRefreshes), so genuinely distinct-key contention is rare.
• End-to-end stage-timeout propagation through the live NativePythonFinderImpl (spawning a real PET process and driving RPC over stdio) is not exercised in-process because the impl is not exported and its constructor spawns a process — adding a spawn/connection injection seam would be a broad test-only export. Instead the composed decision that had the CLI-starvation bug (decideRefreshRetry) is now a pure, directly-tested seam, layered on the real WorkerPool queue path plus the exported budget primitives (Deadline, clampTimeoutToRemaining, toBudgetError, backoffThenCheckBudget, retryWouldStarveCliFallbackMs) the stages are built from.
• PET-side cooperative cancellation is out of scope (see boundary note above).
• The branch could not be renamed to bound-discovery-latency (the rename tooling was unavailable in this environment); the intended name is noted here.

[Copilot]

🟡 Changes recommended

A moderate CLI telemetry issue and two documentation nits remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Bounds NativePythonFinder refresh latency with a shared deadline across queued work, PET stages, and CLI enrichment.

Changes:

• Adds monotonic refresh budgets and timeout clamping.
• Adds optional queued-task expiration to WorkerPool.
• Adds partial CLI telemetry and timeout classification tests.

File summaries

File	Review status
src/test/managers/common/nativePythonFinder.budget.unit.test.ts	Reviewed; no final comments.
src/test/common/utils/workerPool.unit.test.ts	Reviewed; no final comments.
src/test/common/telemetry/errorClassifier.unit.test.ts	Reviewed; no final comments.
src/managers/common/nativePythonFinder.ts	Moderate issue: mark final-batch budget exhaustion as partial telemetry. Nit: correct “at or below” to “below.”
src/common/utils/workerPool.ts	Reviewed; no final comments.
src/common/telemetry/errorClassifier.ts	Reviewed; no final comments.
src/common/telemetry/constants.ts	Nit: document partial as a valid result.

Review details

Suppressed comments (1)

src/managers/common/nativePythonFinder.ts:1533

• The deadline is absolute, but resolveTimeout is sampled once before batch.map and reused for all four child processes. Since each resolveViaJsonCli starts its timer only when that map callback runs, spawn/setup time can make later children run past the shared deadline (especially when only the 1s floor remains). Clamp from the Deadline per child at invocation, or pass the absolute deadline into the process runner.

            let resolveTimeout: number;
            try {
                resolveTimeout = clampTimeoutToRemaining(CLI_FALLBACK_TIMEOUT_MS, deadline, 'cli_resolve');

• Files reviewed: 7/7 changed files
• Comments generated: 3
• 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]

GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for src/managers/common/nativePythonFinder.ts:L663.

Warning · Non-blocking recommendation

After sleeping for all remaining time, restart() starts PET without rechecking the deadline, so timer delay may allow state mutation and process spawning after expiration. Recheck immediately after the backoff.

[verified]

[StellaHuang95]

Warning · Non-blocking recommendation

Track whether the CLI find timeout was actually deadline-clamped and reclassify only that timeout-specific failure. The current post-failure deadline?.isExhausted() check can turn an unclamped CLI timeout or unrelated CLI failure that happens near the budget boundary into RefreshBudgetExceededError; the enrichment catches have the same provenance problem. Add an injected-clock regression for the ordinary-timeout boundary.

[verified]