fix(sandbox): wait for readiness before startup recovery - #8990
Conversation
Starting a stopped sandbox recovered in-sandbox processes and host forwards before OpenShell had re-registered it, so recovery raced cold registration and the command failed while a following recover succeeded. Startup now waits for the sandbox to reach the Ready phase before it recovers, on the same budget the probe-only path uses. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
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:
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSandbox startup now waits for OpenShell ChangesSandbox readiness recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change makes sandbox startup wait for readiness before recovery, with targeted tests and type checks passing; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Docker
participant StartupRecovery
participant OpenShell
participant AgentProcesses
participant GatewayProbe
Docker->>StartupRecovery: Start sandbox container
StartupRecovery->>OpenShell: Wait for Ready
StartupRecovery->>AgentProcesses: Recover process state
StartupRecovery->>GatewayProbe: Probe gateway and host forwards
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-8990.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Maintainer security and cross-issue review for Security review:
Cross-issue sweep:
The automated fingerprint search found no additional candidates. The sweep found no adjacent fix or conflicting requested behavior. |
Maintainer security review — PASSReviewed exact head The change is safe from a security perspective. It reuses the existing owner-pinned OpenShell readiness check and places it after locked startup access is restored but before in-sandbox process or host-forward recovery. A readiness failure terminates before those mutations, and the existing final gateway, forward, and inference checks remain in place. No security findings.
Validation: Files reviewed: |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Summary
Starting a stopped sandbox recovered in-sandbox processes and host forwards before OpenShell had re-registered the sandbox, so recovery raced cold registration.
startreported a recovery failure and left the primary dashboard and API host forward unstarted, while therecoverthe operator then ran succeeded because it waits for readiness first.startnow waits for the sandbox to reach theReadyorRunningstate before it recovers, using the same budget andNEMOCLAW_CONNECT_TIMEOUToverride asconnect --probe-only.Related Issue
Fixes #8978
Changes
src/lib/actions/sandbox/start.ts:restoreStoppedSandboxStartupStatewaits for theReadyorRunningstate between the locked-access restore and process recovery. The locked-access restore stays ahead of the wait because sealed state blocks the container's own startup; it runs over the direct container, so it does not depend on OpenShell re-registration. The new step is injectable through the existingSandboxStartupStateDepsseam, which the three added tests use.src/lib/actions/sandbox/connect.ts: exportwaitForSandboxReadyOrExitand addSANDBOX_REPAIR_READY_TIMEOUT_SEC, replacing the literal300thatconnect --probe-onlyalready used. This is a shared constant rather than a second literal so the two paths that prove the same readiness for the same sandbox cannot drift again; the addedstartordering test protects the contract.src/lib/actions/sandbox/start.test.ts: three tests covering the readiness position for OpenClaw, for a sandbox with no sealed state, and across the wholestartsequence. Each was confirmed to fail without the fix. One existing test is retitled because its old title described the ordering this change corrects.docs/manage-sandboxes/run-sandboxes.mdxanddocs/reference/commands.mdx: document the corrected sequence, the readiness budget, its override, and the timeout result.This restores the ordering established for #7173 by #7192. #8146 introduced process recovery ahead of the readiness wait on the
startpath only, soconnect --probe-onlykept the ordering andstartlost it. The recovery-owned budget inprocess-recovery.tsis deliberately untouched: it also backs thegateway restarthealth wait and the onboarding control-plane wait, and with the ordering corrected the reported failure no longer depends on it.Type of Change
Quality Gates
51b25f270; no findings: fix(sandbox): wait for readiness before startup recovery #8990 (comment)Documentation Writer Review
docs-updateddocs/manage-sandboxes/run-sandboxes.mdxanddocs/reference/commands.mdx. Thestartcommand waits for the OpenShellReadyorRunningstate before agent-specific recovery. Documentation distinguishes gateway-runtime recovery for OpenClaw and Hermes from terminal-runtime verification for Deep Agents Code.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project cli src/lib/actions/sandbox/start.test.ts src/lib/actions/sandbox/process-recovery.test.ts src/lib/actions/sandbox/connect-probe-observe.test.ts— 97 passed.npx vitest run --project integration test/process-recovery.test.ts test/process-recovery-supervisor-relaunch.test.ts test/process-recovery-managed-controller.test.ts test/process-recovery-custom-agent.test.ts test/cli/connect-readiness.test.ts test/cli/connect-recovery-settle.test.ts test/sandbox-stuck-recovery.test.ts test/cli/dispatch-basics.test.ts test/cli/docker-outage.test.ts— 141 passed.npm run typecheck:cliandnpm --prefix nemoclaw run typecheck— 0 errors. The three added tests were each confirmed to fail with the fix reverted.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only) — builds with 0 errors and the 2 warnings already present onmain.Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Documentation