fix(update): roll back when the new release cannot run its post-switch phase - #586
fix(update): roll back when the new release cannot run its post-switch phase#586defangdevs wants to merge 1 commit into
Conversation
…h phase An update switches the profile, then hands over to the agentbox it just installed to render the host configuration and restart onto it. That child rolls back its own failures — but it IS the new release's code, and a release can fail before one line of it runs: argparse exits 2 on an argument this release no longer takes, an import fails, the interpreter or the file is not there. Phase one returned that exit status and stopped. The profile stayed switched, so the box was left running a release that cannot run its own updater, with no way back short of hand-driving `nix profile rollback` twice (the remove-then-install pair means one rollback lands on the empty intermediate generation). Exit status cannot tell "phase two rolled back" from "phase two never ran" — post_switch returns 1, argparse returns 2, neither is reserved. So ask the profile instead: if it is still ahead of where phase one found it, no rollback happened and phase one owns it. Asking is safe in both directions because rollback_to() stops the moment the profile is at or behind its target, so phase two's own rollback is never repeated and the profile is never walked past the release the box was running. An OSError from the exec is folded in as exit 127: FileNotFoundError is neither ConfigError nor UpdateError, so it escaped main() as a traceback with the profile left switched. Co-Authored-By: Claude Opus 5 <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughSelf-update handover now recovers from child launch and execution failures. Recovery checks the active generation, rolls back and reapplies the previous release when needed, restarts services, and reports the original or recovery failure status. Regression tests cover these paths. ChangesSelf-update handover recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The update flow now recovers failed handovers while avoiding duplicate rollback and preserving restart settings. Covered failure and success paths show no remaining merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant UpdateHandover
participant NewAgentbox
participant recover_handover
participant ProfileAndServices
UpdateHandover->>NewAgentbox: Launch post-switch handover
NewAgentbox-->>UpdateHandover: Return exit status or OSError
UpdateHandover->>recover_handover: Report failure
recover_handover->>ProfileAndServices: Inspect active generation
ProfileAndServices-->>recover_handover: Return generation state
recover_handover->>ProfileAndServices: Roll back, reapply release, restart services
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 39.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Local verification note, promised in the unchecked box in the description.
Control, run against unmodified master so the rev is visible in the trace: An aarch64 host cannot evaluate the x86 VM driver, so the attribute the check Worth flagging separately: What is verified:
A local |
|
Closing the loop on the local It was killed twice with no output (evaluating the x86 VM driver from an
So every |
1 participant
Symptom
An update that switches the profile and then cannot run the new release's
agentboxleaves the box on that release, with nothing rolled back. Thebox is then running software that cannot run its own updater, and the only
way out is to hand-drive
nix profile rollback— twice, because theremove-then-install pair means one rollback lands on the empty intermediate
generation rather than on the previous release.
Why the existing rollback misses it
cmd_updateends by handing over to the agentbox it just installed:post_switchdoes roll back its own failures. But it is the new release'scode, and a release can fail before one line of it runs:
argparseexits 2 on an argument this release no longer takes — and itexits before calling the handler that would have rolled back. Renaming or
dropping
--post-switchor--from-generationfails exactly here, which isthe change someone working on the updater itself is most likely to make.
FileNotFoundErroris neitherConfigErrornor
UpdateError, so it escapedmain()as a traceback, still with theprofile switched.
The build succeeding does not rule these out: the flake8 gate catches a syntax
error, not a renamed flag or a runtime import.
The fix
Exit status cannot distinguish "phase two rolled back" from "phase two never
ran" —
post_switchreturns 1,argparsereturns 2, and neither is reserved.So ask the profile instead: if it is still ahead of the generation phase
one recorded, no rollback happened and phase one owns it.
That question is safe in both directions.
rollback_to()stops the moment theprofile is at or behind its target, so phase two's own rollback is never
repeated and the profile is never walked past the release the box was
running.
When phase one does recover, it rolls back, re-applies with the rolled-back
release as a fresh process (the same version-skew reason phase one hands over
at all), and restarts units — mirroring what
post_switch's own handler does.When the rollback itself fails there is nothing safe left to run, so it says
so loudly and re-applies nothing rather than running a release over a profile
that is still switched.
OSErrorfrom the exec is folded in as exit 127.What does not change
rc == 0returns 0 without asking anything.before, and now provably does no second rollback.
same case that already keeps
--from-generationoff the child's argv.Test plan
tests/test_agentbox.py+8 assertions in two new classes.HandoverFailureTestcovers the recovery itself: the profile stillahead (rolls back, re-applies, restarts), the profile already back
(does nothing), a failed rollback (says so, re-applies nothing), no
generation number, and
--no-restart-sessionscarried through.HandoverExitStatusTestcovers the wiring that was the actual bug — arejected argument and an unstartable child both reach the recovery, a
clean handover does not.
bin/agentboxthese fail 7/8(1 failure, 6 errors). The one that passes both ways is
test_a_clean_handover_never_recovers, which is the unchanged-behaviourpin.
python3 tests/test_agentbox.pygreen, sotests/native/expected/does not move — this changes update logic, notrendering.
aarch64-linuxflake checks build clean.x86_64-linuxis covered without a local eval. 31 of its 40 checks arethe set shared with
aarch64-linux, built natively above. Of the 9x86-only ones, 8 (
connect,memory-protection,sessions,sessions-web,settings-page,vm-closure,web-surface,webhook) are enumerated inci.ymland pass here. The 9th,testscript-fits, cannot be evaluated on an aarch64 host and failsidentically on unmodified master — see the comment below.
Not in this PR
The follow-up Lio and I scoped: an explicit
agentbox rollbackverb and adeadman timer that reverts unless disarmed. This PR only closes the case where
the box cannot roll itself back; those cover the case where the update
succeeds and the result is wrong anyway.
Co-Authored-By: Claude Opus 5 [email protected]