Skip to content

fix(sandbox): wait for readiness before startup recovery - #8990

Merged
cv merged 6 commits into
mainfrom
fix/start-readiness-before-recovery
Aug 13, 2026
Merged

fix(sandbox): wait for readiness before startup recovery#8990
cv merged 6 commits into
mainfrom
fix/start-readiness-before-recovery

Conversation

@laitingsheng

@laitingsheng laitingsheng commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Starting a stopped sandbox recovered in-sandbox processes and host forwards before OpenShell had re-registered the sandbox, so recovery raced cold registration. start reported a recovery failure and left the primary dashboard and API host forward unstarted, while the recover the operator then ran succeeded because it waits for readiness first. start now waits for the sandbox to reach the Ready or Running state before it recovers, using the same budget and NEMOCLAW_CONNECT_TIMEOUT override as connect --probe-only.

Related Issue

Fixes #8978

Changes

  • src/lib/actions/sandbox/start.ts: restoreStoppedSandboxStartupState waits for the Ready or Running state 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 existing SandboxStartupStateDeps seam, which the three added tests use.
  • src/lib/actions/sandbox/connect.ts: export waitForSandboxReadyOrExit and add SANDBOX_REPAIR_READY_TIMEOUT_SEC, replacing the literal 300 that connect --probe-only already 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 added start ordering 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 whole start sequence. 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.mdx and docs/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 start path only, so connect --probe-only kept the ordering and start lost it. The recovery-owned budget in process-recovery.ts is deliberately untouched: it also backs the gateway restart health wait and the onboarding control-plane wait, and with the ordering corrected the reported failure no longer depends on it.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Maintainer security and cross-issue review passed for commit 51b25f270; no findings: fix(sandbox): wait for readiness before startup recovery #8990 (comment)
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: docs-updated
  • Evidence: docs/manage-sandboxes/run-sandboxes.mdx and docs/reference/commands.mdx. The start command waits for the OpenShell Ready or Running state before agent-specific recovery. Documentation distinguishes gateway-runtime recovery for OpenClaw and Hermes from terminal-runtime verification for Deep Agents Code.
  • Agent: Codex Desktop

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit:
  • Station profile/scenario:
  • Result:
  • Supporting evidence:

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: npx 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:cli and npm --prefix nemoclaw run typecheck — 0 errors. The three added tests were each confirmed to fail with the fix reverted.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only) — builds with 0 errors and the 2 warnings already present on main.
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

  • New Features

    • Sandbox startup now waits for readiness before recovering agent processes and host forwards.
    • Startup and connection commands use a 300-second readiness timeout, preserve existing containers if readiness times out, and report configured retry settings.
    • OpenClaw restores locked startup access before readiness checks when Shields are enabled.
    • Deep Agents verifies the managed terminal runtime before confirming startup success.
  • Documentation

    • Updated sandbox startup and command references to describe readiness checks, timeout behavior, and recovery ordering.

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>
@laitingsheng laitingsheng added area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression labels Aug 13, 2026
@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5886a476-9fae-4487-9a2d-ee4caabefa84

📥 Commits

Reviewing files that changed from the base of the PR and between 821a494 and df90422.

📒 Files selected for processing (2)
  • docs/manage-sandboxes/run-sandboxes.mdx
  • docs/reference/commands.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
  • docs/reference/commands.mdx
  • docs/manage-sandboxes/run-sandboxes.mdx

📝 Walkthrough

Walkthrough

Sandbox startup now waits for OpenShell Ready before recovering process and gateway state. The readiness timeout is centralized and shared with probe-only connect. Tests and documentation cover startup ordering, timeout handling, and agent-specific behavior.

Changes

Sandbox readiness recovery

Layer / File(s) Summary
Readiness timeout and wait helper
src/lib/actions/sandbox/connect.ts, src/lib/actions/sandbox/start.ts
Exports the 300-second readiness timeout and wait helper. Startup uses the helper with the start retry command.
Startup ordering and validation
src/lib/actions/sandbox/start.ts, src/lib/actions/sandbox/start.test.ts
Waits for readiness before process recovery. Tests verify OpenClaw, Hermes, and complete startup ordering.
Command and behavior documentation
docs/reference/commands.mdx, docs/manage-sandboxes/run-sandboxes.mdx, src/lib/actions/sandbox/start.test.ts
Documents readiness waiting, timeout handling, and agent-specific startup behavior. Tests use the configured timeout and validate recovery behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Mergeability Score: ⚪ Minimal · up to df904

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
Loading

Possibly related PRs

Suggested labels: area: cli

Suggested reviewers: cv

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes wait for readiness before recovery, reuse the configured timeout, add ordering tests, and update documentation for issue #8978.
Out of Scope Changes check ✅ Passed The changes remain focused on sandbox startup readiness, recovery ordering, tests, and related documentation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: waiting for sandbox readiness before startup recovery.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/start-readiness-before-recovery

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit df90422 in the fix/start-readiness-... branch remains at 96%, unchanged from commit d6fc6e0 in the main branch.


Updated August 13, 2026 15:39 UTC

@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · medium confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Model comparison: normalized findings match; normalized terminology decisions differ; normalized E2E selections differ; severity counts match.
2 additional E2E selections from the second opinion

