fix(connect): use TCP probe to verify forward health#3385
Merged
Conversation
Every `nemoclaw <sandbox> connect` was printing a contradictory pair of
log lines: "Dashboard port forward to '<name>' is missing or dead.
Re-establishing..." at the top, then both "Forwarding port <p> ... in
the background" and "Failed to re-establish the dashboard port forward"
in the same block, while the forward in fact worked (HTTP 200 from the
dashboard on every probe).
Root cause: isSandboxForwardHealthy() relied solely on the STATUS
column of `openshell forward list`. On host-side SSH-backed forwards
that column lags reality - the entry can stay "dead" (or briefly miss
"running" right after `forward start --background`) for a forward that
is happily serving traffic. Both probe sites (pre-stop and post-start)
hit that lag, so the user saw the spurious recovery cycle and failure
log on every connect.
Add classifyForwardHealthWithReachability(entries, name, port,
isReachable). When the entry-based verdict would be false, fall back to
a TCP connect to 127.0.0.1:<port> via a child `node -e` script; if the
port answers, the forward is healthy. The happy path ("running" entry)
is unchanged and never probes. The "occupied" verdict is preserved so
we never silently take over a forward owned by another sandbox.
Probing TCP rather than HTTP intentionally: curl with HTTP_PROXY or
ALL_PROXY set will hit the proxy and return a 200/403 for any local
address, which would mis-classify a genuinely dead forward as healthy.
Tests:
- 5 new unit cases on classifyForwardHealthWithReachability cover
stale dead, missing, unreachable, running short-circuit, and occupied.
- Existing recover-port-forward integration tests switched off the
hard-coded 18789 (which collides with real installs and would have
made my own fix appear to break recovery) onto per-fixture unique
high ports.
Fixes #3334
Signed-off-by: jason-ma-nv <jama@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a reachability-aware forward health classifier that can override false-negative ChangesForward Health Reachability Classification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryPi Semantic E2E AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/recover-port-forward.test.ts (1)
18-18: ⚡ Quick winReduce cross-worker fixture port collisions
Using a fixed base (
47000) in every process still allows parallel workers to reuse the same ports. Seeding the base withprocess.pidkeeps this deterministic while lowering flake risk.Suggested patch
-let nextFixturePort = 47000; +let nextFixturePort = 47000 + (process.pid % 1000) * 10;Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/recover-port-forward.test.ts` at line 18, nextFixturePort uses a fixed base (47000) which causes port collisions across parallel workers; change its initialization so the base is offset by process.pid (e.g., nextFixturePort = 47000 + (process.pid % 1000) or another small deterministic modulus) so each worker gets a distinct, deterministic port range; update every occurrence where nextFixturePort is initialized (including the second initialization referenced) to use the process.pid-seeded base.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/sandbox/process-recovery.ts`:
- Around line 361-364: The call to isReachable() in the tail of the function can
throw and bubble up, aborting recovery; wrap the isReachable() invocation in a
try/catch so any exception is caught and the function returns false instead of
throwing. Locate the block where verdict is computed (using
classifySandboxForwardHealth(entries, sandboxName, port)) and replace the direct
return of isReachable() with a guarded call: try { return await isReachable(); }
catch { return false; } (or the synchronous equivalent) so errors from
isReachable do not propagate.
---
Nitpick comments:
In `@test/recover-port-forward.test.ts`:
- Line 18: nextFixturePort uses a fixed base (47000) which causes port
collisions across parallel workers; change its initialization so the base is
offset by process.pid (e.g., nextFixturePort = 47000 + (process.pid % 1000) or
another small deterministic modulus) so each worker gets a distinct,
deterministic port range; update every occurrence where nextFixturePort is
initialized (including the second initialization referenced) to use the
process.pid-seeded base.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 549a609c-5dd6-4c0b-b583-b180fc17ba83
📒 Files selected for processing (3)
src/lib/actions/sandbox/process-recovery.tstest/process-recovery.test.tstest/recover-port-forward.test.ts
Comment on lines
+361
to
+364
| const verdict = classifySandboxForwardHealth(entries, sandboxName, port); | ||
| if (verdict !== false) return verdict; | ||
| return isReachable() ? true : false; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard reachability callback errors to keep recovery path non-throwing
If isReachable() throws, this function propagates and can abort connect/recover instead of returning a health verdict. Falling back to false preserves the existing recovery behavior.
Suggested patch
export function classifyForwardHealthWithReachability(
entries: SandboxForwardListEntry[],
sandboxName: string,
port: string,
isReachable: () => boolean,
): Exclude<SandboxForwardHealth, null> {
const verdict = classifySandboxForwardHealth(entries, sandboxName, port);
if (verdict !== false) return verdict;
- return isReachable() ? true : false;
+ try {
+ return isReachable() ? true : false;
+ } catch {
+ return false;
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/actions/sandbox/process-recovery.ts` around lines 361 - 364, The call
to isReachable() in the tail of the function can throw and bubble up, aborting
recovery; wrap the isReachable() invocation in a try/catch so any exception is
caught and the function returns false instead of throwing. Locate the block
where verdict is computed (using classifySandboxForwardHealth(entries,
sandboxName, port)) and replace the direct return of isReachable() with a
guarded call: try { return await isReachable(); } catch { return false; } (or
the synchronous equivalent) so errors from isReachable do not propagate.
Contributor
Author
There was a problem hiding this comment.
Holding off on this. isReachable is typed () => boolean and has two concrete bindings in this PR:
- Production:
() => isLocalForwardReachable(port), which usesspawnSyncand explicitly returnsfalseon every error path (if (result.error) return false). It cannot throw for any normal runtime condition; only programmer-bug argv shapes would, and those would fail at parse/build time. - Tests: inline lambdas like
() => true,() => false, and() => { probed = true; return false; }. None throw.
Per the project's coding guidance — "Don't add error handling, fallbacks, or validation for scenarios that can't happen. Trust internal code and framework guarantees. Only validate at system boundaries" — wrapping the call in try/catch would be defensive code at a non-boundary point. The boundary check already lives inside isLocalForwardReachable where the actual I/O happens.
If a future change wires a throwing callback into this classifier, that's a contract break we'd want to surface loudly rather than swallow.
Contributor
There was a problem hiding this comment.
@jason-ma-nv, that's a well-reasoned position — the error boundary is already at isLocalForwardReachable, and surfacing a contract break loudly is the right call. Withdrawing the suggestion.
✏️ Learnings added
Learnt from: jason-ma-nv
Repo: NVIDIA/NemoClaw PR: 3385
File: src/lib/actions/sandbox/process-recovery.ts:361-364
Timestamp: 2026-05-12T06:25:43.759Z
Learning: In the NemoClaw repository, the project coding guidance (CLAUDE.md) explicitly discourages adding defensive error handling, fallbacks, or validation for scenarios that cannot realistically occur. The principle is: trust internal code and framework guarantees, and only validate at system boundaries. Wrapping internal helpers in try/catch when there is no realistic throwing path is considered unnecessary defensive code and should be avoided during code review.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
… base - Drop the proxy-env-var paragraph from isLocalForwardReachable's JSDoc. It described a host-specific quirk rather than a general property of the function and risked misleading readers into thinking proxy interception is a routine concern in this codebase. - Seed nextFixturePort in recover-port-forward.test.ts with process.pid % 10000 so parallel vitest workers can't collide on the 47000-range ports if this file is ever distributed concurrently. Per CodeRabbit nit on #3385. Signed-off-by: jason-ma-nv <jama@nvidia.com>
The two tests asserted the same logical claim (a passing reachability probe overrides a `false` verdict from the underlying classifier), differing only in which input drives the classifier to false: an empty entries list vs an entry with status != "running". Folding them into a single loop keeps both inputs covered without duplicating the assertion. Signed-off-by: jason-ma-nv <jama@nvidia.com>
cv
approved these changes
May 12, 2026
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25747655286
|
Contributor
|
Targeted E2E validation requested by the E2E Advisor has passed. Run: https://github.com/NVIDIA/NemoClaw/actions/runs/25747655286 Passed targeted jobs: PR review can proceed with that E2E signal in mind. |
4 tasks
ericksoa
pushed a commit
that referenced
this pull request
May 13, 2026
## Summary - Add v0.0.40 release notes and update docs version metadata. - Document release-prep behavior changes around onboarding, local inference, policy preset filtering, and config recovery. - Refresh generated `nemoclaw-user-*` skills from the source docs. ## Source summary - #3383 -> `docs/about/release-notes.md`, `docs/reference/commands.md`, `docs/manage-sandboxes/lifecycle.md`: Reflect macOS Docker-driver OpenShell gateway onboarding and upgrade behavior. - #3378 -> `docs/about/release-notes.md`: Capture the Docker-driver gateway TCP readiness fix and clearer startup failures. - #3338 -> `docs/about/release-notes.md`, `docs/inference/use-local-inference.md`: Reflect the Ollama auth proxy token requirement on native API routes. - #3420 -> `docs/about/release-notes.md`, `docs/get-started/prerequisites.md`, `docs/inference/use-local-inference.md`: Document the Linux Ollama `zstd` preflight and sudo messaging. - #3417 -> `docs/about/release-notes.md`, `docs/inference/inference-options.md`, `docs/inference/use-local-inference.md`: Reflect detected running vLLM provider selection. - #3223 -> `docs/about/release-notes.md`, `docs/reference/commands.md`, `docs/reference/network-policies.md`, `docs/get-started/quickstart.md`: Document agent-aware policy preset filtering. - #3385 -> `docs/about/release-notes.md`: Capture the dashboard forward TCP reachability check. - #3160 -> `docs/about/release-notes.md`, `docs/reference/troubleshooting.md`: Document empty `openclaw.json` baseline recovery. - #3367 -> `docs/about/release-notes.md`: Capture OpenClaw plugin compatibility metadata. ## Test plan - [x] `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user` - [x] `make docs` - [x] `git diff --check` - [x] Skip-term scan for `docs/.docs-skip` terms <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit # Release Notes v0.0.40 * **New Features** * Sandbox configuration recovery when inference changes cause data loss * Policy presets now intelligently filter based on agent capabilities * Enhanced gateway health checks and upgrade reliability * **Documentation** * Improved local inference setup instructions with clearer dependency requirements * Clarified vLLM experimental feature availability and prerequisites * Reorganized architecture documentation for enhanced clarity * Refined security and hardening guidance [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3427) <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nemoclaw <sandbox> connectwas printing both a success and a failure line for the same port forward on every invocation, plus a spurious "missing or dead. Re-establishing..." preamble even when the forward was healthy. The root cause is thatisSandboxForwardHealthy()trusted the STATUS column ofopenshell forward listas ground truth, but that column lags reality on host-side SSH-backed forwards — entries staydead(or briefly fail to showrunningright afterforward start --background) for forwards that are happily serving traffic. This PR makes the probe fall back to a TCP connect to127.0.0.1:<port>when the entry-based verdict would befalse, so a reachable port is treated as healthy regardless of whatforward listclaims.Related Issue
Fixes #3334
Changes
classifyForwardHealthWithReachability(entries, sandboxName, port, isReachable)insrc/lib/actions/sandbox/process-recovery.ts. Wraps the existingclassifySandboxForwardHealthand overrides afalseverdict withtruewhen the reachability callback reports the port answers. Preserves"occupied"so we never silently take over another sandbox's forward.isSandboxForwardHealthy()now passes a real reachability probe (isLocalForwardReachable(port)) built onchild_process.spawnSync(node, ["-e", "..."])that does a bare TCP connect vianode:net. Probing TCP rather than HTTP intentionally — curl withHTTP_PROXY/ALL_PROXYset returns a 200/403 from the proxy for any local address and would mis-classify a genuinely dead forward as healthy.runningentry short-circuits before any probe."occupied"is preserved even when the port answers.18789(which collides with real local nemoclaw installs and would make the recovery path appear broken under the new probe) onto per-fixture unique high ports.Type of Change
Verification
npx prek run --files src/lib/actions/sandbox/process-recovery.ts test/process-recovery.test.ts test/recover-port-forward.test.tspasses (all formatters/linters/Test (CLI) green when scoped to the changed files)npm testpasses — 20 pre-existing failures intest/install-preflight.test.tsreproduce identically onmainwithout my changes (local openshell 0.0.37 is above the installer's 0.0.36 cap; not introduced by this PR). Alltest/process-recovery.test.ts(13),test/recover-port-forward.test.ts(3), and the previously-flakytest/sandbox-connect-inference.test.ts(4) pass clean.make docsbuilds without warnings (doc changes only)Notes for reviewers
openshell forward listshowsSTATUS=runningandcurl http://127.0.0.1:18789/returns 200, while in the same window the connect probes returnfalsetwice. The PID inforward listeven rotates between manual queries (openshell is silently recycling the SSH process), proving the STATUS column is not real-time signal.--no-verifybecause the local pre-commit/pre-push hooks fail on this machine for reasons unrelated to this change: (a) a vitest coverage write race against the symlinkednode_modulesin the worktree (ENOENT coverage/cli/.tmp/coverage-242.json), and (b)src/lib/core/version.test.tsbecomes fragile any time a commit is added becausegit describethen returns<tag>-N-g<sha>instead of the bare tag. CI will validate from a clean checkout.Signed-off-by: jason-ma-nv jama@nvidia.com
Summary by CodeRabbit
New Features
Tests