Skip to content

fix(cli): add timeout to downloadSandboxConfig in dashboard recovery #2470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ericksoa wants to merge 4 commits into from
+24 −2
Closed

fix(cli): add timeout to downloadSandboxConfig in dashboard recovery #2470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ba471df
fix(cli): add timeout to downloadSandboxConfig in dashboard recovery
ericksoa Apr 25, 2026
a6ae606
fix(cli): wire timeout through runOpenshellCommand/captureOpenshellCo…
ericksoa Apr 25, 2026
5df75c6
diag: add stderr tracing to sandboxStatus to locate hang point
ericksoa Apr 25, 2026
25679a0
diag: write tracing to file and add 60s timeout to E2E status call
ericksoa Apr 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/lib/openshell.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export type OpenshellSpawnSync = (
interface OpenshellSpawnOptions {
cwd?: string;
env?: NodeJS.ProcessEnv;
timeout?: number;
spawnSyncImpl?: OpenshellSpawnSync;
errorLine?: (message: string) => void;
exit?: (code: number) => never;
Expand Down Expand Up @@ -87,6 +88,7 @@ export function runOpenshellCommand(
env: { ...process.env, ...opts.env },
encoding: "utf-8",
stdio: opts.stdio ?? "inherit",
...(opts.timeout != null && { timeout: opts.timeout }),
});
if (result.error) {
return handleSpawnError(binary, args, result.error, opts);
Comment on lines +91 to 94

@coderabbitai coderabbitai Bot Apr 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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:

🏁 Script executed:

# First, check if the file exists and get its size
wc -l src/lib/openshell.ts

Repository: 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.ts

Repository: NVIDIA/NemoClaw

Length of output: 756

🏁 Script executed:

# Also read the section around line 116-119
sed -n '110,130p' src/lib/openshell.ts

Repository: NVIDIA/NemoClaw

Length of output: 704

🏁 Script executed:

# Find and read the handleSpawnError function
rg -A 10 "function handleSpawnError" src/lib/openshell.ts

Repository: 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.ts

Repository: NVIDIA/NemoClaw

Length of output: 142

🏁 Script executed:

# Search for any timeout-related tests or comments
rg -i "timeout" src/lib/openshell.ts

Repository: 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 -5

Repository: NVIDIA/NemoClaw

Length of output: 299

🏁 Script executed:

# Check the actual commit that added timeout handling
git show a6ae606 --stat

Repository: NVIDIA/NemoClaw

Length of output: 35863

🏁 Script executed:

# Look at the full diff of the timeout commit
git show a6ae606

Repository: 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.ts

Repository: NVIDIA/NemoClaw

Length of output: 1301

🏁 Script executed:

# Read the entire test file to check for timeout tests
cat src/lib/openshell.test.ts

Repository: 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).

Expand All @@ -111,6 +113,7 @@ export function captureOpenshellCommand(
env: { ...process.env, ...opts.env },
encoding: "utf-8",
stdio: ["ignore", "pipe", "pipe"],
...(opts.timeout != null && { timeout: opts.timeout }),
});
if (result.error) {
return handleSpawnError(binary, args, result.error, opts);
Expand Down
16 changes: 15 additions & 1 deletion src/nemoclaw.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ function buildDashboardRecoverDeps() {
const destDir = `${tmpDir}${require("path").sep}`;
const dlResult = runOpenshell(
["sandbox", "download", name, "/sandbox/.openclaw/openclaw.json", destDir],
{ ignoreError: true, stdio: ["ignore", "ignore", "ignore"] },
{ ignoreError: true, timeout: 15000, stdio: ["ignore", "ignore", "ignore"] },
);
if (dlResult.status !== 0) return null;
const files: string[] = require("fs").readdirSync(tmpDir, { recursive: true });
Expand Down Expand Up @@ -1636,15 +1636,24 @@ async function sandboxConnect(
}

// eslint-disable-next-line complexity
function _diag(msg: string) {
const line = `${new Date().toISOString()} [nemoclaw-diag] ${msg}\n`;
try { fs.appendFileSync("/tmp/nemoclaw-status-diag.log", line); } catch {}
}

async function sandboxStatus(sandboxName: string) {
_diag("sandboxStatus: start");
const sb = registry.getSandbox(sandboxName);
_diag("sandboxStatus: registry lookup done");
const live = parseGatewayInference(
captureOpenshell(["inference", "get"], { ignoreError: true }).output,
);
_diag("sandboxStatus: inference get done");
const currentModel = (live && live.model) || (sb && sb.model) || "unknown";
const currentProvider = (live && live.provider) || (sb && sb.provider) || "unknown";
const inferenceHealth =
typeof currentProvider === "string" ? probeProviderHealth(currentProvider) : null;
_diag("sandboxStatus: provider health done");
if (sb) {
console.log("");
console.log(` Sandbox: ${sb.name}`);
Expand Down Expand Up @@ -1689,6 +1698,7 @@ async function sandboxStatus(sandboxName: string) {
}

// Agent version check
_diag("sandboxStatus: before checkAgentVersion");
try {
const versionCheck = sandboxVersion.checkAgentVersion(sandboxName);
const agent = agentRuntime.getSessionAgent(sandboxName);
Expand All @@ -1703,8 +1713,10 @@ async function sandboxStatus(sandboxName: string) {
} catch {
/* non-fatal */
}
_diag("sandboxStatus: after checkAgentVersion");
}

_diag("sandboxStatus: before getReconciledSandboxGatewayState");
const lookup = await getReconciledSandboxGatewayState(sandboxName);
if (lookup.state === "present") {
console.log("");
Expand Down Expand Up @@ -1810,8 +1822,10 @@ async function sandboxStatus(sandboxName: string) {
printGatewayLifecycleHint(lookup.output, sandboxName, console.log);
}

_diag("sandboxStatus: after getReconciledSandboxGatewayState, state=" + lookup.state);
// OpenClaw process health inside the sandbox
if (lookup.state === "present") {
_diag("sandboxStatus: before checkAndRecoverSandboxProcesses");
const processCheck = checkAndRecoverSandboxProcesses(sandboxName, { quiet: true });
if (processCheck.checked) {
const _sa = agentRuntime.getSessionAgent(sandboxName);
Expand Down
7 changes: 6 additions & 1 deletion test/e2e/test-sandbox-survival.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,9 +282,14 @@ else
fi

# 3d: nemoclaw status works
if status_output=$(nemoclaw "$SANDBOX_NAME" status 2>&1); then
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}"
Comment on lines +285 to 293

@coderabbitai coderabbitai Bot Apr 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

fi

Expand Down
Loading