Advisory only. The primary lane did not select these E2E jobs or targets.

  • channels-stop-start: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
  • sandbox-operations: The completed second-opinion lane identified E2E coverage that the primary lane omitted.

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

3 semantic terminology decisions

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

  • established — host forwards at docs/manage-sandboxes/run-sandboxes.mdx:137: Keep `host forwards` for host-side port-forward resources.
  • established — readiness budget at src/lib/actions/sandbox/connect.ts:1004: Keep `readiness budget` for the readiness polling timeout.
  • justified — locked startup access at docs/manage-sandboxes/run-sandboxes.mdx:140: Keep `locked startup access`; the modifier distinguishes the Shields-controlled access restoration from general startup recovery.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite for the commit under review.

Recommended E2E: None

Manual-only E2E: onboard-repair, onboard-resume
The manual PR workflow does not run these selectors for the commit under review. Run them from reviewed code on main.

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@cv cv added the v0.0.109 label Aug 13, 2026
@cv

cv commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Maintainer security and cross-issue review for 51b25f270: PASS.

Security review:

  1. Secrets and credentials — PASS. The change adds no credential flow or sensitive output. Readiness diagnostics contain the sandbox name and lifecycle status.
  2. Input validation and data sanitization — PASS. Startup reuses the owner-scoped waitForSandboxReadyOrExit path, argv-based OpenShell calls, phase parsing, and positive-integer timeout validation.
  3. Authentication and authorization — PASS. The change adds no identity or permission decision. OpenClaw locked-access restoration remains before readiness verification.
  4. Dependencies and third-party libraries — PASS. The change adds no dependency or downloaded artifact.
  5. Error handling and logging — PASS. Terminal phases, gateway or Docker outages, a missing sandbox, and timeout remain nonzero results. The diagnostics do not expose credentials.
  6. Cryptography and data protection — PASS. The change does not alter cryptography, transport protection, or protected-data storage.
  7. Configuration and security headers — PASS. Startup reuses NEMOCLAW_CONNECT_TIMEOUT and the existing 300-second repair budget. No security default becomes less restrictive.
  8. Security testing — PASS. Ordering tests cover locked access, OpenShell Ready, process recovery, and final gateway verification. Existing negative tests cover terminal phases and timeouts. The retry test now uses the repository timeout helper under loaded CI shards without changing its assertions.
  9. System security — PASS. lifecycle.start remains before owner-scoped readiness verification. Process recovery follows readiness, and final gateway and host-forward verification remains last. The change adds no control bypass or privilege.

Cross-issue sweep:

  • #8967 is unrelated. It asks recover to start a stopped container. This PR changes start only after lifecycle.start succeeds in src/lib/actions/sandbox/start.ts.
  • #7418 is unrelated. It concerns managed-supervisor probe rollback through relaunch.finalize(false) in src/lib/actions/sandbox/process-recovery.ts, which this PR does not change.
  • #7404 is unrelated. It concerns reboot crash loops and workspace loss during recreation. This PR changes neither restart policy nor backup and recreation logic.

The automated fingerprint search found no additional candidates. The sweep found no adjacent fix or conflicting requested behavior.

@copy-pr-bot

copy-pr-bot Bot commented Aug 13, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@rsliter

rsliter commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Maintainer security review — PASS

Reviewed exact head 821a4944b75d93be1c6ac3b8d9c96be196911475 against base b3b194598c19c97efc0edbff7abf18746380514f.

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.

  1. Secrets and credentials — PASS. No credential flow, logging, storage, or process-argument change.
  2. Input validation and data sanitization — PASS. The only configurable input remains NEMOCLAW_CONNECT_TIMEOUT, validated by the existing positive-integer parser.
  3. Authentication and authorization — PASS. No identity or permission decision changes.
  4. Dependencies and third-party libraries — PASS. No dependency or artifact change.
  5. Error handling and logging — PASS. Readiness, terminal-phase, gateway, and Docker failures still stop recovery and preserve the existing sandbox; retry guidance names the start command.
  6. Cryptography and data protection — PASS. No cryptographic or protected-data surface changes.
  7. Configuration and security controls — PASS. The 300-second repair budget is shared with connect --probe-only; owner-pinned gateway polling and restrictive defaults remain intact.
  8. Security testing — PASS. Tests cover OpenClaw sealed-access ordering, Hermes ordering, and the complete start-to-readiness-to-recovery-to-gateway sequence.
  9. System security — PASS. The state transition is container start → locked-access restore when applicable → owner-pinned Ready observation → process and forward recovery → gateway and inference validation. The change closes the reported recovery race without moving or bypassing an authoritative control.

Validation: git diff --check passed. After building the generated plugin artifact in an isolated exact checkout, the focused CLI suite passed 97 tests across start.test.ts, process-recovery.test.ts, and connect-probe-observe.test.ts.

Files reviewed: docs/manage-sandboxes/run-sandboxes.mdx, docs/reference/commands.mdx, src/lib/actions/sandbox/connect.ts, src/lib/actions/sandbox/start.test.ts, and src/lib/actions/sandbox/start.ts.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv
cv merged commit bce2667 into main Aug 13, 2026
71 of 80 checks passed
@cv
cv deleted the fix/start-readiness-before-recovery branch August 13, 2026 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[All Platforms][Sandbox] sandbox start fails to restore readiness and requires explicit recover

3 participants