From 4ba3dd615382de1dd61dc26d157ad0bb2d725eb9 Mon Sep 17 00:00:00 2001 From: Julie Yaunches Date: Mon, 11 May 2026 18:43:25 -0400 Subject: [PATCH 1/9] docs: acceptance criterion + phased dev plan for #3111 Auto-generated by pr-e2e-loop Phase 4.5. See PR #3362 for the coverage guard (failing E2E that is the acceptance gate for this fix). --- ACCEPTANCE.md | 188 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 ACCEPTANCE.md diff --git a/ACCEPTANCE.md b/ACCEPTANCE.md new file mode 100644 index 00000000000..cf9d445f951 --- /dev/null +++ b/ACCEPTANCE.md @@ -0,0 +1,188 @@ +# Issue #3111 β€” Development Plan + Acceptance Criterion + +**Worktree:** `/Users/jyaunches/Development/NemoClaw-working/issue-3111` +**Branch:** `issue-3111-gateway-health-honesty` +**Base:** `origin/main` at `32cab9d5f` (post PR #3362 merge) +**Related merged:** PR #3362 (coverage guard) + +--- + +## Definition of Done (auto-added by `pr-e2e-loop`) + +Nightly job **`gateway-health-honest-e2e`** (added in PR #3362) must flip from πŸ”΄ red on `main` to 🟒 green on the fix branch. Dispatch locally with: + +```bash +gh workflow run nightly-e2e.yaml -f jobs=gateway-health-honest-e2e --ref +``` + +**Expected failing assertion on main** (reproduced in run [25698031380](https://github.com/NVIDIA/NemoClaw/actions/runs/25698031380)): + +> `[PASS] Sabotage shim was invoked as expected (GLIBC/sabotage markers present in gateway log)` +> `[FAIL] Onboard reported 'βœ“ Docker-driver gateway is healthy' although the gateway binary crashed on startup (#3111 false-positive health check)` + +After the fix the log must show: + +``` +[PASS] Sabotage shim was invoked as expected (GLIBC/sabotage markers present in gateway log) +[PASS] Onboard did not falsely log 'Docker-driver gateway is healthy' when the binary crashed +[PASS] startGateway() did not resolve successfully with a crashed binary (node exit=1 or 3) +[PASS] Onboard surfaced a user-visible gateway failure message +[PASS] No live (non-zombie) gateway process is running after the simulated crash +[PASS] #3111 coverage guard green: onboard correctly surfaces a crashed gateway +``` + +--- + +## Diagnosis + +The Docker-driver gateway startup path in `src/lib/onboard.ts:startDockerDriverGateway` spawns the `openshell-gateway` binary with `spawn(..., { detached: true })` + `child.unref()`, then enters a poll loop that checks `isPidAlive(childPid)` and `isGatewayHealthy(status, gwInfo, activeGwInfo)`. + +Both checks can lie: + +1. `isPidAlive` (`src/lib/onboard.ts:4101`) uses `process.kill(pid, 0)` which returns `true` for **zombies** (detached children that exited but haven't been reaped). Since the parent Node process never `wait()`s on the detached child, zombies persist indefinitely. + +2. `isGatewayHealthy` (`src/lib/state/gateway.ts:99`) is a **pure string-matching function** over `openshell status` / `openshell gateway info` output. `isGatewayConnected` matches `Connected` OR **`Server Status`** β€” the latter is a **header line** that `openshell status` prints unconditionally, even when the body contains `Connection refused (os error 111)`. `registerDockerDriverGatewayEndpoint` (`src/lib/onboard.ts:4347`) is metadata-only (`openshell gateway add --local`), no TCP probe. + +The reported symptom on Ubuntu 22.04 (GLIBC 2.38/2.39 link mismatch) is a *specific trigger*. The class of bug is triggered by anything that makes the gateway binary exit immediately β€” GLIBC mismatch, missing shared library, corrupted binary, permission error, port conflict, CDI spec error. + +--- + +## Root Causes + +### RC-1: `isPidAlive` treats zombies as alive +**Location:** `src/lib/onboard.ts:4101-4109` + +`process.kill(pid, 0)` returns true for both live and zombie processes. For detached `spawn()` children where the parent doesn't `wait()`, zombies accumulate. The poll loop thus never breaks on a crashed-but-unreaped child and proceeds to the metadata check. + +### RC-2: `isGatewayConnected` matches a header, not a state +**Location:** `src/lib/state/gateway.ts:80-85` + +```js +statusOutput.includes("Connected") || statusOutput.includes("Server Status") +``` + +`Server Status` is the **title of the table** `openshell status` prints β€” it's always present. The real connectivity signal is buried in the body (`Connection refused` / `Error: Γ— client error (Connect)`). The function can't distinguish "connected" from "metadata says there's a gateway at 127.0.0.1:8080, but nothing listens there." + +### RC-3: No live TCP probe in the Docker-driver startup poll loop +**Location:** `src/lib/onboard.ts:5519-5540` (poll loop body) + +The K3s path has a live container probe (`verifyGatewayContainerRunning()` added for #2020 β€” checks `docker inspect {{.State.Running}}`). The Docker-driver path has no equivalent β€” it trusts the metadata match. A live TCP connect to `127.0.0.1:${GATEWAY_PORT}` would instantly reveal a crashed binary. + +--- + +## Fix Areas & Test Depth Classification + +| # | Area | Change | Test Level | Rationale | +|---|------|--------|-----------|-----------| +| 1 | `src/lib/onboard.ts` β€” new `verifyDockerDriverGatewayListening()` helper + integrate into `startDockerDriverGateway` poll loop | Add a TCP connect probe to `127.0.0.1:${GATEWAY_PORT}` with short timeout; fail the poll iteration when probe fails | πŸ”΄ **E2E** | Spawns real process, binds real port. `gateway-health-honest-e2e` IS the E2E for this change. | +| 2 | `src/lib/onboard.ts` β€” reap zombie child before `isPidAlive` | After spawn, periodically `waitpid`-equivalent check so zombies get detected | 🟑 Unit + πŸ”΄ E2E | Same E2E catches this; also add a unit test that mocks a zombied PID. | +| 3 | `src/lib/state/gateway.ts` β€” **do NOT modify** | The gateway-liveness-probe test pins this file to pure-function status (see #2020 follow-up test at `test/gateway-liveness-probe.test.ts:74`). Keep the string match; fix at the caller. | 🟒 Unit | No source change. | +| 4 | `scripts/install-openshell.sh` β€” OPTIONAL: add a preflight that verifies the downloaded `openshell-gateway` binary can actually exec (`$BIN --version` or similar) before writing the installed marker | Surface GLIBC / linker errors at install time rather than onboard time | 🟑 Unit + ✨ (install script) | Out of scope for the primary fix but a useful secondary safety net for #3111's specific Ubuntu 22.04 trigger. Split into follow-up issue if larger. | + +--- + +## Refactoring Alignment + +| # | Refactoring Goal | Overlap | Recommendation | +|---|-----------------|---------|----------------| +| 1 | **PR #3306** (cv) β€” `refactor(onboard): extract gateway bootstrap repair helpers` | Extracts `src/lib/onboard/gateway-bootstrap.ts` module. Our fix touches `startDockerDriverGateway` in `onboard.ts`. | **Land-order coordination.** Wait to see if #3306 extracts our target function (`startDockerDriverGateway`) into the new module. If so, rebase onto #3306 and put the fix in the new module. If not, fix in `onboard.ts` and `#3306` can pull in the new helper later. | +| 2 | **PR #3312** (laitingsheng) β€” `fix(onboard): require host HTTP readiness before reusing the gateway` | Adds `isGatewayHttpReady(timeoutMs, url)` β€” an HTTP-level readiness probe. This is the K3s-path equivalent of what we need for Docker-driver. | **Pattern reuse.** Export the `isGatewayHttpReady` helper from wherever #3312 lands it, then call it from our new `verifyDockerDriverGatewayListening`. If #3312 lands first, extend it; if we land first, make our helper exportable and #3312 can adopt. | +| 3 | **#3213** β€” Epic: unify warnings, advisories, and fatal exits under a single registry | When onboard exits with "gateway failed to start", that message should eventually flow through the unified advisory registry. | **Seed the pattern.** Format our new fatal-exit message as `{code: "gateway-failed-to-start", hint: "..."}` structured output so the later registry migration is mechanical. Add `// TODO(#3213): register in advisory registry`. | +| 4 | **#2562** β€” `refactor(arch): unified timeout abstraction for child-process execution` | Our TCP probe needs a timeout. The epic proposes a unified abstraction. | **Use a local timeout for now** (e.g., `AbortSignal.timeout(500)`), but leave a `// TODO(#2562): adopt unified timeout helper` comment. | + +--- + +## Implementation Plan + +### Phase 1 β€” Add TCP liveness probe helper (🟑 Unit) + +**Goal:** Introduce `verifyDockerDriverGatewayListening(port: number, timeoutMs: number): Promise` as a small, pure async helper. + +- **File:** `src/lib/onboard.ts` (or a new `src/lib/onboard/gateway-tcp-probe.ts` if #3306 lands first). +- **Implementation:** Use `net.createConnection({ host: "127.0.0.1", port })` with `AbortSignal.timeout(timeoutMs)`. Resolve `true` on `connect`, `false` on `error` or timeout. +- **Tests:** New `test/onboard-gateway-tcp-probe.test.ts` β€” spin up a real TCP server on an ephemeral port, assert `true`; close it, assert `false`; assert timeout returns `false` within `timeoutMs + margin`. +- **Dependencies:** None. Can run in parallel with Phase 2. +- **Refactoring notes:** If #3312's `isGatewayHttpReady` has already landed, **reuse it** instead of adding a parallel helper β€” a TCP probe IS a strictly weaker signal than an HTTP probe, so prefer the HTTP probe when the gateway eventually serves HTTP. Add `// TODO(#2562): adopt unified timeout helper`. + +### Phase 2 β€” Reap zombie children in the startup poll loop (🟑 Unit) + +**Goal:** Detect crashed-then-zombied children so `isPidAlive` returns false as expected. + +- **File:** `src/lib/onboard.ts:startDockerDriverGateway` β€” the `spawn()` call. +- **Implementation:** Even with `detached: true` / `child.unref()`, we can keep a reference to the `ChildProcess` object, attach a one-shot `child.on("exit", ...)` handler that sets a `childCrashed: boolean` flag, and check that flag at the top of each poll iteration alongside `isPidAlive`. Alternative: `child.removeAllListeners(); child.once("exit", ...);` with a closure-captured flag. +- **Tests:** Extend `test/onboard.test.ts` (or add `test/gateway-zombie-reap.test.ts`) β€” spawn a shim that exits immediately, assert the flag flips within N ms. +- **Dependencies:** None. Runs in parallel with Phase 1. + +### Phase 3 β€” Integrate both checks into the poll loop (πŸ”΄ E2E required) + +**Goal:** Make `startDockerDriverGateway` fail closed β€” "healthy" is only logged when the process is running AND metadata matches AND the port is serving. + +- **File:** `src/lib/onboard.ts:5519-5540` (the poll loop body). +- **Implementation:** + ```ts + for (let i = 0; i < pollCount; i += 1) { + if (childCrashed || !isPidAlive(childPid)) { + break; // existing + } + if (!registerDockerDriverGatewayEndpoint()) { ...continue } + const status = runCaptureOpenshell(["status"], { ignoreError: true }); + const namedInfo = runCaptureOpenshell(["gateway", "info", "-g", GATEWAY_NAME], { ignoreError: true }); + const currentInfo = runCaptureOpenshell(["gateway", "info"], { ignoreError: true }); + if (isGatewayHealthy(status, namedInfo, currentInfo)) { + // NEW: gate the healthy log on a real TCP probe + if (await verifyDockerDriverGatewayListening(GATEWAY_PORT, 500)) { + console.log(" βœ“ Docker-driver gateway is healthy"); + return; + } + // Metadata says healthy but nothing is listening β€” keep polling + } + if (i < pollCount - 1) sleep(pollInterval); + } + ``` +- **Tests:** + - **E2E (primary, already exists):** `gateway-health-honest-e2e` (from PR #3362) must flip from red to green. + - **Unit:** mock `verifyDockerDriverGatewayListening` to return `false`; assert the loop doesn't print "healthy" and eventually throws. +- **Dependencies:** Phase 1 and Phase 2 must land first. +- **Refactoring notes:** Format the final "failed to start" message as structured data with `code: "gateway-failed-to-start"` for future #3213 migration. Leave the log-tail output intact β€” it's already user-facing diagnostic. + +### Phase 4 (OPTIONAL) β€” Install-time GLIBC check (🟑 Unit) + +**Goal:** Surface the GLIBC mismatch at install time, not onboard time. Addresses the specific #3111 trigger, not the class. + +- **File:** `scripts/install-openshell.sh` +- **Implementation:** After `install -m 755 $tmpdir/openshell-gateway $target_dir/openshell-gateway`, run `"$target_dir/openshell-gateway" --version` (or `--help`). Capture exit code + stderr. On non-zero, print a warn/fatal message that includes the stderr (which contains GLIBC requirement) and a suggestion to upgrade to Ubuntu 24.04. +- **Tests:** Extend `test/install-openshell-version-check.test.ts`. +- **Dependencies:** None β€” can run in parallel with any other phase. +- **Verdict:** Ask the user whether to split this into a separate follow-up issue. The primary fix (Phases 1–3) closes the NemoClaw-side false-positive; this Phase 4 is a secondary safety net tied to the OpenShell binary packaging choice. + +--- + +## Recommended Order + +- **Phases 1 and 2 in parallel** β†’ both are small, independent, unit-testable. +- **Phase 3 sequentially after 1 + 2.** +- **Phase 4 optional / follow-up.** + +## Verdict: πŸ”΄ E2E required + +| Phase | Test Depth | +|---|---| +| Phase 1 (TCP probe helper) | 🟑 Unit | +| Phase 2 (zombie-reap) | 🟑 Unit | +| Phase 3 (poll-loop integration) | πŸ”΄ E2E (`gateway-health-honest-e2e`) | +| Phase 4 (install-time check) | 🟑 Unit (optional) | + +The PR-level verdict is πŸ”΄ E2E β€” Phase 3 is the acceptance gate, and it's already implemented as the merged coverage guard. The fix PR's definition-of-done is "this E2E flips green." + +--- + +## Notes for the PR author + +- PR title: `fix(onboard): verify Docker-driver gateway is really serving before reporting healthy (#3111)` +- PR body MUST include `Fixes #3111` to auto-close the issue (and get the coverage guard flipping green). +- Do NOT modify `src/lib/state/gateway.ts:isGatewayHealthy` β€” pinned by `test/gateway-liveness-probe.test.ts:74`. +- Coordinate with open PR #3306 (gateway-bootstrap refactor) and #3312 (HTTP readiness probe) β€” see Refactoring Alignment above. +- The nightly-dispatch for validation: + ```bash + gh workflow run nightly-e2e.yaml -f jobs=gateway-health-honest-e2e --ref issue-3111-gateway-health-honesty + ``` From cf770c399eb7ae77707c21e140532314881d0131 Mon Sep 17 00:00:00 2001 From: Julie Yaunches Date: Mon, 11 May 2026 20:45:55 -0400 Subject: [PATCH 2/9] fix(onboard): verify Docker-driver gateway is really serving before reporting healthy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #3111. ## Problem startDockerDriverGateway() spawned the openshell-gateway binary with `detached: true` + child.unref(), then polled using two checks that could both lie: 1. **isPidAlive(childPid)** uses `process.kill(pid, 0)` which returns true for zombies. Since the parent Node process never wait()s on the detached child, crashed children linger as zombies indefinitely and isPidAlive reports them as alive. 2. **isGatewayHealthy(status, gwInfo, activeGwInfo)** is a pure string match over openshell CLI output. isGatewayConnected() matches on "Server Status" β€” the table *header* that `openshell status` always prints, regardless of whether the gateway endpoint is reachable. On a crashed gateway, the header is still emitted and the body contains `Connection refused` β€” but isGatewayConnected returns true anyway because it only looks for the header string. Result: onboard logged `βœ“ Docker-driver gateway is healthy` immediately after spawning a crashed binary, then proceeded to the next onboard step which failed with `Connection refused (os error 111)`. The reported trigger on Ubuntu 22.04 was a GLIBC 2.38/2.39 mismatch in the shipped openshell-gateway binary, but the class of bug is platform-independent: any reason the binary fails to start (missing shared lib, permissions, CDI spec error, port conflict) surfaces the same false-positive. ## Fix Three coordinated changes in `startDockerDriverGateway`: 1. **Track child-exit via the ChildProcess 'exit' event**, not just isPidAlive. The exit listener flips a `childExited` flag that the poll loop consults alongside isPidAlive. This catches zombies that isPidAlive misses. 2. **Add verifyDockerDriverGatewayListening(port, timeoutMs)** β€” a TCP connect probe to 127.0.0.1:${GATEWAY_PORT} using node:net with an AbortSignal-equivalent timeout. Resolves boolean, never throws. This is the DockerDriver path equivalent of verifyGatewayContainerRunning (added for #2020 on the K3s path). 3. **Gate the 'healthy' log on the TCP probe**: the poll loop now only logs "βœ“ Docker-driver gateway is healthy" after isGatewayHealthy AND a successful TCP connect. On probe failure, keep polling β€” the binary may be slow to bind its listener. The childExited check at the top of the loop terminates us if the process actually died. Also improves the final failure message to include how the gateway exited (signal vs. exit code) so users don't have to dig into the gateway log. ## isGatewayHealthy intentionally not modified The #2020 follow-up test in test/gateway-liveness-probe.test.ts pins isGatewayHealthy to pure-function status (no I/O). Fix at the caller, not the shared helper β€” same pattern as #2020 used for the K3s path. ## Tests - New unit test: test/gateway-tcp-probe.test.ts (9 tests) - verifyDockerDriverGatewayListening resolves true for listening ports - resolves false for closed ports (Connection refused) - resolves false on timeout (non-routable host) - enforces minimum 50ms timeout - never throws - source-shape guards: child-exit tracking, childExited check in poll loop, TCP probe before "healthy" log, exit details in failure msg - E2E acceptance gate: gateway-health-honest-e2e (from PR #3362) must flip from red on main to green on this branch. ## Refactoring alignment - TODO(#2562) added for the timeout abstraction - TODO(#3213) added for structured advisory output - Coordinates with open PRs #3306 (gateway-bootstrap refactor) and #3312 (isGatewayHttpReady for K3s path) β€” same pattern, different surface. See ACCEPTANCE.md. Related: #3111 Coverage guard: #3362 --- src/lib/onboard.ts | 88 ++++++++++++++++- test/gateway-tcp-probe.test.ts | 175 +++++++++++++++++++++++++++++++++ 2 files changed, 260 insertions(+), 3 deletions(-) create mode 100644 test/gateway-tcp-probe.test.ts diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index ebb12ad64b7..abeaede2114 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -4108,6 +4108,48 @@ function isPidAlive(pid: number): boolean { } } +/** + * Probe a TCP endpoint to verify something is actually listening and + * accepting connections. Used to gate the "gateway is healthy" log in + * startDockerDriverGateway against the class of bug reported in #3111 where + * the openshell-gateway binary crashed on startup but metadata-based health + * checks (isGatewayHealthy) still returned true. + * + * Resolves true on successful TCP connect. Resolves false on connection + * refused, unreachable host, or timeout. Never rejects. + * + * TODO(#2562): adopt the unified timeout abstraction once that epic lands. + */ +async function verifyDockerDriverGatewayListening( + port: number, + timeoutMs = 500, + host = "127.0.0.1", +): Promise { + // Lazy-require so tests that don't exercise the gateway path don't pay + // for the net module load, and so the import doesn't fan out across the + // already-bloated top-of-file require block. + // biome-ignore lint/suspicious/noExplicitAny: node:net types are not in scope in this file + const net: any = require("node:net"); + return new Promise((resolve) => { + let settled = false; + const settle = (result: boolean) => { + if (settled) return; + settled = true; + try { + socket.destroy(); + } catch { + /* best effort */ + } + resolve(result); + }; + const socket = net.createConnection({ host, port }); + socket.setTimeout(Math.max(50, timeoutMs)); + socket.once("connect", () => settle(true)); + socket.once("timeout", () => settle(false)); + socket.once("error", () => settle(false)); + }); +} + function getDockerDriverGatewayPid(): number | null { try { const raw = fs.readFileSync(getDockerDriverGatewayPidFile(), "utf-8").trim(); @@ -5507,6 +5549,21 @@ async function startDockerDriverGateway({ ...gatewayEnv, }, }); + // Track early-exit via the ChildProcess event rather than process.kill(pid, 0). + // isPidAlive() returns true for zombies (detached children that exited but + // were not reaped by a wait() call), which masks the failure mode reported + // in #3111: the binary crashes on startup but the pid remains in the + // process table as a zombie. The 'exit' event fires reliably for detached + // children once libuv observes SIGCHLD. We still call unref() below so the + // child doesn't hold the Node event loop open after we return. + let childExited = false; + let childExitCode: number | null = null; + let childExitSignal: NodeJS.Signals | null = null; + child.once("exit", (code: number | null, signal: NodeJS.Signals | null) => { + childExited = true; + childExitCode = code; + childExitSignal = signal; + }); child.unref(); const childPid = child.pid ?? 0; if (childPid <= 0) { @@ -5516,8 +5573,12 @@ async function startDockerDriverGateway({ const pollCount = envInt("NEMOCLAW_HEALTH_POLL_COUNT", 30); const pollInterval = envInt("NEMOCLAW_HEALTH_POLL_INTERVAL", 2); + const tcpProbeTimeoutMs = envInt("NEMOCLAW_GATEWAY_TCP_PROBE_TIMEOUT_MS", 500); for (let i = 0; i < pollCount; i += 1) { - if (!isPidAlive(childPid)) { + // #3111: combine the exit-event check with isPidAlive. exit-event catches + // zombies that isPidAlive misses; isPidAlive catches children we lost + // track of (e.g., if the exit listener was removed). + if (childExited || !isPidAlive(childPid)) { break; } if (!registerDockerDriverGatewayEndpoint()) { @@ -5530,8 +5591,21 @@ async function startDockerDriverGateway({ }); const currentInfo = runCaptureOpenshell(["gateway", "info"], { ignoreError: true }); if (isGatewayHealthy(status, namedInfo, currentInfo)) { - console.log(" βœ“ Docker-driver gateway is healthy"); - return; + // #3111: isGatewayHealthy is a pure string match on openshell CLI + // output. "Server Status" is a header emitted unconditionally, even + // when the gateway endpoint is not reachable (Connection refused). + // Gate the "healthy" log on a real TCP probe so we don't claim + // healthy for a dead or zombied gateway. On probe failure, keep + // polling β€” the binary may still be binding its listener. The + // childExited / isPidAlive check at the top of the loop will + // terminate us if the process actually died. + // + // TODO(#3213): surface this as a structured advisory rather than + // plain log output once the unified registry lands. + if (await verifyDockerDriverGatewayListening(GATEWAY_PORT, tcpProbeTimeoutMs)) { + console.log(" βœ“ Docker-driver gateway is healthy"); + return; + } } if (i < pollCount - 1) sleep(pollInterval); } @@ -5546,6 +5620,13 @@ async function startDockerDriverGateway({ : ""; if (exitOnFailure) { console.error(" Docker-driver gateway failed to start."); + if (childExited) { + const how = + childExitSignal !== null + ? `killed by signal ${childExitSignal}` + : `exited with code ${childExitCode ?? "(unknown)"}`; + console.error(` Gateway process ${how} before becoming ready.`); + } if (tail) { console.error(" Gateway log tail:"); for (const line of tail.split("\n")) console.error(` ${redact(line)}`); @@ -12052,6 +12133,7 @@ module.exports = { getResumeConfigConflicts, isGatewayHealthy, hasStaleGateway, + verifyDockerDriverGatewayListening, getRequestedSandboxNameHint, getResumeSandboxConflict, getSandboxReuseState, diff --git a/test/gateway-tcp-probe.test.ts b/test/gateway-tcp-probe.test.ts new file mode 100644 index 00000000000..7a01b2687b4 --- /dev/null +++ b/test/gateway-tcp-probe.test.ts @@ -0,0 +1,175 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +// Unit tests for verifyDockerDriverGatewayListening() β€” the TCP liveness +// probe that gates the "Docker-driver gateway is healthy" log in +// startDockerDriverGateway against the false-positive class of bug +// reported in #3111. +// +// See: https://github.com/NVIDIA/NemoClaw/issues/3111 + +import { describe, it, expect, afterEach } from "vitest"; +import fs from "node:fs"; +import net from "node:net"; +import path from "node:path"; + +// Import from the compiled output to match other onboard tests +// (src/lib/onboard.ts uses runtime require() calls that only resolve +// correctly under the built layout). +// eslint-disable-next-line @typescript-eslint/no-var-requires +const { verifyDockerDriverGatewayListening } = require("../dist/lib/onboard"); + +const ROOT = path.resolve(import.meta.dirname, ".."); + +// ── Helpers ───────────────────────────────────────────────────────────────── + +/** + * Start a TCP server that accepts connections on 127.0.0.1: and + * returns the port + a close() teardown. + */ +function startDummyServer(): Promise<{ port: number; close: () => Promise }> { + return new Promise((resolve, reject) => { + const server = net.createServer((socket) => { + // Accept and immediately close β€” we only care that the handshake succeeds. + socket.end(); + }); + server.once("error", reject); + server.listen(0, "127.0.0.1", () => { + const addr = server.address(); + if (!addr || typeof addr === "string") { + reject(new Error("Failed to get server address")); + return; + } + resolve({ + port: addr.port, + close: () => + new Promise((res, rej) => + server.close((err) => (err ? rej(err) : res())), + ), + }); + }); + }); +} + +/** + * Return a port that is unlikely to be listened on at test time. We rely on + * the OS-assigned port trick: bind a server, capture its port, close it, + * and use the (now freed) port. Racy in theory, stable in practice for CI. + */ +async function getLikelyClosedPort(): Promise { + const { port, close } = await startDummyServer(); + await close(); + return port; +} + +// ── Tests ─────────────────────────────────────────────────────────────────── + +describe("verifyDockerDriverGatewayListening (#3111)", () => { + let teardown: (() => Promise) | null = null; + + afterEach(async () => { + if (teardown) { + await teardown(); + teardown = null; + } + }); + + it("resolves true when something is accepting connections", async () => { + const { port, close } = await startDummyServer(); + teardown = close; + const reachable = await verifyDockerDriverGatewayListening(port, 500); + expect(reachable).toBe(true); + }); + + it("resolves false when nothing is listening (Connection refused)", async () => { + const port = await getLikelyClosedPort(); + const reachable = await verifyDockerDriverGatewayListening(port, 500); + expect(reachable).toBe(false); + }); + + it("resolves false on timeout (host unreachable)", async () => { + // 10.255.255.1 is a non-routable RFC 1918 address that will SYN-drop + // on most runners, forcing the timeout path rather than immediate + // ECONNREFUSED. We use a short timeout to keep the test fast. + const started = Date.now(); + const reachable = await verifyDockerDriverGatewayListening(9, 200, "10.255.255.1"); + const elapsed = Date.now() - started; + expect(reachable).toBe(false); + // Allow generous slack for slow CI but guard against the probe + // accidentally blocking on a multi-second kernel default timeout. + expect(elapsed).toBeLessThan(2000); + }); + + it("enforces a minimum timeout of 50ms even when caller passes 0", async () => { + // Regression guard: we clamp timeoutMs to 50 to avoid spinning on + // synchronous immediate-timeout settings that some net implementations + // treat as "no timeout". + const port = await getLikelyClosedPort(); + const started = Date.now(); + const reachable = await verifyDockerDriverGatewayListening(port, 0); + const elapsed = Date.now() - started; + expect(reachable).toBe(false); + // Shouldn't hang β€” on a closed localhost port ECONNREFUSED is immediate. + expect(elapsed).toBeLessThan(500); + }); + + it("never throws β€” always resolves with a boolean", async () => { + // Feed garbage parameters (port 0 which will be rewritten by OS, etc.) + // and verify we still resolve instead of rejecting. + await expect(verifyDockerDriverGatewayListening(0, 100)).resolves.toBeTypeOf( + "boolean", + ); + await expect(verifyDockerDriverGatewayListening(65535, 100)).resolves.toBe(false); + }); +}); + +// ── Source-shape guards β€” keep the integration in startDockerDriverGateway ── +// consistent with the design in ACCEPTANCE.md so future edits don't silently +// regress the #3111 fix. + +describe("startDockerDriverGateway integration (#3111)", () => { + const content = fs.readFileSync(path.join(ROOT, "src/lib/onboard.ts"), "utf-8"); + // Extract just the startDockerDriverGateway function body so other + // occurrences of the helpers (e.g., in stale-gateway reuse or module.exports) + // don't satisfy the source-shape checks. + const fnMatch = content.match( + /async function startDockerDriverGateway\([\s\S]*?\n\}\n/, + ); + if (!fnMatch) { + throw new Error( + "Expected 'async function startDockerDriverGateway' block in src/lib/onboard.ts", + ); + } + const fnBody = fnMatch[0]; + + it("tracks child-exit so zombies don't fool isPidAlive", () => { + // The fix pattern: a single 'exit' listener on the spawned ChildProcess + // that flips a flag the poll loop reads instead of relying solely on + // process.kill(pid, 0). Scope the regex to this function body. + expect(fnBody).toMatch(/child\.once\(\s*["']exit["']/); + expect(fnBody).toMatch(/childExited\s*=\s*true/); + }); + + it("breaks the poll loop when the child has exited", () => { + // The top of the loop body should consult childExited OR isPidAlive, + // not isPidAlive alone. + expect(fnBody).toMatch(/childExited\s*\|\|\s*!isPidAlive\(childPid\)/); + }); + + it("gates the 'healthy' log on a real TCP probe", () => { + // The poll loop must call verifyDockerDriverGatewayListening before + // console.log(" βœ“ Docker-driver gateway is healthy"). + // Check ordering by slicing around the 'healthy' log line. + const healthyIdx = fnBody.indexOf("Docker-driver gateway is healthy"); + expect(healthyIdx).toBeGreaterThan(0); + const before = fnBody.slice(0, healthyIdx); + expect(before).toMatch(/verifyDockerDriverGatewayListening\(GATEWAY_PORT/); + }); + + it("surfaces child-exit details in the final failure message", () => { + // On failure, the user must see *why* the gateway didn't come up β€” + // signal or exit code β€” not just "failed to start". + expect(fnBody).toMatch(/childExited/); + expect(fnBody).toMatch(/childExitSignal|childExitCode/); + }); +}); From 29bba3e605904c4d1c59290c0118c09bb16dc085 Mon Sep 17 00:00:00 2001 From: Julie Yaunches Date: Tue, 12 May 2026 11:01:06 -0400 Subject: [PATCH 3/9] refactor(onboard): converge on shared isGatewayHttpReady (#3312) + fix md lint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR review findings: 1. Merge origin/main to pick up PR #3312 (isGatewayHttpReady from src/lib/onboard/gateway-http-readiness.ts). Replace our ad-hoc verifyDockerDriverGatewayListening (TCP probe) with a call to the shared isGatewayHttpReady helper β€” HTTP is strictly stronger than a raw TCP connect and is already the de-facto standard used by every other gateway-reuse decision site in onboard.ts after #3312. Docker-driver and K3s paths now converge on the same probe. 2. Drop the parallel test/gateway-tcp-probe.test.ts (9 tests for a helper that no longer exists). The shared helper's behavior is already covered by test/gateway-http-reuse-wait.test.ts (21 tests). Replace with test/gateway-health-honest-integration.test.ts (6 source-shape tests) guarding the #3111 integration pattern. 3. Fix 7 markdownlint errors in ACCEPTANCE.md (MD040, MD022, MD031) β€” the 'checks' job flagged these; now passes locally. 4. Update ACCEPTANCE.md to reflect the simpler design: Phase 1 is 'reuse existing helper' rather than 'add new TCP probe helper', and the refactoring alignment table records #3312 as 'adopted' rather than 'to be coordinated'. Behavior of the fix is unchanged: - child.once('exit') listener tracks zombies (detached children that process.kill(pid, 0) would falsely report as alive) - poll loop guard is childExited || !isPidAlive(childPid) - healthy log gated on isGatewayHttpReady() after isGatewayHealthy() - user-visible exit code/signal surfaced on failure Related: #3111 Coverage guard: #3362 β€” gateway-health-honest-e2e --- ACCEPTANCE.md | 33 ++-- src/lib/onboard.ts | 67 ++----- .../gateway-health-honest-integration.test.ts | 95 ++++++++++ test/gateway-tcp-probe.test.ts | 175 ------------------ 4 files changed, 127 insertions(+), 243 deletions(-) create mode 100644 test/gateway-health-honest-integration.test.ts delete mode 100644 test/gateway-tcp-probe.test.ts diff --git a/ACCEPTANCE.md b/ACCEPTANCE.md index cf9d445f951..6a7d8faacdf 100644 --- a/ACCEPTANCE.md +++ b/ACCEPTANCE.md @@ -22,7 +22,7 @@ gh workflow run nightly-e2e.yaml -f jobs=gateway-health-honest-e2e --ref ` as a small, pure async helper. +**Goal:** Use the shared `isGatewayHttpReady` helper introduced by PR #3312 (`src/lib/onboard/gateway-http-readiness.ts`) as the liveness gate for the Docker-driver path. -- **File:** `src/lib/onboard.ts` (or a new `src/lib/onboard/gateway-tcp-probe.ts` if #3306 lands first). -- **Implementation:** Use `net.createConnection({ host: "127.0.0.1", port })` with `AbortSignal.timeout(timeoutMs)`. Resolve `true` on `connect`, `false` on `error` or timeout. -- **Tests:** New `test/onboard-gateway-tcp-probe.test.ts` β€” spin up a real TCP server on an ephemeral port, assert `true`; close it, assert `false`; assert timeout returns `false` within `timeoutMs + margin`. -- **Dependencies:** None. Can run in parallel with Phase 2. -- **Refactoring notes:** If #3312's `isGatewayHttpReady` has already landed, **reuse it** instead of adding a parallel helper β€” a TCP probe IS a strictly weaker signal than an HTTP probe, so prefer the HTTP probe when the gateway eventually serves HTTP. Add `// TODO(#2562): adopt unified timeout helper`. +- **File:** `src/lib/onboard.ts` β€” the import is already there, we only add a call site in `startDockerDriverGateway`. +- **Rationale:** #3312 landed before this PR. Its HTTP probe (whitelists `{200, 401}`, handles timeouts, injectable for tests) is strictly stronger than a raw TCP connect and already the de-facto standard at every other gateway-reuse decision site in `onboard.ts` (K3s startup, preflight reuse, adopt-port-listener, etc.). Reusing it keeps the Docker-driver path from drifting away from the K3s path. +- **Tests:** `test/gateway-http-reuse-wait.test.ts` already covers the helper's behavior (21 tests). We only need source-shape guards for the new call site, in `test/gateway-health-honest-integration.test.ts`. +- **Dependencies:** None. ### Phase 2 β€” Reap zombie children in the startup poll loop (🟑 Unit) @@ -119,6 +120,7 @@ The K3s path has a live container probe (`verifyGatewayContainerRunning()` added - **File:** `src/lib/onboard.ts:5519-5540` (the poll loop body). - **Implementation:** + ```ts for (let i = 0; i < pollCount; i += 1) { if (childCrashed || !isPidAlive(childPid)) { @@ -139,6 +141,7 @@ The K3s path has a live container probe (`verifyGatewayContainerRunning()` added if (i < pollCount - 1) sleep(pollInterval); } ``` + - **Tests:** - **E2E (primary, already exists):** `gateway-health-honest-e2e` (from PR #3362) must flip from red to green. - **Unit:** mock `verifyDockerDriverGatewayListening` to return `false`; assert the loop doesn't print "healthy" and eventually throws. @@ -181,8 +184,10 @@ The PR-level verdict is πŸ”΄ E2E β€” Phase 3 is the acceptance gate, and it's al - PR title: `fix(onboard): verify Docker-driver gateway is really serving before reporting healthy (#3111)` - PR body MUST include `Fixes #3111` to auto-close the issue (and get the coverage guard flipping green). - Do NOT modify `src/lib/state/gateway.ts:isGatewayHealthy` β€” pinned by `test/gateway-liveness-probe.test.ts:74`. -- Coordinate with open PR #3306 (gateway-bootstrap refactor) and #3312 (HTTP readiness probe) β€” see Refactoring Alignment above. +- Adopted PR #3312's `isGatewayHttpReady` helper (merged); no parallel TCP probe added. +- No conflict with open PR #3306 (gateway-bootstrap refactor) β€” our change does not touch the files being extracted. - The nightly-dispatch for validation: + ```bash gh workflow run nightly-e2e.yaml -f jobs=gateway-health-honest-e2e --ref issue-3111-gateway-health-honesty ``` diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 888faec3828..465590d60b5 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -3953,47 +3953,6 @@ function isPidAlive(pid: number): boolean { } } -/** - * Probe a TCP endpoint to verify something is actually listening and - * accepting connections. Used to gate the "gateway is healthy" log in - * startDockerDriverGateway against the class of bug reported in #3111 where - * the openshell-gateway binary crashed on startup but metadata-based health - * checks (isGatewayHealthy) still returned true. - * - * Resolves true on successful TCP connect. Resolves false on connection - * refused, unreachable host, or timeout. Never rejects. - * - * TODO(#2562): adopt the unified timeout abstraction once that epic lands. - */ -async function verifyDockerDriverGatewayListening( - port: number, - timeoutMs = 500, - host = "127.0.0.1", -): Promise { - // Lazy-require so tests that don't exercise the gateway path don't pay - // for the net module load, and so the import doesn't fan out across the - // already-bloated top-of-file require block. - // biome-ignore lint/suspicious/noExplicitAny: node:net types are not in scope in this file - const net: any = require("node:net"); - return new Promise((resolve) => { - let settled = false; - const settle = (result: boolean) => { - if (settled) return; - settled = true; - try { - socket.destroy(); - } catch { - /* best effort */ - } - resolve(result); - }; - const socket = net.createConnection({ host, port }); - socket.setTimeout(Math.max(50, timeoutMs)); - socket.once("connect", () => settle(true)); - socket.once("timeout", () => settle(false)); - socket.once("error", () => settle(false)); - }); -} function getDockerDriverGatewayPid(): number | null { try { @@ -5482,11 +5441,10 @@ async function startDockerDriverGateway({ const pollCount = envInt("NEMOCLAW_HEALTH_POLL_COUNT", 30); const pollInterval = envInt("NEMOCLAW_HEALTH_POLL_INTERVAL", 2); - const tcpProbeTimeoutMs = envInt("NEMOCLAW_GATEWAY_TCP_PROBE_TIMEOUT_MS", 500); for (let i = 0; i < pollCount; i += 1) { - // #3111: combine the exit-event check with isPidAlive. exit-event catches - // zombies that isPidAlive misses; isPidAlive catches children we lost - // track of (e.g., if the exit listener was removed). + // #3111: combine the exit-event check with isPidAlive. The exit event + // catches zombies that isPidAlive misses; isPidAlive catches children + // we somehow lost track of (defense in depth). if (childExited || !isPidAlive(childPid)) { break; } @@ -5501,17 +5459,19 @@ async function startDockerDriverGateway({ const currentInfo = runCaptureOpenshell(["gateway", "info"], { ignoreError: true }); if (isGatewayHealthy(status, namedInfo, currentInfo)) { // #3111: isGatewayHealthy is a pure string match on openshell CLI - // output. "Server Status" is a header emitted unconditionally, even - // when the gateway endpoint is not reachable (Connection refused). - // Gate the "healthy" log on a real TCP probe so we don't claim - // healthy for a dead or zombied gateway. On probe failure, keep - // polling β€” the binary may still be binding its listener. The - // childExited / isPidAlive check at the top of the loop will - // terminate us if the process actually died. + // output. isGatewayConnected matches on "Server Status" which is a + // header the CLI prints unconditionally, so the metadata check can + // return true even when the gateway endpoint is unreachable + // (Connection refused). Gate the "healthy" log on a real HTTP probe + // via isGatewayHttpReady() β€” the same helper used at every other + // gateway-reuse decision site after #3312. On probe failure, keep + // polling: the binary may still be binding its listener. The + // childExited / isPidAlive check at the top of the loop terminates + // us if the process actually died. // // TODO(#3213): surface this as a structured advisory rather than // plain log output once the unified registry lands. - if (await verifyDockerDriverGatewayListening(GATEWAY_PORT, tcpProbeTimeoutMs)) { + if (await isGatewayHttpReady()) { console.log(" βœ“ Docker-driver gateway is healthy"); return; } @@ -12093,7 +12053,6 @@ module.exports = { getResumeConfigConflicts, isGatewayHealthy, hasStaleGateway, - verifyDockerDriverGatewayListening, getRequestedSandboxNameHint, getResumeSandboxConflict, getSandboxReuseState, diff --git a/test/gateway-health-honest-integration.test.ts b/test/gateway-health-honest-integration.test.ts new file mode 100644 index 00000000000..695976be0cf --- /dev/null +++ b/test/gateway-health-honest-integration.test.ts @@ -0,0 +1,95 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +// Source-shape guards for the #3111 fix in startDockerDriverGateway. +// +// The fix gates the "Docker-driver gateway is healthy" log on: +// 1. a real HTTP liveness probe (isGatewayHttpReady β€” shared helper +// introduced in #3312 for the K3s reuse path), and +// 2. a child-exit listener that catches zombied detached children that +// process.kill(pid, 0) would otherwise report as alive. +// +// These guards keep future edits from silently regressing #3111. +// +// See: https://github.com/NVIDIA/NemoClaw/issues/3111 +// https://github.com/NVIDIA/NemoClaw/pull/3312 (shared HTTP helper) + +import { describe, it, expect } from "vitest"; +import fs from "node:fs"; +import path from "node:path"; + +const ROOT = path.resolve(import.meta.dirname, ".."); + +describe("startDockerDriverGateway integration (#3111)", () => { + const content = fs.readFileSync(path.join(ROOT, "src/lib/onboard.ts"), "utf-8"); + // Scope assertions to the startDockerDriverGateway function body so other + // occurrences of the helpers (e.g. in stale-gateway reuse paths or in + // module.exports) don't satisfy the source-shape checks and mask a + // regression inside this function. + const fnMatch = content.match( + /async function startDockerDriverGateway\([\s\S]*?\n\}\n/, + ); + if (!fnMatch) { + throw new Error( + "Expected 'async function startDockerDriverGateway' block in src/lib/onboard.ts", + ); + } + const fnBody = fnMatch[0]; + + it("tracks child-exit so zombies don't fool isPidAlive", () => { + // The fix pattern: a single 'exit' listener on the spawned ChildProcess + // that flips a flag the poll loop reads instead of relying solely on + // process.kill(pid, 0), which returns true for zombies. + expect(fnBody).toMatch(/child\.once\(\s*["']exit["']/); + expect(fnBody).toMatch(/childExited\s*=\s*true/); + }); + + it("breaks the poll loop when the child has exited", () => { + // The top of the loop body should consult childExited OR isPidAlive, + // not isPidAlive alone. + expect(fnBody).toMatch(/childExited\s*\|\|\s*!isPidAlive\(childPid\)/); + }); + + it("gates the 'healthy' log on the shared HTTP readiness probe", () => { + // The poll loop must call isGatewayHttpReady() before logging + // "βœ“ Docker-driver gateway is healthy". Reusing the shared helper + // introduced in #3312 keeps the Docker-driver path consistent with + // every other gateway-reuse decision site in onboard.ts. + const healthyIdx = fnBody.indexOf("Docker-driver gateway is healthy"); + expect(healthyIdx).toBeGreaterThan(0); + const before = fnBody.slice(0, healthyIdx); + expect(before).toMatch(/await\s+isGatewayHttpReady\(/); + }); + + it("does NOT add a parallel TCP/HTTP probe inside onboard.ts", () => { + // We deliberately reuse isGatewayHttpReady from the + // ./onboard/gateway-http-readiness module rather than adding a + // second probe implementation. This test catches accidental + // reintroduction of a local probe helper and keeps the surface + // small for future refactoring (#2562, #3213). + expect(fnBody).not.toMatch(/verifyDockerDriverGatewayListening\s*\(/); + expect(fnBody).not.toMatch(/net\.createConnection\(/); + }); + + it("surfaces child-exit details in the final failure message", () => { + // On failure, the user must see *why* the gateway didn't come up β€” + // signal or exit code β€” not just "failed to start". This is a UX + // improvement that falls out of tracking childExitCode/Signal. + expect(fnBody).toMatch(/childExited/); + expect(fnBody).toMatch(/childExitSignal|childExitCode/); + }); +}); + +describe("shared HTTP readiness import (#3111 reuses #3312)", () => { + const content = fs.readFileSync(path.join(ROOT, "src/lib/onboard.ts"), "utf-8"); + + it("imports isGatewayHttpReady from the shared gateway-http-readiness module", () => { + // Guards against someone adding a parallel import or a local + // reimplementation. isGatewayHttpReady must come from the canonical + // ./onboard/gateway-http-readiness module so the Docker-driver and + // K3s paths converge on the same probe semantics. + expect(content).toMatch( + /isGatewayHttpReady,[\s\S]{0,200}?require\(\s*["']\.\/onboard\/gateway-http-readiness["']/, + ); + }); +}); diff --git a/test/gateway-tcp-probe.test.ts b/test/gateway-tcp-probe.test.ts deleted file mode 100644 index 7a01b2687b4..00000000000 --- a/test/gateway-tcp-probe.test.ts +++ /dev/null @@ -1,175 +0,0 @@ -// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -// SPDX-License-Identifier: Apache-2.0 -// -// Unit tests for verifyDockerDriverGatewayListening() β€” the TCP liveness -// probe that gates the "Docker-driver gateway is healthy" log in -// startDockerDriverGateway against the false-positive class of bug -// reported in #3111. -// -// See: https://github.com/NVIDIA/NemoClaw/issues/3111 - -import { describe, it, expect, afterEach } from "vitest"; -import fs from "node:fs"; -import net from "node:net"; -import path from "node:path"; - -// Import from the compiled output to match other onboard tests -// (src/lib/onboard.ts uses runtime require() calls that only resolve -// correctly under the built layout). -// eslint-disable-next-line @typescript-eslint/no-var-requires -const { verifyDockerDriverGatewayListening } = require("../dist/lib/onboard"); - -const ROOT = path.resolve(import.meta.dirname, ".."); - -// ── Helpers ───────────────────────────────────────────────────────────────── - -/** - * Start a TCP server that accepts connections on 127.0.0.1: and - * returns the port + a close() teardown. - */ -function startDummyServer(): Promise<{ port: number; close: () => Promise }> { - return new Promise((resolve, reject) => { - const server = net.createServer((socket) => { - // Accept and immediately close β€” we only care that the handshake succeeds. - socket.end(); - }); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const addr = server.address(); - if (!addr || typeof addr === "string") { - reject(new Error("Failed to get server address")); - return; - } - resolve({ - port: addr.port, - close: () => - new Promise((res, rej) => - server.close((err) => (err ? rej(err) : res())), - ), - }); - }); - }); -} - -/** - * Return a port that is unlikely to be listened on at test time. We rely on - * the OS-assigned port trick: bind a server, capture its port, close it, - * and use the (now freed) port. Racy in theory, stable in practice for CI. - */ -async function getLikelyClosedPort(): Promise { - const { port, close } = await startDummyServer(); - await close(); - return port; -} - -// ── Tests ─────────────────────────────────────────────────────────────────── - -describe("verifyDockerDriverGatewayListening (#3111)", () => { - let teardown: (() => Promise) | null = null; - - afterEach(async () => { - if (teardown) { - await teardown(); - teardown = null; - } - }); - - it("resolves true when something is accepting connections", async () => { - const { port, close } = await startDummyServer(); - teardown = close; - const reachable = await verifyDockerDriverGatewayListening(port, 500); - expect(reachable).toBe(true); - }); - - it("resolves false when nothing is listening (Connection refused)", async () => { - const port = await getLikelyClosedPort(); - const reachable = await verifyDockerDriverGatewayListening(port, 500); - expect(reachable).toBe(false); - }); - - it("resolves false on timeout (host unreachable)", async () => { - // 10.255.255.1 is a non-routable RFC 1918 address that will SYN-drop - // on most runners, forcing the timeout path rather than immediate - // ECONNREFUSED. We use a short timeout to keep the test fast. - const started = Date.now(); - const reachable = await verifyDockerDriverGatewayListening(9, 200, "10.255.255.1"); - const elapsed = Date.now() - started; - expect(reachable).toBe(false); - // Allow generous slack for slow CI but guard against the probe - // accidentally blocking on a multi-second kernel default timeout. - expect(elapsed).toBeLessThan(2000); - }); - - it("enforces a minimum timeout of 50ms even when caller passes 0", async () => { - // Regression guard: we clamp timeoutMs to 50 to avoid spinning on - // synchronous immediate-timeout settings that some net implementations - // treat as "no timeout". - const port = await getLikelyClosedPort(); - const started = Date.now(); - const reachable = await verifyDockerDriverGatewayListening(port, 0); - const elapsed = Date.now() - started; - expect(reachable).toBe(false); - // Shouldn't hang β€” on a closed localhost port ECONNREFUSED is immediate. - expect(elapsed).toBeLessThan(500); - }); - - it("never throws β€” always resolves with a boolean", async () => { - // Feed garbage parameters (port 0 which will be rewritten by OS, etc.) - // and verify we still resolve instead of rejecting. - await expect(verifyDockerDriverGatewayListening(0, 100)).resolves.toBeTypeOf( - "boolean", - ); - await expect(verifyDockerDriverGatewayListening(65535, 100)).resolves.toBe(false); - }); -}); - -// ── Source-shape guards β€” keep the integration in startDockerDriverGateway ── -// consistent with the design in ACCEPTANCE.md so future edits don't silently -// regress the #3111 fix. - -describe("startDockerDriverGateway integration (#3111)", () => { - const content = fs.readFileSync(path.join(ROOT, "src/lib/onboard.ts"), "utf-8"); - // Extract just the startDockerDriverGateway function body so other - // occurrences of the helpers (e.g., in stale-gateway reuse or module.exports) - // don't satisfy the source-shape checks. - const fnMatch = content.match( - /async function startDockerDriverGateway\([\s\S]*?\n\}\n/, - ); - if (!fnMatch) { - throw new Error( - "Expected 'async function startDockerDriverGateway' block in src/lib/onboard.ts", - ); - } - const fnBody = fnMatch[0]; - - it("tracks child-exit so zombies don't fool isPidAlive", () => { - // The fix pattern: a single 'exit' listener on the spawned ChildProcess - // that flips a flag the poll loop reads instead of relying solely on - // process.kill(pid, 0). Scope the regex to this function body. - expect(fnBody).toMatch(/child\.once\(\s*["']exit["']/); - expect(fnBody).toMatch(/childExited\s*=\s*true/); - }); - - it("breaks the poll loop when the child has exited", () => { - // The top of the loop body should consult childExited OR isPidAlive, - // not isPidAlive alone. - expect(fnBody).toMatch(/childExited\s*\|\|\s*!isPidAlive\(childPid\)/); - }); - - it("gates the 'healthy' log on a real TCP probe", () => { - // The poll loop must call verifyDockerDriverGatewayListening before - // console.log(" βœ“ Docker-driver gateway is healthy"). - // Check ordering by slicing around the 'healthy' log line. - const healthyIdx = fnBody.indexOf("Docker-driver gateway is healthy"); - expect(healthyIdx).toBeGreaterThan(0); - const before = fnBody.slice(0, healthyIdx); - expect(before).toMatch(/verifyDockerDriverGatewayListening\(GATEWAY_PORT/); - }); - - it("surfaces child-exit details in the final failure message", () => { - // On failure, the user must see *why* the gateway didn't come up β€” - // signal or exit code β€” not just "failed to start". - expect(fnBody).toMatch(/childExited/); - expect(fnBody).toMatch(/childExitSignal|childExitCode/); - }); -}); From a5fad4e76d6e1377b293c7470a8e212c24394b1d Mon Sep 17 00:00:00 2001 From: Julie Yaunches Date: Tue, 12 May 2026 11:21:56 -0400 Subject: [PATCH 4/9] docs: address CodeRabbit feedback on ACCEPTANCE.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add SPDX license header (HTML-comment style, matching tools/e2e-advisor/README.md) - Replace emoji status markers (πŸ”΄/🟒/🟑/✨) with plain-text labels ([E2E], [OK], [Unit]) per the coding guideline 'No emoji in technical prose'. Literal 'βœ“' characters in log-output examples are retained because they are not decorative β€” they are the actual bytes the onboard code prints. --- ACCEPTANCE.md | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/ACCEPTANCE.md b/ACCEPTANCE.md index 6a7d8faacdf..0e46af983cb 100644 --- a/ACCEPTANCE.md +++ b/ACCEPTANCE.md @@ -1,3 +1,6 @@ + + + # Issue #3111 β€” Development Plan + Acceptance Criterion **Worktree:** `/Users/jyaunches/Development/NemoClaw-working/issue-3111` @@ -9,7 +12,7 @@ ## Definition of Done (auto-added by `pr-e2e-loop`) -Nightly job **`gateway-health-honest-e2e`** (added in PR #3362) must flip from πŸ”΄ red on `main` to 🟒 green on the fix branch. Dispatch locally with: +Nightly job **`gateway-health-honest-e2e`** (added in PR #3362) must flip from red on `main` to green on the fix branch. Dispatch locally with: ```bash gh workflow run nightly-e2e.yaml -f jobs=gateway-health-honest-e2e --ref @@ -77,10 +80,10 @@ The K3s path has a live container probe (`verifyGatewayContainerRunning()` added | # | Area | Change | Test Level | Rationale | |---|------|--------|-----------|-----------| -| 1 | `src/lib/onboard.ts` β€” reuse `isGatewayHttpReady` from `./onboard/gateway-http-readiness` (introduced by #3312) in the `startDockerDriverGateway` poll loop | Gate the `"βœ“ Docker-driver gateway is healthy"` log on a real HTTP probe. Converges with the K3s-path pattern rather than adding a parallel TCP probe. | πŸ”΄ **E2E** | Spawns real process, binds real port. `gateway-health-honest-e2e` IS the E2E for this change. | -| 2 | `src/lib/onboard.ts` β€” reap zombie child before `isPidAlive` | After spawn, periodically `waitpid`-equivalent check so zombies get detected | 🟑 Unit + πŸ”΄ E2E | Same E2E catches this; also add a unit test that mocks a zombied PID. | -| 3 | `src/lib/state/gateway.ts` β€” **do NOT modify** | The gateway-liveness-probe test pins this file to pure-function status (see #2020 follow-up test at `test/gateway-liveness-probe.test.ts:74`). Keep the string match; fix at the caller. | 🟒 Unit | No source change. | -| 4 | `scripts/install-openshell.sh` β€” OPTIONAL: add a preflight that verifies the downloaded `openshell-gateway` binary can actually exec (`$BIN --version` or similar) before writing the installed marker | Surface GLIBC / linker errors at install time rather than onboard time | 🟑 Unit + ✨ (install script) | Out of scope for the primary fix but a useful secondary safety net for #3111's specific Ubuntu 22.04 trigger. Split into follow-up issue if larger. | +| 1 | `src/lib/onboard.ts` β€” reuse `isGatewayHttpReady` from `./onboard/gateway-http-readiness` (introduced by #3312) in the `startDockerDriverGateway` poll loop | Gate the `"βœ“ Docker-driver gateway is healthy"` log on a real HTTP probe. Converges with the K3s-path pattern rather than adding a parallel TCP probe. | **[E2E]** required | Spawns real process, binds real port. `gateway-health-honest-e2e` IS the E2E for this change. | +| 2 | `src/lib/onboard.ts` β€” reap zombie child before `isPidAlive` | After spawn, periodically `waitpid`-equivalent check so zombies get detected | **[Unit]** + **[E2E]** | Same E2E catches this; also add a unit test that mocks a zombied PID. | +| 3 | `src/lib/state/gateway.ts` β€” **do NOT modify** | The gateway-liveness-probe test pins this file to pure-function status (see #2020 follow-up test at `test/gateway-liveness-probe.test.ts:74`). Keep the string match; fix at the caller. | **[Unit]** only | No source change. | +| 4 | `scripts/install-openshell.sh` β€” OPTIONAL: add a preflight that verifies the downloaded `openshell-gateway` binary can actually exec (`$BIN --version` or similar) before writing the installed marker | Surface GLIBC / linker errors at install time rather than onboard time | **[Unit]** (install script) | Out of scope for the primary fix but a useful secondary safety net for #3111's specific Ubuntu 22.04 trigger. Split into follow-up issue if larger. | --- @@ -96,7 +99,7 @@ The K3s path has a live container probe (`verifyGatewayContainerRunning()` added ## Implementation Plan -### Phase 1 β€” Reuse `isGatewayHttpReady` from #3312 (🟒 No new helper needed) +### Phase 1 β€” Reuse `isGatewayHttpReady` from #3312 (**[OK]** No new helper) **Goal:** Use the shared `isGatewayHttpReady` helper introduced by PR #3312 (`src/lib/onboard/gateway-http-readiness.ts`) as the liveness gate for the Docker-driver path. @@ -105,7 +108,7 @@ The K3s path has a live container probe (`verifyGatewayContainerRunning()` added - **Tests:** `test/gateway-http-reuse-wait.test.ts` already covers the helper's behavior (21 tests). We only need source-shape guards for the new call site, in `test/gateway-health-honest-integration.test.ts`. - **Dependencies:** None. -### Phase 2 β€” Reap zombie children in the startup poll loop (🟑 Unit) +### Phase 2 β€” Reap zombie children in the startup poll loop (**[Unit]** only) **Goal:** Detect crashed-then-zombied children so `isPidAlive` returns false as expected. @@ -114,7 +117,7 @@ The K3s path has a live container probe (`verifyGatewayContainerRunning()` added - **Tests:** Extend `test/onboard.test.ts` (or add `test/gateway-zombie-reap.test.ts`) β€” spawn a shim that exits immediately, assert the flag flips within N ms. - **Dependencies:** None. Runs in parallel with Phase 1. -### Phase 3 β€” Integrate both checks into the poll loop (πŸ”΄ E2E required) +### Phase 3 β€” Integrate both checks into the poll loop (**[E2E]** required) **Goal:** Make `startDockerDriverGateway` fail closed β€” "healthy" is only logged when the process is running AND metadata matches AND the port is serving. @@ -148,7 +151,7 @@ The K3s path has a live container probe (`verifyGatewayContainerRunning()` added - **Dependencies:** Phase 1 and Phase 2 must land first. - **Refactoring notes:** Format the final "failed to start" message as structured data with `code: "gateway-failed-to-start"` for future #3213 migration. Leave the log-tail output intact β€” it's already user-facing diagnostic. -### Phase 4 (OPTIONAL) β€” Install-time GLIBC check (🟑 Unit) +### Phase 4 (OPTIONAL) β€” Install-time GLIBC check (**[Unit]** only) **Goal:** Surface the GLIBC mismatch at install time, not onboard time. Addresses the specific #3111 trigger, not the class. @@ -166,16 +169,16 @@ The K3s path has a live container probe (`verifyGatewayContainerRunning()` added - **Phase 3 sequentially after 1 + 2.** - **Phase 4 optional / follow-up.** -## Verdict: πŸ”΄ E2E required +## Verdict: **[E2E]** required | Phase | Test Depth | |---|---| -| Phase 1 (TCP probe helper) | 🟑 Unit | -| Phase 2 (zombie-reap) | 🟑 Unit | -| Phase 3 (poll-loop integration) | πŸ”΄ E2E (`gateway-health-honest-e2e`) | -| Phase 4 (install-time check) | 🟑 Unit (optional) | +| Phase 1 (reuse `isGatewayHttpReady`) | **[Unit]** only | +| Phase 2 (zombie-reap) | **[Unit]** only | +| Phase 3 (poll-loop integration) | **[E2E]** (`gateway-health-honest-e2e`) | +| Phase 4 (install-time check) | **[Unit]** only (optional) | -The PR-level verdict is πŸ”΄ E2E β€” Phase 3 is the acceptance gate, and it's already implemented as the merged coverage guard. The fix PR's definition-of-done is "this E2E flips green." +The PR-level verdict is **[E2E]** β€” Phase 3 is the acceptance gate, and it's already implemented as the merged coverage guard. The fix PR's definition-of-done is "this E2E flips green." --- From c93a5feaa840f7053fe6d1c9947bbc75cf0509d9 Mon Sep 17 00:00:00 2001 From: Julie Yaunches Date: Tue, 12 May 2026 14:04:15 -0400 Subject: [PATCH 5/9] fix(onboard): use TCP probe (not HTTP) for Docker-driver gateway readiness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the failing openshell-gateway-upgrade-e2e surfaced by the e2e-advisor on PR #3378. ## Problem The previous commit converged the Docker-driver gateway's readiness probe onto isGatewayHttpReady from ./onboard/gateway-http-readiness (introduced by #3312). That broke openshell-gateway-upgrade-e2e: Starting OpenShell gateway... [PASS] Stale gateway is healthy with supervisor:0.0.36 [INFO] Invoking NemoClaw gateway start path; it must restart the stale process Docker-driver gateway failed to start. <-- 60s timeout The gateway log showed the process was actually serving correctly: POST /openshell.v1.OpenShell/Health response status=200 latency_ms=0 POST /openshell.v1.OpenShell/Health response status=200 latency_ms=0 GET / response status=404 latency_ms=0 ## Root cause isGatewayHttpReady does GET /, and only accepts {200, 401}. The K3s-path gateway answers GET / via a dispatcher catch-all. The Docker-driver gateway, by contrast, only serves routes under /openshell.v1.OpenShell/* and returns 404 for GET /. So the probe always fails on the Docker- driver path regardless of whether the gateway is up. ## Fix Revert to a plain TCP probe on the gateway port for the Docker-driver path. Add a small isDockerDriverGatewayTcpReady(port, timeoutMs, host) helper next to isPidAlive. This is semantic-free β€” it just asks "is anyone listening" β€” which is exactly what's needed to detect the #3111 failure mode (crashed binary β†’ nothing listening) without making assumptions about HTTP route shape. isGatewayHttpReady is left in place for all K3s-path call sites; the two paths use different probes because the two gateway types expose different HTTP surfaces. TODO(#3213) filed for future convergence. ## Tests test/gateway-health-honest-integration.test.ts now has 10 tests: 5 behavioural: listening / closed / timeout / min-timeout / no-throw 5 source-shape: child-exit tracking, poll-loop guard, TCP probe call site, regression guard against reintroducing the HTTP probe in the Docker-driver loop, exit-details surfaced ## Expected CI result - gateway-health-honest-e2e: GREEN (already was β€” this was never affected) - openshell-gateway-upgrade-e2e: FLIPS BACK to GREEN (the breakage this commit addresses) Related: #3111 Broke in: 29bba3e60 (refactor(onboard): converge on shared isGatewayHttpReady) --- ACCEPTANCE.md | 19 ++- src/lib/onboard.ts | 75 ++++++++- .../gateway-health-honest-integration.test.ts | 154 +++++++++++++----- 3 files changed, 187 insertions(+), 61 deletions(-) diff --git a/ACCEPTANCE.md b/ACCEPTANCE.md index 0e46af983cb..96996c21470 100644 --- a/ACCEPTANCE.md +++ b/ACCEPTANCE.md @@ -80,7 +80,7 @@ The K3s path has a live container probe (`verifyGatewayContainerRunning()` added | # | Area | Change | Test Level | Rationale | |---|------|--------|-----------|-----------| -| 1 | `src/lib/onboard.ts` β€” reuse `isGatewayHttpReady` from `./onboard/gateway-http-readiness` (introduced by #3312) in the `startDockerDriverGateway` poll loop | Gate the `"βœ“ Docker-driver gateway is healthy"` log on a real HTTP probe. Converges with the K3s-path pattern rather than adding a parallel TCP probe. | **[E2E]** required | Spawns real process, binds real port. `gateway-health-honest-e2e` IS the E2E for this change. | +| 1 | `src/lib/onboard.ts` β€” new `isDockerDriverGatewayTcpReady()` helper + integrate into `startDockerDriverGateway` poll loop | Gate the `"βœ“ Docker-driver gateway is healthy"` log on a real TCP connect. (HTTP probe via #3312 was attempted but broke the openshell-gateway-upgrade-e2e test β€” Docker-driver returns 404 for GET /.) | **[E2E]** required | Spawns real process, binds real port. `gateway-health-honest-e2e` AND `openshell-gateway-upgrade-e2e` both exercise this change. | | 2 | `src/lib/onboard.ts` β€” reap zombie child before `isPidAlive` | After spawn, periodically `waitpid`-equivalent check so zombies get detected | **[Unit]** + **[E2E]** | Same E2E catches this; also add a unit test that mocks a zombied PID. | | 3 | `src/lib/state/gateway.ts` β€” **do NOT modify** | The gateway-liveness-probe test pins this file to pure-function status (see #2020 follow-up test at `test/gateway-liveness-probe.test.ts:74`). Keep the string match; fix at the caller. | **[Unit]** only | No source change. | | 4 | `scripts/install-openshell.sh` β€” OPTIONAL: add a preflight that verifies the downloaded `openshell-gateway` binary can actually exec (`$BIN --version` or similar) before writing the installed marker | Surface GLIBC / linker errors at install time rather than onboard time | **[Unit]** (install script) | Out of scope for the primary fix but a useful secondary safety net for #3111's specific Ubuntu 22.04 trigger. Split into follow-up issue if larger. | @@ -91,7 +91,7 @@ The K3s path has a live container probe (`verifyGatewayContainerRunning()` added | # | Refactoring Goal | Overlap | Recommendation | |---|-----------------|---------|----------------| -| 1 | **PR #3312** (laitingsheng, **MERGED**) β€” `fix(onboard): require host HTTP readiness before reusing the gateway` | Added `isGatewayHttpReady(timeoutMs, url)` β€” an HTTP-level readiness probe in `src/lib/onboard/gateway-http-readiness.ts`. Already used at every gateway-reuse decision site on the K3s path. | **Adopted.** We now call `isGatewayHttpReady()` directly from `startDockerDriverGateway`'s poll loop instead of adding a parallel TCP probe. Docker-driver and K3s paths share the same probe semantics. | +| 1 | **PR #3312** (laitingsheng, **MERGED**) β€” `fix(onboard): require host HTTP readiness before reusing the gateway` | Added `isGatewayHttpReady(timeoutMs, url)` β€” HTTP probe for the K3s-path gateway. Assumes `GET /` returns 200/401. | **Considered, not adopted.** The K3s-path and Docker-driver-path gateways expose different root-path semantics (Docker-driver returns 404 on `GET /`). We use a local TCP probe for the Docker-driver path and leave unification to #3213. See `isDockerDriverGatewayTcpReady()`. | | 2 | **PR #3306** (cv) β€” `refactor(onboard): extract gateway bootstrap repair helpers` | Extracts `src/lib/onboard/gateway-bootstrap.ts` module. | No conflict with our change β€” doesn't touch `startDockerDriverGateway`. | | 3 | **#3213** β€” Epic: unify warnings, advisories, and fatal exits under a single registry | When onboard exits with "gateway failed to start", that message should eventually flow through the unified advisory registry. | **Seed the pattern.** `TODO(#3213)` comment added on the `startDockerDriverGateway` poll loop so the future migration to structured advisories is mechanical. | @@ -99,13 +99,14 @@ The K3s path has a live container probe (`verifyGatewayContainerRunning()` added ## Implementation Plan -### Phase 1 β€” Reuse `isGatewayHttpReady` from #3312 (**[OK]** No new helper) +### Phase 1 β€” Add a TCP liveness probe (**[Unit]** only) -**Goal:** Use the shared `isGatewayHttpReady` helper introduced by PR #3312 (`src/lib/onboard/gateway-http-readiness.ts`) as the liveness gate for the Docker-driver path. +**Goal:** Introduce `isDockerDriverGatewayTcpReady(port, timeoutMs, host)` as a small async helper in `src/lib/onboard.ts`, used to gate the "Docker-driver gateway is healthy" log on a real TCP connect. -- **File:** `src/lib/onboard.ts` β€” the import is already there, we only add a call site in `startDockerDriverGateway`. -- **Rationale:** #3312 landed before this PR. Its HTTP probe (whitelists `{200, 401}`, handles timeouts, injectable for tests) is strictly stronger than a raw TCP connect and already the de-facto standard at every other gateway-reuse decision site in `onboard.ts` (K3s startup, preflight reuse, adopt-port-listener, etc.). Reusing it keeps the Docker-driver path from drifting away from the K3s path. -- **Tests:** `test/gateway-http-reuse-wait.test.ts` already covers the helper's behavior (21 tests). We only need source-shape guards for the new call site, in `test/gateway-health-honest-integration.test.ts`. +- **File:** `src/lib/onboard.ts` β€” new helper near `isPidAlive`. +- **Why TCP and not HTTP:** We originally tried reusing `isGatewayHttpReady` from `./onboard/gateway-http-readiness` (introduced by #3312), but that broke the existing `openshell-gateway-upgrade-e2e` test. The K3s gateway and the Docker-driver gateway serve different HTTP routes for the root path β€” K3s answers `GET /` with 200/401, but the Docker-driver gateway returns 404 for `GET /` and only serves `/openshell.v1.OpenShell/*`. A plain TCP connect is semantic-free and sufficient to catch the #3111 failure mode (crashed binary β†’ nothing listening). Convergence with `isGatewayHttpReady` is deferred to #3213 (unified advisory / probe registry). +- **Implementation:** `net.createConnection({ host, port })` with a socket timeout. Resolves boolean, never throws. +- **Tests:** `test/gateway-health-honest-integration.test.ts` β€” 5 behavioural tests (listening / closed / timeout / min-timeout / never-throws) + 5 source-shape guards. - **Dependencies:** None. ### Phase 2 β€” Reap zombie children in the startup poll loop (**[Unit]** only) @@ -173,7 +174,7 @@ The K3s path has a live container probe (`verifyGatewayContainerRunning()` added | Phase | Test Depth | |---|---| -| Phase 1 (reuse `isGatewayHttpReady`) | **[Unit]** only | +| Phase 1 (`isDockerDriverGatewayTcpReady` helper) | **[Unit]** only | | Phase 2 (zombie-reap) | **[Unit]** only | | Phase 3 (poll-loop integration) | **[E2E]** (`gateway-health-honest-e2e`) | | Phase 4 (install-time check) | **[Unit]** only (optional) | @@ -187,7 +188,7 @@ The PR-level verdict is **[E2E]** β€” Phase 3 is the acceptance gate, and it's a - PR title: `fix(onboard): verify Docker-driver gateway is really serving before reporting healthy (#3111)` - PR body MUST include `Fixes #3111` to auto-close the issue (and get the coverage guard flipping green). - Do NOT modify `src/lib/state/gateway.ts:isGatewayHealthy` β€” pinned by `test/gateway-liveness-probe.test.ts:74`. -- Adopted PR #3312's `isGatewayHttpReady` helper (merged); no parallel TCP probe added. +- Added local `isDockerDriverGatewayTcpReady()` helper in `onboard.ts`; did NOT converge onto PR #3312's `isGatewayHttpReady` because the two gateway types have different HTTP root-path semantics (Docker-driver returns 404 on `GET /`, which fails the HTTP probe). TODO(#3213) filed for future unification. - No conflict with open PR #3306 (gateway-bootstrap refactor) β€” our change does not touch the files being extracted. - The nightly-dispatch for validation: diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 465590d60b5..c127e25350e 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -3953,6 +3953,60 @@ function isPidAlive(pid: number): boolean { } } +/** + * Probe a TCP endpoint on localhost to verify something is actually listening + * and accepting connections. Used to gate the "Docker-driver gateway is + * healthy" log in startDockerDriverGateway against the class of bug reported + * in #3111 where the openshell-gateway binary crashed on startup but + * metadata-based health checks (isGatewayHealthy) still returned true. + * + * We intentionally use a plain TCP probe here rather than the stronger HTTP + * probe from ./onboard/gateway-http-readiness (isGatewayHttpReady) because + * the Docker-driver gateway and the K3s gateway have different HTTP routes + * for the root path: + * + * - K3s path: GET / returns 200 / 401 (dispatcher catch-all) + * - Docker-driver path: GET / returns 404; only /openshell.v1.OpenShell/* + * is served + * + * A TCP connect is semantic-free (it just asks "is anyone listening?") + * which is exactly what's needed to detect the #3111 failure mode without + * making assumptions about HTTP route shape. Tracking convergence with + * isGatewayHttpReady is left to #3213 (the unified advisory / probe + * registry). + * + * Resolves true on successful TCP connect. Resolves false on connection + * refused, unreachable host, or timeout. Never rejects. + */ +async function isDockerDriverGatewayTcpReady( + port: number, + timeoutMs = 500, + host = "127.0.0.1", +): Promise { + // Lazy-require so tests that don't exercise the gateway path don't pay + // for the net module load. + // biome-ignore lint/suspicious/noExplicitAny: node:net types are not in scope in this file + const net: any = require("node:net"); + return new Promise((resolve) => { + let settled = false; + const settle = (result: boolean) => { + if (settled) return; + settled = true; + try { + socket.destroy(); + } catch { + /* best effort */ + } + resolve(result); + }; + const socket = net.createConnection({ host, port }); + socket.setTimeout(Math.max(50, timeoutMs)); + socket.once("connect", () => settle(true)); + socket.once("timeout", () => settle(false)); + socket.once("error", () => settle(false)); + }); +} + function getDockerDriverGatewayPid(): number | null { try { @@ -5462,16 +5516,22 @@ async function startDockerDriverGateway({ // output. isGatewayConnected matches on "Server Status" which is a // header the CLI prints unconditionally, so the metadata check can // return true even when the gateway endpoint is unreachable - // (Connection refused). Gate the "healthy" log on a real HTTP probe - // via isGatewayHttpReady() β€” the same helper used at every other - // gateway-reuse decision site after #3312. On probe failure, keep - // polling: the binary may still be binding its listener. The - // childExited / isPidAlive check at the top of the loop terminates - // us if the process actually died. + // (Connection refused). Gate the "healthy" log on a real TCP probe + // to verify something is actually listening on the gateway port. + // + // We use a TCP probe rather than the HTTP probe from + // ./onboard/gateway-http-readiness because the Docker-driver and K3s + // gateways serve different HTTP routes (Docker-driver: only the + // gRPC endpoints under /openshell.v1.OpenShell/; K3s: a dispatcher + // that answers GET /). See isDockerDriverGatewayTcpReady() above. + // + // On probe failure, keep polling: the binary may still be binding + // its listener. The childExited / isPidAlive check at the top of + // the loop terminates us if the process actually died. // // TODO(#3213): surface this as a structured advisory rather than // plain log output once the unified registry lands. - if (await isGatewayHttpReady()) { + if (await isDockerDriverGatewayTcpReady(GATEWAY_PORT)) { console.log(" βœ“ Docker-driver gateway is healthy"); return; } @@ -12053,6 +12113,7 @@ module.exports = { getResumeConfigConflicts, isGatewayHealthy, hasStaleGateway, + isDockerDriverGatewayTcpReady, getRequestedSandboxNameHint, getResumeSandboxConflict, getSandboxReuseState, diff --git a/test/gateway-health-honest-integration.test.ts b/test/gateway-health-honest-integration.test.ts index 695976be0cf..6f63e98102c 100644 --- a/test/gateway-health-honest-integration.test.ts +++ b/test/gateway-health-honest-integration.test.ts @@ -1,31 +1,115 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 // -// Source-shape guards for the #3111 fix in startDockerDriverGateway. +// Source-shape + behavioural guards for the #3111 fix in startDockerDriverGateway. // // The fix gates the "Docker-driver gateway is healthy" log on: -// 1. a real HTTP liveness probe (isGatewayHttpReady β€” shared helper -// introduced in #3312 for the K3s reuse path), and +// 1. a real TCP liveness probe (isDockerDriverGatewayTcpReady) β€” plain +// TCP, not HTTP, because the Docker-driver gateway and the K3s +// gateway expose different root paths (see isGatewayHttpReady in +// ./onboard/gateway-http-readiness for the K3s-path probe); and // 2. a child-exit listener that catches zombied detached children that // process.kill(pid, 0) would otherwise report as alive. // // These guards keep future edits from silently regressing #3111. // // See: https://github.com/NVIDIA/NemoClaw/issues/3111 -// https://github.com/NVIDIA/NemoClaw/pull/3312 (shared HTTP helper) +// https://github.com/NVIDIA/NemoClaw/pull/3312 (K3s-path HTTP helper) -import { describe, it, expect } from "vitest"; +import { describe, it, expect, afterEach } from "vitest"; import fs from "node:fs"; +import net from "node:net"; import path from "node:path"; +// eslint-disable-next-line @typescript-eslint/no-var-requires +const { isDockerDriverGatewayTcpReady } = require("../dist/lib/onboard"); + const ROOT = path.resolve(import.meta.dirname, ".."); +// ── Helpers ───────────────────────────────────────────────────────────────── + +function startDummyServer(): Promise<{ port: number; close: () => Promise }> { + return new Promise((resolve, reject) => { + const server = net.createServer((socket) => socket.end()); + server.once("error", reject); + server.listen(0, "127.0.0.1", () => { + const addr = server.address(); + if (!addr || typeof addr === "string") { + reject(new Error("failed to resolve server address")); + return; + } + resolve({ + port: addr.port, + close: () => + new Promise((res, rej) => + server.close((err) => (err ? rej(err) : res())), + ), + }); + }); + }); +} + +async function getLikelyClosedPort(): Promise { + const { port, close } = await startDummyServer(); + await close(); + return port; +} + +// ── Behavioural tests for isDockerDriverGatewayTcpReady ───────────────────── + +describe("isDockerDriverGatewayTcpReady (#3111)", () => { + let teardown: (() => Promise) | null = null; + + afterEach(async () => { + if (teardown) { + await teardown(); + teardown = null; + } + }); + + it("resolves true when something is accepting connections", async () => { + const { port, close } = await startDummyServer(); + teardown = close; + await expect(isDockerDriverGatewayTcpReady(port, 500)).resolves.toBe(true); + }); + + it("resolves false when nothing is listening (Connection refused)", async () => { + const port = await getLikelyClosedPort(); + await expect(isDockerDriverGatewayTcpReady(port, 500)).resolves.toBe(false); + }); + + it("resolves false on timeout (non-routable host)", async () => { + // 10.255.255.1 is a non-routable RFC 1918 address that SYN-drops on most + // CI runners, forcing the timeout path rather than immediate ECONNREFUSED. + const started = Date.now(); + await expect( + isDockerDriverGatewayTcpReady(9, 200, "10.255.255.1"), + ).resolves.toBe(false); + const elapsed = Date.now() - started; + expect(elapsed).toBeLessThan(2000); + }); + + it("enforces a minimum timeout of 50ms even when caller passes 0", async () => { + const port = await getLikelyClosedPort(); + const started = Date.now(); + await expect(isDockerDriverGatewayTcpReady(port, 0)).resolves.toBe(false); + expect(Date.now() - started).toBeLessThan(500); + }); + + it("never throws β€” always resolves with a boolean", async () => { + await expect(isDockerDriverGatewayTcpReady(0, 100)).resolves.toBeTypeOf( + "boolean", + ); + await expect(isDockerDriverGatewayTcpReady(65535, 100)).resolves.toBe( + false, + ); + }); +}); + +// ── Source-shape guards for the integration in startDockerDriverGateway ──── + describe("startDockerDriverGateway integration (#3111)", () => { const content = fs.readFileSync(path.join(ROOT, "src/lib/onboard.ts"), "utf-8"); - // Scope assertions to the startDockerDriverGateway function body so other - // occurrences of the helpers (e.g. in stale-gateway reuse paths or in - // module.exports) don't satisfy the source-shape checks and mask a - // regression inside this function. const fnMatch = content.match( /async function startDockerDriverGateway\([\s\S]*?\n\}\n/, ); @@ -37,59 +121,39 @@ describe("startDockerDriverGateway integration (#3111)", () => { const fnBody = fnMatch[0]; it("tracks child-exit so zombies don't fool isPidAlive", () => { - // The fix pattern: a single 'exit' listener on the spawned ChildProcess - // that flips a flag the poll loop reads instead of relying solely on - // process.kill(pid, 0), which returns true for zombies. expect(fnBody).toMatch(/child\.once\(\s*["']exit["']/); expect(fnBody).toMatch(/childExited\s*=\s*true/); }); it("breaks the poll loop when the child has exited", () => { - // The top of the loop body should consult childExited OR isPidAlive, - // not isPidAlive alone. expect(fnBody).toMatch(/childExited\s*\|\|\s*!isPidAlive\(childPid\)/); }); - it("gates the 'healthy' log on the shared HTTP readiness probe", () => { - // The poll loop must call isGatewayHttpReady() before logging - // "βœ“ Docker-driver gateway is healthy". Reusing the shared helper - // introduced in #3312 keeps the Docker-driver path consistent with - // every other gateway-reuse decision site in onboard.ts. + it("gates the 'healthy' log on the TCP readiness probe", () => { + // The poll loop must call isDockerDriverGatewayTcpReady(GATEWAY_PORT) + // before logging "βœ“ Docker-driver gateway is healthy". We deliberately + // use a TCP probe rather than the K3s-path isGatewayHttpReady because + // the Docker-driver gateway only serves /openshell.v1.OpenShell/* β€” + // GET / returns 404, which fails the HTTP probe even though the + // gateway is functional. const healthyIdx = fnBody.indexOf("Docker-driver gateway is healthy"); expect(healthyIdx).toBeGreaterThan(0); const before = fnBody.slice(0, healthyIdx); - expect(before).toMatch(/await\s+isGatewayHttpReady\(/); + expect(before).toMatch(/await\s+isDockerDriverGatewayTcpReady\(GATEWAY_PORT/); }); - it("does NOT add a parallel TCP/HTTP probe inside onboard.ts", () => { - // We deliberately reuse isGatewayHttpReady from the - // ./onboard/gateway-http-readiness module rather than adding a - // second probe implementation. This test catches accidental - // reintroduction of a local probe helper and keeps the surface - // small for future refactoring (#2562, #3213). - expect(fnBody).not.toMatch(/verifyDockerDriverGatewayListening\s*\(/); - expect(fnBody).not.toMatch(/net\.createConnection\(/); + it("does NOT use the K3s-path HTTP probe in the Docker-driver loop", () => { + // Regression guard: a previous version of this fix called + // isGatewayHttpReady() here, which broke the existing + // openshell-gateway-upgrade-e2e test because the Docker-driver + // gateway returns 404 on GET /. Do not reintroduce that pattern. + const healthyIdx = fnBody.indexOf("Docker-driver gateway is healthy"); + const before = fnBody.slice(0, healthyIdx); + expect(before).not.toMatch(/await\s+isGatewayHttpReady\(/); }); it("surfaces child-exit details in the final failure message", () => { - // On failure, the user must see *why* the gateway didn't come up β€” - // signal or exit code β€” not just "failed to start". This is a UX - // improvement that falls out of tracking childExitCode/Signal. expect(fnBody).toMatch(/childExited/); expect(fnBody).toMatch(/childExitSignal|childExitCode/); }); }); - -describe("shared HTTP readiness import (#3111 reuses #3312)", () => { - const content = fs.readFileSync(path.join(ROOT, "src/lib/onboard.ts"), "utf-8"); - - it("imports isGatewayHttpReady from the shared gateway-http-readiness module", () => { - // Guards against someone adding a parallel import or a local - // reimplementation. isGatewayHttpReady must come from the canonical - // ./onboard/gateway-http-readiness module so the Docker-driver and - // K3s paths converge on the same probe semantics. - expect(content).toMatch( - /isGatewayHttpReady,[\s\S]{0,200}?require\(\s*["']\.\/onboard\/gateway-http-readiness["']/, - ); - }); -}); From e6c08a7b9de70e7a010442fdfb7f4a73c4f26972 Mon Sep 17 00:00:00 2001 From: Julie Yaunches Date: Tue, 12 May 2026 14:29:04 -0400 Subject: [PATCH 6/9] review: drop ACCEPTANCE.md, tighten tcp-probe min-timeout test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two review findings on PR #3378: 1. cv (human): 'This shouldn't be in the PR' on ACCEPTANCE.md. Fair call β€” ACCEPTANCE.md is a pr-e2e-loop workflow artifact, not a code deliverable the repo should carry forward. The acceptance criterion and green-flip contract already live in the PR body and the commit messages on this branch. 2. CodeRabbit (Minor): the 'enforces a minimum timeout of 50ms' test only verified an upper bound, so a regression that removed the Math.max(50, timeoutMs) clamp and returned immediately would still pass. Rewrite the test to use a non-routable host (10.255.255.1) so an immediate ECONNREFUSED can't mask the regression, and assert elapsed >= 40 ms (10 ms slack under the 50 ms floor) AND < 2000 ms (generous ceiling for slow CI). --- ACCEPTANCE.md | 197 ------------------ .../gateway-health-honest-integration.test.ts | 13 +- 2 files changed, 10 insertions(+), 200 deletions(-) delete mode 100644 ACCEPTANCE.md diff --git a/ACCEPTANCE.md b/ACCEPTANCE.md deleted file mode 100644 index 96996c21470..00000000000 --- a/ACCEPTANCE.md +++ /dev/null @@ -1,197 +0,0 @@ - - - -# Issue #3111 β€” Development Plan + Acceptance Criterion - -**Worktree:** `/Users/jyaunches/Development/NemoClaw-working/issue-3111` -**Branch:** `issue-3111-gateway-health-honesty` -**Base:** `origin/main` at `32cab9d5f` (post PR #3362 merge) -**Related merged:** PR #3362 (coverage guard) - ---- - -## Definition of Done (auto-added by `pr-e2e-loop`) - -Nightly job **`gateway-health-honest-e2e`** (added in PR #3362) must flip from red on `main` to green on the fix branch. Dispatch locally with: - -```bash -gh workflow run nightly-e2e.yaml -f jobs=gateway-health-honest-e2e --ref -``` - -**Expected failing assertion on main** (reproduced in run [25698031380](https://github.com/NVIDIA/NemoClaw/actions/runs/25698031380)): - -> `[PASS] Sabotage shim was invoked as expected (GLIBC/sabotage markers present in gateway log)` -> `[FAIL] Onboard reported 'βœ“ Docker-driver gateway is healthy' although the gateway binary crashed on startup (#3111 false-positive health check)` - -After the fix the log must show: - -```text -[PASS] Sabotage shim was invoked as expected (GLIBC/sabotage markers present in gateway log) -[PASS] Onboard did not falsely log 'Docker-driver gateway is healthy' when the binary crashed -[PASS] startGateway() did not resolve successfully with a crashed binary (node exit=1 or 3) -[PASS] Onboard surfaced a user-visible gateway failure message -[PASS] No live (non-zombie) gateway process is running after the simulated crash -[PASS] #3111 coverage guard green: onboard correctly surfaces a crashed gateway -``` - ---- - -## Diagnosis - -The Docker-driver gateway startup path in `src/lib/onboard.ts:startDockerDriverGateway` spawns the `openshell-gateway` binary with `spawn(..., { detached: true })` + `child.unref()`, then enters a poll loop that checks `isPidAlive(childPid)` and `isGatewayHealthy(status, gwInfo, activeGwInfo)`. - -Both checks can lie: - -1. `isPidAlive` (`src/lib/onboard.ts:4101`) uses `process.kill(pid, 0)` which returns `true` for **zombies** (detached children that exited but haven't been reaped). Since the parent Node process never `wait()`s on the detached child, zombies persist indefinitely. - -2. `isGatewayHealthy` (`src/lib/state/gateway.ts:99`) is a **pure string-matching function** over `openshell status` / `openshell gateway info` output. `isGatewayConnected` matches `Connected` OR **`Server Status`** β€” the latter is a **header line** that `openshell status` prints unconditionally, even when the body contains `Connection refused (os error 111)`. `registerDockerDriverGatewayEndpoint` (`src/lib/onboard.ts:4347`) is metadata-only (`openshell gateway add --local`), no TCP probe. - -The reported symptom on Ubuntu 22.04 (GLIBC 2.38/2.39 link mismatch) is a *specific trigger*. The class of bug is triggered by anything that makes the gateway binary exit immediately β€” GLIBC mismatch, missing shared library, corrupted binary, permission error, port conflict, CDI spec error. - ---- - -## Root Causes - -### RC-1: `isPidAlive` treats zombies as alive - -**Location:** `src/lib/onboard.ts:4101-4109` - -`process.kill(pid, 0)` returns true for both live and zombie processes. For detached `spawn()` children where the parent doesn't `wait()`, zombies accumulate. The poll loop thus never breaks on a crashed-but-unreaped child and proceeds to the metadata check. - -### RC-2: `isGatewayConnected` matches a header, not a state - -**Location:** `src/lib/state/gateway.ts:80-85` - -```js -statusOutput.includes("Connected") || statusOutput.includes("Server Status") -``` - -`Server Status` is the **title of the table** `openshell status` prints β€” it's always present. The real connectivity signal is buried in the body (`Connection refused` / `Error: Γ— client error (Connect)`). The function can't distinguish "connected" from "metadata says there's a gateway at 127.0.0.1:8080, but nothing listens there." - -### RC-3: No live TCP probe in the Docker-driver startup poll loop - -**Location:** `src/lib/onboard.ts:5519-5540` (poll loop body) - -The K3s path has a live container probe (`verifyGatewayContainerRunning()` added for #2020 β€” checks `docker inspect {{.State.Running}}`). The Docker-driver path has no equivalent β€” it trusts the metadata match. A live TCP connect to `127.0.0.1:${GATEWAY_PORT}` would instantly reveal a crashed binary. - ---- - -## Fix Areas & Test Depth Classification - -| # | Area | Change | Test Level | Rationale | -|---|------|--------|-----------|-----------| -| 1 | `src/lib/onboard.ts` β€” new `isDockerDriverGatewayTcpReady()` helper + integrate into `startDockerDriverGateway` poll loop | Gate the `"βœ“ Docker-driver gateway is healthy"` log on a real TCP connect. (HTTP probe via #3312 was attempted but broke the openshell-gateway-upgrade-e2e test β€” Docker-driver returns 404 for GET /.) | **[E2E]** required | Spawns real process, binds real port. `gateway-health-honest-e2e` AND `openshell-gateway-upgrade-e2e` both exercise this change. | -| 2 | `src/lib/onboard.ts` β€” reap zombie child before `isPidAlive` | After spawn, periodically `waitpid`-equivalent check so zombies get detected | **[Unit]** + **[E2E]** | Same E2E catches this; also add a unit test that mocks a zombied PID. | -| 3 | `src/lib/state/gateway.ts` β€” **do NOT modify** | The gateway-liveness-probe test pins this file to pure-function status (see #2020 follow-up test at `test/gateway-liveness-probe.test.ts:74`). Keep the string match; fix at the caller. | **[Unit]** only | No source change. | -| 4 | `scripts/install-openshell.sh` β€” OPTIONAL: add a preflight that verifies the downloaded `openshell-gateway` binary can actually exec (`$BIN --version` or similar) before writing the installed marker | Surface GLIBC / linker errors at install time rather than onboard time | **[Unit]** (install script) | Out of scope for the primary fix but a useful secondary safety net for #3111's specific Ubuntu 22.04 trigger. Split into follow-up issue if larger. | - ---- - -## Refactoring Alignment - -| # | Refactoring Goal | Overlap | Recommendation | -|---|-----------------|---------|----------------| -| 1 | **PR #3312** (laitingsheng, **MERGED**) β€” `fix(onboard): require host HTTP readiness before reusing the gateway` | Added `isGatewayHttpReady(timeoutMs, url)` β€” HTTP probe for the K3s-path gateway. Assumes `GET /` returns 200/401. | **Considered, not adopted.** The K3s-path and Docker-driver-path gateways expose different root-path semantics (Docker-driver returns 404 on `GET /`). We use a local TCP probe for the Docker-driver path and leave unification to #3213. See `isDockerDriverGatewayTcpReady()`. | -| 2 | **PR #3306** (cv) β€” `refactor(onboard): extract gateway bootstrap repair helpers` | Extracts `src/lib/onboard/gateway-bootstrap.ts` module. | No conflict with our change β€” doesn't touch `startDockerDriverGateway`. | -| 3 | **#3213** β€” Epic: unify warnings, advisories, and fatal exits under a single registry | When onboard exits with "gateway failed to start", that message should eventually flow through the unified advisory registry. | **Seed the pattern.** `TODO(#3213)` comment added on the `startDockerDriverGateway` poll loop so the future migration to structured advisories is mechanical. | - ---- - -## Implementation Plan - -### Phase 1 β€” Add a TCP liveness probe (**[Unit]** only) - -**Goal:** Introduce `isDockerDriverGatewayTcpReady(port, timeoutMs, host)` as a small async helper in `src/lib/onboard.ts`, used to gate the "Docker-driver gateway is healthy" log on a real TCP connect. - -- **File:** `src/lib/onboard.ts` β€” new helper near `isPidAlive`. -- **Why TCP and not HTTP:** We originally tried reusing `isGatewayHttpReady` from `./onboard/gateway-http-readiness` (introduced by #3312), but that broke the existing `openshell-gateway-upgrade-e2e` test. The K3s gateway and the Docker-driver gateway serve different HTTP routes for the root path β€” K3s answers `GET /` with 200/401, but the Docker-driver gateway returns 404 for `GET /` and only serves `/openshell.v1.OpenShell/*`. A plain TCP connect is semantic-free and sufficient to catch the #3111 failure mode (crashed binary β†’ nothing listening). Convergence with `isGatewayHttpReady` is deferred to #3213 (unified advisory / probe registry). -- **Implementation:** `net.createConnection({ host, port })` with a socket timeout. Resolves boolean, never throws. -- **Tests:** `test/gateway-health-honest-integration.test.ts` β€” 5 behavioural tests (listening / closed / timeout / min-timeout / never-throws) + 5 source-shape guards. -- **Dependencies:** None. - -### Phase 2 β€” Reap zombie children in the startup poll loop (**[Unit]** only) - -**Goal:** Detect crashed-then-zombied children so `isPidAlive` returns false as expected. - -- **File:** `src/lib/onboard.ts:startDockerDriverGateway` β€” the `spawn()` call. -- **Implementation:** Even with `detached: true` / `child.unref()`, we can keep a reference to the `ChildProcess` object, attach a one-shot `child.on("exit", ...)` handler that sets a `childCrashed: boolean` flag, and check that flag at the top of each poll iteration alongside `isPidAlive`. Alternative: `child.removeAllListeners(); child.once("exit", ...);` with a closure-captured flag. -- **Tests:** Extend `test/onboard.test.ts` (or add `test/gateway-zombie-reap.test.ts`) β€” spawn a shim that exits immediately, assert the flag flips within N ms. -- **Dependencies:** None. Runs in parallel with Phase 1. - -### Phase 3 β€” Integrate both checks into the poll loop (**[E2E]** required) - -**Goal:** Make `startDockerDriverGateway` fail closed β€” "healthy" is only logged when the process is running AND metadata matches AND the port is serving. - -- **File:** `src/lib/onboard.ts:5519-5540` (the poll loop body). -- **Implementation:** - - ```ts - for (let i = 0; i < pollCount; i += 1) { - if (childCrashed || !isPidAlive(childPid)) { - break; // existing - } - if (!registerDockerDriverGatewayEndpoint()) { ...continue } - const status = runCaptureOpenshell(["status"], { ignoreError: true }); - const namedInfo = runCaptureOpenshell(["gateway", "info", "-g", GATEWAY_NAME], { ignoreError: true }); - const currentInfo = runCaptureOpenshell(["gateway", "info"], { ignoreError: true }); - if (isGatewayHealthy(status, namedInfo, currentInfo)) { - // NEW: gate the healthy log on a real TCP probe - if (await verifyDockerDriverGatewayListening(GATEWAY_PORT, 500)) { - console.log(" βœ“ Docker-driver gateway is healthy"); - return; - } - // Metadata says healthy but nothing is listening β€” keep polling - } - if (i < pollCount - 1) sleep(pollInterval); - } - ``` - -- **Tests:** - - **E2E (primary, already exists):** `gateway-health-honest-e2e` (from PR #3362) must flip from red to green. - - **Unit:** mock `verifyDockerDriverGatewayListening` to return `false`; assert the loop doesn't print "healthy" and eventually throws. -- **Dependencies:** Phase 1 and Phase 2 must land first. -- **Refactoring notes:** Format the final "failed to start" message as structured data with `code: "gateway-failed-to-start"` for future #3213 migration. Leave the log-tail output intact β€” it's already user-facing diagnostic. - -### Phase 4 (OPTIONAL) β€” Install-time GLIBC check (**[Unit]** only) - -**Goal:** Surface the GLIBC mismatch at install time, not onboard time. Addresses the specific #3111 trigger, not the class. - -- **File:** `scripts/install-openshell.sh` -- **Implementation:** After `install -m 755 $tmpdir/openshell-gateway $target_dir/openshell-gateway`, run `"$target_dir/openshell-gateway" --version` (or `--help`). Capture exit code + stderr. On non-zero, print a warn/fatal message that includes the stderr (which contains GLIBC requirement) and a suggestion to upgrade to Ubuntu 24.04. -- **Tests:** Extend `test/install-openshell-version-check.test.ts`. -- **Dependencies:** None β€” can run in parallel with any other phase. -- **Verdict:** Ask the user whether to split this into a separate follow-up issue. The primary fix (Phases 1–3) closes the NemoClaw-side false-positive; this Phase 4 is a secondary safety net tied to the OpenShell binary packaging choice. - ---- - -## Recommended Order - -- **Phases 1 and 2 in parallel** β†’ both are small, independent, unit-testable. -- **Phase 3 sequentially after 1 + 2.** -- **Phase 4 optional / follow-up.** - -## Verdict: **[E2E]** required - -| Phase | Test Depth | -|---|---| -| Phase 1 (`isDockerDriverGatewayTcpReady` helper) | **[Unit]** only | -| Phase 2 (zombie-reap) | **[Unit]** only | -| Phase 3 (poll-loop integration) | **[E2E]** (`gateway-health-honest-e2e`) | -| Phase 4 (install-time check) | **[Unit]** only (optional) | - -The PR-level verdict is **[E2E]** β€” Phase 3 is the acceptance gate, and it's already implemented as the merged coverage guard. The fix PR's definition-of-done is "this E2E flips green." - ---- - -## Notes for the PR author - -- PR title: `fix(onboard): verify Docker-driver gateway is really serving before reporting healthy (#3111)` -- PR body MUST include `Fixes #3111` to auto-close the issue (and get the coverage guard flipping green). -- Do NOT modify `src/lib/state/gateway.ts:isGatewayHealthy` β€” pinned by `test/gateway-liveness-probe.test.ts:74`. -- Added local `isDockerDriverGatewayTcpReady()` helper in `onboard.ts`; did NOT converge onto PR #3312's `isGatewayHttpReady` because the two gateway types have different HTTP root-path semantics (Docker-driver returns 404 on `GET /`, which fails the HTTP probe). TODO(#3213) filed for future unification. -- No conflict with open PR #3306 (gateway-bootstrap refactor) β€” our change does not touch the files being extracted. -- The nightly-dispatch for validation: - - ```bash - gh workflow run nightly-e2e.yaml -f jobs=gateway-health-honest-e2e --ref issue-3111-gateway-health-honesty - ``` diff --git a/test/gateway-health-honest-integration.test.ts b/test/gateway-health-honest-integration.test.ts index 6f63e98102c..aabf4ee40c9 100644 --- a/test/gateway-health-honest-integration.test.ts +++ b/test/gateway-health-honest-integration.test.ts @@ -90,10 +90,17 @@ describe("isDockerDriverGatewayTcpReady (#3111)", () => { }); it("enforces a minimum timeout of 50ms even when caller passes 0", async () => { - const port = await getLikelyClosedPort(); + // Use a non-routable host so the probe can't short-circuit via an + // immediate ECONNREFUSED. If timeout clamping regressed to 0 ms, the + // probe would return essentially instantly; the >=40 ms lower bound + // (generous 10 ms slack under the 50 ms floor) catches that regression. const started = Date.now(); - await expect(isDockerDriverGatewayTcpReady(port, 0)).resolves.toBe(false); - expect(Date.now() - started).toBeLessThan(500); + await expect( + isDockerDriverGatewayTcpReady(9, 0, "10.255.255.1"), + ).resolves.toBe(false); + const elapsed = Date.now() - started; + expect(elapsed).toBeGreaterThanOrEqual(40); + expect(elapsed).toBeLessThan(2000); }); it("never throws β€” always resolves with a boolean", async () => { From 01368aee7c2246bc80dc97dfcc17a828d8ac3ef8 Mon Sep 17 00:00:00 2001 From: Julie Yaunches Date: Tue, 12 May 2026 16:09:20 -0400 Subject: [PATCH 7/9] refactor(onboard): extract isGatewayTcpReady into a focused module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR review feedback β€” onboard.ts is the God Object being decomposed, and growing its exports is an anti-pattern. Follow the pattern established by PR #3312 (which added src/lib/onboard/gateway-http-readiness.ts) and create a peer module for the Docker-driver path. New file: src/lib/onboard/gateway-tcp-readiness.ts Exports: isGatewayTcpReady(port, timeoutMs, host) Model: mirrors gateway-http-readiness.ts structure exactly β€” default port from GATEWAY_PORT, minimum-timeout clamp, pure async, never throws. New file: src/lib/onboard/gateway-tcp-readiness.test.ts Co-located behavioural tests per the established pattern (see temp-files.test.ts, legacy-command.test.ts, etc.) 6 tests: listening / closed / timeout / min-timeout / no-throw / default-port. Imports via ES module, not via dist/ indirection. onboard.ts net change: Before: +88 lines (+48 helper + integration) After: +22 lines (2 require lines + integration only) Related to: #3111 --- src/lib/onboard.ts | 81 ++-------- src/lib/onboard/gateway-tcp-readiness.test.ts | 102 +++++++++++++ src/lib/onboard/gateway-tcp-readiness.ts | 108 ++++++++++++++ .../gateway-health-honest-integration.test.ts | 139 +++++------------- 4 files changed, 254 insertions(+), 176 deletions(-) create mode 100644 src/lib/onboard/gateway-tcp-readiness.test.ts create mode 100644 src/lib/onboard/gateway-tcp-readiness.ts diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index c127e25350e..e1f503eeb7c 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -280,6 +280,8 @@ const { isGatewayHttpReady, waitForGatewayHttpReady, } = require("./onboard/gateway-http-readiness") as typeof import("./onboard/gateway-http-readiness"); +const { isGatewayTcpReady } = + require("./onboard/gateway-tcp-readiness") as typeof import("./onboard/gateway-tcp-readiness"); const preflightUtils: typeof import("./onboard/preflight") = require("./onboard/preflight"); const clusterImagePatch: typeof import("./cluster-image-patch") = require("./cluster-image-patch"); const { @@ -3953,60 +3955,6 @@ function isPidAlive(pid: number): boolean { } } -/** - * Probe a TCP endpoint on localhost to verify something is actually listening - * and accepting connections. Used to gate the "Docker-driver gateway is - * healthy" log in startDockerDriverGateway against the class of bug reported - * in #3111 where the openshell-gateway binary crashed on startup but - * metadata-based health checks (isGatewayHealthy) still returned true. - * - * We intentionally use a plain TCP probe here rather than the stronger HTTP - * probe from ./onboard/gateway-http-readiness (isGatewayHttpReady) because - * the Docker-driver gateway and the K3s gateway have different HTTP routes - * for the root path: - * - * - K3s path: GET / returns 200 / 401 (dispatcher catch-all) - * - Docker-driver path: GET / returns 404; only /openshell.v1.OpenShell/* - * is served - * - * A TCP connect is semantic-free (it just asks "is anyone listening?") - * which is exactly what's needed to detect the #3111 failure mode without - * making assumptions about HTTP route shape. Tracking convergence with - * isGatewayHttpReady is left to #3213 (the unified advisory / probe - * registry). - * - * Resolves true on successful TCP connect. Resolves false on connection - * refused, unreachable host, or timeout. Never rejects. - */ -async function isDockerDriverGatewayTcpReady( - port: number, - timeoutMs = 500, - host = "127.0.0.1", -): Promise { - // Lazy-require so tests that don't exercise the gateway path don't pay - // for the net module load. - // biome-ignore lint/suspicious/noExplicitAny: node:net types are not in scope in this file - const net: any = require("node:net"); - return new Promise((resolve) => { - let settled = false; - const settle = (result: boolean) => { - if (settled) return; - settled = true; - try { - socket.destroy(); - } catch { - /* best effort */ - } - resolve(result); - }; - const socket = net.createConnection({ host, port }); - socket.setTimeout(Math.max(50, timeoutMs)); - socket.once("connect", () => settle(true)); - socket.once("timeout", () => settle(false)); - socket.once("error", () => settle(false)); - }); -} - function getDockerDriverGatewayPid(): number | null { try { @@ -5512,26 +5460,20 @@ async function startDockerDriverGateway({ }); const currentInfo = runCaptureOpenshell(["gateway", "info"], { ignoreError: true }); if (isGatewayHealthy(status, namedInfo, currentInfo)) { - // #3111: isGatewayHealthy is a pure string match on openshell CLI - // output. isGatewayConnected matches on "Server Status" which is a - // header the CLI prints unconditionally, so the metadata check can - // return true even when the gateway endpoint is unreachable - // (Connection refused). Gate the "healthy" log on a real TCP probe - // to verify something is actually listening on the gateway port. - // - // We use a TCP probe rather than the HTTP probe from - // ./onboard/gateway-http-readiness because the Docker-driver and K3s - // gateways serve different HTTP routes (Docker-driver: only the - // gRPC endpoints under /openshell.v1.OpenShell/; K3s: a dispatcher - // that answers GET /). See isDockerDriverGatewayTcpReady() above. + // #3111: isGatewayHealthy is a string match on openshell CLI output + // ("Server Status" header is always printed, even when the body is + // Connection refused), so gate the "healthy" log on a real TCP probe + // via isGatewayTcpReady() β€” the Docker-driver peer of the K3s path's + // isGatewayHttpReady() (see ./onboard/gateway-tcp-readiness for why + // this path uses TCP rather than HTTP). // // On probe failure, keep polling: the binary may still be binding - // its listener. The childExited / isPidAlive check at the top of - // the loop terminates us if the process actually died. + // its listener. The childExited / isPidAlive check at the top of the + // loop terminates us if the process actually died. // // TODO(#3213): surface this as a structured advisory rather than // plain log output once the unified registry lands. - if (await isDockerDriverGatewayTcpReady(GATEWAY_PORT)) { + if (await isGatewayTcpReady()) { console.log(" βœ“ Docker-driver gateway is healthy"); return; } @@ -12113,7 +12055,6 @@ module.exports = { getResumeConfigConflicts, isGatewayHealthy, hasStaleGateway, - isDockerDriverGatewayTcpReady, getRequestedSandboxNameHint, getResumeSandboxConflict, getSandboxReuseState, diff --git a/src/lib/onboard/gateway-tcp-readiness.test.ts b/src/lib/onboard/gateway-tcp-readiness.test.ts new file mode 100644 index 00000000000..ca4a9d025e4 --- /dev/null +++ b/src/lib/onboard/gateway-tcp-readiness.test.ts @@ -0,0 +1,102 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +// Behavioural tests for isGatewayTcpReady β€” the Docker-driver gateway's +// TCP liveness probe. Co-located with the module per the pattern +// established by gateway-http-readiness and other src/lib/onboard/* +// modules. +// +// See: https://github.com/NVIDIA/NemoClaw/issues/3111 + +import net from "node:net"; +import { afterEach, describe, expect, it } from "vitest"; + +import { isGatewayTcpReady } from "./gateway-tcp-readiness"; + +// ── Helpers ───────────────────────────────────────────────────────────────── + +function startDummyServer(): Promise<{ port: number; close: () => Promise }> { + return new Promise((resolve, reject) => { + const server = net.createServer((socket) => socket.end()); + server.once("error", reject); + server.listen(0, "127.0.0.1", () => { + const addr = server.address(); + if (!addr || typeof addr === "string") { + reject(new Error("failed to resolve server address")); + return; + } + resolve({ + port: addr.port, + close: () => + new Promise((res, rej) => + server.close((err) => (err ? rej(err) : res())), + ), + }); + }); + }); +} + +async function getLikelyClosedPort(): Promise { + const { port, close } = await startDummyServer(); + await close(); + return port; +} + +// ── Tests ─────────────────────────────────────────────────────────────────── + +describe("isGatewayTcpReady (#3111)", () => { + let teardown: (() => Promise) | null = null; + + afterEach(async () => { + if (teardown) { + await teardown(); + teardown = null; + } + }); + + it("resolves true when something is accepting connections", async () => { + const { port, close } = await startDummyServer(); + teardown = close; + await expect(isGatewayTcpReady(port, 500)).resolves.toBe(true); + }); + + it("resolves false when nothing is listening (Connection refused)", async () => { + const port = await getLikelyClosedPort(); + await expect(isGatewayTcpReady(port, 500)).resolves.toBe(false); + }); + + it("resolves false on timeout (non-routable host)", async () => { + // 10.255.255.1 is a non-routable RFC 1918 address that SYN-drops on most + // CI runners, forcing the timeout path rather than immediate ECONNREFUSED. + const started = Date.now(); + await expect(isGatewayTcpReady(9, 200, "10.255.255.1")).resolves.toBe( + false, + ); + const elapsed = Date.now() - started; + expect(elapsed).toBeLessThan(2000); + }); + + it("enforces a minimum timeout of 50ms even when caller passes 0", async () => { + // Use a non-routable host so the probe can't short-circuit via an + // immediate ECONNREFUSED. If timeout clamping regressed to 0 ms, the + // probe would return essentially instantly; the >=40 ms lower bound + // (generous 10 ms slack under the 50 ms floor) catches that regression. + const started = Date.now(); + await expect(isGatewayTcpReady(9, 0, "10.255.255.1")).resolves.toBe(false); + const elapsed = Date.now() - started; + expect(elapsed).toBeGreaterThanOrEqual(40); + expect(elapsed).toBeLessThan(2000); + }); + + it("never throws β€” always resolves with a boolean", async () => { + await expect(isGatewayTcpReady(0, 100)).resolves.toBeTypeOf("boolean"); + await expect(isGatewayTcpReady(65535, 100)).resolves.toBe(false); + }); + + it("defaults to GATEWAY_PORT when no port is supplied", async () => { + // Nothing is listening on GATEWAY_PORT in test context; the call should + // resolve false (not throw) and do so without the caller threading the + // constant. This guards the default-parameter wiring. + await expect(isGatewayTcpReady(undefined, 200)).resolves.toBe(false); + }); +}); diff --git a/src/lib/onboard/gateway-tcp-readiness.ts b/src/lib/onboard/gateway-tcp-readiness.ts new file mode 100644 index 00000000000..7de691885a0 --- /dev/null +++ b/src/lib/onboard/gateway-tcp-readiness.ts @@ -0,0 +1,108 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Host-level TCP readiness probe for the OpenShell Docker-driver gateway. + * + * Plain TCP connect to `127.0.0.1:${GATEWAY_PORT}` β€” semantic-free, just asks + * "is anyone listening?". Used by `startDockerDriverGateway` in `onboard.ts` + * to gate the "βœ“ Docker-driver gateway is healthy" log against the class of + * bug reported in #3111, where the openshell-gateway binary crashed on + * startup but metadata-based health checks (isGatewayHealthy) still + * returned true because: + * + * - `isPidAlive` (`process.kill(pid, 0)`) returns true for zombies; + * - `registerDockerDriverGatewayEndpoint` is metadata-only + * (`openshell gateway add --local`) and doesn't probe the endpoint; + * - `isGatewayHealthy` is a string match on `openshell status` / + * `openshell gateway info` output, not a live probe. + * + * ## Why TCP and not HTTP + * + * There is a peer module `gateway-http-readiness.ts` (introduced by #3312) + * that exposes `isGatewayHttpReady` β€” a stronger HTTP-level probe used on + * the K3s path. It cannot be reused on the Docker-driver path because the + * two gateway types expose different HTTP routes for the root path: + * + * - K3s gateway answers `GET /` with 200/401 via a dispatcher catch-all. + * - Docker-driver gateway returns 404 for `GET /`; only routes under + * `/openshell.v1.OpenShell/*` (gRPC-over-HTTP) are served. + * + * Using the HTTP probe against a running Docker-driver gateway therefore + * always fails the `{200, 401}` whitelist, which was the regression the + * e2e-advisor caught in the first attempt at a shared fix. + * + * A plain TCP probe is sufficient for detecting #3111's failure mode + * (crashed binary β†’ nothing listening β†’ TCP fails) without making + * assumptions about HTTP route shape. + * + * ## Prior art in the codebase + * + * `isLocalForwardReachable` in + * `src/lib/actions/sandbox/process-recovery.ts` (added by #3385) does a + * similar TCP probe for the sandbox-forward domain, but spawns a child + * node process per probe and is intentionally private to that module. + * The two helpers serve different domains (onboard gateway vs. sandbox + * forward) and have different sync/async requirements, but a future + * unified "port liveness probe" API could subsume both. Tracked in #3213 + * (unified advisory / probe registry). + */ + +import net from "node:net"; + +import { GATEWAY_PORT } from "../core/ports"; + +const ISGATEWAY_TCP_READY_DEFAULT_TIMEOUT_MS = 500; + +/** + * Minimum timeout clamp. Callers are allowed to pass 0 or small values, but + * the effective timeout never drops below this, so a regression that removes + * the clamp or passes a zero from a misconfigured env var can't turn this + * into a spin-loop or an instant-return. + */ +const ISGATEWAY_TCP_READY_MIN_TIMEOUT_MS = 50; + +/** + * Probe a TCP endpoint on localhost to verify something is actually + * listening and accepting connections on the gateway port. + * + * Resolves true on a successful TCP connect. Resolves false on connection + * refused, unreachable host, or timeout. Never rejects. + * + * @param port Gateway port. Defaults to `GATEWAY_PORT` so normal call + * sites don't have to thread the constant themselves. + * @param timeoutMs Per-connect timeout. Clamped to a 50 ms minimum so the + * probe can't spin or return instantly when a caller + * passes 0. + * @param host Target host. Defaults to `127.0.0.1`; overridable only + * for unit testing (exercising the timeout path against + * a non-routable address). + */ +export function isGatewayTcpReady( + port: number = GATEWAY_PORT, + timeoutMs: number = ISGATEWAY_TCP_READY_DEFAULT_TIMEOUT_MS, + host = "127.0.0.1", +): Promise { + const effectiveTimeout = + Number.isFinite(timeoutMs) && timeoutMs > ISGATEWAY_TCP_READY_MIN_TIMEOUT_MS + ? Math.round(timeoutMs) + : ISGATEWAY_TCP_READY_MIN_TIMEOUT_MS; + return new Promise((resolve) => { + let settled = false; + const socket = net.createConnection({ host, port }); + const settle = (result: boolean) => { + if (settled) return; + settled = true; + try { + socket.destroy(); + } catch { + /* best effort */ + } + resolve(result); + }; + socket.setTimeout(effectiveTimeout); + socket.once("connect", () => settle(true)); + socket.once("timeout", () => settle(false)); + socket.once("error", () => settle(false)); + }); +} diff --git a/test/gateway-health-honest-integration.test.ts b/test/gateway-health-honest-integration.test.ts index aabf4ee40c9..884510379a0 100644 --- a/test/gateway-health-honest-integration.test.ts +++ b/test/gateway-health-honest-integration.test.ts @@ -1,122 +1,38 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 // -// Source-shape + behavioural guards for the #3111 fix in startDockerDriverGateway. +// Source-shape guards for the #3111 fix in startDockerDriverGateway. // // The fix gates the "Docker-driver gateway is healthy" log on: -// 1. a real TCP liveness probe (isDockerDriverGatewayTcpReady) β€” plain -// TCP, not HTTP, because the Docker-driver gateway and the K3s -// gateway expose different root paths (see isGatewayHttpReady in -// ./onboard/gateway-http-readiness for the K3s-path probe); and +// 1. a real TCP liveness probe (isGatewayTcpReady from +// src/lib/onboard/gateway-tcp-readiness.ts) β€” plain TCP, not HTTP, +// because the Docker-driver gateway and the K3s gateway expose +// different root paths (see gateway-http-readiness for the K3s path +// and the docstring of gateway-tcp-readiness for the rationale); and // 2. a child-exit listener that catches zombied detached children that // process.kill(pid, 0) would otherwise report as alive. // // These guards keep future edits from silently regressing #3111. // +// Behavioural tests for isGatewayTcpReady live co-located with the module +// at src/lib/onboard/gateway-tcp-readiness.test.ts β€” this file only +// verifies the wiring inside onboard.ts. +// // See: https://github.com/NVIDIA/NemoClaw/issues/3111 // https://github.com/NVIDIA/NemoClaw/pull/3312 (K3s-path HTTP helper) -import { describe, it, expect, afterEach } from "vitest"; +import { describe, it, expect } from "vitest"; import fs from "node:fs"; -import net from "node:net"; import path from "node:path"; -// eslint-disable-next-line @typescript-eslint/no-var-requires -const { isDockerDriverGatewayTcpReady } = require("../dist/lib/onboard"); - const ROOT = path.resolve(import.meta.dirname, ".."); -// ── Helpers ───────────────────────────────────────────────────────────────── - -function startDummyServer(): Promise<{ port: number; close: () => Promise }> { - return new Promise((resolve, reject) => { - const server = net.createServer((socket) => socket.end()); - server.once("error", reject); - server.listen(0, "127.0.0.1", () => { - const addr = server.address(); - if (!addr || typeof addr === "string") { - reject(new Error("failed to resolve server address")); - return; - } - resolve({ - port: addr.port, - close: () => - new Promise((res, rej) => - server.close((err) => (err ? rej(err) : res())), - ), - }); - }); - }); -} - -async function getLikelyClosedPort(): Promise { - const { port, close } = await startDummyServer(); - await close(); - return port; -} - -// ── Behavioural tests for isDockerDriverGatewayTcpReady ───────────────────── - -describe("isDockerDriverGatewayTcpReady (#3111)", () => { - let teardown: (() => Promise) | null = null; - - afterEach(async () => { - if (teardown) { - await teardown(); - teardown = null; - } - }); - - it("resolves true when something is accepting connections", async () => { - const { port, close } = await startDummyServer(); - teardown = close; - await expect(isDockerDriverGatewayTcpReady(port, 500)).resolves.toBe(true); - }); - - it("resolves false when nothing is listening (Connection refused)", async () => { - const port = await getLikelyClosedPort(); - await expect(isDockerDriverGatewayTcpReady(port, 500)).resolves.toBe(false); - }); - - it("resolves false on timeout (non-routable host)", async () => { - // 10.255.255.1 is a non-routable RFC 1918 address that SYN-drops on most - // CI runners, forcing the timeout path rather than immediate ECONNREFUSED. - const started = Date.now(); - await expect( - isDockerDriverGatewayTcpReady(9, 200, "10.255.255.1"), - ).resolves.toBe(false); - const elapsed = Date.now() - started; - expect(elapsed).toBeLessThan(2000); - }); - - it("enforces a minimum timeout of 50ms even when caller passes 0", async () => { - // Use a non-routable host so the probe can't short-circuit via an - // immediate ECONNREFUSED. If timeout clamping regressed to 0 ms, the - // probe would return essentially instantly; the >=40 ms lower bound - // (generous 10 ms slack under the 50 ms floor) catches that regression. - const started = Date.now(); - await expect( - isDockerDriverGatewayTcpReady(9, 0, "10.255.255.1"), - ).resolves.toBe(false); - const elapsed = Date.now() - started; - expect(elapsed).toBeGreaterThanOrEqual(40); - expect(elapsed).toBeLessThan(2000); - }); - - it("never throws β€” always resolves with a boolean", async () => { - await expect(isDockerDriverGatewayTcpReady(0, 100)).resolves.toBeTypeOf( - "boolean", - ); - await expect(isDockerDriverGatewayTcpReady(65535, 100)).resolves.toBe( - false, - ); - }); -}); - -// ── Source-shape guards for the integration in startDockerDriverGateway ──── - describe("startDockerDriverGateway integration (#3111)", () => { const content = fs.readFileSync(path.join(ROOT, "src/lib/onboard.ts"), "utf-8"); + // Scope assertions to the startDockerDriverGateway function body so other + // occurrences of the helpers (e.g. in stale-gateway reuse paths or in + // module.exports) don't satisfy the source-shape checks and mask a + // regression inside this function. const fnMatch = content.match( /async function startDockerDriverGateway\([\s\S]*?\n\}\n/, ); @@ -137,16 +53,15 @@ describe("startDockerDriverGateway integration (#3111)", () => { }); it("gates the 'healthy' log on the TCP readiness probe", () => { - // The poll loop must call isDockerDriverGatewayTcpReady(GATEWAY_PORT) - // before logging "βœ“ Docker-driver gateway is healthy". We deliberately - // use a TCP probe rather than the K3s-path isGatewayHttpReady because - // the Docker-driver gateway only serves /openshell.v1.OpenShell/* β€” - // GET / returns 404, which fails the HTTP probe even though the - // gateway is functional. + // The poll loop must call isGatewayTcpReady() before logging + // "βœ“ Docker-driver gateway is healthy". We use a TCP probe rather than + // the K3s-path isGatewayHttpReady because the Docker-driver gateway + // only serves /openshell.v1.OpenShell/* β€” GET / returns 404 and + // fails the HTTP probe even though the gateway is functional. const healthyIdx = fnBody.indexOf("Docker-driver gateway is healthy"); expect(healthyIdx).toBeGreaterThan(0); const before = fnBody.slice(0, healthyIdx); - expect(before).toMatch(/await\s+isDockerDriverGatewayTcpReady\(GATEWAY_PORT/); + expect(before).toMatch(/await\s+isGatewayTcpReady\(/); }); it("does NOT use the K3s-path HTTP probe in the Docker-driver loop", () => { @@ -159,6 +74,18 @@ describe("startDockerDriverGateway integration (#3111)", () => { expect(before).not.toMatch(/await\s+isGatewayHttpReady\(/); }); + it("does NOT define isGatewayTcpReady inline (it must live in its own module)", () => { + // The probe must come from src/lib/onboard/gateway-tcp-readiness.ts, + // not from an inline declaration in onboard.ts. onboard.ts is the + // God Object being decomposed β€” new helpers should land in focused + // modules, mirroring the pattern established by + // src/lib/onboard/gateway-http-readiness.ts. + expect(content).not.toMatch(/function\s+isGatewayTcpReady\s*\(/); + expect(content).toMatch( + /require\(\s*["']\.\/onboard\/gateway-tcp-readiness["']/, + ); + }); + it("surfaces child-exit details in the final failure message", () => { expect(fnBody).toMatch(/childExited/); expect(fnBody).toMatch(/childExitSignal|childExitCode/); From b1cd0c956551c16cc3d1a50bfa9dc9fcb8542fae Mon Sep 17 00:00:00 2001 From: Julie Yaunches Date: Tue, 12 May 2026 16:19:10 -0400 Subject: [PATCH 8/9] refactor(onboard): extract child-exit tracker + failure reporter to modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR review: 'This PR is adding lines to onboard.ts β€” which is now an antipattern' and the onboard-entrypoint-budget CI check. src/lib/onboard.ts is the God Object being decomposed; growing it is explicitly flagged as "away from target architecture" in the repo's review guidelines, and the onboard-entrypoint-budget workflow fails the PR if the entrypoint grows net-positive. ## Changes Extract three focused modules under src/lib/onboard/ that collectively replace the inline logic added in earlier commits on this branch: - src/lib/onboard/gateway-tcp-readiness.ts isGatewayTcpReady(port, timeoutMs, host): plain TCP liveness probe for the Docker-driver gateway. Peer of gateway-http-readiness.ts (introduced by #3312). Uses a minimum-timeout clamp so a regression that drops Math.max(50, timeoutMs) is caught by tests. - src/lib/onboard/child-exit-tracker.ts trackChildExit(child): attaches a one-shot 'exit' listener to a ChildProcess and returns a ChildExitState view. Zombie-safe: fires reliably for detached children that isPidAlive(pid, 0) would falsely report as alive (#3111). Also exposes describeExit() β€” 'exited with code 127' / 'killed by signal SIGKILL' β€” so callers don't have to reformat the signal/code themselves. - src/lib/onboard/docker-driver-gateway-failure.ts reportDockerDriverGatewayStartFailure(logPath, childExit, opts): the full diagnostic bundle emitted on startup failure β€” 'failed to start' header, exit descriptor, redacted log tail, troubleshooting footer. Honors exitOnFailure (production) vs. throw-only (recovery). Each module has co-located behavioural tests (6 + 5 + 6 tests = 17), using direct ES imports instead of the dist/ indirection that test/onboard.test.ts uses. Source-shape guards in test/gateway-health-honest-integration.test.ts enforce that the three modules stay extracted (the entrypoint cannot reintroduce the inline implementations). ## Result src/lib/onboard.ts: 15 additions, 21 deletions β€” net -6 lines. src/lib/onboard/**: 3 new modules, 17 new unit tests, full behavioural + signal-vs-code coverage on all three helpers. Both E2E tests remain green on this branch: - gateway-health-honest-e2e (#3111 coverage guard) - openshell-gateway-upgrade-e2e (regression target the earlier HTTP-probe attempt broke; TCP probe is the correct fit) Related: #3111 Aligns with architecture goal tracked by onboard-entrypoint-budget CI. --- src/lib/onboard.ts | 79 +++-------- src/lib/onboard/child-exit-tracker.test.ts | 84 ++++++++++++ src/lib/onboard/child-exit-tracker.ts | 83 ++++++++++++ .../docker-driver-gateway-failure.test.ts | 124 ++++++++++++++++++ .../onboard/docker-driver-gateway-failure.ts | 81 ++++++++++++ .../gateway-health-honest-integration.test.ts | 45 +++++-- 6 files changed, 420 insertions(+), 76 deletions(-) create mode 100644 src/lib/onboard/child-exit-tracker.test.ts create mode 100644 src/lib/onboard/child-exit-tracker.ts create mode 100644 src/lib/onboard/docker-driver-gateway-failure.test.ts create mode 100644 src/lib/onboard/docker-driver-gateway-failure.ts diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 36f2eaf4fc9..74af338d04a 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -288,6 +288,10 @@ const { } = require("./onboard/gateway-http-readiness") as typeof import("./onboard/gateway-http-readiness"); const { isGatewayTcpReady } = require("./onboard/gateway-tcp-readiness") as typeof import("./onboard/gateway-tcp-readiness"); +const { trackChildExit } = + require("./onboard/child-exit-tracker") as typeof import("./onboard/child-exit-tracker"); +const { reportDockerDriverGatewayStartFailure } = + require("./onboard/docker-driver-gateway-failure") as typeof import("./onboard/docker-driver-gateway-failure"); const preflightUtils: typeof import("./onboard/preflight") = require("./onboard/preflight"); const clusterImagePatch: typeof import("./cluster-image-patch") = require("./cluster-image-patch"); const { @@ -3823,7 +3827,6 @@ function isPidAlive(pid: number): boolean { } } - function getDockerDriverGatewayPid(): number | null { try { const raw = fs.readFileSync(getDockerDriverGatewayPidFile(), "utf-8").trim(); @@ -5287,21 +5290,7 @@ async function startDockerDriverGateway({ ...gatewayEnv, }, }); - // Track early-exit via the ChildProcess event rather than process.kill(pid, 0). - // isPidAlive() returns true for zombies (detached children that exited but - // were not reaped by a wait() call), which masks the failure mode reported - // in #3111: the binary crashes on startup but the pid remains in the - // process table as a zombie. The 'exit' event fires reliably for detached - // children once libuv observes SIGCHLD. We still call unref() below so the - // child doesn't hold the Node event loop open after we return. - let childExited = false; - let childExitCode: number | null = null; - let childExitSignal: NodeJS.Signals | null = null; - child.once("exit", (code: number | null, signal: NodeJS.Signals | null) => { - childExited = true; - childExitCode = code; - childExitSignal = signal; - }); + const childExit = trackChildExit(child); // #3111 zombie-safe liveness child.unref(); const childPid = child.pid ?? 0; if (childPid <= 0) { @@ -5312,10 +5301,7 @@ async function startDockerDriverGateway({ const pollCount = envInt("NEMOCLAW_HEALTH_POLL_COUNT", 30); const pollInterval = envInt("NEMOCLAW_HEALTH_POLL_INTERVAL", 2); for (let i = 0; i < pollCount; i += 1) { - // #3111: combine the exit-event check with isPidAlive. The exit event - // catches zombies that isPidAlive misses; isPidAlive catches children - // we somehow lost track of (defense in depth). - if (childExited || !isPidAlive(childPid)) { + if (childExit.exited || !isPidAlive(childPid)) { break; } if (!registerDockerDriverGatewayEndpoint()) { @@ -5327,54 +5313,19 @@ async function startDockerDriverGateway({ ignoreError: true, }); const currentInfo = runCaptureOpenshell(["gateway", "info"], { ignoreError: true }); - if (isGatewayHealthy(status, namedInfo, currentInfo)) { - // #3111: isGatewayHealthy is a string match on openshell CLI output - // ("Server Status" header is always printed, even when the body is - // Connection refused), so gate the "healthy" log on a real TCP probe - // via isGatewayTcpReady() β€” the Docker-driver peer of the K3s path's - // isGatewayHttpReady() (see ./onboard/gateway-tcp-readiness for why - // this path uses TCP rather than HTTP). - // - // On probe failure, keep polling: the binary may still be binding - // its listener. The childExited / isPidAlive check at the top of the - // loop terminates us if the process actually died. - // - // TODO(#3213): surface this as a structured advisory rather than - // plain log output once the unified registry lands. - if (await isGatewayTcpReady()) { - console.log(" βœ“ Docker-driver gateway is healthy"); - return; - } + // #3111: gate the healthy log on a real TCP probe. See + // ./onboard/gateway-tcp-readiness for why TCP, not HTTP. TODO(#3213). + if ( + isGatewayHealthy(status, namedInfo, currentInfo) && + (await isGatewayTcpReady()) + ) { + console.log(" βœ“ Docker-driver gateway is healthy"); + return; } if (i < pollCount - 1) sleep(pollInterval); } - const tail = fs.existsSync(logPath) - ? fs - .readFileSync(logPath, "utf-8") - .split("\n") - .filter(Boolean) - .slice(-20) - .join("\n") - : ""; - if (exitOnFailure) { - console.error(" Docker-driver gateway failed to start."); - if (childExited) { - const how = - childExitSignal !== null - ? `killed by signal ${childExitSignal}` - : `exited with code ${childExitCode ?? "(unknown)"}`; - console.error(` Gateway process ${how} before becoming ready.`); - } - if (tail) { - console.error(" Gateway log tail:"); - for (const line of tail.split("\n")) console.error(` ${redact(line)}`); - } - console.error(" Troubleshooting:"); - console.error(` tail -100 ${logPath}`); - console.error(" docker info --format '{{json .CDISpecDirs}}'"); - process.exit(1); - } + reportDockerDriverGatewayStartFailure(logPath, childExit, { exitOnFailure }); throw new Error("Docker-driver gateway failed to start"); } diff --git a/src/lib/onboard/child-exit-tracker.test.ts b/src/lib/onboard/child-exit-tracker.test.ts new file mode 100644 index 00000000000..6c9d88b11c1 --- /dev/null +++ b/src/lib/onboard/child-exit-tracker.test.ts @@ -0,0 +1,84 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +// Behavioural tests for trackChildExit. Co-located with the module. +// +// See: https://github.com/NVIDIA/NemoClaw/issues/3111 + +import { spawn } from "node:child_process"; +import { describe, expect, it } from "vitest"; + +import { trackChildExit } from "./child-exit-tracker"; + +async function waitFor( + predicate: () => T | null | undefined, + timeoutMs = 2000, +): Promise { + const started = Date.now(); + while (Date.now() - started < timeoutMs) { + const v = predicate(); + if (v !== null && v !== undefined && v !== (false as unknown as T)) return v; + await new Promise((r) => setTimeout(r, 20)); + } + throw new Error(`waitFor timed out after ${timeoutMs}ms`); +} + +describe("trackChildExit (#3111)", () => { + it("reports exited=false while the child is running", async () => { + const child = spawn("sleep", ["5"], { detached: true }); + const state = trackChildExit(child); + expect(state.exited).toBe(false); + expect(state.describeExit()).toBeNull(); + child.kill("SIGKILL"); + await waitFor(() => state.exited); + }); + + it("reports exited=true and the exit code after normal exit", async () => { + const child = spawn("sh", ["-c", "exit 42"]); + const state = trackChildExit(child); + await waitFor(() => state.exited); + expect(state.code).toBe(42); + expect(state.signal).toBeNull(); + expect(state.describeExit()).toBe("exited with code 42"); + }); + + it("reports the signal when the child is killed", async () => { + const child = spawn("sleep", ["5"], { detached: true }); + const state = trackChildExit(child); + child.kill("SIGKILL"); + await waitFor(() => state.exited); + expect(state.signal).toBe("SIGKILL"); + expect(state.code).toBeNull(); + expect(state.describeExit()).toBe("killed by signal SIGKILL"); + }); + + it("survives child.unref() β€” detached-mode-safe", async () => { + // Regression guard for the #3111 scenario: startDockerDriverGateway + // spawns with detached:true + unref(). The exit listener must still + // fire after unref(), otherwise the whole point of this helper + // (zombie detection) collapses. + const child = spawn("sh", ["-c", "exit 7"], { detached: true }); + const state = trackChildExit(child); + child.unref(); + await waitFor(() => state.exited); + expect(state.code).toBe(7); + expect(state.describeExit()).toBe("exited with code 7"); + }); + + it("handles 'exited with code unknown' when both code and signal are null", async () => { + // Contrived guard: if someone synthesises a minimal mock ChildProcess + // that emits an exit event with (null, null), describeExit should not + // crash or produce 'undefined' in the message. + const fake = { + once: (event: string, cb: (code: null, signal: null) => void) => { + if (event === "exit") { + queueMicrotask(() => cb(null, null)); + } + }, + // biome-ignore lint/suspicious/noExplicitAny: narrowed mock shape + } as any; + const state = trackChildExit(fake); + await waitFor(() => state.exited); + expect(state.describeExit()).toBe("exited with code (unknown)"); + }); +}); diff --git a/src/lib/onboard/child-exit-tracker.ts b/src/lib/onboard/child-exit-tracker.ts new file mode 100644 index 00000000000..6807424c4f2 --- /dev/null +++ b/src/lib/onboard/child-exit-tracker.ts @@ -0,0 +1,83 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Observe exit of a detached ChildProcess without relying on + * `process.kill(pid, 0)`. + * + * ## Why not just `isPidAlive`? + * + * `isPidAlive` (in `onboard.ts`) uses `process.kill(pid, 0)` which returns + * **true for zombies** (detached children that exited but were not reaped + * by a `wait()` call). `onboard.ts:startDockerDriverGateway` spawns its + * gateway binary with `detached: true` + `child.unref()` and never calls + * `wait()`, so a crashed child lingers as a zombie indefinitely and + * `isPidAlive` reports it as alive. + * + * This module solves that by registering a one-shot `'exit'` listener on + * the `ChildProcess` object. The event fires reliably for detached + * children once libuv observes `SIGCHLD`, regardless of whether the + * parent ever reaps. Poll loops can then consult the returned state + * alongside `isPidAlive` for defence in depth. + * + * This is the caller-side half of the #3111 fix. The other half is the + * real TCP liveness probe in `./gateway-tcp-readiness`. + */ + +import type { ChildProcess } from "node:child_process"; + +export type ChildExitState = { + /** True once the 'exit' event has fired for this child. */ + readonly exited: boolean; + /** Exit code, or null if the child was killed by a signal or is still running. */ + readonly code: number | null; + /** Terminating signal, or null if the child exited normally or is still running. */ + readonly signal: NodeJS.Signals | null; + /** + * Human-readable description of how the child exited, or null if it + * hasn't exited yet. Suitable for inclusion in user-facing failure + * messages like: + * + * console.error(`Gateway process ${state.describeExit()} before becoming ready.`); + * // β†’ "Gateway process exited with code 127 before becoming ready." + * // β†’ "Gateway process killed by signal SIGKILL before becoming ready." + */ + describeExit(): string | null; +}; + +/** + * Attach a one-shot 'exit' listener to `child` and return a read-only + * view of its exit state. The listener runs even after `child.unref()`, + * so the caller is free to detach the child from the event loop. + */ +export function trackChildExit(child: ChildProcess): ChildExitState { + const state = { + exited: false, + code: null as number | null, + signal: null as NodeJS.Signals | null, + }; + child.once( + "exit", + (code: number | null, signal: NodeJS.Signals | null) => { + state.exited = true; + state.code = code; + state.signal = signal; + }, + ); + return { + get exited() { + return state.exited; + }, + get code() { + return state.code; + }, + get signal() { + return state.signal; + }, + describeExit(): string | null { + if (!state.exited) return null; + if (state.signal !== null) return `killed by signal ${state.signal}`; + return `exited with code ${state.code ?? "(unknown)"}`; + }, + }; +} diff --git a/src/lib/onboard/docker-driver-gateway-failure.test.ts b/src/lib/onboard/docker-driver-gateway-failure.test.ts new file mode 100644 index 00000000000..2bfe3f03731 --- /dev/null +++ b/src/lib/onboard/docker-driver-gateway-failure.test.ts @@ -0,0 +1,124 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +// Behavioural tests for reportDockerDriverGatewayStartFailure. +// +// See: https://github.com/NVIDIA/NemoClaw/issues/3111 + +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import type { ChildExitState } from "./child-exit-tracker"; +import { reportDockerDriverGatewayStartFailure } from "./docker-driver-gateway-failure"; + +function makeExitState(partial: Partial = {}): ChildExitState { + return { + exited: false, + code: null, + signal: null, + describeExit: () => null, + ...partial, + } as ChildExitState; +} + +describe("reportDockerDriverGatewayStartFailure (#3111)", () => { + let errSpy: ReturnType; + let exitSpy: ReturnType; + + beforeEach(() => { + errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + // Stub process.exit so assertions can still run. + exitSpy = vi + .spyOn(process, "exit") + .mockImplementation(((code?: number) => { + throw new Error(`process.exit(${code})`); + }) as never); + }); + + afterEach(() => { + errSpy.mockRestore(); + exitSpy.mockRestore(); + }); + + it("prints the 'failed to start' header and troubleshooting footer", () => { + expect(() => + reportDockerDriverGatewayStartFailure( + "/tmp/nonexistent-gateway.log", + makeExitState(), + { exitOnFailure: false }, + ), + ).not.toThrow(); + const joined = errSpy.mock.calls.map((c) => c.join(" ")).join("\n"); + expect(joined).toContain("Docker-driver gateway failed to start"); + expect(joined).toContain("Troubleshooting:"); + expect(joined).toContain("tail -100 /tmp/nonexistent-gateway.log"); + expect(joined).toContain("docker info"); + }); + + it("surfaces the child exit description when the child has exited", () => { + reportDockerDriverGatewayStartFailure( + "/tmp/nonexistent-gateway.log", + makeExitState({ + exited: true, + code: 127, + describeExit: () => "exited with code 127", + }), + { exitOnFailure: false }, + ); + const joined = errSpy.mock.calls.map((c) => c.join(" ")).join("\n"); + expect(joined).toContain( + "Gateway process exited with code 127 before becoming ready", + ); + }); + + it("omits the child-exit line when the child is still running", () => { + reportDockerDriverGatewayStartFailure( + "/tmp/nonexistent-gateway.log", + makeExitState(), + { exitOnFailure: false }, + ); + const joined = errSpy.mock.calls.map((c) => c.join(" ")).join("\n"); + expect(joined).not.toContain("before becoming ready"); + }); + + it("includes a tail of the gateway log when the file exists", () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "gw-fail-")); + const log = path.join(dir, "openshell-gateway.log"); + fs.writeFileSync( + log, + ["line-a", "line-b", "", "line-c", "GLIBC_2.38 not found"].join("\n"), + ); + try { + reportDockerDriverGatewayStartFailure(log, makeExitState(), { + exitOnFailure: false, + }); + const joined = errSpy.mock.calls.map((c) => c.join(" ")).join("\n"); + expect(joined).toContain("Gateway log tail"); + expect(joined).toContain("GLIBC_2.38 not found"); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("calls process.exit(1) when exitOnFailure is true", () => { + expect(() => + reportDockerDriverGatewayStartFailure( + "/tmp/nonexistent-gateway.log", + makeExitState(), + { exitOnFailure: true }, + ), + ).toThrow("process.exit(1)"); + expect(exitSpy).toHaveBeenCalledWith(1); + }); + + it("does NOT call process.exit when exitOnFailure is false", () => { + reportDockerDriverGatewayStartFailure( + "/tmp/nonexistent-gateway.log", + makeExitState(), + { exitOnFailure: false }, + ); + expect(exitSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/onboard/docker-driver-gateway-failure.ts b/src/lib/onboard/docker-driver-gateway-failure.ts new file mode 100644 index 00000000000..cfddf9b2b3b --- /dev/null +++ b/src/lib/onboard/docker-driver-gateway-failure.ts @@ -0,0 +1,81 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Failure-reporting helper for the Docker-driver gateway startup path in + * `onboard.ts:startDockerDriverGateway`. + * + * When the gateway fails to become healthy within the poll budget, users + * need three things to debug: + * + * 1. The fact of failure ("Docker-driver gateway failed to start."). + * 2. **Why** the child process died β€” signal or exit code β€” when + * applicable. Surfaced via `ChildExitState.describeExit()` so users + * don't have to `tail` the gateway log just to learn "the binary + * was killed by SIGKILL" or "exited with code 127" (#3111). + * 3. The tail of the gateway log plus a couple of troubleshooting + * commands so they know where to look next. + * + * Separated from `onboard.ts` because (a) it's a cohesive unit that + * doesn't depend on any onboard-private state besides the inputs, and + * (b) `onboard.ts` is the God Object being decomposed β€” new diagnostic + * logic should land in focused modules. + */ + +import fs from "node:fs"; + +import { redact } from "../security/redact"; + +import type { ChildExitState } from "./child-exit-tracker"; + +export type ReportDockerDriverGatewayStartFailureOpts = { + /** + * If true (the default for production call sites), print the failure + * message set and call `process.exit(1)`. If false (the recovery + * path), just print and let the caller decide. + */ + exitOnFailure: boolean; +}; + +/** + * Print the standard Docker-driver-gateway-start failure diagnostic set + * to stderr and either exit or return. Always prints: + * + * - the "failed to start" header, + * - the child-exit descriptor when available, + * - the last 20 non-blank lines of the gateway log (redacted), and + * - a short Troubleshooting footer with the log path and a docker CDI + * inspection command. + */ +export function reportDockerDriverGatewayStartFailure( + logPath: string, + childExit: ChildExitState, + { exitOnFailure }: ReportDockerDriverGatewayStartFailureOpts, +): void { + const tail = fs.existsSync(logPath) + ? fs + .readFileSync(logPath, "utf-8") + .split("\n") + .filter(Boolean) + .slice(-20) + .join("\n") + : ""; + + console.error(" Docker-driver gateway failed to start."); + if (childExit.exited) { + console.error( + ` Gateway process ${childExit.describeExit()} before becoming ready.`, + ); + } + if (tail) { + console.error(" Gateway log tail:"); + for (const line of tail.split("\n")) console.error(` ${redact(line)}`); + } + console.error(" Troubleshooting:"); + console.error(` tail -100 ${logPath}`); + console.error(" docker info --format '{{json .CDISpecDirs}}'"); + + if (exitOnFailure) { + process.exit(1); + } +} diff --git a/test/gateway-health-honest-integration.test.ts b/test/gateway-health-honest-integration.test.ts index 884510379a0..8fe075af34c 100644 --- a/test/gateway-health-honest-integration.test.ts +++ b/test/gateway-health-honest-integration.test.ts @@ -44,12 +44,17 @@ describe("startDockerDriverGateway integration (#3111)", () => { const fnBody = fnMatch[0]; it("tracks child-exit so zombies don't fool isPidAlive", () => { - expect(fnBody).toMatch(/child\.once\(\s*["']exit["']/); - expect(fnBody).toMatch(/childExited\s*=\s*true/); + // The concrete tracker lives in src/lib/onboard/child-exit-tracker.ts. + // startDockerDriverGateway must use it (not reimplement the listener) + // so the extraction stays extracted and the God-Object doesn't regrow. + expect(fnBody).toMatch(/trackChildExit\(\s*child\s*\)/); + expect(fnBody).not.toMatch(/child\.once\(\s*["']exit["']/); }); it("breaks the poll loop when the child has exited", () => { - expect(fnBody).toMatch(/childExited\s*\|\|\s*!isPidAlive\(childPid\)/); + expect(fnBody).toMatch( + /childExit\.exited\s*\|\|\s*!isPidAlive\(childPid\)/, + ); }); it("gates the 'healthy' log on the TCP readiness probe", () => { @@ -74,20 +79,36 @@ describe("startDockerDriverGateway integration (#3111)", () => { expect(before).not.toMatch(/await\s+isGatewayHttpReady\(/); }); - it("does NOT define isGatewayTcpReady inline (it must live in its own module)", () => { - // The probe must come from src/lib/onboard/gateway-tcp-readiness.ts, - // not from an inline declaration in onboard.ts. onboard.ts is the - // God Object being decomposed β€” new helpers should land in focused - // modules, mirroring the pattern established by - // src/lib/onboard/gateway-http-readiness.ts. + it("does NOT define the probe, tracker, or failure reporter inline (they must live in their own modules)", () => { + // onboard.ts is the God Object being decomposed β€” new helpers should + // land in focused modules, mirroring the pattern established by + // src/lib/onboard/gateway-http-readiness.ts. These imports enforce + // that the fix stays extracted and doesn't regrow the entrypoint. expect(content).not.toMatch(/function\s+isGatewayTcpReady\s*\(/); + expect(content).not.toMatch(/function\s+trackChildExit\s*\(/); + expect(content).not.toMatch( + /function\s+reportDockerDriverGatewayStartFailure\s*\(/, + ); expect(content).toMatch( /require\(\s*["']\.\/onboard\/gateway-tcp-readiness["']/, ); + expect(content).toMatch( + /require\(\s*["']\.\/onboard\/child-exit-tracker["']/, + ); + expect(content).toMatch( + /require\(\s*["']\.\/onboard\/docker-driver-gateway-failure["']/, + ); }); - it("surfaces child-exit details in the final failure message", () => { - expect(fnBody).toMatch(/childExited/); - expect(fnBody).toMatch(/childExitSignal|childExitCode/); + it("routes child-exit details through the failure reporter, not inline", () => { + // The failure message showing HOW the gateway exited (signal or code) + // comes from reportDockerDriverGatewayStartFailure's call to + // childExit.describeExit() inside its own module. The call site here + // only needs to pass childExit along. + expect(fnBody).toMatch( + /reportDockerDriverGatewayStartFailure\([^)]*childExit[^)]*\)/, + ); + // And the old inline implementation must be gone. + expect(fnBody).not.toMatch(/childExit\.describeExit\(\)/); }); }); From f8f7033ea3a30b85e038b47f0580387fca498b75 Mon Sep 17 00:00:00 2001 From: Julie Yaunches Date: Tue, 12 May 2026 16:21:29 -0400 Subject: [PATCH 9/9] style: type mock callback param in docker-driver-gateway-failure test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TS7006 under strict mode β€” the implicit 'any' on errSpy.mock.calls.map's callback parameter. No behavior change. --- src/lib/onboard/docker-driver-gateway-failure.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/lib/onboard/docker-driver-gateway-failure.test.ts b/src/lib/onboard/docker-driver-gateway-failure.test.ts index 2bfe3f03731..194e5764edf 100644 --- a/src/lib/onboard/docker-driver-gateway-failure.test.ts +++ b/src/lib/onboard/docker-driver-gateway-failure.test.ts @@ -50,7 +50,7 @@ describe("reportDockerDriverGatewayStartFailure (#3111)", () => { { exitOnFailure: false }, ), ).not.toThrow(); - const joined = errSpy.mock.calls.map((c) => c.join(" ")).join("\n"); + const joined = errSpy.mock.calls.map((c: string[]) => c.join(" ")).join("\n"); expect(joined).toContain("Docker-driver gateway failed to start"); expect(joined).toContain("Troubleshooting:"); expect(joined).toContain("tail -100 /tmp/nonexistent-gateway.log"); @@ -67,7 +67,7 @@ describe("reportDockerDriverGatewayStartFailure (#3111)", () => { }), { exitOnFailure: false }, ); - const joined = errSpy.mock.calls.map((c) => c.join(" ")).join("\n"); + const joined = errSpy.mock.calls.map((c: string[]) => c.join(" ")).join("\n"); expect(joined).toContain( "Gateway process exited with code 127 before becoming ready", ); @@ -79,7 +79,7 @@ describe("reportDockerDriverGatewayStartFailure (#3111)", () => { makeExitState(), { exitOnFailure: false }, ); - const joined = errSpy.mock.calls.map((c) => c.join(" ")).join("\n"); + const joined = errSpy.mock.calls.map((c: string[]) => c.join(" ")).join("\n"); expect(joined).not.toContain("before becoming ready"); }); @@ -94,7 +94,7 @@ describe("reportDockerDriverGatewayStartFailure (#3111)", () => { reportDockerDriverGatewayStartFailure(log, makeExitState(), { exitOnFailure: false, }); - const joined = errSpy.mock.calls.map((c) => c.join(" ")).join("\n"); + const joined = errSpy.mock.calls.map((c: string[]) => c.join(" ")).join("\n"); expect(joined).toContain("Gateway log tail"); expect(joined).toContain("GLIBC_2.38 not found"); } finally {