fix(cli): add timeout to downloadSandboxConfig in dashboard recovery - #2470
fix(cli): add timeout to downloadSandboxConfig in dashboard recovery#2470ericksoa wants to merge 4 commits into
Conversation
The CORS verification step introduced in #2398 calls `openshell sandbox download` (SSH into sandbox) with no timeout. If SSH stalls — common in CI right after sandbox creation — the entire `nemoclaw status` command hangs indefinitely, causing E2E test timeouts (exit 124) on sandbox-survival, skip-permissions, and sandbox-operations. Add a 15s timeout matching executeSandboxCommand. On timeout the download returns non-zero, downloadSandboxConfig returns null, and CORS reports "could not download" instead of blocking forever.
📝 WalkthroughWalkthroughEnforces a 15s execution timeout for the OpenClaw sandbox download, adds timestamped diagnostic markers to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
…mmand The timeout option was accepted by the nemoclaw.ts runOpenshell wrapper but never passed to spawnSync in openshell.ts — silently dropped. Add timeout to OpenshellSpawnOptions and forward it to spawnSync in both runOpenshellCommand and captureOpenshellCommand. This makes the 15s timeout on downloadSandboxConfig (added in the previous commit) actually take effect, preventing nemoclaw status from hanging when openshell sandbox download stalls.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/openshell.ts`:
- Around line 91-94: The code currently routes any spawnSync errors to
handleSpawnError(binary, args, result.error, opts), which exits on timeout;
update the logic around the spawnSync result check (where result, binary, args,
opts are used) to detect timeout-specific cases (result.error.code ===
"ETIMEDOUT" and result.status === null) before calling handleSpawnError and
instead return a recoverable failure object (e.g., success: false with a
timedOut flag and captured stdout/stderr) so callers can handle recovery; apply
the same timeout-special-case check and return behavior in the second spawnSync
error branch around the later check (the other block that currently calls
handleSpawnError).
🪄 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: a41c8466-121d-4fcb-abc9-4b73b35b545a
📒 Files selected for processing (1)
src/lib/openshell.ts
| ...(opts.timeout != null && { timeout: opts.timeout }), | ||
| }); | ||
| if (result.error) { | ||
| return handleSpawnError(binary, args, result.error, opts); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js child_process.spawnSync, when a timeout is hit, what fields are populated in the return object (error, status, signal), and is error.codeset toETIMEDOUT?
💡 Result:
When child_process.spawnSync hits a timeout, it kills the child process with the killSignal (default 'SIGTERM'), waits for it to exit, and returns an object with: - error: populated with an Error instance where error.code === 'ETIMEDOUT' - status: null (since terminated by signal) - signal: the kill signal used (e.g., 'SIGTERM') To detect timeout: check if (result.error && result.error.code === 'ETIMEDOUT').
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: https://nodejs.org/api/child%5Fprocess.html
- 3:
timeoutoption's improper behavior sindresorhus/execa#726 - 4: https://stackoverflow.com/questions/70903984/how-to-check-if-nodejs-process-failed-due-to-a-timeout
🏁 Script executed:
# First, check if the file exists and get its size
wc -l src/lib/openshell.tsRepository: NVIDIA/NemoClaw
Length of output: 84
🏁 Script executed:
# Read the relevant sections around lines 91-94 and 116-119
sed -n '85,105p' src/lib/openshell.tsRepository: NVIDIA/NemoClaw
Length of output: 756
🏁 Script executed:
# Also read the section around line 116-119
sed -n '110,130p' src/lib/openshell.tsRepository: NVIDIA/NemoClaw
Length of output: 704
🏁 Script executed:
# Find and read the handleSpawnError function
rg -A 10 "function handleSpawnError" src/lib/openshell.tsRepository: NVIDIA/NemoClaw
Length of output: 376
🏁 Script executed:
# Check git history to see if this is new timeout handling
git log --oneline -n 20 -- src/lib/openshell.tsRepository: NVIDIA/NemoClaw
Length of output: 142
🏁 Script executed:
# Search for any timeout-related tests or comments
rg -i "timeout" src/lib/openshell.tsRepository: NVIDIA/NemoClaw
Length of output: 199
🏁 Script executed:
# Look for any tests related to openshell or spawn errors
fd -e "test\.(ts|js)" -o -e "spec\.(ts|js)" | xargs rg -l "openshell|spawnSync" 2>/dev/null | head -5Repository: NVIDIA/NemoClaw
Length of output: 299
🏁 Script executed:
# Check the actual commit that added timeout handling
git show a6ae606 --statRepository: NVIDIA/NemoClaw
Length of output: 35863
🏁 Script executed:
# Look at the full diff of the timeout commit
git show a6ae606Repository: NVIDIA/NemoClaw
Length of output: 50375
🏁 Script executed:
# Check if there are timeout-related tests
cat src/lib/openshell.test.ts | grep -A 10 -B 2 "timeout\|ETIMEDOUT"Repository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Also check the full test file to understand test coverage
wc -l src/lib/openshell.test.ts
head -50 src/lib/openshell.test.tsRepository: NVIDIA/NemoClaw
Length of output: 1301
🏁 Script executed:
# Read the entire test file to check for timeout tests
cat src/lib/openshell.test.tsRepository: NVIDIA/NemoClaw
Length of output: 4468
Timeout errors still cause hard exit instead of returning failed status for recovery.
When spawnSync times out, it populates result.error with code === "ETIMEDOUT" and status === null. Currently, lines 93 and 118 route all result.error cases to handleSpawnError(), which exits with status 1. This bypasses any recovery flow the caller might implement.
The timeout parameter was added to prevent nemoclaw status from hanging indefinitely, but the current code exits immediately on timeout instead of allowing the caller to handle it. Add a timeout-specific check before calling handleSpawnError():
Suggested fix
+function isSpawnTimeoutError(error: Error): boolean {
+ return (error as NodeJS.ErrnoException).code === "ETIMEDOUT";
+}
+
export function runOpenshellCommand(
binary: string,
args: string[],
opts: RunOpenshellOptions = {},
): SpawnSyncReturns<string> {
@@
});
if (result.error) {
+ if (isSpawnTimeoutError(result.error)) {
+ const timedOutResult: SpawnSyncReturns<string> = {
+ ...result,
+ error: undefined,
+ status: result.status ?? 124,
+ };
+ if (timedOutResult.status !== 0 && !opts.ignoreError) {
+ (opts.errorLine ?? console.error)(
+ ` Command timed out: openshell ${args.join(" ")}`,
+ );
+ return (opts.exit ?? ((code) => process.exit(code)))(timedOutResult.status);
+ }
+ return timedOutResult;
+ }
return handleSpawnError(binary, args, result.error, opts);
}
@@
export function captureOpenshellCommand(
binary: string,
args: string[],
opts: CaptureOpenshellOptions = {},
): CaptureOpenshellResult {
@@
});
if (result.error) {
+ if (isSpawnTimeoutError(result.error)) {
+ return {
+ status: result.status ?? 124,
+ output: `${result.stdout || ""}${opts.ignoreError ? "" : result.stderr || ""}`.trim(),
+ };
+ }
return handleSpawnError(binary, args, result.error, opts);
}Also applies to: 116-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/openshell.ts` around lines 91 - 94, The code currently routes any
spawnSync errors to handleSpawnError(binary, args, result.error, opts), which
exits on timeout; update the logic around the spawnSync result check (where
result, binary, args, opts are used) to detect timeout-specific cases
(result.error.code === "ETIMEDOUT" and result.status === null) before calling
handleSpawnError and instead return a recoverable failure object (e.g., success:
false with a timedOut flag and captured stdout/stderr) so callers can handle
recovery; apply the same timeout-special-case check and return behavior in the
second spawnSync error branch around the later check (the other block that
currently calls handleSpawnError).
Temporary instrumentation — will be removed once we identify which openshell call hangs in the E2E environment.
Write diag markers to /tmp/nemoclaw-status-diag.log (survives subshell capture). Add 60s timeout to the nemoclaw status call in test-sandbox-survival so the test prints the diag log and fails fast instead of waiting for the job timeout.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/nemoclaw.ts (1)
1639-1642: Diagnostic helper looks reasonable; consider cleanup in test harness.The implementation is appropriately defensive (empty catch prevents diagnostics from breaking the main flow). Note that this log file will accumulate entries across invocations. The test already handles this by resetting the file before each run (
rm -f /tmp/nemoclaw-status-diag.log), which is the right approach for E2E diagnostics.For production, you may eventually want to either:
- Truncate/rotate on
sandboxStatusentry, or- Gate behind a
NEMOCLAW_DEBUGenv varLow priority since this is diagnostic instrumentation for debugging the specific hang issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 1639 - 1642, The _diag diagnostic currently always appends to /tmp/nemoclaw-status-diag.log; update it to avoid unbounded growth by gating writes behind an environment flag and/or truncating on key events: modify function _diag to check process.env.NEMOCLAW_DEBUG (only append when truthy) and preserve the defensive empty catch, and additionally implement logic inside _diag (or a small helper called by it) to truncate the log file the first time a "sandboxStatus" message is logged (or rotate/rename existing file) so tests can still rm -f but production won’t grow unbounded; keep references to the same function name (_diag) and preserve the timestamped line format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/test-sandbox-survival.sh`:
- Around line 285-293: The snippet uses the external timeout binary directly
(timeout 60 ...) which is not portable on macOS; update the call to use the
previously-detected timeout wrapper (the TIMEOUT_STATUS/TIMEOUT_CMD pattern used
later) or introduce a small helper (e.g., get_timeout_cmd or TIMEOUT_CMD
variable) that selects between timeout and gtimeout and then invoke it with the
60s argument (e.g., "$TIMEOUT_CMD" 60 nemoclaw "$SANDBOX_NAME" status) so the
same portability check logic as lines that set TIMEOUT_STATUS/TIMEOUT_CMD is
reused.
---
Nitpick comments:
In `@src/nemoclaw.ts`:
- Around line 1639-1642: The _diag diagnostic currently always appends to
/tmp/nemoclaw-status-diag.log; update it to avoid unbounded growth by gating
writes behind an environment flag and/or truncating on key events: modify
function _diag to check process.env.NEMOCLAW_DEBUG (only append when truthy) and
preserve the defensive empty catch, and additionally implement logic inside
_diag (or a small helper called by it) to truncate the log file the first time a
"sandboxStatus" message is logged (or rotate/rename existing file) so tests can
still rm -f but production won’t grow unbounded; keep references to the same
function name (_diag) and preserve the timestamped line format.
🪄 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: 84348731-a217-4e41-b188-af7e9f41b618
📒 Files selected for processing (2)
src/nemoclaw.tstest/e2e/test-sandbox-survival.sh
| rm -f /tmp/nemoclaw-status-diag.log | ||
| if status_output=$(timeout 60 nemoclaw "$SANDBOX_NAME" status 2>&1); then | ||
| pass "nemoclaw $SANDBOX_NAME status exits 0" | ||
| else | ||
| echo "[diag] nemoclaw status exit code: $?" | ||
| echo "[diag] status output: ${status_output:0:500}" | ||
| echo "[diag] diag log:" | ||
| cat /tmp/nemoclaw-status-diag.log 2>/dev/null || echo "(no diag log)" | ||
| fail "nemoclaw $SANDBOX_NAME status failed: ${status_output:0:200}" |
There was a problem hiding this comment.
Portability issue: timeout command used without availability check.
Line 286 uses timeout directly, but on macOS the command is gtimeout (from coreutils). Later in this same file (lines 534-536), there's a proper portability check:
command -v timeout >/dev/null 2>&1 && TIMEOUT_STATUS="timeout 120"
command -v gtimeout >/dev/null 2>&1 && TIMEOUT_STATUS="gtimeout 120"Consider applying the same pattern here for consistency:
Proposed fix for timeout portability
# 3d: nemoclaw status works
rm -f /tmp/nemoclaw-status-diag.log
-if status_output=$(timeout 60 nemoclaw "$SANDBOX_NAME" status 2>&1); then
+TIMEOUT_CMD=""
+command -v timeout >/dev/null 2>&1 && TIMEOUT_CMD="timeout 60"
+command -v gtimeout >/dev/null 2>&1 && TIMEOUT_CMD="gtimeout 60"
+if status_output=$($TIMEOUT_CMD nemoclaw "$SANDBOX_NAME" status 2>&1); then
pass "nemoclaw $SANDBOX_NAME status exits 0"Alternatively, extract a helper function since this pattern is used multiple times.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/test-sandbox-survival.sh` around lines 285 - 293, The snippet uses
the external timeout binary directly (timeout 60 ...) which is not portable on
macOS; update the call to use the previously-detected timeout wrapper (the
TIMEOUT_STATUS/TIMEOUT_CMD pattern used later) or introduce a small helper
(e.g., get_timeout_cmd or TIMEOUT_CMD variable) that selects between timeout and
gtimeout and then invoke it with the 60s argument (e.g., "$TIMEOUT_CMD" 60
nemoclaw "$SANDBOX_NAME" status) so the same portability check logic as lines
that set TIMEOUT_STATUS/TIMEOUT_CMD is reused.
3 participants
Summary
openshell sandbox downloadcall indownloadSandboxConfig(used byverifyDashboardChainCORS check)nemoclaw statusfrom hanging indefinitely when sandbox SSH stallsRoot cause
PR #2398 added
verifyDashboardChain()to thenemoclaw status→checkAndRecoverSandboxProcesses()→recoverDashboardChain()path. The CORS verification callsdownloadSandboxConfig()which runsopenshell sandbox download(SSH into sandbox) with no timeout. The old recovery path never downloadedopenclaw.json.When sandbox SSH is slow (common in CI right after creation), the download blocks indefinitely →
nemoclaw statushangs → E2E tests hit their job timeout (exit 124).The recovery chain calls
verifyDashboardChaintwice (before and after recovery), so there are two unbounded SSH calls per status check.Fix
Add
timeout: 15000(15s, matchingexecuteSandboxCommand) to therunOpenshellcall. On timeout, the download returns non-zero →downloadSandboxConfigreturnsnull→ CORS reports "could not download openclaw.json" → recovery continues without hanging.Bisect evidence
de97a00d(Apr 24 16:06)9fbfbaca(#2398 only)b804db09(#2398 + #2408)f7dff7b4(4 commits incl #2398)f41f5ec4(7 commits incl #2398)79c8e2a9(Apr 25 00:10)Test plan
npx tsc -p tsconfig.src.json --noEmitpassesSummary by CodeRabbit
Bug Fixes
New Features
Tests
Chores