perf(onboard): overlap gateway readiness probes

[HOYALIM]

Summary

• start the host HTTP readiness probe while OpenShell gateway metadata is collected during gateway health polling
• abort the in-flight HTTP probe when metadata is unhealthy, so dependent work does not continue in the background
• add AbortSignal support to the production host HTTP readiness probe and cover cancellation behavior

Refs #3775

Scope

This PR covers the dependency-safe gateway-readiness overlap slice of #3775. It intentionally does not close the broader prompt-cancellation, full resume-flow, or background setup failure criteria from that issue; those remain follow-up work.

Notes

This keeps the existing polling budget and output behavior. The overlap is limited to one dependency-safe probe inside each health polling attempt: HTTP readiness can run while metadata commands collect status/info, but the result is only used after metadata is healthy.

Tests

• PATH=/Users/holim/.nvm/versions/node/v22.22.2/bin:$PATH npx vitest run src/lib/onboard/gateway-health-wait.test.ts src/lib/onboard/gateway-binding.test.ts src/lib/onboard/gateway-http-readiness.test.ts
• PATH=/Users/holim/.nvm/versions/node/v22.22.2/bin:$PATH npx @biomejs/biome check src/lib/onboard.ts src/lib/onboard/gateway-binding.ts src/lib/onboard/gateway-binding.test.ts src/lib/onboard/gateway-health-wait.ts src/lib/onboard/gateway-health-wait.test.ts src/lib/onboard/gateway-http-readiness.ts src/lib/onboard/gateway-http-readiness.test.ts
• PATH=/Users/holim/.nvm/versions/node/v22.22.2/bin:$PATH npm run build:cli
• PATH=/Users/holim/.nvm/versions/node/v22.22.2/bin:$PATH npm run typecheck:cli
• git diff --check
• git diff origin/main -- src/lib/onboard/gateway-health-wait.test.ts src/lib/onboard/gateway-http-readiness.test.ts | (! rg '^\+.\bif\s\(')

Summary by CodeRabbit

• New Features

  • Gateway HTTP readiness checks now accept an optional cancellation signal to stop probing promptly during startup.

• Bug Fixes

  • Gateway health polling now triggers the HTTP readiness probe on every attempt, running it concurrently with other health checks.
  • If the gateway fails health criteria, any in-flight HTTP readiness probe is canceled and treated as not ready.

• Tests

  • Updated probe-call expectations.
  • Added coverage for probe start order relative to metadata checks and for abort behavior (including ensuring the probe observes the abort).

Signed-off-by: Ho Lim subhoya@gmail.com

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The gateway HTTP readiness probe now accepts an optional AbortSignal and forwards it through onboarding and gateway-binding wrappers. The health-wait loop starts the probe before gateway checks, aborts it on failure, and tests cover call counts, ordering, and abort behavior.

Changes

Cancellable gateway HTTP readiness

Layer / File(s)	Summary
HTTP readiness probe accepts AbortSignal src/lib/onboard/gateway-http-readiness.ts	isGatewayHttpReady and isGatewayHttpReadyImpl accept an optional signal that is passed into the HTTP request and resolves false on abort; the module import order is also adjusted.
Gateway binding forwards signal src/lib/onboard/gateway-binding.ts, src/lib/onboard/gateway-binding.test.ts	The runtime helper now accepts an optional signal and forwards it to probeGatewayHttpReady; tests update expected probe arguments to include the extra trailing undefined.
Concurrent probe start and abort in health-wait loop src/lib/onboard.ts, src/lib/onboard/gateway-health-wait.ts	startAbortableGatewayHttpProbe wraps the readiness check with an AbortController; waitForGatewayHealth starts the probe before CLI status checks, aborts it on health failure, otherwise awaits it. onboard.ts wires the signal through the callback.
Probe and wait-loop test updates src/lib/onboard/gateway-health-wait.test.ts, src/lib/onboard/gateway-http-readiness.test.ts	Health-wait assertions update probe call counts and add ordering/abort coverage; new probe tests verify pre-aborted and in-flight abort behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant onboard.ts
  participant waitForGatewayHealth
  participant HttpProbe
  participant GatewayCommands

  onboard.ts->>waitForGatewayHealth: provide isGatewayHttpReady(signal => ...)
  loop each poll attempt
    waitForGatewayHealth->>HttpProbe: startAbortableGatewayHttpProbe(isGatewayHttpReady)
    waitForGatewayHealth->>GatewayCommands: run status/namedInfo/currentInfo checks
    GatewayCommands-->>waitForGatewayHealth: status outputs
    alt isGatewayHealthy fails
      waitForGatewayHealth->>HttpProbe: abort()
      waitForGatewayHealth-->>onboard.ts: return false
    else healthy
      waitForGatewayHealth->>HttpProbe: await ready
      HttpProbe-->>waitForGatewayHealth: readiness result
      waitForGatewayHealth-->>onboard.ts: return readiness result
    end
  end

