fix(onboard): add gateway start timeout and increase health poll window (#1830) - #2006
Conversation
…ow (#1830) Two defensive improvements to gateway startup: 1. streamGatewayStart fallback timeout: Add a 600s hard timeout (configurable via NEMOCLAW_GATEWAY_START_TIMEOUT) that kills the openshell gateway start child process if it never exits. Prevents NemoClaw from hanging indefinitely when Docker daemon is unresponsive or k3s enters a restart loop. 2. Increase post-start health poll defaults from 5×2s=10s to 12×5s=60s on x86. After openshell gateway start returns (container healthy at the Docker layer), the application-layer connectivity check (gRPC, TLS handshake, port mapping) may need additional time. 60s provides a reasonable buffer. ARM64 defaults (30×10s=300s) unchanged. All values remain overridable via NEMOCLAW_HEALTH_POLL_COUNT and NEMOCLAW_HEALTH_POLL_INTERVAL. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a configurable hard timeout to the gateway start stream and lengthened the default post-start health polling parameters in Changes
Sequence Diagram(s)sequenceDiagram
participant Nemoclaw as Nemoclaw (parent)
participant OpenShell as OpenShell CLI (child)
participant Gateway as Gateway Cluster (k3s/Docker)
Nemoclaw->>OpenShell: spawn gateway start
Note right of OpenShell: streamGatewayStart() streams logs and starts hard timeout
OpenShell->>Gateway: init/start cluster
Gateway-->>OpenShell: progress / readiness updates
Nemoclaw->>Gateway: periodic health checks (NEMOCLAW_HEALTH_POLL_*)
alt start exceeds timeout
Nemoclaw->>OpenShell: send SIGTERM (timeout)
Nemoclaw->>OpenShell: schedule SIGKILL fallback (10s)
OpenShell-->>Nemoclaw: close with status:1 (timeout-triggered)
else start completes
OpenShell-->>Nemoclaw: close with exit code
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 2613-2615: The envInt calls for health polling can return 0 which
disables the loop; clamp both results to a minimum of 1 by wrapping the
envInt(...) calls in Math.max(1, envInt(...)) (or equivalent) so healthPollCount
and healthPollInterval are never 0; update the assignments to healthPollCount
and healthPollInterval (the variables used in the for loop starting at the for
(let i = 0; i < healthPollCount; i++) {) accordingly.
- Around line 387-391: The timeout handler currently calls child.kill("SIGTERM")
and immediately calls finish(), which can resolve before the child actually
exits; change the handler to send SIGTERM then wait for the child process to
emit its exit/close event (or await a short grace-period fallback and then
escalate to SIGKILL) before calling finish(), using the existing child reference
and the finish/settled guard; ensure killTimer is still cleared and any
escalation to SIGKILL is logged, and reuse/detect destroyGateway() semantics so
the promise only resolves once the child has truly terminated (use
child.on("exit"/"close") or a Promise that races a grace timeout and then
force-kills).
🪄 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: Pro Plus
Run ID: a2ee58c5-989d-4108-8a05-56e3bb04ea72
📒 Files selected for processing (1)
src/lib/onboard.ts
…tions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
…#1830) On timeout, send SIGTERM and let the `close` event resolve the promise so the child has actually exited before the caller proceeds to retry. If SIGTERM is ignored, escalate to SIGKILL after 10s. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard.ts (1)
2616-2652:⚠️ Potential issue | 🟠 MajorRetry still destroys gateways that may only be slow-starting.
At Line 2640 every exhausted Layer 2 poll path throws the same generic failure, and Lines 2650-2652 destroy the gateway on any retriable failure. That still tears down a container that is merely late to finish app-layer bring-up, which loses the k3s image cache this flow is trying to preserve. Please gate
destroyGateway()on an explicit container state/health check and skip destroy while the gateway is still running in a transientstartingstate.
♻️ Duplicate comments (1)
src/lib/onboard.ts (1)
2623-2624:⚠️ Potential issue | 🟡 MinorClamp health-poll overrides to at least 1.
envInt()allows0, soNEMOCLAW_HEALTH_POLL_COUNT=0skips the loop entirely and fails immediately.NEMOCLAW_HEALTH_POLL_INTERVAL=0also collapses retries into effectively back-to-back probes.Suggested fix
- const healthPollCount = envInt("NEMOCLAW_HEALTH_POLL_COUNT", isArm64 ? 30 : 12); - const healthPollInterval = envInt("NEMOCLAW_HEALTH_POLL_INTERVAL", isArm64 ? 10 : 5); + const healthPollCount = Math.max( + 1, + envInt("NEMOCLAW_HEALTH_POLL_COUNT", isArm64 ? 30 : 12), + ); + const healthPollInterval = Math.max( + 1, + envInt("NEMOCLAW_HEALTH_POLL_INTERVAL", isArm64 ? 10 : 5), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 2623 - 2624, envInt currently permits zero which allows health polling to be skipped or interval to be zero; clamp the computed values for healthPollCount and healthPollInterval to a minimum of 1 after calling envInt (i.e., compute healthPollCount = Math.max(1, envInt("NEMOCLAW_HEALTH_POLL_COUNT", isArm64 ? 30 : 12)) and healthPollInterval = Math.max(1, envInt("NEMOCLAW_HEALTH_POLL_INTERVAL", isArm64 ? 10 : 5))) so the loop always runs at least once and intervals are never zero; update any references to healthPollCount and healthPollInterval accordingly.
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
2537-2686: SplitstartGatewayWithOptions()before adding more startup paths.This function is already handling reuse detection, cleanup, retry orchestration, timeout logging, and Layer 2 probing in one block. Extracting the retry/poll path into helpers would make these new failure modes much easier to reason about.
As per coding guidelines, "Limit cyclomatic complexity to 20 in JavaScript/TypeScript files, with target of 15".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 2537 - 2686, startGatewayWithOptions is too large/complex; extract the retry and Layer-2 health-poll logic into helper functions to reduce cyclomatic complexity. Specifically, move the pRetry wrapper and its inner async block (the call to streamGatewayStart + the health polling loop that calls runCaptureOpenshell and isGatewayHealthy) into a new function (e.g., attemptGatewayStart or startGatewayWithRetry) and factor the inner polling into a separate helper (e.g., waitForGatewayHealth) that returns success/throws on failure; keep reuse detection, cleanup (ssh-keygen/known_hosts), and env setup in startGatewayWithOptions and call the new helpers (referencing startGatewayWithOptions, streamGatewayStart, pRetry, waitForGatewayHealth, and isGatewayHealthy) so the main function’s complexity drops below the threshold.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/onboard.ts`:
- Around line 2623-2624: envInt currently permits zero which allows health
polling to be skipped or interval to be zero; clamp the computed values for
healthPollCount and healthPollInterval to a minimum of 1 after calling envInt
(i.e., compute healthPollCount = Math.max(1,
envInt("NEMOCLAW_HEALTH_POLL_COUNT", isArm64 ? 30 : 12)) and healthPollInterval
= Math.max(1, envInt("NEMOCLAW_HEALTH_POLL_INTERVAL", isArm64 ? 10 : 5))) so the
loop always runs at least once and intervals are never zero; update any
references to healthPollCount and healthPollInterval accordingly.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 2537-2686: startGatewayWithOptions is too large/complex; extract
the retry and Layer-2 health-poll logic into helper functions to reduce
cyclomatic complexity. Specifically, move the pRetry wrapper and its inner async
block (the call to streamGatewayStart + the health polling loop that calls
runCaptureOpenshell and isGatewayHealthy) into a new function (e.g.,
attemptGatewayStart or startGatewayWithRetry) and factor the inner polling into
a separate helper (e.g., waitForGatewayHealth) that returns success/throws on
failure; keep reuse detection, cleanup (ssh-keygen/known_hosts), and env setup
in startGatewayWithOptions and call the new helpers (referencing
startGatewayWithOptions, streamGatewayStart, pRetry, waitForGatewayHealth, and
isGatewayHealthy) so the main function’s complexity drops below the threshold.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 247ffc72-7761-4bb8-b588-7a3c81aff80f
📒 Files selected for processing (1)
src/lib/onboard.ts
As detailed in the Root Cause Analysis section of the PR description, this scenario does not occur with current OpenShell (0.0.26+): When The Layer 2 poll only runs after See the "What happens when openshell gateway start fails (exit non-zero)" section in the PR description for the full trace with source code references. |
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
LGTM — well-scoped defensive fix with correct timeout escalation (SIGTERM → grace → SIGKILL), proper timer cleanup, and thorough root cause analysis.
Suggestion for a follow-up PR: Consider clamping envInt results for NEMOCLAW_HEALTH_POLL_COUNT and NEMOCLAW_HEALTH_POLL_INTERVAL to a minimum of 1 (e.g. Math.max(1, envInt(...))). Setting either to 0 via env var currently skips the poll loop or collapses retries. Pre-existing issue, not introduced here, but worth hardening.
3 participants
fix(onboard): add gateway start timeout and increase health poll window (#1830)
Summary
Two defensive improvements to gateway startup: (1) add a hard timeout to
streamGatewayStartto prevent NemoClaw from hanging indefinitely if theopenshell gateway startprocess never exits, and (2) increase the post-start health poll window from 10s to 60s to give the application-layer connectivity check (gRPC,TLS, port mapping) more time after the container reports healthy.
Related Issue
Fixes #1830
Changes
NEMOCLAW_GATEWAY_START_TIMEOUT) tostreamGatewayStart()that kills the child process if it hasn't exited. Covers edge cases where Docker daemon is unresponsive or k3s enters a restart loop that never terminates.openshell gateway start) confirms k3s/pods/TLS are ready; Layer 2 (NemoClaw's post-start poll viaopenshell status+gateway info)confirms the host-side CLI can connect over gRPC. The previous 10s window was tight for Layer 2 propagation. ARM64 defaults (30×10s=300s) are unchanged.
NEMOCLAW_HEALTH_POLL_COUNTandNEMOCLAW_HEALTH_POLL_INTERVAL.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)AI Disclosure
Root Cause Analysis
Full analysis of the #1830 failure scenario, based on reading OpenShell source code (
crates/openshell-bootstrap/src/runtime.rs,lib.rs,docker.rs):Two-Layer Health Checking Architecture
Layer 1 — Container health (inside OpenShell):
wait_for_gateway_ready()polls Docker container HEALTHCHECK status for up to 360s (180×2s). The healthcheck script verifies DNS, k8s API, StatefulSet readiness, TLS secrets, and NodePort connectivity. Docker HEALTHCHECK config:interval=5s, timeout=5s, start-period=20s, retries=60— container staysstartingfor up to 320s before becomingunhealthy.Layer 2 — Application connectivity (inside NemoClaw): Post-start poll checks
openshell status(gRPC connectivity) andopenshell gateway info(metadata). Container HEALTHY ≠ CLI connected — gRPC init, TLS handshake, and port mapping propagation may need additional seconds.Why the 15+ minute hang occurs on slow environments
On slow environments (macOS + Docker Desktop, first-run image pulls):
openshell gateway startcreates container + volumeunhealthywait_for_gateway_ready()sees unhealthy → returns errordestroy_gateway_resources(), added in fix: macOS local inference DNS resolution + oMLX provider #464) deletes container + volume + network + imagedestroyGateway()is a no-op (OpenShell already cleaned up)The cache loss happens inside OpenShell's auto-cleanup, not in NemoClaw's
destroyGateway(). The root constraint is Docker's 320s healthcheck window being insufficient for slow first-run environments.What this PR fixes vs. what requires OpenShell changes
Reproduction
Reproduced on Ubuntu 24.04 with network throttling (200kbit). Confirmed via
docker psthat container washealth: startingduringopenshell gateway startbut gone after it returned non-zero — OpenShell's auto-cleanup removed it before NemoClaw's post-start poll ran.Verification of fix
Tested on Ubuntu 24.04 (clean install, no cached images):
Gateway started successfully. Layer 2 poll passed within 2 iterations (~10s).
Summary by CodeRabbit
Signed-off-by: Yanyun Liao yanyunl@nvidia.com