refactor(cli): wire recoverDashboardChain into checkAndRecoverSandboxProcesses - #2710
refactor(cli): wire recoverDashboardChain into checkAndRecoverSandboxProcesses#2710jyaunches wants to merge 6 commits into
Conversation
…Processes Replace the manual gateway-restart + port-forward logic in checkAndRecoverSandboxProcesses with delegation to the Dashboard Delivery Contract's recoverDashboardChain(). This verifies all chain links (gateway, forward, CORS) and only repairs what's broken. Implements bounded DashboardRecoverDeps in nemoclaw.ts: - captureForwardList: bounded with OPENSHELL_PROBE_TIMEOUT_MS - downloadSandboxConfig: bounded with OPENSHELL_DOWNLOAD_TIMEOUT_MS - stopForward/startForward: bounded with OPENSHELL_OPERATION_TIMEOUT_MS - executeSandboxCommand: already bounded (15s SSH timeout) - restartGateway: delegates to existing recoverSandboxProcesses This is Phase 5 of NVIDIA#2562 and completes the nemoclaw.ts integration that was reverted after the NVIDIA#2398 E2E hang. All openshell calls in the recovery path are now explicitly bounded. Closes: NVIDIA#2390 Refs: NVIDIA#2562
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReworks sandbox dashboard recovery in Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "nemoclaw CLI"
participant Core as "src/nemoclaw.ts (buildDashboardRecoverDeps)"
participant Recover as "dashboard-recover (recoverDashboardChain)"
participant Sandbox as "Sandbox (exec in-sandbox cmds)"
participant Forward as "Port Forward (openshell forward)"
participant Gateway as "Gateway process"
participant Probe as "HTTP probe"
CLI->>Core: request sandbox recovery (name, optional port)
Core->>Recover: start recovery with built deps (CHAT_UI_URL, port)
Recover->>Sandbox: download/parse in-sandbox config
Recover->>Forward: check/restore port forward
Recover->>Gateway: restart gateway (openclaw gateway run --port)
Recover->>Probe: probe /health on resolved host:port
Probe-->>Recover: health result
Recover-->>Core: return {attempted, after?.healthy, actions, diagnosis}
Core-->>CLI: surface actions, diagnostics, manual commands
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nemoclaw.ts (1)
414-420:⚠️ Potential issue | 🟠 MajorThis still skips chain recovery when only the forward/CORS link is broken.
The early return on
running === truemeansrecoverDashboardChain()never runs for the cases this refactor is meant to handle: the in-sandbox gateway is healthy, but the dashboard chain is not. In those casesconnect/statuswill keep reporting success here and never repair the broken forward/CORS path.Suggested direction
- const running = isSandboxGatewayRunning(sandboxName); - if (running === null) { - return { checked: false, wasRunning: null, recovered: false }; - } - if (running) { - return { checked: true, wasRunning: true, recovered: false }; - } + const running = isSandboxGatewayRunning(sandboxName); + if (running === null) { + return { checked: false, wasRunning: null, recovered: false }; + }Then always build the chain and call
recoverDashboardChain(), and map the legacy return shape from the chain status, e.g.wasRunningfrom the gateway link andrecoveredfrombefore/after.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 414 - 420, The current early return when isSandboxGatewayRunning(sandboxName) === true prevents running recoverDashboardChain(), so always construct the chain status and call recoverDashboardChain() regardless of gateway health; use isSandboxGatewayRunning(sandboxName) only to set wasRunning (legacy behavior) and derive recovered by comparing chain status before/after (the results from recoverDashboardChain or chain status checks), then return the legacy-shaped object { checked: true/false, wasRunning: <gateway boolean|null>, recovered: <before_vs_after boolean> } so forward/CORS link repairs run even when the in-sandbox gateway is healthy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nemoclaw.ts`:
- Line 387: restartGateway currently ignores the chain's port/agent and calls
recoverSandboxProcesses(name), which causes recovery to fall back to the
hardcoded DASHBOARD_PORT; change restartGateway to forward the provided _port
and _agent into recoverSandboxProcesses (e.g. recoverSandboxProcesses(name, {
port: _port, agent: _agent })), and update recoverSandboxProcesses to accept an
opts parameter (port?, agent?) and derive port as opts.port ??
opts.agent?.forwardPort ?? DASHBOARD_PORT, then use that computed port
everywhere the helper currently references DASHBOARD_PORT.
---
Outside diff comments:
In `@src/nemoclaw.ts`:
- Around line 414-420: The current early return when
isSandboxGatewayRunning(sandboxName) === true prevents running
recoverDashboardChain(), so always construct the chain status and call
recoverDashboardChain() regardless of gateway health; use
isSandboxGatewayRunning(sandboxName) only to set wasRunning (legacy behavior)
and derive recovered by comparing chain status before/after (the results from
recoverDashboardChain or chain status checks), then return the legacy-shaped
object { checked: true/false, wasRunning: <gateway boolean|null>, recovered:
<before_vs_after boolean> } so forward/CORS link repairs run even when the
in-sandbox gateway is healthy.
🪄 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: 1722ca17-04da-4a37-91dc-206fe5f4abb5
📒 Files selected for processing (1)
src/nemoclaw.ts
Address CodeRabbit feedback: 1. Remove early return when gateway is running — always run recoverDashboardChain() so broken forward/CORS links get repaired even when the in-sandbox gateway is healthy (NVIDIA#2042, NVIDIA#1178). 2. Forward port param through restartGateway → recoverSandboxProcesses so the fallback recovery script uses the chain's port instead of hardcoded DASHBOARD_PORT. The wasRunning field now reflects the actual gateway probe result regardless of whether recovery was attempted. Refs: NVIDIA#2390
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
I found two blocking issues in the new recovery integration:
-
checkAndRecoverSandboxProcesses()builds the recovery chain fromagent?.forwardPort ?? DASHBOARD_PORT, but onboarding persists the actual allocated/overridden dashboard port in the registry asdashboardPort. For a second/custom-port OpenClaw sandbox, e.g. one registered on18790, this path will still probe/restart/re-forward18789, sostatus/connectcan misdiagnose a healthy sandbox or recover the wrong port. Please derive the recovery port fromregistry.getSandbox(sandboxName)?.dashboardPortbefore falling back to agent/default ports. -
recoverDashboardChain()now runs even whenisSandboxGatewayRunning()is true, butbuildDashboardRecoverDeps()hard-codes the OpenClaw CORS config path/sandbox/.openclaw/openclaw.json. Non-OpenClaw agents such as Hermes use/sandbox/.hermes/config.yamland expose an API, not the OpenClaw control UI. A healthy Hermes sandbox will therefore fail the CORS link and print an incomplete dashboard-chain recovery message on connect. Please gate this OpenClaw dashboard/CORS recovery to OpenClaw sandboxes, or make the dashboard contract/deps agent-aware.
…ailures The E2E tests (sandbox-survival-e2e, skip-permissions-e2e) failed with exit code 124 (timeout hang) because recoverDashboardChain immediately re-verified after restarting the gateway — the gateway HTTP listener hadn't bound yet, so verifyDashboardChain reported it unhealthy. Add sleepSeconds(3) after a successful restartGateway call to give the gateway time to bind its HTTP port before the chain re-verification. This matches the original recovery code's behavior (sleepSeconds(3) between recoverSandboxProcesses and isSandboxGatewayRunning). Also removes a redundant type annotation in downloadSandboxConfig. Refs: NVIDIA#2390
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nemoclaw.ts (1)
417-430:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse one gateway-health signal for both reporting and recovery.
This path now mixes
isSandboxGatewayRunning()withrecoverDashboardChain()'sbefore.links.gateway.ok. Those probes do not agree on auth-protected responses (isSandboxGatewayRunning()rejects HTTP 401,verifyDashboardChain()treats it as alive), so you can return{ checked: true, wasRunning: false, recovered: false }even when the chain is already healthy.sandbox statuswill then report the gateway as down incorrectly.Suggested fix
const deps = buildDashboardRecoverDeps(); const result = recoverDashboardChain(sandboxName, chain, deps); + const gatewayRunning = result.before.links.gateway.ok; // Chain was already healthy — nothing to do if (!result.attempted) { - return { checked: true, wasRunning: running, recovered: false }; + return { checked: true, wasRunning: gatewayRunning, recovered: false }; } // Recovery was attempted — report progress - if (!quiet && !running) { + if (!quiet && !gatewayRunning) { console.log(""); console.log( ` ${agentRuntime.getAgentDisplayName(agent)} gateway is not running inside the sandbox (sandbox likely restarted).`, @@ - return { checked: true, wasRunning: running, recovered: true }; + return { checked: true, wasRunning: gatewayRunning, recovered: true }; @@ - return { checked: true, wasRunning: running, recovered: false }; + return { checked: true, wasRunning: gatewayRunning, recovered: false };Also applies to: 433-448, 468-468
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 417 - 430, The current logic mixes two different gateway-health signals (isSandboxGatewayRunning vs recoverDashboardChain.before.links.gateway.ok) causing mismatched auth-handling; remove the initial isSandboxGatewayRunning() call and instead run recoverDashboardChain(...) first and use its before.links.gateway.ok value as the source of truth for "wasRunning" (treating 401-like responses as healthy the same way verifyDashboardChain does); update all other occurrences (the blocks around lines 433-448 and 468) to derive checked/wasRunning from the recover/verify result rather than calling isSandboxGatewayRunning() separately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nemoclaw.ts`:
- Around line 422-424: The code pre-resolves a fallback port (const port =
agent?.forwardPort ?? DASHBOARD_PORT) before calling buildChain, preventing
buildChain from deriving a port from CHAT_UI_URL; change to pass the raw
forwardPort (const port = agent?.forwardPort) and call buildChain({ port,
chatUiUrl: process.env.CHAT_UI_URL }) so buildChain can perform its own
fallback/derivation logic (referencing agentRuntime.getSessionAgent,
agent?.forwardPort, DASHBOARD_PORT, and buildChain).
---
Outside diff comments:
In `@src/nemoclaw.ts`:
- Around line 417-430: The current logic mixes two different gateway-health
signals (isSandboxGatewayRunning vs
recoverDashboardChain.before.links.gateway.ok) causing mismatched auth-handling;
remove the initial isSandboxGatewayRunning() call and instead run
recoverDashboardChain(...) first and use its before.links.gateway.ok value as
the source of truth for "wasRunning" (treating 401-like responses as healthy the
same way verifyDashboardChain does); update all other occurrences (the blocks
around lines 433-448 and 468) to derive checked/wasRunning from the
recover/verify result rather than calling isSandboxGatewayRunning() separately.
🪄 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: 7fb27e62-0ba5-49be-a724-06125dda535b
📒 Files selected for processing (1)
src/nemoclaw.ts
| const agent = agentRuntime.getSessionAgent(sandboxName); | ||
| const port = agent?.forwardPort ?? DASHBOARD_PORT; | ||
| const chain = buildChain({ port, chatUiUrl: process.env.CHAT_UI_URL }); |
There was a problem hiding this comment.
Let buildChain() own the fallback port resolution.
Line 423 always supplies a port, so buildChain() never gets to derive one from CHAT_UI_URL. That breaks custom loopback/local-port setups by verifying CORS and forward state against the default port instead of the configured URL.
Suggested fix
const agent = agentRuntime.getSessionAgent(sandboxName);
- const port = agent?.forwardPort ?? DASHBOARD_PORT;
- const chain = buildChain({ port, chatUiUrl: process.env.CHAT_UI_URL });
+ const chain = buildChain({
+ port: agent?.forwardPort,
+ chatUiUrl: process.env.CHAT_UI_URL,
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const agent = agentRuntime.getSessionAgent(sandboxName); | |
| const port = agent?.forwardPort ?? DASHBOARD_PORT; | |
| const chain = buildChain({ port, chatUiUrl: process.env.CHAT_UI_URL }); | |
| const agent = agentRuntime.getSessionAgent(sandboxName); | |
| const chain = buildChain({ | |
| port: agent?.forwardPort, | |
| chatUiUrl: process.env.CHAT_UI_URL, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/nemoclaw.ts` around lines 422 - 424, The code pre-resolves a fallback
port (const port = agent?.forwardPort ?? DASHBOARD_PORT) before calling
buildChain, preventing buildChain from deriving a port from CHAT_UI_URL; change
to pass the raw forwardPort (const port = agent?.forwardPort) and call
buildChain({ port, chatUiUrl: process.env.CHAT_UI_URL }) so buildChain can
perform its own fallback/derivation logic (referencing
agentRuntime.getSessionAgent, agent?.forwardPort, DASHBOARD_PORT, and
buildChain).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nemoclaw.ts`:
- Around line 455-469: The fallback message always points users to run the
gateway manually and omits the resolved chain port; update the post-recovery
messaging in the block that checks result.after and prints the manual command
(around result.actions/result.after handling) to: inspect result.after.diagnosis
(or any failed-link indicator on result.after) and choose a tailored remediation
(e.g., if diagnosis indicates only CORS, print a CORS-specific hint instead of
the gateway command), and when you do print the gateway command via
agentRuntime.getGatewayCommand(agent) include the actual resolved port from the
agent/chain object (e.g., agent.chain.port or agent.getResolvedPort()) so
non-default ports are shown; modify the conditional that logs the manual
instruction to branch on the failure type and to include the resolved chain.port
in the constructed command.
🪄 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: eeadee98-0e75-4961-b165-c5034f2193d2
📒 Files selected for processing (1)
src/nemoclaw.ts
| // Chain recovery didn't fully succeed — report diagnosis | ||
| if (!quiet) { | ||
| if (result.actions.length > 0) { | ||
| for (const action of result.actions) { | ||
| console.log(` • ${action}`); | ||
| } | ||
| } | ||
| if (result.after && !result.after.healthy) { | ||
| console.error(` Recovery incomplete: ${result.after.diagnosis || "unknown"}`); | ||
| } | ||
| } else if (!quiet) { | ||
| console.error( | ||
| ` Could not restart ${agentRuntime.getAgentDisplayName(_recoveryAgent)} gateway automatically.`, | ||
| ` Could not fully recover ${agentRuntime.getAgentDisplayName(agent)} dashboard chain.`, | ||
| ); | ||
| console.error(" Connect to the sandbox and run manually:"); | ||
| console.error(` ${agentRuntime.getGatewayCommand(_recoveryAgent)}`); | ||
| console.error(` ${agentRuntime.getGatewayCommand(agent)}`); |
There was a problem hiding this comment.
Tailor the fallback remediation to the failed link.
After chain recovery fails, this always tells the user to run the gateway manually. That is misleading when the only broken link is CORS, and the printed command also omits the resolved chain.port, so non-default-port setups get the wrong manual instruction too.
Suggested fix
if (!quiet) {
if (result.actions.length > 0) {
for (const action of result.actions) {
console.log(` • ${action}`);
}
}
if (result.after && !result.after.healthy) {
console.error(` Recovery incomplete: ${result.after.diagnosis || "unknown"}`);
}
+ const failedLinks = result.after?.links ?? result.before.links;
console.error(
` Could not fully recover ${agentRuntime.getAgentDisplayName(agent)} dashboard chain.`,
);
- console.error(" Connect to the sandbox and run manually:");
- console.error(` ${agentRuntime.getGatewayCommand(agent)}`);
+ if (!failedLinks.gateway.ok) {
+ console.error(" Connect to the sandbox and run manually:");
+ console.error(` ${agentRuntime.getGatewayCommand(agent)} --port ${chain.port}`);
+ } else if (!failedLinks.cors.ok) {
+ console.error(" Rebuild the sandbox to refresh the dashboard allowedOrigins.");
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/nemoclaw.ts` around lines 455 - 469, The fallback message always points
users to run the gateway manually and omits the resolved chain port; update the
post-recovery messaging in the block that checks result.after and prints the
manual command (around result.actions/result.after handling) to: inspect
result.after.diagnosis (or any failed-link indicator on result.after) and choose
a tailored remediation (e.g., if diagnosis indicates only CORS, print a
CORS-specific hint instead of the gateway command), and when you do print the
gateway command via agentRuntime.getGatewayCommand(agent) include the actual
resolved port from the agent/chain object (e.g., agent.chain.port or
agent.getResolvedPort()) so non-default ports are shown; modify the conditional
that logs the manual instruction to branch on the failure type and to include
the resolved chain.port in the constructed command.
…port Address Aaron's review feedback: 1. Port derivation: use registry.getSandbox(name)?.dashboardPort instead of agent?.forwardPort to correctly recover multi-sandbox setups with auto-allocated or user-overridden ports (e.g. 18790). 2. Agent gating: only run full dashboard chain recovery (CORS check via /sandbox/.openclaw/openclaw.json) for OpenClaw sandboxes. Non-OpenClaw agents (Hermes) use different config paths and don't expose the OpenClaw control UI — fall back to gateway-only recovery via new checkAndRecoverGatewayOnly() helper. The gateway-only path preserves the original pre-NVIDIA#2398 behavior for Hermes: check gateway → restart if dead → re-forward → done. Refs: NVIDIA#2390
|
Thanks Aaron — both great catches. Fixed in f76baa5: 1. Port derivation: Now uses 2. Agent gating: Dashboard chain recovery (including CORS check against Also: the nightly E2E (sandbox-survival, sandbox-operations, skip-permissions) confirmed the gateway-bind-wait blocker — both survival and skip-permissions timed out with exit 124. That's already fixed in this push with |
The E2E tests (sandbox-survival, skip-permissions) timed out because recoverDashboardChain was called even when the gateway was running. The downloadSandboxConfig call inside verifyDashboardChain takes 30s to timeout when SSH isn't reachable, and it's called twice (before and after recovery) — stacking to 60s+ of blocked I/O on a path that should be instant. Add a fast-path: when gateway IS running AND forward list shows the correct port is active, return immediately without invoking the full chain verification. The expensive chain recovery only runs when either the gateway is dead or the forward is missing. This preserves the fix for NVIDIA#2042/NVIDIA#1178 (forward-only failures) while avoiding the performance regression that caused E2E timeouts. Refs: NVIDIA#2390
✅ E2E Validation PassedNightly lifecycle E2E suite run on
Previous failures (run 25139006233) confirmed the gateway-bind-wait bug — both tests timed out with exit code 124. Fixed by adding a fast-path that skips expensive chain verification when gateway + forward are already healthy. |
4 participants
Summary
Wire the Dashboard Delivery Contract's
recoverDashboardChain()intocheckAndRecoverSandboxProcesses()in nemoclaw.ts. This replaces the manual gateway-restart + port-forward logic with chain-level verification and link-aware recovery. All openshell calls in the recovery path are explicitly bounded using the timeout constants landed in #2683.Related Issue
Closes #2390
Changes
buildDashboardRecoverDeps()in nemoclaw.ts with bounded openshell calls:captureForwardList→OPENSHELL_PROBE_TIMEOUT_MS(15s)downloadSandboxConfig→OPENSHELL_DOWNLOAD_TIMEOUT_MS(30s)stopForward/startForward→OPENSHELL_OPERATION_TIMEOUT_MS(30s)executeSandboxCommand→ already bounded (15s SSH timeout)restartGateway→ delegates to existingrecoverSandboxProcesses()checkAndRecoverSandboxProcessesbody with delegation torecoverDashboardChain(){ checked, wasRunning, recovered }— callers unaffectedbuildChain,recoverDashboardChain, andOPENSHELL_DOWNLOAD_TIMEOUT_MSType of Change
Verification
npx prek run --all-filespassesnpm testpasses (dashboard-contract, dashboard-health, dashboard-recover, openshell-timeouts — 30 tests)AI Disclosure
Signed-off-by: Jessica Yaunches jyaunches@nvidia.com
Summary by CodeRabbit