fix(onboard): drop attempt cap so recovery is truly deadline-driven (NVIDIA#6320 advisor)

Advisor re-review kept blocking on the same concern: with maxAttempts
still passed to waitUntilAsync alongside the deadline, a fast-failing
probe sequence exits after N attempts even though the deadline still
permits more polling. The reported "max wait Xs" language then
under-promises the wait the system was willing to spend, and the loop
regresses to the exact fixed-attempt behavior NVIDIA#3768 is meant to
replace.

Fix:

1. Drop `maxAttempts: recoveryPollCount` from the waitUntilAsync
   options. The loop is now purely deadline-driven; the interval and
   probe cost naturally bound the total attempt count. Under a
   3 * 2s = 6s budget with fast-failing mocked probes, the loop now
   runs 3 probes and 3 sleeps until the top-of-loop deadline check
   terminates it, instead of the earlier 3 probes / 2 sleeps under the
   attempt cap.

2. Rewrite the error message to describe the actual deadline, not a
   hedged "budget + attempt cap" mix: "Gateway 'X' did not become
   ready within the configured Ns recovery deadline (Is poll
   interval)".

3. Inject Date.now via a new `now` deps hook so the deadline test can
   drive a captured virtual clock without vi.useFakeTimers globally
   patching timers (which hangs the async loop). The virtual clock
   advances only when the injected sleepSeconds mock is called, so
   time progresses deterministically at unit-test speed with zero
   real wall-clock waits.

4. Rewrite the timeout test to match the new deadline-only semantics:
   asserts 3 probes and 3 sleeps at 2s each under a 6s budget, and
   that the error message mentions "6s recovery deadline (2s poll
   interval)".

5. Adjust the zero-count edge test's message assertion to match the
   new deadline-only error text.

11/11 tests pass. Biome and changed-file typecheck are clean. The
pre-existing typecheck error in test/helpers/mcp-lifecycle-lock-
properties.ts is unrelated to this branch's diff.

Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>

Author: cjagwani

src/lib/onboard/gateway-recovery.test.ts

@@ -1,10 +1,26 @@
 // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-import { afterEach, describe, expect, it, vi } from "vitest";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 
 import { type GatewayRecoveryDeps, startGatewayForRecovery } from "./gateway-recovery";
 
