From c2228b3ad5e631f21aff35a90a9bda06ae26d3b9 Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Sat, 9 May 2026 10:11:23 +0000 Subject: [PATCH 01/10] fix(onboard): require host HTTP readiness before reusing the gateway Closes #3258 Signed-off-by: Tinson Lai --- src/lib/onboard.ts | 195 ++++++++++++++++- test/gateway-http-reuse-wait.test.ts | 312 +++++++++++++++++++++++++++ 2 files changed, 500 insertions(+), 7 deletions(-) create mode 100644 test/gateway-http-reuse-wait.test.ts diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 2985e19dafe..ec11cde9ba3 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -3480,6 +3480,107 @@ function gatewayClusterHealthcheckPassed(): boolean { return result.status === 0; } +type WaitForGatewayHttpReadyOpts = { + probe?: () => Promise; + sleeper?: (seconds: number) => void; + maxAttempts?: number; + intervalSeconds?: number; +}; + +/** + * Resolve poll count and interval (seconds) for the reuse-time gateway HTTP + * readiness wait. Defaults are tighter than the startup health wait because + * reuse only needs to verify a previously-warm gateway is still serving — + * not wait for a cold k3s cluster to come up. + * + * Override via `NEMOCLAW_REUSE_HEALTH_POLL_COUNT` and + * `NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL`. + */ +function getGatewayReuseHealthWaitConfig(): { count: number; interval: number } { + return { + count: envInt("NEMOCLAW_REUSE_HEALTH_POLL_COUNT", 6), + interval: envInt("NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL", 5), + }; +} + +/** + * HTTP status codes that indicate the gateway dispatcher is healthy. + * + * Mirrors the established whitelist in `verify-deployment.ts`: 200 = serving, + * 401 = device-auth gate is enabled but the gateway is running. Anything else + * — including 404, 403, 502, transport errors — is treated as not ready. + */ +const GATEWAY_HTTP_ALIVE_CODES = new Set([200, 401]); + +/** + * Probe the host-level gateway HTTP endpoint at `http://127.0.0.1:${GATEWAY_PORT}/`. + * + * Returns true when the gateway responds with a known-alive status code, + * false on any other status (notably 5xx from a warming upstream) or any + * transport-level error. + * + * Doesn't depend on Docker — issues a direct HTTP request to the host port. + * That makes it the right probe for the Docker-state-`unknown` branch where + * the docker daemon is itself flaky. + * + * `url` is overridable for unit tests; production callers use the default. + */ +function isGatewayHttpReady( + timeoutMs = 3000, + url = `http://127.0.0.1:${GATEWAY_PORT}/`, +): Promise { + const http = require("http"); + return new Promise((resolve) => { + let settled = false; + const settle = (ready: boolean) => { + if (settled) return; + settled = true; + resolve(ready); + }; + const request = http + .get(url, (res: import("node:http").IncomingMessage) => { + res.resume(); + const code = res.statusCode || 0; + settle(GATEWAY_HTTP_ALIVE_CODES.has(code)); + }) + .on("error", () => settle(false)); + request.setTimeout(timeoutMs, () => { + request.destroy(); + settle(false); + }); + }); +} + +/** + * Poll the gateway HTTP endpoint until it returns ready or the configured + * budget is exhausted. Returns true on the first ready response, false if + * no attempt succeeds within the budget. + * + * Used at gateway-reuse decision sites to catch the case where the container + * is running (or Docker can't be probed) but the gateway upstream is still + * warming up — e.g. immediately after `colima stop && colima start`. Without + * this, openshell CLI metadata reports "healthy" from the previous run and + * onboard skips startup, only to fail later in step 4 with "Connection + * refused". See #3258 (regression of #2020). + * + * `probe` and `sleeper` are injectable for unit testing. + */ +async function waitForGatewayHttpReady( + opts: WaitForGatewayHttpReadyOpts = {}, +): Promise { + const probe = opts.probe ?? (() => isGatewayHttpReady()); + const sleeper = opts.sleeper ?? sleep; + const config = getGatewayReuseHealthWaitConfig(); + const maxAttempts = opts.maxAttempts ?? config.count; + const intervalSeconds = opts.intervalSeconds ?? config.interval; + + for (let attempt = 0; attempt < maxAttempts; attempt++) { + if (await probe()) return true; + if (attempt < maxAttempts - 1) sleeper(intervalSeconds); + } + return false; +} + function repairGatewayBootstrapSecrets(): { repaired: boolean; missingSecrets: string[] } { const missingSecrets = listMissingGatewayBootstrapSecrets(); const plan = getGatewayBootstrapRepairPlan(missingSecrets); @@ -3970,9 +4071,42 @@ async function preflight( gatewayReuseState = "missing"; console.log(" ✓ Stale gateway metadata cleaned up"); } else if (containerState === "unknown") { + // Docker probe failed but cached metadata says healthy. Try the host-level + // HTTP probe — it doesn't depend on Docker, so it can confirm the gateway + // is genuinely serving even when the daemon is flaky. + // + // Per #2020 the "unknown" state must remain non-destructive: if Docker is + // unavailable, destroying and recreating cannot succeed anyway. Just warn + // and let the caller surface the failure later. + if (await waitForGatewayHttpReady()) { + console.log( + " Warning: could not verify gateway container state (Docker may be unavailable), but the gateway is responding on HTTP. Proceeding with reuse.", + ); + } else { + // Per #2020 we must not tear the gateway down in the unknown branch + // (if Docker is unavailable, recreate cannot succeed either). But + // continuing to trust cached "healthy" leads to the #3258 step-4 + // "Connection refused" failure. Compromise: downgrade reuse state so + // the regular gateway-start path takes over and surfaces a clearer + // error if it cannot proceed. + console.log( + " Warning: could not verify gateway container state and the gateway is not responding on HTTP. Treating as unhealthy; will attempt to start a fresh gateway.", + ); + gatewayReuseState = "missing"; + } + } else if (!(await waitForGatewayHttpReady())) { + // Container is running but the gateway HTTP endpoint is not responding. + // Common immediately after a Docker daemon restart — the container comes + // back before the OpenShell gateway upstream finishes warming up. Safe to + // recreate because Docker is functional. See #3258. console.log( - " Warning: could not verify gateway container state (Docker may be unavailable). Proceeding with cached health status.", + ` Gateway container is running but http://127.0.0.1:${GATEWAY_PORT}/ is not responding. Recreating...`, ); + runOpenshell(["forward", "stop", String(DASHBOARD_PORT)], { ignoreError: true }); + destroyGateway(); + registry.clearAll(); + gatewayReuseState = "missing"; + console.log(" ✓ Stale gateway cleaned up"); } else { const imageDrift = getGatewayClusterImageDrift(); if (imageDrift) { @@ -4204,10 +4338,20 @@ async function startGatewayWithOptions( gatewaySnapshot.activeGatewayInfo, ) ) { - console.log(" ✓ Reusing existing gateway"); - runOpenshell(["gateway", "select", GATEWAY_NAME], { ignoreError: true }); - process.env.OPENSHELL_GATEWAY = GATEWAY_NAME; - return; + // Final reuse gate — `isGatewayHealthy()` parses openshell CLI metadata, + // which can be stale when the gateway container was just restarted (e.g. + // after `colima stop && colima start`). Verify the gateway HTTP endpoint + // is actually serving before declaring reuse, so we don't skip startup + // and fail later in step 4 with "Connection refused". See #3258. + if (await isGatewayHttpReady()) { + console.log(" ✓ Reusing existing gateway"); + runOpenshell(["gateway", "select", GATEWAY_NAME], { ignoreError: true }); + process.env.OPENSHELL_GATEWAY = GATEWAY_NAME; + return; + } + console.log( + ` Gateway metadata reports healthy but http://127.0.0.1:${GATEWAY_PORT}/ is not responding. Starting a fresh gateway...`, + ); } // When a stale gateway is detected (metadata exists but container is gone, @@ -4304,7 +4448,12 @@ async function startGatewayWithOptions( ignoreError: true, }); const currentInfo = runCaptureOpenshell(["gateway", "info"], { ignoreError: true }); - if (isGatewayHealthy(status, namedInfo, currentInfo)) { + // Require BOTH the openshell CLI metadata to report healthy AND the + // host HTTP endpoint to be serving — the CLI metadata can report + // healthy from the previous run while the upstream is still warming + // up after a Docker daemon restart, leading to "Connection refused" + // in step 4. See #3258. + if (isGatewayHealthy(status, namedInfo, currentInfo) && (await isGatewayHttpReady())) { return; // success } if (i < healthPollCount - 1) sleep(healthPollInterval); @@ -9871,9 +10020,38 @@ async function onboard(opts: OnboardOptions = {}): Promise { gatewayReuseState = "missing"; console.log(" ✓ Stale gateway metadata cleaned up"); } else if (containerState === "unknown") { + // Docker probe failed but cached metadata says healthy. Try the host-level + // HTTP probe — it doesn't depend on Docker, so it can confirm the gateway + // is genuinely serving even when the daemon is flaky. + if (await waitForGatewayHttpReady()) { + console.log( + " Warning: could not verify gateway container state (Docker may be unavailable), but the gateway is responding on HTTP. Proceeding with reuse.", + ); + } else { + // Per #2020 we must not tear the gateway down in the unknown branch + // (if Docker is unavailable, recreate cannot succeed either). But + // continuing to trust cached "healthy" leads to the #3258 step-4 + // "Connection refused" failure. Compromise: downgrade reuse state so + // the regular gateway-start path takes over and surfaces a clearer + // error if it cannot proceed. + console.log( + " Warning: could not verify gateway container state and the gateway is not responding on HTTP. Treating as unhealthy; will attempt to start a fresh gateway.", + ); + gatewayReuseState = "missing"; + } + } else if (!(await waitForGatewayHttpReady())) { + // Container is running but the gateway HTTP endpoint is not responding. + // Common immediately after a Docker daemon restart — the container comes + // back before the OpenShell gateway upstream finishes warming up. Safe to + // recreate because Docker is functional. See #3258. console.log( - " Warning: could not verify gateway container state (Docker may be unavailable). Proceeding with cached health status.", + ` Gateway container is running but http://127.0.0.1:${GATEWAY_PORT}/ is not responding. Recreating...`, ); + runOpenshell(["forward", "stop", String(DASHBOARD_PORT)], { ignoreError: true }); + destroyGateway(); + registry.clearAll(); + gatewayReuseState = "missing"; + console.log(" ✓ Stale gateway cleaned up"); } else { const imageDrift = getGatewayClusterImageDrift(); if (imageDrift) { @@ -10451,7 +10629,10 @@ module.exports = { getGatewayStartEnv, getGatewayClusterContainerState, getGatewayHealthWaitConfig, + getGatewayReuseHealthWaitConfig, getGatewayReuseState, + isGatewayHttpReady, + waitForGatewayHttpReady, handleFinalGatewayStartFailure, getNavigationChoice, getSandboxInferenceConfig, diff --git a/test/gateway-http-reuse-wait.test.ts b/test/gateway-http-reuse-wait.test.ts new file mode 100644 index 00000000000..2312d3bad38 --- /dev/null +++ b/test/gateway-http-reuse-wait.test.ts @@ -0,0 +1,312 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +// Verify that gateway-reuse waits for the host-level HTTP endpoint to start +// returning 2xx (or 401) before declaring the gateway reusable. Without this, +// a gateway whose container is up but whose upstream is still warming up +// (e.g. immediately after a Docker daemon restart) gets reused with stale +// CLI metadata, leading to "Connection refused" later in onboard. +// +// Also verifies the Docker-state-`unknown` branch stays non-destructive +// (#2020 invariant) — when the docker daemon is itself flaky, destroying and +// recreating the gateway cannot succeed anyway. +// +// See: https://github.com/NVIDIA/NemoClaw/issues/3258 +// Regression of: https://github.com/NVIDIA/NemoClaw/issues/2020 + +import http from "node:http"; +import { createRequire } from "node:module"; +import fs from "node:fs"; +import { type AddressInfo } from "node:net"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +const require = createRequire(import.meta.url); +const onboardModule = require("../dist/lib/onboard.js") as { + getGatewayReuseHealthWaitConfig: () => { count: number; interval: number }; + isGatewayHttpReady: (timeoutMs?: number, url?: string) => Promise; + waitForGatewayHttpReady: (opts?: { + probe?: () => Promise; + sleeper?: (seconds: number) => void; + maxAttempts?: number; + intervalSeconds?: number; + }) => Promise; +}; +const { getGatewayReuseHealthWaitConfig, isGatewayHttpReady, waitForGatewayHttpReady } = + onboardModule; + +/** Spin up a tiny HTTP server that returns the given status code, return its URL. */ +async function startStatusServer(statusCode: number): Promise<{ + url: string; + close: () => Promise; +}> { + const server = http.createServer((_req, res) => { + res.statusCode = statusCode; + res.end(); + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const port = (server.address() as AddressInfo).port; + return { + url: `http://127.0.0.1:${port}/`, + close: () => + new Promise((resolve, reject) => + server.close((err) => (err ? reject(err) : resolve())), + ), + }; +} + +const ROOT = path.resolve(import.meta.dirname, ".."); + +describe("getGatewayReuseHealthWaitConfig (#3258)", () => { + const originalCount = process.env.NEMOCLAW_REUSE_HEALTH_POLL_COUNT; + const originalInterval = process.env.NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL; + + beforeEach(() => { + delete process.env.NEMOCLAW_REUSE_HEALTH_POLL_COUNT; + delete process.env.NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL; + }); + + afterEach(() => { + if (originalCount === undefined) delete process.env.NEMOCLAW_REUSE_HEALTH_POLL_COUNT; + else process.env.NEMOCLAW_REUSE_HEALTH_POLL_COUNT = originalCount; + if (originalInterval === undefined) delete process.env.NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL; + else process.env.NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL = originalInterval; + }); + + it("defaults to 6 polls × 5s when no env overrides are set", () => { + expect(getGatewayReuseHealthWaitConfig()).toEqual({ count: 6, interval: 5 }); + }); + + it("respects NEMOCLAW_REUSE_HEALTH_POLL_COUNT", () => { + process.env.NEMOCLAW_REUSE_HEALTH_POLL_COUNT = "12"; + expect(getGatewayReuseHealthWaitConfig().count).toBe(12); + }); + + it("respects NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL", () => { + process.env.NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL = "2"; + expect(getGatewayReuseHealthWaitConfig().interval).toBe(2); + }); + + it("falls back to defaults when env values are non-finite", () => { + process.env.NEMOCLAW_REUSE_HEALTH_POLL_COUNT = "not-a-number"; + process.env.NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL = ""; + expect(getGatewayReuseHealthWaitConfig()).toEqual({ count: 6, interval: 5 }); + }); +}); + +describe("isGatewayHttpReady status-code semantics (#3258)", () => { + it("returns true for 200", async () => { + const server = await startStatusServer(200); + try { + expect(await isGatewayHttpReady(2000, server.url)).toBe(true); + } finally { + await server.close(); + } + }); + + it("returns true for 401 (device-auth gate enabled, gateway is alive)", async () => { + const server = await startStatusServer(401); + try { + expect(await isGatewayHttpReady(2000, server.url)).toBe(true); + } finally { + await server.close(); + } + }); + + it("returns false for 502 (gateway up but k3s upstream still warming)", async () => { + const server = await startStatusServer(502); + try { + expect(await isGatewayHttpReady(2000, server.url)).toBe(false); + } finally { + await server.close(); + } + }); + + it("returns false for 404 (root not handled — not a healthy signal)", async () => { + const server = await startStatusServer(404); + try { + expect(await isGatewayHttpReady(2000, server.url)).toBe(false); + } finally { + await server.close(); + } + }); + + it("returns false for 403", async () => { + const server = await startStatusServer(403); + try { + expect(await isGatewayHttpReady(2000, server.url)).toBe(false); + } finally { + await server.close(); + } + }); + + it("returns false on connection refused", async () => { + // 127.0.0.1:1 is reserved and not in use — get connection refused. + expect(await isGatewayHttpReady(2000, "http://127.0.0.1:1/")).toBe(false); + }); +}); + +describe("waitForGatewayHttpReady (#3258)", () => { + it("returns true on the first probe call when the gateway is already responding", async () => { + let calls = 0; + const sleeps: number[] = []; + const result = await waitForGatewayHttpReady({ + probe: async () => { + calls += 1; + return true; + }, + sleeper: (s: number) => sleeps.push(s), + maxAttempts: 6, + intervalSeconds: 5, + }); + expect(result).toBe(true); + expect(calls).toBe(1); + expect(sleeps).toEqual([]); + }); + + it("retries until the probe passes, sleeping between attempts", async () => { + let calls = 0; + const sleeps: number[] = []; + const result = await waitForGatewayHttpReady({ + probe: async () => { + calls += 1; + return calls >= 3; + }, + sleeper: (s: number) => sleeps.push(s), + maxAttempts: 6, + intervalSeconds: 5, + }); + expect(result).toBe(true); + expect(calls).toBe(3); + // Sleeps happen between attempts only — two failures → two sleeps before the success. + expect(sleeps).toEqual([5, 5]); + }); + + it("returns false when the probe never passes within the budget", async () => { + let calls = 0; + const sleeps: number[] = []; + const result = await waitForGatewayHttpReady({ + probe: async () => { + calls += 1; + return false; + }, + sleeper: (s: number) => sleeps.push(s), + maxAttempts: 4, + intervalSeconds: 3, + }); + expect(result).toBe(false); + expect(calls).toBe(4); + // No trailing sleep after the final failed attempt. + expect(sleeps).toEqual([3, 3, 3]); + }); + + it("respects an attempt count of 1 — single probe, no sleeps", async () => { + let calls = 0; + const sleeps: number[] = []; + const result = await waitForGatewayHttpReady({ + probe: async () => { + calls += 1; + return false; + }, + sleeper: (s: number) => sleeps.push(s), + maxAttempts: 1, + intervalSeconds: 5, + }); + expect(result).toBe(false); + expect(calls).toBe(1); + expect(sleeps).toEqual([]); + }); +}); + +describe("gateway-reuse HTTP readiness wait integration (#3258)", () => { + const content = fs.readFileSync(path.join(ROOT, "src/lib/onboard.ts"), "utf-8"); + + it("preflight uses host-level HTTP probe, not docker exec", () => { + const preflightStart = content.indexOf("async function preflight("); + const preflightEnd = content.indexOf("async function startGatewayWithOptions("); + expect(preflightStart).toBeGreaterThanOrEqual(0); + expect(preflightEnd).toBeGreaterThan(preflightStart); + const preflightSection = content.slice(preflightStart, preflightEnd); + expect(preflightSection).toMatch(/await waitForGatewayHttpReady\(\)/); + }); + + it("main onboard uses host-level HTTP probe, not docker exec", () => { + const onboardSection = content.slice(content.indexOf("async function onboard(")); + expect(onboardSection).toMatch(/await waitForGatewayHttpReady\(\)/); + }); + + it("isGatewayHttpReady probes 127.0.0.1:GATEWAY_PORT and only accepts an explicit alive whitelist", () => { + expect(content).toMatch(/`http:\/\/127\.0\.0\.1:\$\{GATEWAY_PORT\}\/`/); + // Mirrors verify-deployment.ts: only 200 (serving) and 401 (device-auth + // gate enabled, gateway is running) count as alive. Everything else — + // including 404, 403, 502, transport errors — is "not ready". + expect(content).toMatch(/GATEWAY_HTTP_ALIVE_CODES = new Set\(\[200, 401\]\)/); + expect(content).toMatch(/GATEWAY_HTTP_ALIVE_CODES\.has\(code\)/); + }); + + it("downgrades reuse state to 'missing' when container is running and HTTP never recovers", () => { + // Both reuse sites must follow the pattern: containerState === "running" + // path → wait for HTTP → on failure, destroy and downgrade. + const downgrades = content.match( + /!\(await waitForGatewayHttpReady\(\)\)[\s\S]{0,1500}?destroyGateway\(\)[\s\S]{0,300}?gatewayReuseState = "missing"/g, + ); + expect(downgrades).toBeTruthy(); + if (!downgrades) { + throw new Error('Expected !(await waitForGatewayHttpReady) → destroyGateway → "missing" downgrade'); + } + expect(downgrades.length).toBeGreaterThanOrEqual(2); + }); + + it("startGatewayWithOptions health-poll requires HTTP readiness alongside CLI metadata (#3258)", () => { + // After the reuse gate falls through and the start path runs, the + // post-start health-poll loop must require BOTH `isGatewayHealthy(...)` + // (CLI metadata) AND host HTTP readiness before declaring success. + // The CLI metadata can be stale-but-healthy while the upstream is still + // warming up; the HTTP probe rules that out. + const startGwIdx = content.indexOf("async function startGatewayWithOptions("); + expect(startGwIdx).toBeGreaterThanOrEqual(0); + const startGwSection = content.slice(startGwIdx, startGwIdx + 8000); + // The single-loop success gate must AND the two probes together. + expect(startGwSection).toMatch( + /isGatewayHealthy\([^)]*\) && \(await isGatewayHttpReady\(\)\)/, + ); + }); + + it("startGatewayWithOptions HTTP-gates the final reuse decision (#3258)", () => { + // The early-return reuse path inside startGatewayWithOptions must also + // probe HTTP readiness — otherwise a fresh OpenShell CLI snapshot can + // re-trigger "Reusing existing gateway" with stale-but-CLI-healthy + // metadata, bypassing the upstream HTTP wait performed in onboard(). + const startGwIdx = content.indexOf("async function startGatewayWithOptions("); + expect(startGwIdx).toBeGreaterThanOrEqual(0); + const startGwSection = content.slice(startGwIdx, startGwIdx + 4000); + // Within the isGatewayHealthy(...) reuse branch, isGatewayHttpReady must + // be awaited before "Reusing existing gateway" is logged. + expect(startGwSection).toMatch( + /isGatewayHealthy\([\s\S]*?await isGatewayHttpReady\(\)[\s\S]*?Reusing existing gateway/, + ); + }); + + it("downgrades reuse state to 'missing' in unknown+HTTP-fail without destroying the gateway", () => { + // Per #2020 we must not destroyGateway() in the unknown branch (Docker may + // be unavailable; recreate cannot succeed). But per #3258 we must not let + // cached "healthy" carry through either. Compromise: set state to + // "missing" so the regular gateway-start path runs and surfaces a clearer + // error if it can't proceed. + const unknownBlockRegex = + /containerState === "unknown"[\s\S]*?(?=\}\s*else if \(!\(await waitForGatewayHttpReady)/g; + const unknownBlocks = content.match(unknownBlockRegex); + expect(unknownBlocks).toBeTruthy(); + if (!unknownBlocks) { + throw new Error('Expected containerState === "unknown" branches followed by HTTP-wait fallthrough'); + } + expect(unknownBlocks.length).toBeGreaterThanOrEqual(2); + for (const block of unknownBlocks) { + // No destructive cleanup inside the unknown branch. + expect(block).not.toMatch(/destroyGateway\(\)/); + expect(block).not.toMatch(/registry\.clearAll\(\)/); + // But the HTTP-fail sub-branch must downgrade reuse state. + expect(block).toMatch(/gatewayReuseState = "missing"/); + } + }); +}); From 0517eb9fa5534875694771e4436e46aeb4b56eb1 Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Sat, 9 May 2026 10:29:29 +0000 Subject: [PATCH 02/10] fix(onboard): require HTTP probe in recoverGatewayRuntime; drop source-text test asserts Address CodeRabbit review on #3258: - Extend host HTTP readiness gate to both return sites in recoverGatewayRuntime() so the recovery path cannot return success on stale CLI metadata while http://localhost:8080/ is still 502. - Replace port-1 connection-refused probe with an ephemeral-port bind+close helper for deterministic local closure. - Drop the source-text integration assertions; runtime behaviour is covered by the live HTTP server tests against isGatewayHttpReady and the injectable-probe tests for waitForGatewayHttpReady. Signed-off-by: Tinson Lai --- src/lib/onboard.ts | 12 ++- test/gateway-http-reuse-wait.test.ts | 114 ++++----------------------- 2 files changed, 26 insertions(+), 100 deletions(-) diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index ec11cde9ba3..f2c8be8aa3c 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -4616,7 +4616,11 @@ function applyOverlayfsAutoFix(upstreamImage: string): string | null { async function recoverGatewayRuntime() { runOpenshell(["gateway", "select", GATEWAY_NAME], { ignoreError: true }); let status = runCaptureOpenshell(["status"], { ignoreError: true }); - if (status.includes("Connected") && isSelectedGateway(status)) { + if ( + status.includes("Connected") && + isSelectedGateway(status) && + (await isGatewayHttpReady()) + ) { process.env.OPENSHELL_GATEWAY = GATEWAY_NAME; return true; } @@ -4658,7 +4662,11 @@ async function recoverGatewayRuntime() { attachGatewayMetadataIfNeeded(); } status = runCaptureOpenshell(["status"], { ignoreError: true }); - if (status.includes("Connected") && isSelectedGateway(status)) { + if ( + status.includes("Connected") && + isSelectedGateway(status) && + (await isGatewayHttpReady()) + ) { process.env.OPENSHELL_GATEWAY = GATEWAY_NAME; const runtime = getContainerRuntime(); if (shouldPatchCoredns(runtime)) { diff --git a/test/gateway-http-reuse-wait.test.ts b/test/gateway-http-reuse-wait.test.ts index 2312d3bad38..d676a1b4873 100644 --- a/test/gateway-http-reuse-wait.test.ts +++ b/test/gateway-http-reuse-wait.test.ts @@ -16,9 +16,7 @@ import http from "node:http"; import { createRequire } from "node:module"; -import fs from "node:fs"; import { type AddressInfo } from "node:net"; -import path from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; const require = createRequire(import.meta.url); @@ -35,6 +33,18 @@ const onboardModule = require("../dist/lib/onboard.js") as { const { getGatewayReuseHealthWaitConfig, isGatewayHttpReady, waitForGatewayHttpReady } = onboardModule; +/** Bind an ephemeral localhost port, close it, and return its URL — a port + * that's guaranteed to refuse connections for the lifetime of the test. */ +async function getClosedLocalUrl(): Promise { + const server = http.createServer(); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const port = (server.address() as AddressInfo).port; + await new Promise((resolve, reject) => + server.close((err) => (err ? reject(err) : resolve())), + ); + return `http://127.0.0.1:${port}/`; +} + /** Spin up a tiny HTTP server that returns the given status code, return its URL. */ async function startStatusServer(statusCode: number): Promise<{ url: string; @@ -55,8 +65,6 @@ async function startStatusServer(statusCode: number): Promise<{ }; } -const ROOT = path.resolve(import.meta.dirname, ".."); - describe("getGatewayReuseHealthWaitConfig (#3258)", () => { const originalCount = process.env.NEMOCLAW_REUSE_HEALTH_POLL_COUNT; const originalInterval = process.env.NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL; @@ -141,8 +149,10 @@ describe("isGatewayHttpReady status-code semantics (#3258)", () => { }); it("returns false on connection refused", async () => { - // 127.0.0.1:1 is reserved and not in use — get connection refused. - expect(await isGatewayHttpReady(2000, "http://127.0.0.1:1/")).toBe(false); + // Bind and immediately close an ephemeral port so the address is + // guaranteed unreachable — more deterministic than relying on port 1. + const url = await getClosedLocalUrl(); + expect(await isGatewayHttpReady(2000, url)).toBe(false); }); }); @@ -218,95 +228,3 @@ describe("waitForGatewayHttpReady (#3258)", () => { }); }); -describe("gateway-reuse HTTP readiness wait integration (#3258)", () => { - const content = fs.readFileSync(path.join(ROOT, "src/lib/onboard.ts"), "utf-8"); - - it("preflight uses host-level HTTP probe, not docker exec", () => { - const preflightStart = content.indexOf("async function preflight("); - const preflightEnd = content.indexOf("async function startGatewayWithOptions("); - expect(preflightStart).toBeGreaterThanOrEqual(0); - expect(preflightEnd).toBeGreaterThan(preflightStart); - const preflightSection = content.slice(preflightStart, preflightEnd); - expect(preflightSection).toMatch(/await waitForGatewayHttpReady\(\)/); - }); - - it("main onboard uses host-level HTTP probe, not docker exec", () => { - const onboardSection = content.slice(content.indexOf("async function onboard(")); - expect(onboardSection).toMatch(/await waitForGatewayHttpReady\(\)/); - }); - - it("isGatewayHttpReady probes 127.0.0.1:GATEWAY_PORT and only accepts an explicit alive whitelist", () => { - expect(content).toMatch(/`http:\/\/127\.0\.0\.1:\$\{GATEWAY_PORT\}\/`/); - // Mirrors verify-deployment.ts: only 200 (serving) and 401 (device-auth - // gate enabled, gateway is running) count as alive. Everything else — - // including 404, 403, 502, transport errors — is "not ready". - expect(content).toMatch(/GATEWAY_HTTP_ALIVE_CODES = new Set\(\[200, 401\]\)/); - expect(content).toMatch(/GATEWAY_HTTP_ALIVE_CODES\.has\(code\)/); - }); - - it("downgrades reuse state to 'missing' when container is running and HTTP never recovers", () => { - // Both reuse sites must follow the pattern: containerState === "running" - // path → wait for HTTP → on failure, destroy and downgrade. - const downgrades = content.match( - /!\(await waitForGatewayHttpReady\(\)\)[\s\S]{0,1500}?destroyGateway\(\)[\s\S]{0,300}?gatewayReuseState = "missing"/g, - ); - expect(downgrades).toBeTruthy(); - if (!downgrades) { - throw new Error('Expected !(await waitForGatewayHttpReady) → destroyGateway → "missing" downgrade'); - } - expect(downgrades.length).toBeGreaterThanOrEqual(2); - }); - - it("startGatewayWithOptions health-poll requires HTTP readiness alongside CLI metadata (#3258)", () => { - // After the reuse gate falls through and the start path runs, the - // post-start health-poll loop must require BOTH `isGatewayHealthy(...)` - // (CLI metadata) AND host HTTP readiness before declaring success. - // The CLI metadata can be stale-but-healthy while the upstream is still - // warming up; the HTTP probe rules that out. - const startGwIdx = content.indexOf("async function startGatewayWithOptions("); - expect(startGwIdx).toBeGreaterThanOrEqual(0); - const startGwSection = content.slice(startGwIdx, startGwIdx + 8000); - // The single-loop success gate must AND the two probes together. - expect(startGwSection).toMatch( - /isGatewayHealthy\([^)]*\) && \(await isGatewayHttpReady\(\)\)/, - ); - }); - - it("startGatewayWithOptions HTTP-gates the final reuse decision (#3258)", () => { - // The early-return reuse path inside startGatewayWithOptions must also - // probe HTTP readiness — otherwise a fresh OpenShell CLI snapshot can - // re-trigger "Reusing existing gateway" with stale-but-CLI-healthy - // metadata, bypassing the upstream HTTP wait performed in onboard(). - const startGwIdx = content.indexOf("async function startGatewayWithOptions("); - expect(startGwIdx).toBeGreaterThanOrEqual(0); - const startGwSection = content.slice(startGwIdx, startGwIdx + 4000); - // Within the isGatewayHealthy(...) reuse branch, isGatewayHttpReady must - // be awaited before "Reusing existing gateway" is logged. - expect(startGwSection).toMatch( - /isGatewayHealthy\([\s\S]*?await isGatewayHttpReady\(\)[\s\S]*?Reusing existing gateway/, - ); - }); - - it("downgrades reuse state to 'missing' in unknown+HTTP-fail without destroying the gateway", () => { - // Per #2020 we must not destroyGateway() in the unknown branch (Docker may - // be unavailable; recreate cannot succeed). But per #3258 we must not let - // cached "healthy" carry through either. Compromise: set state to - // "missing" so the regular gateway-start path runs and surfaces a clearer - // error if it can't proceed. - const unknownBlockRegex = - /containerState === "unknown"[\s\S]*?(?=\}\s*else if \(!\(await waitForGatewayHttpReady)/g; - const unknownBlocks = content.match(unknownBlockRegex); - expect(unknownBlocks).toBeTruthy(); - if (!unknownBlocks) { - throw new Error('Expected containerState === "unknown" branches followed by HTTP-wait fallthrough'); - } - expect(unknownBlocks.length).toBeGreaterThanOrEqual(2); - for (const block of unknownBlocks) { - // No destructive cleanup inside the unknown branch. - expect(block).not.toMatch(/destroyGateway\(\)/); - expect(block).not.toMatch(/registry\.clearAll\(\)/); - // But the HTTP-fail sub-branch must downgrade reuse state. - expect(block).toMatch(/gatewayReuseState = "missing"/); - } - }); -}); From 0a847e5ba68b557494b45b9916f44513cadec8e5 Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Sat, 9 May 2026 10:38:28 +0000 Subject: [PATCH 03/10] fix(test): strip trailing blank line in gateway-http-reuse-wait end-of-file-fixer hook in CI rejects extra trailing newline. Signed-off-by: Tinson Lai --- test/gateway-http-reuse-wait.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/gateway-http-reuse-wait.test.ts b/test/gateway-http-reuse-wait.test.ts index d676a1b4873..0df68aa16fa 100644 --- a/test/gateway-http-reuse-wait.test.ts +++ b/test/gateway-http-reuse-wait.test.ts @@ -227,4 +227,3 @@ describe("waitForGatewayHttpReady (#3258)", () => { expect(sleeps).toEqual([]); }); }); - From 42026a493bd21a1599660ced7e3f728c2fe56920 Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Sat, 9 May 2026 10:52:32 +0000 Subject: [PATCH 04/10] docs(commands): document NEMOCLAW_REUSE_HEALTH_POLL_* env vars Mention the new gateway-reuse HTTP readiness probe added in #3258 and the two override knobs (NEMOCLAW_REUSE_HEALTH_POLL_COUNT, NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL) in the onboard preflight section. Signed-off-by: Tinson Lai --- docs/reference/commands.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/reference/commands.md b/docs/reference/commands.md index ccc622f805f..4805fb3cae2 100644 --- a/docs/reference/commands.md +++ b/docs/reference/commands.md @@ -185,6 +185,9 @@ It verifies that Docker is reachable, warns on untested runtimes such as Podman, The preflight also enforces the OpenShell version range declared in the blueprint (`min_openshell_version` and `max_openshell_version`). If the installed OpenShell version falls outside this range, onboarding exits with an actionable error and a link to compatible releases. +When an existing gateway is detected for reuse, NemoClaw probes the host gateway HTTP endpoint (`http://127.0.0.1:${NEMOCLAW_GATEWAY_PORT}/`) before declaring it reusable, so a gateway whose container is running but whose upstream is still warming up (e.g. immediately after a Docker daemon restart) is rebuilt instead of trusted. +Tune the wait via `NEMOCLAW_REUSE_HEALTH_POLL_COUNT` (default `6`) and `NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL` (default `5` seconds). + #### `--from ` Build the sandbox image from a custom Dockerfile instead of the stock NemoClaw image. From fadcf699f61260f84c2bebbbf585dad63810d91c Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Sat, 9 May 2026 11:22:52 +0000 Subject: [PATCH 05/10] fix(onboard): clamp gateway HTTP wait params at the consumer layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Centralise the validation guards in waitForGatewayHttpReady so env-derived defaults and caller-supplied options are normalised in one place — Math.max(1, attempts) and Math.max(0, intervalSeconds). The config function is now just an env reader. Restructure the wait loop to always probe at least once before any sleep, eliminating the trailing-attempt guard and naturally enforcing the minimum. Validate the per-probe timeoutMs in isGatewayHttpReady too: a non-positive or non-finite value falls back to the safe default rather than tearing the request down immediately. Signed-off-by: Tinson Lai --- src/lib/onboard.ts | 39 +++++++++++++++++--------- test/gateway-http-reuse-wait.test.ts | 42 ++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 13 deletions(-) diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index f2c8be8aa3c..219cb49a9f2 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -3488,13 +3488,17 @@ type WaitForGatewayHttpReadyOpts = { }; /** - * Resolve poll count and interval (seconds) for the reuse-time gateway HTTP - * readiness wait. Defaults are tighter than the startup health wait because - * reuse only needs to verify a previously-warm gateway is still serving — - * not wait for a cold k3s cluster to come up. - * - * Override via `NEMOCLAW_REUSE_HEALTH_POLL_COUNT` and + * Resolve raw poll count and interval (seconds) for the reuse-time gateway + * HTTP readiness wait, from `NEMOCLAW_REUSE_HEALTH_POLL_COUNT` and * `NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL`. + * + * Defaults are tighter than the startup health wait because reuse only needs + * to verify a previously-warm gateway is still serving — not wait for a cold + * k3s cluster to come up. + * + * The values are normalised in `waitForGatewayHttpReady`, not here, so the + * consumer-layer guards (probe at least once; non-negative interval) cover + * both env-derived and caller-supplied options uniformly. */ function getGatewayReuseHealthWaitConfig(): { count: number; interval: number } { return { @@ -3525,10 +3529,16 @@ const GATEWAY_HTTP_ALIVE_CODES = new Set([200, 401]); * * `url` is overridable for unit tests; production callers use the default. */ +const ISGATEWAY_HTTP_READY_DEFAULT_TIMEOUT_MS = 3000; + function isGatewayHttpReady( - timeoutMs = 3000, + timeoutMs = ISGATEWAY_HTTP_READY_DEFAULT_TIMEOUT_MS, url = `http://127.0.0.1:${GATEWAY_PORT}/`, ): Promise { + const effectiveTimeout = + Number.isFinite(timeoutMs) && timeoutMs > 0 + ? Math.round(timeoutMs) + : ISGATEWAY_HTTP_READY_DEFAULT_TIMEOUT_MS; const http = require("http"); return new Promise((resolve) => { let settled = false; @@ -3544,7 +3554,7 @@ function isGatewayHttpReady( settle(GATEWAY_HTTP_ALIVE_CODES.has(code)); }) .on("error", () => settle(false)); - request.setTimeout(timeoutMs, () => { + request.setTimeout(effectiveTimeout, () => { request.destroy(); settle(false); }); @@ -3571,12 +3581,15 @@ async function waitForGatewayHttpReady( const probe = opts.probe ?? (() => isGatewayHttpReady()); const sleeper = opts.sleeper ?? sleep; const config = getGatewayReuseHealthWaitConfig(); - const maxAttempts = opts.maxAttempts ?? config.count; - const intervalSeconds = opts.intervalSeconds ?? config.interval; - - for (let attempt = 0; attempt < maxAttempts; attempt++) { + // Always probe at least once, even if the caller passed a non-positive + // maxAttempts. Negative or non-finite intervals fall back to no sleep. + const maxAttempts = Math.max(1, opts.maxAttempts ?? config.count); + const intervalSeconds = Math.max(0, opts.intervalSeconds ?? config.interval); + + if (await probe()) return true; + for (let attempt = 1; attempt < maxAttempts; attempt++) { + sleeper(intervalSeconds); if (await probe()) return true; - if (attempt < maxAttempts - 1) sleeper(intervalSeconds); } return false; } diff --git a/test/gateway-http-reuse-wait.test.ts b/test/gateway-http-reuse-wait.test.ts index 0df68aa16fa..24057d16a18 100644 --- a/test/gateway-http-reuse-wait.test.ts +++ b/test/gateway-http-reuse-wait.test.ts @@ -100,6 +100,15 @@ describe("getGatewayReuseHealthWaitConfig (#3258)", () => { process.env.NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL = ""; expect(getGatewayReuseHealthWaitConfig()).toEqual({ count: 6, interval: 5 }); }); + + it("returns env values unclamped — normalisation is the consumer's job", () => { + // The wait helper applies `Math.max(1, count)` and `Math.max(0, interval)`, + // covering both env-derived and caller-supplied values in one place. The + // config function itself just reads the env. + process.env.NEMOCLAW_REUSE_HEALTH_POLL_COUNT = "0"; + process.env.NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL = "0"; + expect(getGatewayReuseHealthWaitConfig()).toEqual({ count: 0, interval: 0 }); + }); }); describe("isGatewayHttpReady status-code semantics (#3258)", () => { @@ -154,6 +163,20 @@ describe("isGatewayHttpReady status-code semantics (#3258)", () => { const url = await getClosedLocalUrl(); expect(await isGatewayHttpReady(2000, url)).toBe(false); }); + + it("falls back to the default timeout when given a non-positive value", async () => { + // A non-positive timeoutMs must not cause the request to be torn down + // immediately — the helper falls back to the safe default and lets the + // probe complete normally against a healthy server. + const server = await startStatusServer(200); + try { + for (const bad of [0, -1, Number.NaN]) { + expect(await isGatewayHttpReady(bad, server.url)).toBe(true); + } + } finally { + await server.close(); + } + }); }); describe("waitForGatewayHttpReady (#3258)", () => { @@ -226,4 +249,23 @@ describe("waitForGatewayHttpReady (#3258)", () => { expect(calls).toBe(1); expect(sleeps).toEqual([]); }); + + it("always probes at least once even when maxAttempts is 0 or negative", async () => { + for (const bad of [0, -1, -100]) { + let calls = 0; + const sleeps: number[] = []; + const result = await waitForGatewayHttpReady({ + probe: async () => { + calls += 1; + return false; + }, + sleeper: (s: number) => sleeps.push(s), + maxAttempts: bad, + intervalSeconds: 5, + }); + expect(result).toBe(false); + expect(calls).toBe(1); + expect(sleeps).toEqual([]); + } + }); }); From dd2dc9bcd4f5235f41b4676df17f97190928cec2 Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Sat, 9 May 2026 11:27:38 +0000 Subject: [PATCH 06/10] docs(commands): note the clamp behaviour for reuse health-poll knobs Document that NEMOCLAW_REUSE_HEALTH_POLL_COUNT is floored at 1 (so the probe always runs at least once) and the interval is floored at 0. Signed-off-by: Tinson Lai --- docs/reference/commands.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/reference/commands.md b/docs/reference/commands.md index 4805fb3cae2..efea36d8536 100644 --- a/docs/reference/commands.md +++ b/docs/reference/commands.md @@ -187,6 +187,7 @@ If the installed OpenShell version falls outside this range, onboarding exits wi When an existing gateway is detected for reuse, NemoClaw probes the host gateway HTTP endpoint (`http://127.0.0.1:${NEMOCLAW_GATEWAY_PORT}/`) before declaring it reusable, so a gateway whose container is running but whose upstream is still warming up (e.g. immediately after a Docker daemon restart) is rebuilt instead of trusted. Tune the wait via `NEMOCLAW_REUSE_HEALTH_POLL_COUNT` (default `6`) and `NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL` (default `5` seconds). +The poll count is clamped to a minimum of `1` so the probe always runs at least once, and the interval is clamped to a minimum of `0` (no sleep between attempts). #### `--from ` From 1e9c1e6816402e83ab45d075e3f51092d088de83 Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Sat, 9 May 2026 11:58:02 +0000 Subject: [PATCH 07/10] fix(onboard): keep preflight 'unknown' branch fully non-destructive; guard NaN/Infinity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two CodeRabbit findings on #3258: * Don't downgrade reuseState to "missing" in the preflight unknown+ HTTP-fail branch. The orphan-cleanup that runs on "missing" can destroy a possibly-live gateway when Docker's transient inspect failure coincides with an HTTP warm-up miss — exactly the case the branch is supposed to protect. The main-onboard branch (which has no orphan-cleanup) and the startGatewayWithOptions HTTP gate together still ensure the actual reuse decision goes through HTTP verification. * Add Number.isFinite guards in waitForGatewayHttpReady so caller- supplied Infinity/NaN don't bypass the Math.max clamps and either hang the loop or feed NaN into the sleeper. Signed-off-by: Tinson Lai --- src/lib/onboard.ts | 30 ++++++++++++---------- test/gateway-http-reuse-wait.test.ts | 38 ++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 14 deletions(-) diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 219cb49a9f2..5d294d7bc84 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -3582,9 +3582,13 @@ async function waitForGatewayHttpReady( const sleeper = opts.sleeper ?? sleep; const config = getGatewayReuseHealthWaitConfig(); // Always probe at least once, even if the caller passed a non-positive - // maxAttempts. Negative or non-finite intervals fall back to no sleep. - const maxAttempts = Math.max(1, opts.maxAttempts ?? config.count); - const intervalSeconds = Math.max(0, opts.intervalSeconds ?? config.interval); + // maxAttempts. Non-finite (NaN, Infinity) values fall back to safe defaults + // — Math.max alone would let Infinity through and hang the loop, and NaN + // would propagate into sleeper(). + const rawAttempts = opts.maxAttempts ?? config.count; + const maxAttempts = Number.isFinite(rawAttempts) ? Math.max(1, Math.round(rawAttempts)) : 1; + const rawInterval = opts.intervalSeconds ?? config.interval; + const intervalSeconds = Number.isFinite(rawInterval) ? Math.max(0, rawInterval) : 0; if (await probe()) return true; for (let attempt = 1; attempt < maxAttempts; attempt++) { @@ -4088,24 +4092,22 @@ async function preflight( // HTTP probe — it doesn't depend on Docker, so it can confirm the gateway // is genuinely serving even when the daemon is flaky. // - // Per #2020 the "unknown" state must remain non-destructive: if Docker is - // unavailable, destroying and recreating cannot succeed anyway. Just warn - // and let the caller surface the failure later. + // Per #2020 the "unknown" state must stay non-destructive end-to-end: + // we cannot downgrade to "missing" in preflight even when HTTP probe + // fails. Doing so would feed the orphan-cleanup block below, and a + // transient `docker inspect` failure plus an HTTP warm-up miss would + // delete a live gateway. The main onboard's "unknown" branch (which + // does not orphan-cleanup) handles the actual reuse decision, and + // `startGatewayWithOptions` HTTP-gates the final reuse so a stale-but- + // CLI-healthy gateway still cannot slip through. if (await waitForGatewayHttpReady()) { console.log( " Warning: could not verify gateway container state (Docker may be unavailable), but the gateway is responding on HTTP. Proceeding with reuse.", ); } else { - // Per #2020 we must not tear the gateway down in the unknown branch - // (if Docker is unavailable, recreate cannot succeed either). But - // continuing to trust cached "healthy" leads to the #3258 step-4 - // "Connection refused" failure. Compromise: downgrade reuse state so - // the regular gateway-start path takes over and surfaces a clearer - // error if it cannot proceed. console.log( - " Warning: could not verify gateway container state and the gateway is not responding on HTTP. Treating as unhealthy; will attempt to start a fresh gateway.", + " Warning: could not verify gateway container state and the gateway is not responding on HTTP. Onboard will re-verify before reuse; restart Docker and re-run if onboard fails.", ); - gatewayReuseState = "missing"; } } else if (!(await waitForGatewayHttpReady())) { // Container is running but the gateway HTTP endpoint is not responding. diff --git a/test/gateway-http-reuse-wait.test.ts b/test/gateway-http-reuse-wait.test.ts index 24057d16a18..bd59a293243 100644 --- a/test/gateway-http-reuse-wait.test.ts +++ b/test/gateway-http-reuse-wait.test.ts @@ -268,4 +268,42 @@ describe("waitForGatewayHttpReady (#3258)", () => { expect(sleeps).toEqual([]); } }); + + it("does not loop forever when maxAttempts is Infinity or NaN", async () => { + for (const bad of [Number.POSITIVE_INFINITY, Number.NaN]) { + let calls = 0; + const sleeps: number[] = []; + const result = await waitForGatewayHttpReady({ + probe: async () => { + calls += 1; + return false; + }, + sleeper: (s: number) => sleeps.push(s), + maxAttempts: bad, + intervalSeconds: 5, + }); + expect(result).toBe(false); + expect(calls).toBe(1); + expect(sleeps).toEqual([]); + } + }); + + it("does not pass NaN/Infinity through to the sleeper", async () => { + for (const bad of [Number.NaN, Number.POSITIVE_INFINITY]) { + let calls = 0; + const sleeps: number[] = []; + const result = await waitForGatewayHttpReady({ + probe: async () => { + calls += 1; + return calls >= 2; + }, + sleeper: (s: number) => sleeps.push(s), + maxAttempts: 3, + intervalSeconds: bad, + }); + expect(result).toBe(true); + // One sleep before the second probe — must be 0, not NaN/Infinity. + expect(sleeps).toEqual([0]); + } + }); }); From 31ee065c48c0fac5b06a1064778d0ec8e5abee2a Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Tue, 12 May 2026 00:39:35 +0000 Subject: [PATCH 08/10] refactor(onboard): extract gateway HTTP readiness helpers to src/lib/onboard/ Signed-off-by: Tinson Lai --- src/lib/onboard.ts | 123 +------------------ src/lib/onboard/gateway-http-readiness.ts | 140 ++++++++++++++++++++++ 2 files changed, 145 insertions(+), 118 deletions(-) create mode 100644 src/lib/onboard/gateway-http-readiness.ts diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 5d294d7bc84..81a34f95cf1 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -282,6 +282,11 @@ const policies: typeof import("./policies") = require("./policies"); const shields = require("./shields"); const tiers: typeof import("./tiers") = require("./tiers"); const { ensureUsageNoticeConsent } = require("./onboard/usage-notice"); +const { + getGatewayReuseHealthWaitConfig, + isGatewayHttpReady, + waitForGatewayHttpReady, +} = require("./onboard/gateway-http-readiness") as typeof import("./onboard/gateway-http-readiness"); const preflightUtils: typeof import("./onboard/preflight") = require("./onboard/preflight"); const clusterImagePatch: typeof import("./cluster-image-patch") = require("./cluster-image-patch"); const { @@ -3480,124 +3485,6 @@ function gatewayClusterHealthcheckPassed(): boolean { return result.status === 0; } -type WaitForGatewayHttpReadyOpts = { - probe?: () => Promise; - sleeper?: (seconds: number) => void; - maxAttempts?: number; - intervalSeconds?: number; -}; - -/** - * Resolve raw poll count and interval (seconds) for the reuse-time gateway - * HTTP readiness wait, from `NEMOCLAW_REUSE_HEALTH_POLL_COUNT` and - * `NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL`. - * - * Defaults are tighter than the startup health wait because reuse only needs - * to verify a previously-warm gateway is still serving — not wait for a cold - * k3s cluster to come up. - * - * The values are normalised in `waitForGatewayHttpReady`, not here, so the - * consumer-layer guards (probe at least once; non-negative interval) cover - * both env-derived and caller-supplied options uniformly. - */ -function getGatewayReuseHealthWaitConfig(): { count: number; interval: number } { - return { - count: envInt("NEMOCLAW_REUSE_HEALTH_POLL_COUNT", 6), - interval: envInt("NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL", 5), - }; -} - -/** - * HTTP status codes that indicate the gateway dispatcher is healthy. - * - * Mirrors the established whitelist in `verify-deployment.ts`: 200 = serving, - * 401 = device-auth gate is enabled but the gateway is running. Anything else - * — including 404, 403, 502, transport errors — is treated as not ready. - */ -const GATEWAY_HTTP_ALIVE_CODES = new Set([200, 401]); - -/** - * Probe the host-level gateway HTTP endpoint at `http://127.0.0.1:${GATEWAY_PORT}/`. - * - * Returns true when the gateway responds with a known-alive status code, - * false on any other status (notably 5xx from a warming upstream) or any - * transport-level error. - * - * Doesn't depend on Docker — issues a direct HTTP request to the host port. - * That makes it the right probe for the Docker-state-`unknown` branch where - * the docker daemon is itself flaky. - * - * `url` is overridable for unit tests; production callers use the default. - */ -const ISGATEWAY_HTTP_READY_DEFAULT_TIMEOUT_MS = 3000; - -function isGatewayHttpReady( - timeoutMs = ISGATEWAY_HTTP_READY_DEFAULT_TIMEOUT_MS, - url = `http://127.0.0.1:${GATEWAY_PORT}/`, -): Promise { - const effectiveTimeout = - Number.isFinite(timeoutMs) && timeoutMs > 0 - ? Math.round(timeoutMs) - : ISGATEWAY_HTTP_READY_DEFAULT_TIMEOUT_MS; - const http = require("http"); - return new Promise((resolve) => { - let settled = false; - const settle = (ready: boolean) => { - if (settled) return; - settled = true; - resolve(ready); - }; - const request = http - .get(url, (res: import("node:http").IncomingMessage) => { - res.resume(); - const code = res.statusCode || 0; - settle(GATEWAY_HTTP_ALIVE_CODES.has(code)); - }) - .on("error", () => settle(false)); - request.setTimeout(effectiveTimeout, () => { - request.destroy(); - settle(false); - }); - }); -} - -/** - * Poll the gateway HTTP endpoint until it returns ready or the configured - * budget is exhausted. Returns true on the first ready response, false if - * no attempt succeeds within the budget. - * - * Used at gateway-reuse decision sites to catch the case where the container - * is running (or Docker can't be probed) but the gateway upstream is still - * warming up — e.g. immediately after `colima stop && colima start`. Without - * this, openshell CLI metadata reports "healthy" from the previous run and - * onboard skips startup, only to fail later in step 4 with "Connection - * refused". See #3258 (regression of #2020). - * - * `probe` and `sleeper` are injectable for unit testing. - */ -async function waitForGatewayHttpReady( - opts: WaitForGatewayHttpReadyOpts = {}, -): Promise { - const probe = opts.probe ?? (() => isGatewayHttpReady()); - const sleeper = opts.sleeper ?? sleep; - const config = getGatewayReuseHealthWaitConfig(); - // Always probe at least once, even if the caller passed a non-positive - // maxAttempts. Non-finite (NaN, Infinity) values fall back to safe defaults - // — Math.max alone would let Infinity through and hang the loop, and NaN - // would propagate into sleeper(). - const rawAttempts = opts.maxAttempts ?? config.count; - const maxAttempts = Number.isFinite(rawAttempts) ? Math.max(1, Math.round(rawAttempts)) : 1; - const rawInterval = opts.intervalSeconds ?? config.interval; - const intervalSeconds = Number.isFinite(rawInterval) ? Math.max(0, rawInterval) : 0; - - if (await probe()) return true; - for (let attempt = 1; attempt < maxAttempts; attempt++) { - sleeper(intervalSeconds); - if (await probe()) return true; - } - return false; -} - function repairGatewayBootstrapSecrets(): { repaired: boolean; missingSecrets: string[] } { const missingSecrets = listMissingGatewayBootstrapSecrets(); const plan = getGatewayBootstrapRepairPlan(missingSecrets); diff --git a/src/lib/onboard/gateway-http-readiness.ts b/src/lib/onboard/gateway-http-readiness.ts new file mode 100644 index 00000000000..1f3267cb2f2 --- /dev/null +++ b/src/lib/onboard/gateway-http-readiness.ts @@ -0,0 +1,140 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Host-level HTTP readiness probe for the OpenShell gateway. + * + * Hits `http://127.0.0.1:${GATEWAY_PORT}/` directly (no Docker dependency), + * which lets the reuse path verify the gateway is genuinely serving even when + * the Docker daemon is flaky and openshell CLI metadata is stale. See #3258 + * (regression of #2020) for the original motivation. + */ + +import http from "node:http"; + +import { GATEWAY_PORT } from "../core/ports"; +import { sleepSeconds } from "../core/wait"; + +/** + * HTTP status codes that indicate the gateway dispatcher is healthy. + * + * Mirrors the established whitelist in `verify-deployment.ts`: 200 = serving, + * 401 = device-auth gate is enabled but the gateway is running. Anything else + * — including 404, 403, 502, transport errors — is treated as not ready. + */ +const GATEWAY_HTTP_ALIVE_CODES = new Set([200, 401]); + +const ISGATEWAY_HTTP_READY_DEFAULT_TIMEOUT_MS = 3000; + +export type WaitForGatewayHttpReadyOpts = { + probe?: () => Promise; + sleeper?: (seconds: number) => void; + maxAttempts?: number; + intervalSeconds?: number; +}; + +function envInt(name: string, fallback: number): number { + const raw = process.env[name]; + if (raw === undefined || raw === "") return fallback; + const n = Number(raw); + return Number.isFinite(n) ? Math.max(0, Math.round(n)) : fallback; +} + +/** + * Resolve raw poll count and interval (seconds) for the reuse-time gateway + * HTTP readiness wait, from `NEMOCLAW_REUSE_HEALTH_POLL_COUNT` and + * `NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL`. + * + * Defaults are tighter than the startup health wait because reuse only needs + * to verify a previously-warm gateway is still serving — not wait for a cold + * k3s cluster to come up. + * + * The values are normalised in `waitForGatewayHttpReady`, not here, so the + * consumer-layer guards (probe at least once; non-negative interval) cover + * both env-derived and caller-supplied options uniformly. + */ +export function getGatewayReuseHealthWaitConfig(): { count: number; interval: number } { + return { + count: envInt("NEMOCLAW_REUSE_HEALTH_POLL_COUNT", 6), + interval: envInt("NEMOCLAW_REUSE_HEALTH_POLL_INTERVAL", 5), + }; +} + +/** + * Probe the host-level gateway HTTP endpoint at `http://127.0.0.1:${GATEWAY_PORT}/`. + * + * Returns true when the gateway responds with a known-alive status code, + * false on any other status (notably 5xx from a warming upstream) or any + * transport-level error. + * + * Doesn't depend on Docker — issues a direct HTTP request to the host port. + * That makes it the right probe for the Docker-state-`unknown` branch where + * the docker daemon is itself flaky. + * + * `url` is overridable for unit tests; production callers use the default. + */ +export function isGatewayHttpReady( + timeoutMs = ISGATEWAY_HTTP_READY_DEFAULT_TIMEOUT_MS, + url = `http://127.0.0.1:${GATEWAY_PORT}/`, +): Promise { + const effectiveTimeout = + Number.isFinite(timeoutMs) && timeoutMs > 0 + ? Math.round(timeoutMs) + : ISGATEWAY_HTTP_READY_DEFAULT_TIMEOUT_MS; + return new Promise((resolve) => { + let settled = false; + const settle = (ready: boolean) => { + if (settled) return; + settled = true; + resolve(ready); + }; + const request = http + .get(url, (res) => { + res.resume(); + const code = res.statusCode || 0; + settle(GATEWAY_HTTP_ALIVE_CODES.has(code)); + }) + .on("error", () => settle(false)); + request.setTimeout(effectiveTimeout, () => { + request.destroy(); + settle(false); + }); + }); +} + +/** + * Poll the gateway HTTP endpoint until it returns ready or the configured + * budget is exhausted. Returns true on the first ready response, false if + * no attempt succeeds within the budget. + * + * Used at gateway-reuse decision sites to catch the case where the container + * is running (or Docker can't be probed) but the gateway upstream is still + * warming up — e.g. immediately after `colima stop && colima start`. Without + * this, openshell CLI metadata reports "healthy" from the previous run and + * onboard skips startup, only to fail later in step 4 with "Connection + * refused". See #3258 (regression of #2020). + * + * `probe` and `sleeper` are injectable for unit testing. + */ +export async function waitForGatewayHttpReady( + opts: WaitForGatewayHttpReadyOpts = {}, +): Promise { + const probe = opts.probe ?? (() => isGatewayHttpReady()); + const sleeper = opts.sleeper ?? sleepSeconds; + const config = getGatewayReuseHealthWaitConfig(); + // Always probe at least once, even if the caller passed a non-positive + // maxAttempts. Non-finite (NaN, Infinity) values fall back to safe defaults + // — Math.max alone would let Infinity through and hang the loop, and NaN + // would propagate into sleeper(). + const rawAttempts = opts.maxAttempts ?? config.count; + const maxAttempts = Number.isFinite(rawAttempts) ? Math.max(1, Math.round(rawAttempts)) : 1; + const rawInterval = opts.intervalSeconds ?? config.interval; + const intervalSeconds = Number.isFinite(rawInterval) ? Math.max(0, rawInterval) : 0; + + if (await probe()) return true; + for (let attempt = 1; attempt < maxAttempts; attempt++) { + sleeper(intervalSeconds); + if (await probe()) return true; + } + return false; +} From 373b888a8db74c660e2191c66e8a85b8483c08b6 Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Tue, 12 May 2026 00:40:06 +0000 Subject: [PATCH 09/10] fix(onboard): abort instead of downgrading reuse state when Docker is unknown and gateway HTTP is unready Signed-off-by: Tinson Lai --- src/lib/onboard.ts | 35 ++++++++++++++++------------- test/gateway-liveness-probe.test.ts | 24 ++++++++++++++++++++ 2 files changed, 43 insertions(+), 16 deletions(-) diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 81a34f95cf1..741092b9d11 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -3980,20 +3980,18 @@ async function preflight( // is genuinely serving even when the daemon is flaky. // // Per #2020 the "unknown" state must stay non-destructive end-to-end: - // we cannot downgrade to "missing" in preflight even when HTTP probe - // fails. Doing so would feed the orphan-cleanup block below, and a - // transient `docker inspect` failure plus an HTTP warm-up miss would - // delete a live gateway. The main onboard's "unknown" branch (which - // does not orphan-cleanup) handles the actual reuse decision, and - // `startGatewayWithOptions` HTTP-gates the final reuse so a stale-but- - // CLI-healthy gateway still cannot slip through. + // do not downgrade to "missing" in preflight even when HTTP probe fails. + // Doing so would feed the orphan-cleanup block below, and a transient + // `docker inspect` failure plus an HTTP warm-up miss would delete a + // live gateway. The main onboard "unknown" branch makes the abort/reuse + // decision once preflight has surfaced the warning to the user. if (await waitForGatewayHttpReady()) { console.log( " Warning: could not verify gateway container state (Docker may be unavailable), but the gateway is responding on HTTP. Proceeding with reuse.", ); } else { console.log( - " Warning: could not verify gateway container state and the gateway is not responding on HTTP. Onboard will re-verify before reuse; restart Docker and re-run if onboard fails.", + " Warning: could not verify gateway container state and the gateway is not responding on HTTP. Onboard will abort before reuse if this persists; restart Docker and re-run.", ); } } else if (!(await waitForGatewayHttpReady())) { @@ -9938,16 +9936,21 @@ async function onboard(opts: OnboardOptions = {}): Promise { " Warning: could not verify gateway container state (Docker may be unavailable), but the gateway is responding on HTTP. Proceeding with reuse.", ); } else { - // Per #2020 we must not tear the gateway down in the unknown branch - // (if Docker is unavailable, recreate cannot succeed either). But - // continuing to trust cached "healthy" leads to the #3258 step-4 - // "Connection refused" failure. Compromise: downgrade reuse state so - // the regular gateway-start path takes over and surfaces a clearer - // error if it cannot proceed. + // Docker can't be probed AND the gateway HTTP endpoint isn't + // responding. We cannot tell whether the existing gateway is live + // (transient `docker inspect` flake + warm-up miss) or genuinely + // gone. Per #2020 we must not destroy in this branch, and we must + // not downgrade to "missing" either: that would push execution into + // `startGatewayWithOptions`, whose retry hook calls + // `destroyGateway()` between attempts — which would tear down a + // possibly-live gateway. Bail with an actionable error instead. console.log( - " Warning: could not verify gateway container state and the gateway is not responding on HTTP. Treating as unhealthy; will attempt to start a fresh gateway.", + ` Error: could not verify gateway container state and http://127.0.0.1:${GATEWAY_PORT}/ is not responding.`, ); - gatewayReuseState = "missing"; + console.log( + " Refusing to proceed without a clear Docker signal — restarting Docker and re-running onboard is the safe path. See #3258 / #2020.", + ); + process.exit(1); } } else if (!(await waitForGatewayHttpReady())) { // Container is running but the gateway HTTP endpoint is not responding. diff --git a/test/gateway-liveness-probe.test.ts b/test/gateway-liveness-probe.test.ts index e05c4752d82..1799b858a7c 100644 --- a/test/gateway-liveness-probe.test.ts +++ b/test/gateway-liveness-probe.test.ts @@ -77,6 +77,30 @@ describe("gateway liveness probe (#2020)", () => { expect(cleanupAfterProbe).toBeTruthy(); }); + it("main onboard flow aborts (does not downgrade or destroy) when Docker is unknown and HTTP is unready (#3258, #2020)", () => { + // Regression guard: when verifyGatewayContainerRunning() returns "unknown" + // and the host HTTP probe also fails, we cannot tell whether the existing + // gateway is live. Per #2020 the branch must stay non-destructive, and + // it must not downgrade gatewayReuseState to "missing" — that would feed + // execution into startGatewayWithOptions, whose retry hook destroys the + // gateway between failed attempts and would tear down a possibly-live + // gateway when Docker is just temporarily unavailable. + const onboardSection = content.slice(content.indexOf("async function onboard(")); + const unknownBranchMatch = onboardSection.match( + /containerState === "unknown"[\s\S]*?waitForGatewayHttpReady\(\)[\s\S]*?\} else \{[\s\S]*?\}\s*\}/, + ); + expect(unknownBranchMatch).toBeTruthy(); + if (!unknownBranchMatch) { + throw new Error('Expected containerState === "unknown" branch in main onboard()'); + } + const branchBody = unknownBranchMatch[0]; + // Must bail out — no fall-through into start-gateway / orphan-cleanup paths. + expect(branchBody).toMatch(/process\.exit\(/); + // Must not downgrade reuse state (would feed startGatewayWithOptions whose + // retry hook calls destroyGateway, tearing down a possibly-live gateway). + expect(branchBody).not.toMatch(/gatewayReuseState\s*=\s*"missing"/); + }); + it("does not modify isGatewayHealthy() in src/lib/state/gateway.ts", () => { // isGatewayHealthy() must remain a pure function — no I/O. // Scope the check to the function body so unrelated helpers don't cause false failures. From e4ca4fa1c004830a9dde2eb0ec2721b048835d38 Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Tue, 12 May 2026 00:56:38 +0000 Subject: [PATCH 10/10] fix(onboard): treat probe rejection as 'not ready' in waitForGatewayHttpReady Signed-off-by: Tinson Lai --- src/lib/onboard/gateway-http-readiness.ts | 15 ++++++++-- test/gateway-http-reuse-wait.test.ts | 35 +++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/lib/onboard/gateway-http-readiness.ts b/src/lib/onboard/gateway-http-readiness.ts index 4be1afedf93..93515175c20 100644 --- a/src/lib/onboard/gateway-http-readiness.ts +++ b/src/lib/onboard/gateway-http-readiness.ts @@ -125,10 +125,21 @@ export async function waitForGatewayHttpReady( const rawInterval = opts.intervalSeconds ?? config.interval; const intervalSeconds = Number.isFinite(rawInterval) ? Math.max(0, rawInterval) : 0; - if (await probe()) return true; + // The default probe (isGatewayHttpReady) never rejects, but injected probes + // can. Treat a rejection as "not ready this attempt" so we exhaust the + // budget instead of bailing on the first transient failure. + const safeProbe = async (): Promise => { + try { + return await probe(); + } catch { + return false; + } + }; + + if (await safeProbe()) return true; for (let attempt = 1; attempt < maxAttempts; attempt++) { sleeper(intervalSeconds); - if (await probe()) return true; + if (await safeProbe()) return true; } return false; } diff --git a/test/gateway-http-reuse-wait.test.ts b/test/gateway-http-reuse-wait.test.ts index bd59a293243..fd6dcf0d882 100644 --- a/test/gateway-http-reuse-wait.test.ts +++ b/test/gateway-http-reuse-wait.test.ts @@ -306,4 +306,39 @@ describe("waitForGatewayHttpReady (#3258)", () => { expect(sleeps).toEqual([0]); } }); + + it("treats a probe rejection as 'not ready' and continues to the next attempt", async () => { + let calls = 0; + const sleeps: number[] = []; + const result = await waitForGatewayHttpReady({ + probe: async () => { + calls += 1; + if (calls === 1) throw new Error("transient probe failure"); + return true; + }, + sleeper: (s: number) => sleeps.push(s), + maxAttempts: 4, + intervalSeconds: 2, + }); + expect(result).toBe(true); + expect(calls).toBe(2); + expect(sleeps).toEqual([2]); + }); + + it("returns false when every probe rejects across the whole budget", async () => { + let calls = 0; + const sleeps: number[] = []; + const result = await waitForGatewayHttpReady({ + probe: async () => { + calls += 1; + throw new Error("probe is broken"); + }, + sleeper: (s: number) => sleeps.push(s), + maxAttempts: 3, + intervalSeconds: 1, + }); + expect(result).toBe(false); + expect(calls).toBe(3); + expect(sleeps).toEqual([1, 1]); + }); });