Loading

Suggested labels: refactor

Suggested reviewers: kjw3, ericksoa

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: overlapping gateway readiness probes during onboarding.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

This PR reduces onboarding latency by overlapping the host HTTP gateway readiness probe with OpenShell gateway metadata collection during gateway health polling, while adding cancellation support so in-flight probes can be aborted when metadata is unhealthy.

Changes:

• Add AbortSignal support to the production host HTTP readiness probe (isGatewayHttpReady) and wire abort handling into request lifecycle.
• Start the HTTP readiness probe earlier in each gateway health polling attempt and abort it when metadata indicates the gateway is unhealthy.
• Add new unit tests covering abort behavior and update existing gateway health wait tests for the new probe-start timing.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/lib/onboard/gateway-http-readiness.ts	Adds optional AbortSignal support and ensures aborted/failed probes resolve false cleanly.
src/lib/onboard/gateway-http-readiness.test.ts	Adds cancellation-focused tests for the HTTP readiness probe.
src/lib/onboard/gateway-health-wait.ts	Overlaps HTTP probe with OpenShell metadata collection and aborts probe when metadata is unhealthy.
src/lib/onboard/gateway-health-wait.test.ts	Updates expectations for new probe timing and adds tests for overlap + abort propagation.
src/lib/onboard/gateway-binding.ts	Extends the dynamic runtime helper wrapper to optionally forward an AbortSignal to the probe.
src/lib/onboard.ts	Plumbs an abortable isGatewayHttpReady wrapper into waitForGatewayHealth.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/lib/onboard/gateway-binding.ts (1)

235-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant branch: signal === undefined check has no behavioral effect.

deps.probeGatewayHttpReady's 4th parameter is optional with no default value, so passing signal as undefined explicitly is identical to omitting it. The ternary can be dropped.

