fix(backup-all): catch per-sandbox errors to avoid aborting the batch by kagura-agent · Pull Request #5740 · NVIDIA/NemoClaw

kagura-agent · 2026-06-24T12:23:46Z

Summary

When loadAgent() throws for a sandbox whose agent manifest is missing (e.g. an orphan from a previous higher-version install), the exception now gets caught inside the backup loop. The affected sandbox is counted as skipped with a warning, and the remaining sandboxes continue to be backed up normally.

This allows the pre-upgrade backup step to succeed and the installer to proceed — the upgrade itself would re-introduce the missing agent manifest.

Changes

src/lib/actions/maintenance.ts: Wrap backupSandboxState() call in try/catch. On throw, log a warning and increment skipped counter instead of crashing the loop.
src/lib/actions/maintenance.test.ts (new): Unit tests verifying the loop continues after a throw, and the thrown sandbox is counted as skipped (not failed).

Testing

All existing tests pass (vitest run — agent/defs, global actions, snapshot, domain/maintenance)
New tests verify the catch-path behavior
Biome lint clean
TypeScript type-check passes (tsc --noEmit)

Closes #5734

Summary by CodeRabbit

Bug Fixes
- Improved bulk backup handling so one sandbox failure no longer stops the entire process.
- Failed sandboxes are now reported as skipped with a warning, while remaining sandboxes continue backing up.
- Backup summaries now correctly show skipped items and accurate failure counts.
Tests
- Added unit tests covering skip/continue behavior and summary reporting for partial backup failures.

copy-pr-bot · 2026-06-24T12:23:50Z

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.

coderabbitai · 2026-06-24T12:24:21Z

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: 8e1593ee-fd7e-4f55-95cf-9db8cea5fbae

📥 Commits

Reviewing files that changed from the base of the PR and between f535a86 and ea758d5.

📒 Files selected for processing (2)

src/lib/actions/maintenance.test.ts
src/lib/actions/maintenance.ts

🚧 Files skipped from review as they are similar to previous changes (2)

src/lib/actions/maintenance.ts
src/lib/actions/maintenance.test.ts

📝 Walkthrough

Walkthrough

backupAll now catches per-sandbox backup errors, logs them as skipped, and continues processing remaining sandboxes. A new Vitest file covers both continuation after a thrown backup and skipped output for missing-agent errors.

Changes

Per-sandbox error isolation in backupAll

Layer / File(s)	Summary
try/catch in `backupAll` loop and error-isolation tests `src/lib/actions/maintenance.ts`, `src/lib/actions/maintenance.test.ts`	`maintenance.ts` wraps `backupSandboxState(sb.name)` in `try/catch`, logging a `⚠` warning and incrementing `skipped` on thrown errors. `maintenance.test.ts` adds hoisted mocks and two cases covering loop continuation after a per-sandbox throw and `skipped` output for agent-not-found errors.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

area: install

Suggested reviewers

cv
prekshivyas

Poem

🐇 A sandbox sighed and almost fell,
But backup caught it back quite well.
With skipped counts shown and warnings bright,
The rest march on through upgrade night.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: handling per-sandbox backup errors so the batch does not abort.
Linked Issues check	✅ Passed	The code now isolates per-sandbox backup failures, logs them as skipped, continues the loop, and adds tests for the expected behavior.
Out of Scope Changes check	✅ Passed	The changes stay focused on backup-all error isolation and its tests, with no clear unrelated additions.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

wscurran · 2026-06-24T14:56:29Z

✨ Thanks for catching per-sandbox errors in the backup-all loop so a single missing agent manifest no longer aborts the entire batch. This proposes a way to wrap each backupSandboxState() call in try/catch, log a warning, increment a skipped counter, and continue processing remaining sandboxes.

Related open issues:

#5734 [Ubuntu][Upgrade] pre-upgrade backup-all throws on first sandbox with an unknown agent manifest and aborts the install

When loadAgent() throws for a sandbox whose agent manifest is missing (e.g. an orphan from a previous higher-version install), the exception now gets caught inside the backup loop. The affected sandbox is counted as 'skipped' with a warning, and the remaining sandboxes continue to be backed up normally. This allows the pre-upgrade backup step to succeed and the installer to proceed. Closes NVIDIA#5734 Signed-off-by: kagura-agent <kagura.agent.ai@gmail.com>

cjagwani · 2026-06-25T20:29:21Z

Hi @kagura-agent, opened #5819 with a narrower version of this fix. The broad catch in this PR also swallows real backup failures (disk full, SSH timeout, permission denied) and would let the installer march forward with a corrupt or absent backup. #5819 only catches the exact orphan-manifest error pattern from loadAgent and re-throws everything else.

kagura-agent · 2026-06-25T21:04:42Z

Closing in favor of #5819 — the narrower error-handling approach is better. Thanks @cjagwani for the refined fix and @wscurran for the review! 🙏

