fix(sandbox): remove the incomplete snapshot when creation fails - #8211
Conversation
Writing the manifest publishes a snapshot, and it ran before the failed directories and state files were weighed, so a failed create left a listable and restorable capture behind. Pass --keep-failed or set NEMOCLAW_KEEP_FAILED_SNAPSHOT=1 to keep it for diagnosis. Rebuild and backup-all keep their partial-manifest salvage. 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:
📝 WalkthroughWalkthroughSnapshot creation now removes incomplete published snapshots after failed directory or state-file captures. Cleanup verifies that the snapshot directory is absent and reports the path when removal fails. Tests and documentation cover cleanup, listing, lookup, versioning, and error behavior. ChangesFailed snapshot cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SnapshotCommand
participant CleanupHelper
participant SnapshotDirectory
SnapshotCommand->>CleanupHelper: Remove published incomplete snapshot
CleanupHelper->>SnapshotDirectory: Delete and verify directory
SnapshotDirectory-->>CleanupHelper: Removal result
CleanupHelper-->>SnapshotCommand: Cleanup status or recovery path
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 5692bf9 in the TypeScript / code-coverage/cliThe overall coverage in commit 5692bf9 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-8211.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 terminology difference from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
3 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. They do not change the primary assessment or E2E / PR Gate. 1 semantic terminology decisionTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/reference/commands.mdx (1)
2970-2970: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnd the flag description with a period.
Line 2970 adds an imperative sentence without a final period.
As per coding guidelines, “Keep one sentence per line and end every sentence with a period.”🤖 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 `@docs/reference/commands.mdx` at line 2970, Update the --keep-failed entry in the command reference table so its description ends with a period, without changing the documented behavior or environment variable reference.Source: Coding guidelines
🤖 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/reference/commands.mdx`:
- Around line 2978-2982: The failed-snapshot documentation currently claims
incomplete snapshots are always removed; update the failure descriptions to
state that cleanup is attempted, but a cleanup failure leaves the incomplete
snapshot listed and restorable and requires manual removal. Apply this wording
consistently in docs/reference/commands.mdx lines 2978-2982 and
docs/manage-sandboxes/backup-restore.mdx lines 103-107.
In `@src/lib/actions/sandbox/snapshot.ts`:
- Line 105: Update the { kind: "help" } branch in runSandboxSnapshot to include
the --keep-failed option in its action-level usage, matching the keepFailed
field in the create request type. Add or update coverage to verify the default
action usage advertises --keep-failed.
---
Nitpick comments:
In `@docs/reference/commands.mdx`:
- Line 2970: Update the --keep-failed entry in the command reference table so
its description ends with a period, without changing the documented behavior or
environment variable reference.
🪄 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: c3985e57-c3d0-46a0-a1c3-137a703d782f
📒 Files selected for processing (11)
docs/manage-sandboxes/backup-restore.mdxdocs/reference/commands.mdxsrc/commands/sandbox/snapshot.test.tssrc/commands/sandbox/snapshot/create.tssrc/lib/actions/sandbox/snapshot-failed-create-cleanup.test.tssrc/lib/actions/sandbox/snapshot.test.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/cli/public-display-defaults.tssrc/lib/state/sandbox-incomplete-snapshot-removal.test.tssrc/lib/state/sandbox.tstest/snapshot.test.ts
💤 Files with no reviewable changes (1)
- src/lib/actions/sandbox/snapshot.test.ts
laitingsheng
added
area: cli
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Requesting changes on product scope and exact behavior before this can be approved.
Issue #8201 establishes one accepted outcome: a failed snapshot is removed so it is no longer listed or restorable. This PR also adds a new public --keep-failed flag and NEMOCLAW_KEEP_FAILED_SNAPSHOT contract that deliberately preserves a listable, restorable incomplete snapshot. That supported diagnostic surface is not established by the linked issue or another accepted design decision. Please either remove the flag/environment-variable path and keep the fix scoped to fail-closed cleanup, or link an accepted maintainer decision that defines its ownership, lifecycle, security expectations, and validation.
The two unresolved CodeRabbit findings are valid as written if the retention surface remains: action-level help omits the new flag, and the docs do not describe cleanup failure consistently. The documentation-writer receipt is also still blocked; refresh it after the final behavior is settled.
I am withholding the sensitive-path approval receipt until the product-scope decision is resolved.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/actions/sandbox/snapshot-failed-create-cleanup.test.ts`:
- Around line 71-82: Update the beforeEach setup in the snapshot cleanup tests
to explicitly set NEMOCLAW_KEEP_FAILED_SNAPSHOT to an empty value before each
test. Preserve individual tests’ ability to override the variable, and leave the
existing mock clearing and teardown behavior unchanged.
🪄 Autofix
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: f1c06a5f-fe4a-4340-bc41-69c7c1203b44
📒 Files selected for processing (11)
docs/manage-sandboxes/backup-restore.mdxdocs/reference/commands.mdxsrc/commands/sandbox/snapshot.test.tssrc/commands/sandbox/snapshot/create.tssrc/lib/actions/sandbox/snapshot-failed-create-cleanup.test.tssrc/lib/actions/sandbox/snapshot.test.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/cli/public-display-defaults.tssrc/lib/state/sandbox-incomplete-snapshot-removal.test.tssrc/lib/state/sandbox.tstest/snapshot.test.ts
💤 Files with no reviewable changes (1)
- src/lib/actions/sandbox/snapshot.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- test/snapshot.test.ts
- docs/manage-sandboxes/backup-restore.mdx
- src/commands/sandbox/snapshot.test.ts
- src/lib/state/sandbox-incomplete-snapshot-removal.test.ts
- src/lib/cli/public-display-defaults.ts
- src/lib/actions/sandbox/snapshot.ts
- src/commands/sandbox/snapshot/create.ts
- src/lib/state/sandbox.ts
- docs/reference/commands.mdx
Issue #8201 establishes removal of a failed snapshot as the accepted outcome, and no accepted decision covers a supported flag and variable that keep an incomplete snapshot listed and restorable. Cleanup is now unconditional, and the docs describe the removal-failure path instead. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Security review — PASSReviewed revision No security findings were identified.
Validation:
|
|
The requested scope change is addressed.
The PR is now scoped to the accepted #8201 outcome and is ready for follow-up review after the new CI run completes. |
|
All current required checks are green after the branch refresh, including the full CLI shard set, aggregate CLI tests, type/build checks, E2E gate, CodeQL, documentation receipt, DCO, and commit verification. The documentation writer reviewed revision GitHub still records the earlier change-request decision from before the unsupported retention option was removed. That independent follow-up review is the only remaining merge gate. |
|
The new CLI shard-8 failure is a current- Current I am not rerunning this deterministic failure. Once the base fix merges, this branch can refresh and run a clean gate. |
The requested product-scope and documentation changes were addressed by later commits: the unsupported retention surface was removed, cleanup recovery guidance was clarified, the documentation receipt was refreshed, and no review threads remain. Current CI status remains an independent merge gate.
5 participants
Summary
A
snapshot createthat could not capture a state directory or file reported the failed items and exited nonzero, but left the incomplete snapshot on disk and listed bysnapshot listas a normal versioned entry, so a later restore could select a capture that never completed. Writing the manifest is what publishes a snapshot, and it ran before the failed items were weighed. The command now removes the incomplete snapshot, and reports the path that is still listed when the removal itself fails.Related Issue
Fixes #8201.
Changes
snapshot createfailure path insrc/lib/actions/sandbox/snapshot.ts, covering failed directories and failed state files. When removal itself fails, the command names the path that is still listed instead of claiming the snapshot was removed.removeIncompleteSnapshottosrc/lib/state/sandbox.ts, next to the credential-sanitization and publication-validation paths that already remove an unpublished backup. It reports whether the snapshot is off disk rather than throwing, so the command can report an unremovable snapshot and still exit on the original failure.docs/reference/commands.mdxanddocs/manage-sandboxes/backup-restore.mdx: a removed snapshot is no longer listed or restorable, and a snapshot that could not be removed is named for manual cleanup before the next restore.src/lib/actions/sandbox/snapshot.test.tswithin the test-file size budget.backupSandboxStateis unchanged.rebuild --forcedeliberately keeps a partial manifest so it can restore what was salvageable, andbackup-allshares that source, so the cleanup belongs at thesnapshot createcall site rather than at the capture.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/manage-sandboxes/backup-restore.mdxanddocs/reference/commands.mdxnow state that a cleanup failure leaves the incomplete capture selectable and requires manual directory removal before restore.npm run docs:sync-agent-variantspassed;npm run docspassed with 0 errors and 2 existing warnings; route and diff checks passed.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 unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation