fix(onboard): recover --resume when sandbox container already destroyed#4761
Closed
jason-ma-nv wants to merge 1 commit into
Closed
fix(onboard): recover --resume when sandbox container already destroyed#4761jason-ma-nv wants to merge 1 commit into
jason-ma-nv wants to merge 1 commit into
Conversation
…ed (#4757) A non-atomic update/rebuild can destroy a sandbox's container before `onboard --resume` runs. The pre-recreate state backup pulls state out of the live container over SSH, so with the container already gone the backup returns empty and onboard aborted with "State backup failed — aborting recreate to prevent data loss", forcing the operator to discover NEMOCLAW_RECREATE_WITHOUT_BACKUP=1 to recover. Teach backupSandboxBeforeRecreate a tri-state container presence: when the sandbox is confirmed gone there is nothing left to back up, so it short-circuits (failureKind "no-container") and lets recreate proceed. Presence is derived from the already-computed reuse state — only "missing" (OpenShell `sandbox get` returns NotFound) is treated as absent; a transport error or stale-Ready entry stays "unknown" and keeps the fail-safe abort so a transient gateway blip never destroys live state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jason Ma <jama@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe PR adds container presence detection to the sandbox pre-recreate backup flow, allowing ChangesContainer Presence & Backup Skip for Recreate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
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 |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@src/lib/onboard.ts`:
- Around line 3242-3248: The added explanatory comment increases file length;
keep behavior but shrink lines by moving or condensing the rationale: replace
the multi-line comment above the backupSandboxBeforeRecreate call with a single
short inline comment (e.g., "// Skip backup if sandbox already removed") and/or
move the detailed rationale into the onboarding backup module or tests; leave
the call to backupSandboxBeforeRecreate({ sandboxName, containerPresence:
containerPresenceFromReuseState(existingSandboxState) }) and the
existingSandboxState/sandboxName references unchanged so functionality is
preserved.
🪄 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: 1c0389c9-2c87-406d-8f98-c6462091eecd
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/onboard/sandbox-backup-on-recreate.test.tssrc/lib/onboard/sandbox-backup-on-recreate.ts
Comment on lines
+3242
to
+3248
| // #4757: if the sandbox is already gone (e.g. a non-atomic update | ||
| // destroyed the container before `onboard --resume` ran), there is | ||
| // nothing to back up — skip rather than abort so recovery can proceed. | ||
| const result = backupSandboxBeforeRecreate({ | ||
| sandboxName, | ||
| containerPresence: containerPresenceFromReuseState(existingSandboxState), | ||
| }); |
Contributor
There was a problem hiding this comment.
Reduce net line growth in src/lib/onboard.ts to unblock CI.
This change is functionally good, but CI is currently blocked by the codebase growth guardrail on this file (+7 net lines). Please keep the behavior and trim/move lines (e.g., condense the inline rationale or move extra rationale to the onboarding backup module/tests) so this file no longer grows.
Minimal trim suggestion in this hunk
- // `#4757`: if the sandbox is already gone (e.g. a non-atomic update
- // destroyed the container before `onboard --resume` ran), there is
- // nothing to back up — skip rather than abort so recovery can proceed.
const result = backupSandboxBeforeRecreate({
sandboxName,
- containerPresence: containerPresenceFromReuseState(existingSandboxState),
+ containerPresence: containerPresenceFromReuseState(existingSandboxState), // `#4757`: confirmed-missing sandbox skips backup
});🤖 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/onboard.ts` around lines 3242 - 3248, The added explanatory comment
increases file length; keep behavior but shrink lines by moving or condensing
the rationale: replace the multi-line comment above the
backupSandboxBeforeRecreate call with a single short inline comment (e.g., "//
Skip backup if sandbox already removed") and/or move the detailed rationale into
the onboarding backup module or tests; leave the call to
backupSandboxBeforeRecreate({ sandboxName, containerPresence:
containerPresenceFromReuseState(existingSandboxState) }) and the
existingSandboxState/sandboxName references unchanged so functionality is
preserved.
Contributor
PR Review AdvisorFindings: 1 needs attention, 1 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
jason-ma-nv
marked this pull request as draft
June 4, 2026 07:22
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
wscurran
added
area: sandbox
Contributor
|
✨ Thanks for submitting this detailed PR about fixing the onboard --resume issue when a sandbox container is already destroyed, which improves the robustness of the recreate process. This proposes a bug fix for the onboard flow that affects sandbox management and error handling. Related open issues: |
Collaborator
Author
|
Closing — the target issue #4757 is already resolved. #4757 was closed as completed: QA re-verified on NemoClaw v0.1.0 (DGX Spark) that the recovery flow was redesigned upstream — the chicken-and-egg "State backup failed" dead-end is gone and the recreate command ( Since the underlying problem this PR targeted is fixed in the latest release by a different (broader) redesign, this change is superseded and no longer needed. Closing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nemoclaw onboard --resumecould not recover a sandbox whose container was already destroyed (e.g. by a non-atomic update/rebuild): the pre-recreate state backup pulls state out of the live container over SSH, so with the container gone the backup returned empty and onboard aborted withState backup failed — aborting recreate to prevent data loss, forcing the operator to discoverNEMOCLAW_RECREATE_WITHOUT_BACKUP=1. This teaches the backup step that a confirmed-gone sandbox has nothing to back up, so recovery proceeds automatically.Related Issue
Fixes #4757
Changes
src/lib/onboard/sandbox-backup-on-recreate.ts: add a tri-statecontainerPresenceoption tobackupSandboxBeforeRecreate; when the container is"absent"it short-circuits with a newfailureKind: "no-container"(logs "already gone — nothing to back up; proceeding with recreate") instead of aborting. AddcontainerPresenceFromReuseState()mapping helper.src/lib/onboard.ts: passcontainerPresenceFromReuseState(existingSandboxState)into the pre-recreate backup so a confirmed-missing sandbox skips backup and recreate proceeds.sandbox get→ gRPCNotFound, surfaced bygetSandboxStateFromOutputsas"missing") maps to"absent". A gateway transport error or a staleReadyentry stays"unknown"and keeps the fail-safe abort, so a transient gateway blip can never destroy live state."unknown"fail-safe path, and all three branches of the presence mapping.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Jason Ma jama@nvidia.com
Summary by CodeRabbit