feat(update): agentbox rollback, the undo for an update that succeeded (#676) - #677
Open
lionello wants to merge 1 commit into
Open
feat(update): agentbox rollback, the undo for an update that succeeded (#676)#677lionello wants to merge 1 commit into
agentbox rollback, the undo for an update that succeeded (#676)#677lionello wants to merge 1 commit into
Conversation
…ded (#676) Every rollback path in the updater lives inside the update transaction: a build that fails, a profile switch that fails, an apply that raises, a release that cannot run its own post-switch phase. None of them can see the failure this verb is for - a release that applies cleanly, restarts every unit, returns 0, and is wrong anyway. restart_units() never asks whether the daemons came back, and apply is not a health check with a timeout, so the moment post_switch returns 0 the release is permanent. Until now the only way back was a root shell. Going back is a PROFILE GENERATION, not a git revert: the previous release is already realized in the store and already a GC root, so the undo is rollback_to() plus an apply, with the source tree then reset to whatever rev the restored generation names. It also avoids spending --force, since returning to the rev behind the current one is a downgrade in git's terms and the fast-forward guard would refuse it. The target is the newest generation below the current one that actually HOLDS a release, not simply N-1. cmd_update installs by removing the runtime element and adding it back, and `nix profile remove` makes a generation of its own, so every successful update leaves an empty one directly behind the release it installed. Measured on a live native box: generation 3 held the running release, 2 held `"elements": []`, 1 held the release the box was installed with - so the naive version would have switched that box onto a profile with no runtime and no agentbox to put it back. The same walk skips a generation whose closure the collector took, which is a real limit worth knowing: agent-box-nix-gc.timer runs `--delete-older-than 7d`, so a box that has not updated inside that window has nothing to go back to, and the command refuses rather than half-doing it. Native only, like the mechanism: a NixOS box has no runtime profile to walk, so its equivalent is a system generation and a different unit. Declared as one-sided in both parity checks, owned by #676. Coverage: 15 cases in tests/test_agentbox.py over the happy path (the restored release's own agentbox re-applies, the tree comes back with the profile, --check touches nothing), the refusals (not a generation symlink, nothing behind generation 1, a forward --to-generation, a collected generation, one holding no release) and the partial failures (a tree that cannot be reset is a note and not a failed rollback, a failed apply after the switch is reported, a rollback that will not move re-applies nothing). The empty-generation skip has its own regression test carrying the live measurement. Checks run on aarch64: `python3 tests/test_agentbox.py` (165 tests, OK), check_backend_parity.py, check_one_spec.py, check_vendor.py, test-assemble-module.py, `bin/assemble-module.py --check` (no module drift - nothing under modules/src changed), test-source-tree.sh, test-checkout-bootstrap.sh. tests/native/expected regenerated. The VM lanes are x86_64-only and run in CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017yjGLnvZGzGXqrz9biF3xZ
|
Warning Review limit reachedNext included review available in 18 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
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.
First of the four pieces in #676. Standalone and useful on its own: it closes the gap that has no workaround today.
What is missing
Every rollback path in the updater lives inside the update transaction - a build that fails, a profile switch that fails, an apply that raises, a release that cannot run its own post-switch phase (
cmd_update,post_switch,recover_handover). None of them can see the failure this verb is for: a release that applies cleanly, restarts every unit, returns 0, and is wrong anyway.restart_units()never asks whether the daemons came back, and the code says outright that "apply is not a health check with a timeout" - so the momentpost_switchreturns 0 the release is permanent. Until this, the only way back was a root shell.The verb
Switches the runtime profile back to the previous release, resets the source tree to the rev that generation names, re-applies with THAT release's own agentbox, and restarts the services.
agentbox rollback --checknames the target and touches nothing.Going back is a profile generation, not a git revert. Generation N-1 is already realized in the store and already a GC root, so the undo is
rollback_to()plus an apply. Reverting the tree instead would move the record without moving the closure, which since #242 is backwards - the tree says what the profile was built from, and the profile is what the box runs. It also avoids spending--force: returning to the rev behind the current one is a downgrade in git's terms and the fast-forward guard would refuse it, so an update-based undo would have to skip the one check that stops a replay of an older rev.The bug the live box caught
The target is the newest generation below the current one that actually holds a release, not simply N-1.
cmd_updateinstalls by removing the runtime element and adding it back, andnix profile removemakes a generation of its own - so every successful update leaves an empty generation directly behind the release it installed.Measured on a live native box while writing this (
agent@20-237-183-155.sslip.io, running 770ef4e):The first draft of this trusted N-1 and refused with "generation 2 has no bin/agentbox" against that profile. Had the check not been there, it would have switched the box onto a profile with no runtime and no agentbox to put it back.
holds_a_release()andprevious_release()are that fix, and the walk skips a half-collected generation for the same reason.A real limit worth stating:
agent-box-nix-gc.timerrunsnix-collect-garbage --delete-older-than 7d, which deletes old generations and their closures. A box that has not updated inside that window has nothing to go back to. The command refuses rather than half-doing it, and says why. Piece 3 of #676 (the commit window) should hold its own GC root for the candidate; that is out of scope here.Scope and one-sidedness
Native only, like the mechanism - a NixOS box has no runtime profile to walk, so its equivalent is a system generation (
nixos-rebuild switch --rollback) and a different unit. The capability gap is real and one-sided though, so it is declared inUNITS_KNOWN_GAPSandSUDOERS_KNOWN_GAPSrather than the by-design tables, owned by #676.The sudo grant is implied by the same rule that implies the update trigger, deliberately: an agent that can move this box forward and cannot move it back has the half of the pair that breaks boxes. Both units are root oneshots with a fixed ExecStart.
Coverage
15 cases in
tests/test_agentbox.py:--checkchanges nothing,--to-generationnames an older release exactly.--to-generation, a collected generation, a--to-generationholding no release.Checks run
On aarch64, all passing:
python3 tests/test_agentbox.py(165 tests, OK),scripts/check_backend_parity.py,scripts/check_one_spec.py,scripts/check_vendor.py,tests/test-assemble-module.py,bin/assemble-module.py --check(no module drift - nothing undermodules/srcchanged, sotests/golden/does not move either),tests/test-source-tree.sh,tests/test-checkout-bootstrap.sh.tests/native/expectedregenerated. The VM lanes are x86_64-only and run in CI.One thing found while running these, not fixed here:
tests/test_agentbox.pyfails on any box that has/etc/agent-box/agent-nixpkgs-pin-Specreads that host path, so the renderedAGENT_BOX_NIXPKGSpicks up the live box's resolved channel release instead of the fixture'schannels.nixos.orgURL. It passes in the Nix sandbox and fails for anyone running it on a real box, which is the population most likely to run it while debugging. Two tests,test_matches_committed_fixtureandtest_both_backends_expose_the_same_jit_pin_knob, both red on unmodified master here. Worked around by pointingAGENT_PIN_FILEat a nonexistent path while regenerating; worth its own change.Fixes nothing on its own - #676 stays open for pieces 2 through 4.