fix(state): stream sandbox backup archives - #6947
Conversation
|
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:
📝 WalkthroughWalkthroughThe tar pipeline now supports file-backed archives. Sandbox backups stream SSH tar output to disk, measure the resulting file, extract through shared safety checks, and remove the temporary archive. Snapshot tests verify file-backed stdout and archive-creation failures. ChangesSandbox tar backup pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant backupSandboxState
participant SSH
participant archiveTar as archive.tar
participant safeTarExtract
participant tarProcess as tar process
backupSandboxState->>SSH: request sandbox tar stream
SSH->>archiveTar: write stdout through file descriptor
backupSandboxState->>archiveTar: stat downloaded bytes
backupSandboxState->>safeTarExtract: provide filePath source
safeTarExtract->>tarProcess: validate and extract archive
safeTarExtract-->>backupSandboxState: return extraction result
backupSandboxState->>archiveTar: remove temporary archive
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / medium confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/state/sandbox.ts`:
- Around line 1163-1174: Update backupSandboxState around the openSync call for
downloadedTarPath to catch filesystem errors, including disk-full, quota, and
permission failures, and return the function’s established {success:false,...}
structured failure result. Preserve the existing spawnSync and
descriptor-closing behavior, ensuring the descriptor is only closed after
successful opening and all existing success and subprocess-failure paths remain
unchanged.
- Around line 1162-1163: Move the temporary tar staging used by the
download/extraction flow around downloadedTarPath and downloadedTarFd from
backupPath into a unique directory under os.tmpdir(). Update all reads and
cleanup references to use the staged path, and remove the temporary directory in
the existing finally block.
🪄 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: e9a25f9c-857d-4487-af2a-1b94e2f95347
📒 Files selected for processing (3)
src/lib/state/sandbox.tssrc/lib/state/tar-listing.tstest/snapshot.test.ts
|
✨ Thanks for the fix. Streaming sandbox backup archives directly to a temporary file removes the 256 MiB ceiling that was blocking snapshot, recreate, rebuild, and upgrade protection for long-lived agent state. Ready for maintainer review. Related open issues: Related open issues: |
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
Signed-off-by: Ho Lim <subhoya@gmail.com>
1999bba to
71063cd
Compare
Co-authored-by: Ho Lim <subhoya@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
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 `@test/security-sandbox-tar-traversal.test.ts`:
- Around line 266-289: Extend the postconditions in the safeTarExtract test to
assert that workDir/outside.json does not exist, alongside the existing
escape.txt and outside.txt checks. This ensures the hard-link fixture in the
parameterized cases verifies its specific target path.
🪄 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: fe5fad19-bf4a-4fcd-b297-b2ec168d3d05
📒 Files selected for processing (2)
src/lib/state/sandbox.tstest/security-sandbox-tar-traversal.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/state/sandbox.ts
Co-authored-by: Ho Lim <subhoya@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Ho Lim <subhoya@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approved at exact head d1cd68b. File-backed archive handling preserves traversal, hard-link, symlink, cleanup, and failure safety; the missing outside.json escape assertion is now covered. Focused validation passed 81/81, all ordinary CI/security/advisor lanes are green, and there are zero unresolved threads. The selected credential-bearing fork E2E jobs were recorded as a policy-approved skip without exposing repository secrets. I accept the non-blocking 256 MiB fixture warning because the regression contract directly verifies stdout is redirected to a file descriptor rather than buffered in memory.
<!-- markdownlint-disable MD041 --> ## Summary Add the v0.0.87 changelog entry and align the DGX Station, platform-support, and rebuild documentation with behavior merged since v0.0.86. The Station documentation retains the Deferred support status while recording the two exact factory-image qualification profiles and the post-reboot receipt compatibility fix from #7130. ## Changes - Add the v0.0.87 changelog summary, including the merged Station resume receipt fix, with links to the owning documentation pages. - Document the exact April 2026 Colossus BaseOS and June 2026 AI Developer Tools Station identities, validation boundaries, and permitted host preparation. - Synchronize those Station qualification paths into the canonical platform matrix and generated provider/platform pages. - Document how an OpenClaw rebuild clears stale managed-provider session-model pins after an inference switch. ### Source summary - [#7130](#7130) -> `docs/changelog/2026-07-17.mdx`: Document compatibility with current six-field and legacy three-field Station resume receipts after host preparation. - [#7128](#7128) -> `docs/changelog/2026-07-17.mdx`: Document restart-safe managed DCode startup and required Docker resource limits. - [#7126](#7126) -> `docs/changelog/2026-07-17.mdx`, `docs/get-started/dgx-station-preparation.mdx`, `ci/platform-matrix.json`: Document the two bounded Station factory-image qualification profiles without promoting Deferred support and synchronize the generated platform/provider references. - [#6947](#6947) -> `docs/changelog/2026-07-17.mdx`: Document streaming sandbox backup archive creation. - [#7117](#7117) -> `docs/changelog/2026-07-17.mdx`: Document Hermes post-restore gateway and managed MCP health verification. - [#7109](#7109) -> `docs/changelog/2026-07-17.mdx`, `docs/manage-sandboxes/recover-rebuild-sandboxes.mdx`: Document stale managed session-model pin reconciliation after rebuild. - [#7068](#7068) -> `docs/changelog/2026-07-17.mdx`: Document strict-provider compatibility for Hermes tool schemas. - [#6965](#6965) -> `docs/changelog/2026-07-17.mdx`: Document managed vLLM download storage estimation. - [#7114](#7114) -> `docs/changelog/2026-07-17.mdx`: Document preserved, redacted rebuild diagnostics. ## 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: Documentation-only release-prep update; the changelog, platform-generation contracts, and docs build validate the changed pages and links. - [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 a `Signed-off-by:` line 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 test/generate-platform-docs.test.ts test/station-doc-ownership.test.ts test/changelog-docs.test.ts`: 29 passed; `python3 scripts/generate-platform-docs.py --check`: all generated tables in sync - [ ] 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 - [x] `npm run docs` builds without warnings (doc changes only) - [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) --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added filesystem-aware managed vLLM storage preflight (cold download sizing; interactive vs non-interactive capacity checks). - Improved tool-schema compatibility for strict OpenAI-compatible providers (including Gemini schema handling) using a strict single envelope. - Enhanced sandbox backup creation with streamed archive generation and incremental entry validation. - **Bug Fixes** - Strengthened rebuild/recovery checks with Hermes sandbox health validation and cleanup of stale managed-provider session pins. - Persisted onboarding startup commands with required `nproc`/`nofile` limits across sandbox recreation. - Improved replacement-image rebuild diagnostics with bounded, redacted output handling. - For OpenCLAW “rebuild while preserving state,” stale model/provider pins are cleared when appropriate. - **Documentation** - Expanded DGX Station GB300 no-OTA factory profile/qualification criteria and clarified managed vLLM provider/sandbox constraints. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Summary
Stream sandbox state tar downloads directly into a mode-0600 temporary archive instead of buffering child-process stdout in memory. This removes the fixed 256 MiB transfer ceiling that blocks snapshot, recreate, rebuild, and upgrade protection for long-lived agent state while preserving the existing synchronous API and security validation order.
Related Issue
Closes #6946.
Changes
Verification
npm cinpm run build:clinpm run typechecknpx vitest run test/snapshot.test.ts test/security-sandbox-tar-traversal.test.ts(77 passed)npm run check:diffSigned-off-by: Ho Lim subhoya@gmail.com
Summary by CodeRabbit