From 91a9f56a314c75cad3f588bcb62fa34f0e3e1942 Mon Sep 17 00:00:00 2001 From: Defang Agent <140542220+defangdevs@users.noreply.github.com> Date: Sat, 5 Sep 2026 05:52:39 +0000 Subject: [PATCH] fix(update): roll back when the new release cannot run its post-switch phase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- bin/agentbox | 61 ++++++++++- tests/test_agentbox.py | 225 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 285 insertions(+), 1 deletion(-) diff --git a/bin/agentbox b/bin/agentbox index 3c99e010..f809f026 100755 --- a/bin/agentbox +++ b/bin/agentbox @@ -3996,7 +3996,66 @@ def cmd_update(args): if args.no_restart_sessions: child.append("--no-restart-sessions") print(f"switched to {target[:12]}; applying with the new agentbox") - return subprocess.run(child).returncode + try: + rc = subprocess.run(child).returncode + except OSError as e: + # Not "the new release failed" but "the new release could not be + # STARTED" — the file is not there, the interpreter is not there, + # the execute bit is not set. Phase two has run no code of its own, + # so it has rolled nothing back. + print(f"agentbox: cannot execute the new agentbox: {e}", + file=sys.stderr) + rc = 127 + if rc == 0: + return 0 + return recover_handover(args, profile, generation, target, rc) + + +def recover_handover(args, profile, generation, target, rc): + """Roll back when phase two failed BEFORE it could roll itself back. + + post_switch() rolls back its own failures, so a non-zero child usually + means the box is already on the previous release and phase one has + nothing left to do. That is not always true. The child IS the newly + installed agentbox, and a release can fail before one line of its own + code runs: argparse exits 2 on an argument this release no longer + takes, an import fails, the interpreter or the file is missing. The + profile is already switched by then, so without this the box is left + running a release that cannot run its own updater, and nothing puts it + back — the one failure the rollback exists for and the one it missed. + + Exit status cannot tell the two apart: post_switch returns 1, argparse + returns 2, and neither is reserved. So ask the PROFILE instead. If it + is still ahead of where phase one found it, no rollback has happened + and phase one owns it. That question is safe to ask when phase two DID + roll back, because rollback_to() stops the moment the profile is at or + behind its target. + """ + if generation is None: + # Nothing to go back to — the profile is not a generation symlink, + # the same case that keeps --from-generation off the child's argv. + return rc + current = profile_generation(profile) + if current is not None and current <= generation: + return rc # phase two already put it back + rolled = rollback_to(profile, generation) + if rolled: + # Re-apply with the rolled-back release, as a fresh process for + # the same version-skew reason phase one handed over at all. The + # profile is a symlink, so it now resolves to the OLD agentbox. + run([str(profile / "bin" / "agentbox"), "apply", + "--config", args.config, "--profile", str(profile)], check=False) + restart_units(restart_sessions=not args.no_restart_sessions) + wall(f"update to {target[:12]} FAILED before the new release could run " + "its post-switch phase — profile " + + ("rolled back to the previous release." + if rolled else "COULD NOT be rolled back; see journalctl -u " + f"{UPDATE_UNIT}.")) + print("agentbox: the new release could not run its post-switch phase " + f"(exit {rc}); profile " + + ("rolled back" if rolled else "NOT rolled back"), + file=sys.stderr) + return rc def post_switch(args, profile): diff --git a/tests/test_agentbox.py b/tests/test_agentbox.py index 588c48a3..84931890 100644 --- a/tests/test_agentbox.py +++ b/tests/test_agentbox.py @@ -3401,6 +3401,231 @@ def test_a_clean_apply_keeps_the_new_release(self): self.assertEqual(self.rolled_back, []) +class HandoverFailureTest(unittest.TestCase): + """Phase one rolls back when phase two never got to RUN. + + post_switch() rolls back its own failures, but it is the new release's + own code. A release whose agentbox cannot start rolls nothing back: + argparse exits 2 on an argument this release no longer takes, before + the handler it would have called; an import fails; the interpreter or + the file is not there. The profile is switched by then, so phase one + used to return that exit status and leave the box on a release that + cannot run its own updater — recoverable only by hand-driving nix. + + Phase one cannot read the child's intent from its exit status (1 from + post_switch, 2 from argparse, neither reserved), so it asks the + profile whether a rollback actually happened. These pin that question + and both of its answers. + """ + + def setUp(self): + self.mod = load_agentbox() + self.calls = [] + self.rolled_back = [] + self.restarted = [] + self.walls = [] + self.mod.run = self._record + self.mod.wall = self.walls.append + self.mod.restart_units = self._restart + self.mod.rollback_to = self._rollback + self.rollback_works = True + + def _record(self, cmd, check=True, capture=False): + self.calls.append([str(c) for c in cmd]) + return subprocess.CompletedProcess(cmd, 0, "", "") + + def _restart(self, restart_sessions=True): + self.restarted.append(restart_sessions) + return [] + + def _rollback(self, profile, generation): + self.rolled_back.append(generation) + if self.rollback_works: + # Move the symlink the way a real rollback would, so a caller + # that re-reads the generation sees the profile come back. + self._point_at(generation) + return self.rollback_works + + def _point_at(self, generation): + link = self.tmp / f"profile-{generation}-link" + if not link.exists(): + link.mkdir() + self.prof.unlink() + self.prof.symlink_to(link) + + def _profile(self, tmp, generation): + """A profile that is a generation SYMLINK, the way the real one is. + + build_fake_profile makes a plain directory, for which + profile_generation() is None — the case that keeps + --from-generation off the child's argv. Here the number has to be + readable, and has to move, because it is the whole signal. + """ + self.tmp = Path(tmp) + built = build_fake_profile(tmp) + link = self.tmp / f"profile-{generation}-link" + built.rename(link) + self.prof = self.tmp / "profile" + self.prof.symlink_to(link) + return self.prof + + def _args(self, **over): + base = dict(config="/etc/agent-box/config.json", + no_restart_sessions=False) + base.update(over) + return argparse.Namespace(**base) + + def _recover(self, rc=2, at=9, frm=8, **over): + """Run the recovery with the profile sitting at generation `at`.""" + with tempfile.TemporaryDirectory() as tmp: + prof = self._profile(tmp, at) + out, err = io.StringIO(), io.StringIO() + with contextlib.redirect_stdout(out), \ + contextlib.redirect_stderr(err): + got = self.mod.recover_handover( + self._args(**over), prof, frm, "b" * 40, rc) + return got, err.getvalue() + + def test_a_child_that_never_rolled_back_is_rolled_back_by_phase_one(self): + """The profile is still ahead, so the rollback is phase one's.""" + got, err = self._recover(rc=2, at=9, frm=8) + self.assertEqual(got, 2, "the child's exit status is preserved") + self.assertEqual(self.rolled_back, [8]) + self.assertIn("rolled back", err) + # The rolled-back release has to re-apply, or the box keeps the + # units the half-finished update rendered. + self.assertIn("agentbox", [Path(c[0]).name for c in self.calls]) + self.assertIn("apply", [c[1] for c in self.calls if len(c) > 1]) + self.assertEqual(self.restarted, [True]) + + def test_a_child_that_already_rolled_back_is_left_alone(self): + """Phase two's own rollback must not be repeated. + + post_switch() returns non-zero AFTER putting the profile back, so + the profile is already at the generation phase one recorded. Going + again would walk it PAST the release the box was running. + """ + got, _ = self._recover(rc=1, at=8, frm=8) + self.assertEqual(got, 1) + self.assertEqual(self.rolled_back, []) + self.assertEqual(self.calls, []) + self.assertEqual(self.restarted, []) + + def test_a_profile_with_no_generation_attempts_nothing(self): + """No generation number is no rollback target, and no traceback.""" + with tempfile.TemporaryDirectory() as tmp: + prof = build_fake_profile(tmp) # a plain dir, not a symlink + self.assertIsNone(self.mod.profile_generation(prof)) + with contextlib.redirect_stdout(io.StringIO()), \ + contextlib.redirect_stderr(io.StringIO()): + got = self.mod.recover_handover( + self._args(), prof, None, "b" * 40, 2) + self.assertEqual(got, 2) + self.assertEqual(self.rolled_back, []) + + def test_a_rollback_that_fails_says_so_and_does_not_re_apply(self): + """Nothing safe is left to run, so say it loudly instead.""" + self.rollback_works = False + got, err = self._recover(rc=2, at=9, frm=8) + self.assertEqual(got, 2) + self.assertEqual(self.rolled_back, [8]) + self.assertIn("NOT rolled back", err) + self.assertEqual(self.calls, [], "nothing re-applies over a switched " + "profile") + self.assertEqual(self.restarted, []) + self.assertTrue(any("COULD NOT be rolled back" in w + for w in self.walls)) + + def test_no_restart_sessions_is_carried_into_the_recovery(self): + self._recover(rc=2, at=9, frm=8, no_restart_sessions=True) + self.assertEqual(self.restarted, [False]) + + +class HandoverExitStatusTest(unittest.TestCase): + """Phase one's wiring: which child outcomes reach the recovery. + + Separate from the recovery's own behaviour above, because the bug was + in the WIRING — `return subprocess.run(child).returncode` never asked + the question at all. + """ + + def setUp(self): + self.mod = load_agentbox() + self.recovered = [] + self.mod.recover_handover = self._recover + + def _recover(self, args, profile, generation, target, rc): + self.recovered.append(rc) + return rc + + def _update(self, child_rc=None, exc=None): + """Drive cmd_update to the handover with a fake install + child.""" + target = "2" * 40 + real_run = subprocess.run + self.addCleanup(setattr, subprocess, "run", real_run) + + def fake_child(cmd, *a, **k): + if exc is not None: + raise exc + return subprocess.CompletedProcess(cmd, child_rc) + subprocess.run = fake_child + + with tempfile.TemporaryDirectory() as tmp: + prof = build_fake_profile(tmp) + self.mod.source_tree = lambda *a, **k: target + + def install(cmd, check=True, capture=False): + if "install" in cmd: + manifest = prof / "manifest.json" + data = json.loads(manifest.read_text()) + data["elements"]["runtime"]["url"] = ( + f"github:{FAKE_REPO}/{target}") + manifest.write_text(json.dumps(data)) + return subprocess.CompletedProcess(cmd, 0, "", "") + + self.mod.run = install + self.mod.wall = lambda message: None + args = argparse.Namespace( + profile=str(prof), post_switch=False, repo=None, rev=None, + branch=None, src="/var/lib/agent-box/src", force=False, + check=False, config="/etc/agent-box/x.json", + no_restart_sessions=False, from_generation=None, + from_rev=None) + out, err = io.StringIO(), io.StringIO() + with contextlib.redirect_stdout(out), \ + contextlib.redirect_stderr(err): + got = self.mod.cmd_update(args) + return got, err.getvalue() + + def test_a_clean_handover_never_recovers(self): + got, _ = self._update(child_rc=0) + self.assertEqual(got, 0) + self.assertEqual(self.recovered, []) + + def test_a_rejected_argument_reaches_the_recovery(self): + """The regression: argparse exits 2 before the handler runs. + + A release that renames or drops --post-switch or --from-generation + fails exactly here, and it is the change an agent-box-on-agent-box + developer is most likely to make. + """ + got, _ = self._update(child_rc=2) + self.assertEqual(got, 2) + self.assertEqual(self.recovered, [2]) + + def test_an_unstartable_child_reaches_the_recovery(self): + """No file, no interpreter, no execute bit — an OSError, not a rc. + + Before this it escaped cmd_update as a traceback: FileNotFoundError + is neither ConfigError nor UpdateError, so main() did not catch it + either, and the profile stayed switched. + """ + got, err = self._update(exc=FileNotFoundError(2, "no such file")) + self.assertEqual(got, 127) + self.assertEqual(self.recovered, [127]) + self.assertIn("cannot execute the new agentbox", err) + + def update_fixture(): with tempfile.TemporaryDirectory() as tmp: out = render(tmp, CONFIG_JSON)