♻️ Simplify

   const isGatewayHttpReady = (
     timeoutMs?: number,
     url?: string,
     method?: "GET" | "POST",
     signal?: AbortSignal,
   ) => {
     const targetUrl = url ?? `${deps.getDockerDriverGatewayEndpoint(deps.getGatewayPort())}/`;
-    return signal === undefined
-      ? deps.probeGatewayHttpReady(timeoutMs, targetUrl, method)
-      : deps.probeGatewayHttpReady(timeoutMs, targetUrl, method, signal);
+    return deps.probeGatewayHttpReady(timeoutMs, targetUrl, method, signal);
   };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/onboard/gateway-binding.ts` around lines 235 - 245, The
isGatewayHttpReady helper in gateway-binding.ts has a redundant signal ===
undefined branch because deps.probeGatewayHttpReady already treats the 4th
AbortSignal parameter as optional. Simplify the function by removing the ternary
and calling deps.probeGatewayHttpReady directly with timeoutMs, targetUrl,
method, and the signal argument so the behavior stays the same while avoiding
duplicate logic.

src/lib/onboard/gateway-http-readiness.ts (1)

75-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Solid abort implementation — consider Node's native signal support to simplify.

The manual settled/onAbort/listener-cleanup dance is correct (idempotent settle, early-abort short-circuit, listener removed on every settlement path), but Node's http.request() has accepted a signal option natively since v15.3.0 that destroys the request and rejects with an AbortError automatically. Using it would remove most of the manual bookkeeping here (onAbort, request?.destroy() on abort, explicit listener add/remove).

Given the current code is correct and tested, this is optional cleanup rather than a fix.

♻️ Sketch using native `signal` support

   return new Promise<boolean>((resolve) => {
     let settled = false;
-    let request: http.ClientRequest | null = null;
+    let request: http.ClientRequest | null = null;
     const settle = (ready: boolean) => {
       if (settled) return;
       settled = true;
-      signal?.removeEventListener("abort", onAbort);
       resolve(ready);
     };
-    const onAbort = () => {
-      request?.destroy();
-      settle(false);
-    };
-    if (signal?.aborted) {
-      settle(false);
-      return;
-    }
-    signal?.addEventListener("abort", onAbort, { once: true });
     try {
       request = http
-        .request(url, { method }, (res) => {
+        .request(url, { method, signal }, (res) => {
           res.resume();
           const code = res.statusCode || 0;
           settle(GATEWAY_HTTP_ALIVE_CODES.has(code));
         })
         .on("error", () => settle(false));
     } catch {
       settle(false);
       return;
     }

Please confirm this behaves identically (resolves false rather than rejecting) with the Node version this repo targets before adopting.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/onboard/gateway-http-readiness.ts` around lines 75 - 131, The abort
handling in isGatewayHttpReadyImpl is currently implemented manually with
settled/onAbort/listener cleanup, but the comment suggests simplifying it with
Node’s native http.request signal support. Update the request creation path in
isGatewayHttpReady and isGatewayHttpReadyImpl to use the native signal option
only if the repo’s supported Node version behaves identically, and keep the
public behavior unchanged so aborted probes still resolve false instead of
rejecting.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/onboard/gateway-http-readiness.test.ts`:
- Around line 12-32: The test helpers in gateway-http-readiness.test.ts
introduced two new conditional branches in closeServer and listen, triggering
the test-conditionals scan. Refactor those checks in the closeServer and listen
helpers to avoid explicit if statements by using expression-based handling (for
example, ternary/non-null assertion style) or move the helpers into a shared
non-test utility so the test-file scan no longer counts them. Keep the behavior
of server.close and server.address handling unchanged while removing the counted
conditionals.

---

Nitpick comments:
In `@src/lib/onboard/gateway-binding.ts`:
- Around line 235-245: The isGatewayHttpReady helper in gateway-binding.ts has a
redundant signal === undefined branch because deps.probeGatewayHttpReady already
treats the 4th AbortSignal parameter as optional. Simplify the function by
removing the ternary and calling deps.probeGatewayHttpReady directly with
timeoutMs, targetUrl, method, and the signal argument so the behavior stays the
same while avoiding duplicate logic.

In `@src/lib/onboard/gateway-http-readiness.ts`:
- Around line 75-131: The abort handling in isGatewayHttpReadyImpl is currently
implemented manually with settled/onAbort/listener cleanup, but the comment
suggests simplifying it with Node’s native http.request signal support. Update
the request creation path in isGatewayHttpReady and isGatewayHttpReadyImpl to
use the native signal option only if the repo’s supported Node version behaves
identically, and keep the public behavior unchanged so aborted probes still
resolve false instead of rejecting.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 22d84b7a-f1be-4ca4-9956-b0bd37fd7fe9

📥 Commits

Reviewing files that changed from the base of the PR and between f0d2549 and 0a7b82b.

📒 Files selected for processing (6)

• src/lib/onboard.ts
• src/lib/onboard/gateway-binding.ts
• src/lib/onboard/gateway-health-wait.test.ts
• src/lib/onboard/gateway-health-wait.ts
• src/lib/onboard/gateway-http-readiness.test.ts
• src/lib/onboard/gateway-http-readiness.ts

[coderabbitai]

🧹 Nitpick comments (1)

src/lib/onboard/gateway-binding.test.ts (1)

50-58: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a test that actually exercises signal forwarding.

These updates only add a trailing undefined to existing assertions, which passes whether or not signal forwarding is actually implemented correctly — none of the calls pass a real AbortSignal. Consider adding a case that invokes helpers.isGatewayHttpReady(timeoutMs, url, method, someSignal) and asserts probeGatewayHttpReady was called with that exact signal instance, to give behavioral confidence for the new cancellation contract described in context snippet 1 (gateway-binding.ts:230-267).

Based on path instructions for **/*.test.{ts,js,mts,mjs,cts,cjs}: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions" and "Flag... conditionals that make a test pass without exercising its claim."

Also applies to: 68-76, 95-112

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/onboard/gateway-binding.test.ts` around lines 50 - 58, The current
assertions in gateway-binding.test only verify trailing undefined values, so
they do not prove signal forwarding works. Add a test case around
helpers.isGatewayHttpReady (and related gateway helpers if needed) that passes a
real AbortSignal into the public API and asserts probeGatewayHttpReady receives
that exact signal instance. Use the existing helper names to locate the call
path and keep the assertion focused on the observable forwarding behavior rather
than unchanged undefined arguments.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/lib/onboard/gateway-binding.test.ts`:
- Around line 50-58: The current assertions in gateway-binding.test only verify
trailing undefined values, so they do not prove signal forwarding works. Add a
test case around helpers.isGatewayHttpReady (and related gateway helpers if
needed) that passes a real AbortSignal into the public API and asserts
probeGatewayHttpReady receives that exact signal instance. Use the existing
helper names to locate the call path and keep the assertion focused on the
observable forwarding behavior rather than unchanged undefined arguments.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0c56259f-078b-444d-9924-ffec90487ebe

📥 Commits

Reviewing files that changed from the base of the PR and between 2672226 and b1c663d.

📒 Files selected for processing (7)

• src/lib/onboard.ts
• src/lib/onboard/gateway-binding.test.ts
• src/lib/onboard/gateway-binding.ts
• src/lib/onboard/gateway-health-wait.test.ts
• src/lib/onboard/gateway-health-wait.ts
• src/lib/onboard/gateway-http-readiness.test.ts
• src/lib/onboard/gateway-http-readiness.ts

🚧 Files skipped from review as they are similar to previous changes (4)

• src/lib/onboard/gateway-binding.ts
• src/lib/onboard/gateway-health-wait.test.ts
• src/lib/onboard/gateway-health-wait.ts
• src/lib/onboard.ts

[coderabbitai]

🧹 Nitpick comments (1)

src/lib/onboard/gateway-http-readiness.test.ts (1)

21-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor: assertion embedded inside the listen helper.

The expect(address).toEqual(...) at Line 29 lives inside a shared setup helper rather than the test body, so a failure here is attributed to helper code rather than the specific test. Not blocking, purely a clarity nit.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/onboard/gateway-http-readiness.test.ts` around lines 21 - 35, The
assertion inside the shared listen helper is too indirect and should be moved
into the actual test body for clearer failure attribution. Update listen in
gateway-http-readiness.test.ts so it only starts the server and returns the
address/URL, and keep the address shape check in the specific test that calls
listen, using the listen helper’s returned value to assert the port is present.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/lib/onboard/gateway-http-readiness.test.ts`:
- Around line 21-35: The assertion inside the shared listen helper is too
indirect and should be moved into the actual test body for clearer failure
attribution. Update listen in gateway-http-readiness.test.ts so it only starts
the server and returns the address/URL, and keep the address shape check in the
specific test that calls listen, using the listen helper’s returned value to
assert the port is present.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 438cea69-d68f-4875-b10d-ad75a140cff0

📥 Commits

Reviewing files that changed from the base of the PR and between b1c663d and e28bd3d.

📒 Files selected for processing (7)

• src/lib/onboard.ts
• src/lib/onboard/gateway-binding.test.ts
• src/lib/onboard/gateway-binding.ts
• src/lib/onboard/gateway-health-wait.test.ts
• src/lib/onboard/gateway-health-wait.ts
• src/lib/onboard/gateway-http-readiness.test.ts
• src/lib/onboard/gateway-http-readiness.ts

🚧 Files skipped from review as they are similar to previous changes (6)

• src/lib/onboard/gateway-http-readiness.ts
• src/lib/onboard/gateway-health-wait.test.ts
• src/lib/onboard/gateway-binding.ts
• src/lib/onboard/gateway-health-wait.ts
• src/lib/onboard.ts
• src/lib/onboard/gateway-binding.test.ts

[wscurran]

✨ Thanks for the PR. This overlaps gateway readiness probes during onboarding for a performance gain while preserving existing behavior. Ready for maintainer review.

---

Related open issues:

• #3775 perf(onboard): parallelize safe onboard waits with cancellation handling

---

Related open issues:

• #3775 perf(onboard): parallelize safe onboard waits with cancellation handling

[apurvvkumaria]

Exact-head maintainer audit at 3c62590 is complete.

Validated:

• gateway health, binding, and HTTP readiness suites: 33/33 passed;
• Biome check passed for all seven changed files;
• all CodeRabbit/Copilot review threads are resolved;
• the contributor commit is GitHub Verified;
• the scope correctly uses Refs #3775 and does not claim to close the broader issue.

One required contributor gate remains: please add this declaration to the PR description yourself:

Signed-off-by: Ho Lim subhoya@gmail.com

The commit trailer does not replace the required PR-body declaration, and maintainers cannot attest to contributor DCO on the author’s behalf. After that update, the branch needs a current required-check run because the reported CI predates today’s main.

Resolved on exact head 27ed51a: the contributor added the PR-body DCO declaration, the commit is Verified, and the newly released full workflow matrix completed with all 29 checks green.

[cv]

Exact-head independent review at 27ed51a42 is clean. The abortable HTTP readiness probe is started only for the existing gateway-health polling attempt, is cancelled when metadata is unhealthy, and preserves the boolean failure contract; the helper forwards the signal through the production boundary. All 29 checks are green, DCO and the Verified commit pass, CodeRabbit has no unresolved major findings, and the focused gateway health/binding/HTTP readiness coverage passed 33/33.