fix(maintenance): skip non-Ready sandboxes in pre-upgrade backup loop - #4256
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a Ready-phase-aware parser ChangesSandbox backup eligibility fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
PR Review AdvisorFindings: 0 needs attention, 0 worth checking, 0 nice ideas This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/runtime-recovery.ts`:
- Line 46: The current regex /^ (NAME|No sandboxes found\.?$)/i.test(line) is
too broad and can match real sandbox names (e.g., "name-prod"); update the check
in parseLiveSandboxNames to use exact whole-line matches like /^NAME$/i and /^No
sandboxes found\.?$/i (or combine as /^(?:NAME|No sandboxes found\.?)$/i) so
only header/empty-result lines are skipped, and apply the same precise regex
change wherever the original pattern is used to ensure consistency.
🪄 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: 247c4f6c-a50d-4748-8a64-59c566482349
📒 Files selected for processing (3)
src/lib/actions/maintenance.tssrc/lib/runtime-recovery.test.tssrc/lib/runtime-recovery.ts
…upgrade-sandboxes Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 26477118771
|
4 participants
Summary
nemoclaw backup-all(run by the installer at phase[2/3]) attempted to back up every registered sandbox regardless of state.parseLiveSandboxNamesreturned every row ofopenshell sandbox list— including rows withPHASE=Errorfor stopped containers — so theif (!liveNames.has(sb.name))skip insrc/lib/actions/maintenance.ts:71never fired for stopped sandboxes. The backup then failed, the installer aborted, and the upgrade was blocked for anyone with a stopped sandbox.nemoclaw upgrade-sandboxeshad the same bug:running=truefor anError-phase sandbox, so a stopped sandbox was offered for in-place rebuild instead of being classified as stopped.Related Issue
Fixes #4233
Changes
src/lib/runtime-recovery.ts— newparseReadySandboxNames(listOutput)that mirrorsparseLiveSandboxNamesbut only includes rows whose last whitespace-delimited column equalsReady. Both parsers now share anisNonSandboxRowhelper that compares the first column to the literal string"NAME"and the full line to the exactNo sandboxes foundnotice, so sandboxes with names likename-prodorno-sandboxesare no longer dropped by the old/^(NAME|No sandboxes found\.?$)/iregex.src/lib/actions/maintenance.ts—backupAllswitches toparseReadySandboxNames, so stopped or errored sandboxes hit theSkipping '<name>' (not running)branch and increment the skipped counter instead of attempting an impossible backup.src/lib/actions/upgrade-sandboxes.ts— sameReady-only swap soclassifyUpgradeableSandboxesrecordsrunning=falsefor non-Ready sandboxes andsplitRebuildableSandboxesroutes them through the stopped path.src/lib/runtime-recovery.test.ts— adds aparseReadySandboxNamessuite (Ready-only filter against a mixedReady/Provisioning/Errorlist, the stopped-containerErrorrow directly,No sandboxes found./Error: …/ protobuf-mismatch lines, and the empty-input fallback) plus a regression case forparseLiveSandboxNamescoveringname-prodandno-sandboxesnames that the old regex skipped.src/lib/actions/gateway-drift-preflight.test.ts— newbackup-all skips sandboxes that are not in Ready phasecase that drivesbackupAllend-to-end with a mixed registry and mixedsandbox listoutput, asserting thatbackupSandboxStateis called for the Ready sandbox only and the skip log fires for theError-phase sandbox.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Improvements
Tests