… failures (#5819) ## Summary Catches the `loadAgent()` orphan-manifest case from #5734 so the pre-upgrade backup loop survives a sandbox whose agent manifest is missing, **without** silently swallowing real failures (disk full, SSH timeout, permission denied, programming bugs) the way #5740's broad catch did. ## Related Issue Closes #5734. **Supersedes #5740**. ## Why a new PR #5740 (kagura-agent) implemented the right outcome with a broad try/catch that swallowed every error and counted it as 'skipped', then exited 0. That lets the installer march forward with an upgrade after a real disk-full or SSH-timeout failure, and the restore path later reads a manifest pointing at a corrupt or absent backup. Data loss scenario. That PR is on a fork I cannot push to, so this is a fresh PR on the NVIDIA upstream so the PR Advisor and full CI run. ## Changes - `src/lib/actions/maintenance.ts`: narrow try/catch on `backupSandboxState()` that only matches the exact orphan-manifest pattern (`Agent '...' not found`) thrown by `loadAgent()` at `src/lib/agent/defs.ts:367`. Anything else re-throws so the installer aborts as it did before any wrapping existed. - `src/lib/actions/maintenance.test.ts`: new test file with 3 cases: - Orphan manifest in one sandbox does not abort the batch - Orphan manifest is counted as `skipped`, not `failed` - **Non-orphan errors (e.g. EACCES) are re-thrown so the installer aborts the upgrade** ## Verification - `npx vitest run src/lib/actions/maintenance.test.ts` — 3/3 pass - The narrow regex `/^Agent '[^']*' not found/` matches the exact error shape from `loadAgent` at `defs.ts:367` ## 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)  ## Summary by CodeRabbit * **Bug Fixes** * Improved batch backup reliability: if a sandbox fails due to a missing/orphan agent manifest (matching the expected “not found: <path>…manifest.yaml” pattern), the batch now logs a warning, marks the sandbox as skipped, updates counters, and continues processing remaining sandboxes. * Non-matching or unrecoverable errors still abort the batch as before. * **Tests** * Expanded maintenance test coverage for partial success/continuation, skipped behavior, and correct abort behavior for non-matching “not found” cases.  --------- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>

… failures (NVIDIA#5819) ## Summary Catches the `loadAgent()` orphan-manifest case from NVIDIA#5734 so the pre-upgrade backup loop survives a sandbox whose agent manifest is missing, **without** silently swallowing real failures (disk full, SSH timeout, permission denied, programming bugs) the way NVIDIA#5740's broad catch did. ## Related Issue Closes NVIDIA#5734. **Supersedes NVIDIA#5740**. ## Why a new PR NVIDIA#5740 (kagura-agent) implemented the right outcome with a broad try/catch that swallowed every error and counted it as 'skipped', then exited 0. That lets the installer march forward with an upgrade after a real disk-full or SSH-timeout failure, and the restore path later reads a manifest pointing at a corrupt or absent backup. Data loss scenario. That PR is on a fork I cannot push to, so this is a fresh PR on the NVIDIA upstream so the PR Advisor and full CI run. ## Changes - `src/lib/actions/maintenance.ts`: narrow try/catch on `backupSandboxState()` that only matches the exact orphan-manifest pattern (`Agent '...' not found`) thrown by `loadAgent()` at `src/lib/agent/defs.ts:367`. Anything else re-throws so the installer aborts as it did before any wrapping existed. - `src/lib/actions/maintenance.test.ts`: new test file with 3 cases: - Orphan manifest in one sandbox does not abort the batch - Orphan manifest is counted as `skipped`, not `failed` - **Non-orphan errors (e.g. EACCES) are re-thrown so the installer aborts the upgrade** ## Verification - `npx vitest run src/lib/actions/maintenance.test.ts` — 3/3 pass - The narrow regex `/^Agent '[^']*' not found/` matches the exact error shape from `loadAgent` at `defs.ts:367` ## 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)  ## Summary by CodeRabbit * **Bug Fixes** * Improved batch backup reliability: if a sandbox fails due to a missing/orphan agent manifest (matching the expected “not found: <path>…manifest.yaml” pattern), the batch now logs a warning, marks the sandbox as skipped, updates counters, and continues processing remaining sandboxes. * Non-matching or unrecoverable errors still abort the batch as before. * **Tests** * Expanded maintenance test coverage for partial success/continuation, skipped behavior, and correct abort behavior for non-matching “not found” cases.  --------- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>

wscurran added area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression labels Jun 24, 2026

wscurran added the v0.0.68 label Jun 24, 2026

cjagwani self-assigned this Jun 24, 2026

jyaunches mentioned this pull request Jun 25, 2026

Epic: Restore Vitest parity for post-migration bash E2E changes #5800

Closed

5 tasks

kagura-agent force-pushed the fix/5734-backup-all-error-handling branch from 6b5685e to ea758d5 Compare June 25, 2026 17:07

jyaunches added v0.0.69 and removed v0.0.68 labels Jun 25, 2026

cjagwani mentioned this pull request Jun 25, 2026

fix(backup-all): catch orphan-manifest errors without swallowing real failures #5819

Merged

4 tasks

kagura-agent closed this Jun 25, 2026

Navigation Menu

Uh oh!

fix(backup-all): catch per-sandbox errors to avoid aborting the batch - #5740

fix(backup-all): catch per-sandbox errors to avoid aborting the batch#5740
kagura-agent wants to merge 1 commit into
NVIDIA:mainfrom
kagura-agent:fix/5734-backup-all-error-handling

kagura-agent commented Jun 24, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

copy-pr-bot Bot commented Jun 24, 2026

Uh oh!

coderabbitai Bot commented Jun 24, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Suggested labels

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

wscurran commented Jun 24, 2026

Uh oh!

cjagwani commented Jun 25, 2026

Uh oh!

kagura-agent commented Jun 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

kagura-agent commented Jun 24, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing

Summary by CodeRabbit

Uh oh!

copy-pr-bot Bot commented Jun 24, 2026

Uh oh!

coderabbitai Bot commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested labels

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

wscurran commented Jun 24, 2026

Uh oh!

cjagwani commented Jun 25, 2026

Uh oh!

kagura-agent commented Jun 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kagura-agent commented Jun 24, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 24, 2026 •

edited

Loading