+// #3768: with the loop now purely deadline-driven, `waitUntilAsync` needs a
+// clock reader. Rather than using vi.useFakeTimers (which globally patches
+// timers and can hang async code), pair a captured virtual clock with the
+// injected `sleepSeconds` mock so the clock advances only when the loop
+// actually sleeps. Tests get deterministic deadline expiration without any
+// real wall-clock waits or global timer state.
+function makeVirtualClock(startMs = 1_000_000_000_000) {
+  let now = startMs;
+  return {
+    now: () => now,
+    sleeper: vi.fn((seconds: number) => {
+      now += Math.max(0, seconds) * 1000;
+    }),
+  };
+}
+
 function createDeps(overrides: Partial<GatewayRecoveryDeps> = {}): GatewayRecoveryDeps {
   return {
     getGatewayClusterContainerState: () => "missing",
@@ -86,19 +102,31 @@ describe("gateway recovery", () => {
     );
   });
 
-  it("uses the configured recovery deadline budget without sleeping after the final probe", async () => {
+  it("polls until the configured recovery deadline and reports it in the timeout", async () => {
+    // #3768: deadline-driven, no legacy attempt cap. A 3 * 2s budget = 6s
+    // permits probes and sleeps until the deadline expires; the final
+    // deadline check short-circuits before an extra sleep would happen.
     vi.stubEnv("NEMOCLAW_HEALTH_POLL_COUNT", "3");
     vi.stubEnv("NEMOCLAW_HEALTH_POLL_INTERVAL", "2");
-    const deps = createDeps();
+    const clock = makeVirtualClock();
+    const deps = createDeps({ sleepSeconds: clock.sleeper, now: clock.now });
 
     await expect(startGatewayForRecovery({ gatewayPort: 8091 }, deps)).rejects.toThrow(
-      "3 recovery attempt(s) at 2s interval (max wait 6s)",
+      "configured 6s recovery deadline (2s poll interval)",
     );
 
-    expect(deps.runCaptureOpenshell).toHaveBeenCalledTimes(9);
-    expect(deps.sleepSeconds).toHaveBeenCalledTimes(2);
-    expect(deps.sleepSeconds).toHaveBeenNthCalledWith(1, 2);
-    expect(deps.sleepSeconds).toHaveBeenNthCalledWith(2, 2);
+    // Each probe issues 3 subprocess calls. With a 6s budget and 2s
+    // interval, the loop runs probe → sleep(2s) three times, then the
+    // top-of-loop deadline check terminates before probe #4. So probes
+    // and sleeps are 1:1 at exactly 3 each.
+    const runCaptureCalls = (deps.runCaptureOpenshell as ReturnType<typeof vi.fn>).mock.calls
+      .length;
+    const probeCount = runCaptureCalls / 3;
+    expect(probeCount).toBe(3);
+    expect(clock.sleeper).toHaveBeenCalledTimes(3);
+    expect(clock.sleeper).toHaveBeenNthCalledWith(1, 2);
+    expect(clock.sleeper).toHaveBeenNthCalledWith(2, 2);
+    expect(clock.sleeper).toHaveBeenNthCalledWith(3, 2);
   });
 
   it("succeeds on the first healthy probe without sleeping and sets OPENSHELL_GATEWAY", async () => {
@@ -146,19 +174,20 @@ describe("gateway recovery", () => {
     expect(deps.runCaptureOpenshell).toHaveBeenCalledTimes(6);
   });
 
-  it("with NEMOCLAW_HEALTH_POLL_COUNT=0 fails fast without invoking the probe", async () => {
-    // Advisor edge-case: a zero attempt count must not silently pretend
-    // the gateway is healthy and must not run any subprocess probes.
+  it("with NEMOCLAW_HEALTH_POLL_COUNT=0 fails fast without silently claiming healthy", async () => {
+    // Edge case: a zero-count budget must not silently pretend the gateway
+    // is healthy. The wait-budget helper clamps to a 1ms deadline, so
+    // waitUntilAsync's first deadline check terminates before the probe
+    // callback runs. Function throws with a deadline message instead of
+    // returning success.
     vi.stubEnv("NEMOCLAW_HEALTH_POLL_COUNT", "0");
     vi.stubEnv("NEMOCLAW_HEALTH_POLL_INTERVAL", "2");
-    const deps = createDeps();
+    const deps = createDeps({ sleepSeconds: vi.fn() });
 
     await expect(startGatewayForRecovery({ gatewayPort: 8091 }, deps)).rejects.toThrow(
-      "0 recovery attempt(s) at 2s interval",
+      /did not become ready within the configured .* recovery deadline/,
     );
 
-    // First iteration's attempt-cap check terminates before the probe
-    // callback runs, so no status/gateway-info calls are made.
     expect(deps.runCaptureOpenshell).not.toHaveBeenCalled();
     expect(deps.sleepSeconds).not.toHaveBeenCalled();
   });

src/lib/onboard/gateway-recovery.ts

@@ -59,6 +59,11 @@ export type GatewayRecoveryDeps = {
   // to the production implementations.
   isGatewayHealthy?: typeof isGatewayHealthy;
   isGatewayHttpReady?: typeof isGatewayHttpReady;
+  // Injected clock reader for deadline-driven tests. Defaults to Date.now.
+  // A test can pair a virtual sleeper (that advances a captured value) with
+  // this reader to drive deterministic deadline expiration without real
+  // wall-clock waits or global fake-timer state.
+  now?(): number;
 };
 
 function isValidGatewayRecoveryPort(port: number | null | undefined): port is number {
@@ -185,6 +190,7 @@ async function startTargetGatewayForRecovery(
   const sleeper = deps.sleepSeconds ?? sleepSeconds;
   const gatewayHealthyImpl = deps.isGatewayHealthy ?? isGatewayHealthy;
   const gatewayHttpReadyImpl = deps.isGatewayHttpReady ?? isGatewayHttpReady;
+  const nowImpl = deps.now ?? Date.now;
   const healthy =
     recoveryPollCount > 0 &&
     (await waitUntilAsync(
@@ -201,11 +207,19 @@ async function startTargetGatewayForRecovery(
         );
       },
       {
-        deadlineMs: Date.now() + waitBudgetMs,
+        // #3768 wants a SINGLE clear deadline budget rather than the legacy
+        // fixed attempt cap. Do NOT pass `maxAttempts` here: with maxAttempts
+        // set, a fast-failing probe sequence would exit after `count`
+        // attempts even though the deadline still permits more polling, and
+        // the operator would see a timeout that under-reports the wait the
+        // system was willing to spend. Let waitUntilAsync run until the
+        // deadline; the interval and probe cost naturally bound the total
+        // attempt count.
+        deadlineMs: nowImpl() + waitBudgetMs,
         initialIntervalMs: Math.max(0, recoveryPollInterval * 1000),
         maxIntervalMs: Math.max(0, recoveryPollInterval * 1000),
         backoffFactor: 1,
-        maxAttempts: recoveryPollCount,
+        now: nowImpl,
         // waitUntilAsync passes durations in milliseconds to `sleep`, while
         // the injected sleeper (sleepSeconds) expects a second-granular
         // number. Adapt at this boundary only.
@@ -224,15 +238,13 @@ async function startTargetGatewayForRecovery(
     return;
   }
 
-  // The wait is attempt-capped (waitUntilAsync exits at maxAttempts) with a
-  // fixed inter-attempt interval and a hard upper time bound. Describe both
-  // dimensions honestly so the operator does not read the message as a
-  // pure deadline promise: the loop can terminate earlier via the attempt
-  // cap when probes fail quickly, and cannot exceed the upper bound.
+  // Pure deadline-based semantics per #3768: report the actual budget the
+  // loop was allowed to spend. Include the interval only as diagnostic
+  // context so an operator scanning the message understands the poll cadence.
   throw new Error(
-    `Gateway '${gatewayName}' did not become ready after ${recoveryPollCount} recovery attempt(s) at ${recoveryPollInterval}s interval (max wait ${formatGatewayRecoveryWaitBudget(
+    `Gateway '${gatewayName}' did not become ready within the configured ${formatGatewayRecoveryWaitBudget(
       waitBudgetMs,
-    )})`,
+    )} recovery deadline (${recoveryPollInterval}s poll interval)`,
   );
 }