fix(backup): back up stopped docker sandboxes by starting them for the backup - #6723
Conversation
…e backup A registered docker-driver sandbox whose container is exited or created is backupable: the backup transport is SSH+tar through the container's PID 1 and does not need the agent gateway. backup-all now starts such a container for the duration of the backup and returns it to its stopped state after, so the installer-strict gate (NVIDIA#6114) can pass without weakening what it protects. Containers that are running-but-not-Ready, paused, absent, or fail to start keep the existing skip and remediation message. Addresses NVIDIA#6500 Signed-off-by: Hokonoken <41166525+Hokonoken@users.noreply.github.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:
📝 WalkthroughWalkthroughStopped Docker-driver sandboxes are temporarily started during ChangesStopped Sandbox Backup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant backupAll
participant stoppedSandboxBackup
participant Docker
participant sandboxState
Operator->>backupAll: Run strict backup-all
backupAll->>stoppedSandboxBackup: Start stopped sandbox for backup
stoppedSandboxBackup->>Docker: Start container
backupAll->>stoppedSandboxBackup: Back up started sandbox state
stoppedSandboxBackup->>sandboxState: Retry unreachable backup
backupAll->>stoppedSandboxBackup: Restore stopped state
stoppedSandboxBackup->>Docker: Stop and verify container
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/actions/maintenance.test.ts (1)
204-328: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing test: container returned to stopped when the orphan-manifest catch fires for a started container.
The PR objective explicitly states the container is returned to stopped "including when the backup fails or is skipped due to an orphan manifest," and the
finallyblock's placement inmaintenance.tsrelies on JS semantics (finally runs before acatch'scontinuecompletes) to guarantee this. None of the four new tests exercise a started container whose backup call throws the orphan-manifest error — only the plain-failure and stop-fails paths are covered. Since this interaction depends on subtle control-flow semantics, a dedicated regression test would catch a future refactor that breaks it (e.g., moving thecontinue/returnbefore thefinally, or restructuring the try/catch).🧪 Suggested additional test
it("returns a started container to stopped even when the backup call throws (orphan manifest, `#6500`)", async () => { mocks.listSandboxes.mockReturnValue({ sandboxes: [{ name: "sb-stopped" }], defaultSandbox: null, }); mocks.parseReadySandboxNames.mockReturnValue(new Set()); mocks.startStoppedSandboxContainerForBackup.mockReturnValue({ containerName: "openshell-sb-stopped-abc", }); mocks.backupStartedSandboxState.mockRejectedValue( new Error("Agent 'sb-stopped' not found: /path/to/manifest.yaml"), ); const logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); await backupAll(); expect(mocks.returnSandboxContainerToStopped).toHaveBeenCalledWith("openshell-sb-stopped-abc"); expect(logSpy.mock.calls.flat().join("\n")).toContain("Returned 'sb-stopped' to its stopped state"); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/maintenance.test.ts` around lines 204 - 328, Add a regression test alongside the existing backupAll tests for a started stopped-sandbox whose backupStartedSandboxState call rejects with an orphan-manifest error. Assert backupAll completes, returnSandboxContainerToStopped is called with the started container name, and the logs confirm the sandbox was returned to its stopped state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/manage-sandboxes/backup-restore.mdx`:
- Line 271: Update the sentence describing backup-all’s container behavior to
use the American English spelling “afterward” instead of “afterwards,” without
changing the documented behavior or surrounding wording.
In `@docs/reference/commands.mdx`:
- Line 2500: Update the sentence in the registered docker-driver sandbox
description to use the US spelling “afterward” instead of “afterwards,” matching
the wording in backup-restore.mdx.
---
Nitpick comments:
In `@src/lib/actions/maintenance.test.ts`:
- Around line 204-328: Add a regression test alongside the existing backupAll
tests for a started stopped-sandbox whose backupStartedSandboxState call rejects
with an orphan-manifest error. Assert backupAll completes,
returnSandboxContainerToStopped is called with the started container name, and
the logs confirm the sandbox was returned to its stopped state.
🪄 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: 4db17c3f-8f8e-43af-b11d-0a09d3232c14
📒 Files selected for processing (6)
docs/manage-sandboxes/backup-restore.mdxdocs/reference/commands.mdxsrc/lib/actions/maintenance.test.tssrc/lib/actions/maintenance.tssrc/lib/actions/sandbox/stopped-sandbox-backup.test.tssrc/lib/actions/sandbox/stopped-sandbox-backup.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
E2E Target Results —
|
| Test | Result |
|---|---|
| onboard-repair | |
| onboard-resume | |
| snapshot-commands | |
| state-backup-restore | |
| upgrade-stale-sandbox |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/manage-sandboxes/backup-restore.mdx`:
- Around line 271-273: Update the relevant section heading and introductory
sentence to describe registered sandboxes generally, explicitly including
stopped Docker-driver sandboxes that can be started temporarily; keep the
existing backup behavior details unchanged.
🪄 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: 9523c1a7-31b3-46bc-9fc3-6027fa0c1dc7
📒 Files selected for processing (7)
docs/manage-sandboxes/backup-restore.mdxdocs/reference/commands.mdxsrc/lib/actions/maintenance.test.tssrc/lib/actions/maintenance.tssrc/lib/actions/sandbox/stopped-sandbox-backup.test.tssrc/lib/actions/sandbox/stopped-sandbox-backup.tstest/e2e/live/snapshot-commands.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/actions/sandbox/stopped-sandbox-backup.test.ts
- docs/reference/commands.mdx
- src/lib/actions/sandbox/stopped-sandbox-backup.ts
- src/lib/actions/maintenance.ts
E2E Target Results —
|
| Test | Result |
|---|---|
| onboard-repair | |
| onboard-resume | |
| snapshot-commands | |
| state-backup-restore | |
| upgrade-stale-sandbox |
E2E Target Results — ✅ All requested tests passedRun: 29223032339
|
E2E Target Results — ✅ All requested tests passedRun: 29233970615
|
<!-- markdownlint-disable MD041 --> ## Summary Interrupted onboarding can leave a route-only registry reservation before sandbox registration. The installer previously counted that reservation as a real sandbox, requested legacy managed-image provenance, and failed strict backup even though there was no sandbox to back up. This completes the remaining #6500 registry case after #6723 while keeping real and legacy sandboxes fail-closed. ## Related Issue Fixes #6500 ## Changes - Define a route-only reservation narrowly as `pendingRouteReservation: true` without `createdAt`. - Exclude only those reservations from installer counting and provenance checks, strict `backup-all`, automatic sandbox recovery, and the installer's existing-session guard. - Preserve backup and recovery handling for legacy rows and registered sandboxes that temporarily carry the pending marker. - Add pending-only and mixed-registry regressions across registry, installer, backup, recovery, and full installer onboarding paths. ## Type of Change - [x] 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 - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This corrects transient internal registry classification; existing docs already describe backup/recovery for registered sandboxes and direct interrupted onboarding to `nemoclaw onboard --resume`. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent read-only review found no correctness or safety issues; the predicate remains fail-closed for legacy and registered sandbox rows. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project cli src/lib/state/registry-route-reservation.test.ts src/lib/actions/maintenance.test.ts src/lib/actions/upgrade-sandboxes-preflight.test.ts src/lib/actions/upgrade-sandboxes-recovery.test.ts` (67 passed); `npx vitest run --project integration test/install-openshell-upgrade-prompt.test.ts test/install-preexisting-sandbox-recovery.test.ts` (24 passed); `npm run typecheck:cli`; `npm run test:projects:check`; `shellcheck scripts/install.sh`. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: San Dang <sdang@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Route-only sandbox reservations (pending with no creation timestamp) are now excluded from session detection, backups, upgrades, and recovery. * Mixed registries process only fully registered sandboxes, avoiding placeholder entries during preflight and rebuild flows. * Guard logic during OpenShell upgrade now ignores route-only reservations and confirms only actionable backups. * **Tests** * Added/extended coverage for reservation-only and mixed-registry scenarios across installation, backup, upgrade preflight, recovery, and retargeting. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: San Dang <sdang@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Release-prep documentation for v0.0.82 now summarizes user-facing changes merged since v0.0.81. It also closes stale wording in the stopped-sandbox backup, snapshot-clone, Ollama selection, and custom-policy authoring guidance. ## Changes - Add the `v0.0.82` section to `docs/about/release-notes.mdx` with links to the focused user guides. - Document that snapshot clones receive a destination-owned dashboard port before destructive replacement begins. - Align `backup-all` guidance with eligible stopped Docker-driver sandboxes that NemoClaw starts temporarily. - Describe the running and stopped Ollama menu states without claiming one fixed label. - Document runtime rejection of catch-all hosts in custom policy files. ### Source summary - [#6748](#6748) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/lifecycle.mdx`, and `docs/reference/commands.mdx`: Summarize non-destructive sandbox `stop` and `start` commands. - [#6723](#6723) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/backup-restore.mdx`, and `docs/reference/commands.mdx`: Record temporary startup and cleanup for eligible stopped-sandbox backups. - [#6749](#6749) -> `docs/about/release-notes.mdx` and `docs/manage-sandboxes/backup-restore.mdx`: Document destination-owned dashboard ports for snapshot clones. - [#6764](#6764) -> `docs/about/release-notes.mdx`: Summarize installer handling of route-only onboarding placeholders. - [#6771](#6771) -> `docs/about/release-notes.mdx`, `docs/inference/set-up-vllm.mdx`, `docs/inference/choose-inference-provider.mdx`, `docs/reference/commands.mdx`, and `docs/reference/platform-support.mdx`: Summarize managed-vLLM storage gates, immutable image digests, and the explicit override boundary. - [#6759](#6759) -> `docs/about/release-notes.mdx`: Record early, actionable OpenShell gateway-port conflict diagnostics. - [#6753](#6753) -> `docs/about/release-notes.mdx` and `docs/inference/set-up-ollama.mdx`: Document truthful running and stopped Ollama menu states. - [#6776](#6776) -> `docs/about/release-notes.mdx`: Summarize proxy-independent loopback readiness checks. - [#6769](#6769) -> `docs/about/release-notes.mdx`: Record compatible endpoint and agent guidance when Chat Completions is unavailable. - [#6730](#6730) -> `docs/about/release-notes.mdx`: Summarize bounded reuse of an eligible successful Chat Completions check. - [#6768](#6768) -> `docs/about/release-notes.mdx`: Record route-reservation repair during resumed onboarding. - [#6742](#6742) -> `docs/about/release-notes.mdx`: Summarize pre-mutation resolution of secret-free sandbox create intent. - [#6721](#6721) -> `docs/about/release-notes.mdx` and `docs/get-started/quickstart-langchain-deepagents-code.mdx`: Record bounded cleanup of completed managed Deep Agents headless sessions. - [#6731](#6731) -> `docs/about/release-notes.mdx` and `docs/network-policy/customize-network-policy.mdx`: Document runtime rejection of catch-all custom-policy destinations. - [#6729](#6729) -> `docs/about/release-notes.mdx` and `docs/get-started/prerequisites.mdx`: Record the Node.js 22.19 minimum. - [#6735](#6735) -> `docs/about/release-notes.mdx` and `docs/reference/platform-support.mdx`: Summarize the Ubuntu 26.04 userspace contract without claiming pending host or live validation. - [#6775](#6775) -> `docs/about/release-notes.mdx` and `docs/resources/community-contributions.mdx`: Route independent solutions outside canonical supported-product documentation. - [#6740](#6740) -> `docs/about/release-notes.mdx`: Summarize the semantic dependency-upgrade contributor workflow. - [#6777](#6777) -> `docs/about/release-notes.mdx` and `docs/CONTRIBUTING.md`: Summarize the route-safe documentation-refactor workflow. - [#6741](#6741) -> `docs/about/release-notes.mdx` and `docs/security/openclaw-2026.6.10-dependency-review.md`: Summarize reviewed npm archive verification and audit enforcement. - [#6739](#6739) -> `docs/about/release-notes.mdx` and `docs/security/openclaw-2026.6.10-dependency-review.md`: Record the locked offline dependency graph for the managed OpenClaw WeChat runtime. - [#6737](#6737) -> `docs/about/release-notes.mdx`: Record removal of the messaging build plan from final OpenClaw and Hermes image environments. - [#6733](#6733) -> `docs/about/release-notes.mdx`: Summarize cached plugin dependency layers for source and blueprint rebuilds. ### Skipped from docs-skip - None. No commit or changed path in `v0.0.81..origin/main` matched `openclaw-sandbox-permissive.yaml` or `config-show`, and the drafted content contains none of the configured skip terms. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] 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: - [x] Tests not applicable — justification: This is a documentation-only release-prep update; behavior is protected by the merged source PRs, and the documentation build validates the changed routes and agent variants. - [x] 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: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — tests are not applicable for this documentation-only change. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not run for this documentation-only change. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — 0 errors; two pre-existing Fern warnings remain. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) — no new pages. --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated release notes with improvements to sandbox recovery, onboarding, session management, policy validation, storage checks, and system requirements. * Clarified Ollama setup instructions and status labels. * Documented safer snapshot restoration, including dedicated ports and protection against destructive failures. * Expanded `backup-all` coverage to include eligible stopped sandboxes. * Added guidance rejecting broad or catch-all network destinations in custom policies. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Summary
Under installer-strict mode,
backup-allfailed when a registered sandbox was stopped even though its state was still backupable. It now temporarily starts an eligible stopped Docker-driver container, captures the normal snapshot, and returns the container to its stopped state. Before:0 backed up, 0 failed, 1 skippedwith exit 1. After:1 backed up, 0 failed, 0 skippedwith exit 0.Related Issue
Addresses #6500 using the issue's preferred outcome. This keeps the strict gate from #6114 intact: a sandbox still fails strict backup when its container cannot be identified safely, started, backed up, or returned to the stopped state.
Changes
exitedorcreatedstate.unreachablebackup results while the newly started container's SSH endpoint becomes ready.backup-alltoexitedin afinallyblock, including backup failure and orphan-manifest paths.docker stop.Type of Change
Quality Gates
cvrequested; human review remains outstanding.Verification
Verifiedby GitHubnpx vitest run --project cli src/lib/actions/maintenance.test.ts src/lib/actions/sandbox/stopped-sandbox-backup.test.ts-> 2 files, 38 tests passed (21 added by this PR)npm run checks,npm run build:cli, andnpm run typecheck:cliripgrepbootstrap failure; 41 successful PR checks, no pending or failed checksmerge_as_is, high confidence, no findings or warningsLive Verification
E2E run 29223032339 passed every required suite:
onboard-repaironboard-resumestate-backup-restoreupgrade-stale-sandboxsnapshot-commandsThe live
snapshot-commandsartifact proves the exact #6500 path: it stops the uniquely labelled container, runs strictbackup-all, records1 backed up, 0 failed, 0 skipped, verifies the container isexited, restarts it, restores the newly created snapshot, verifies the original marker content, finds zero credential leaks, and completes cleanup without failures.Signed-off-by: Hokonoken 41166525+Hokonoken@users.noreply.github.com