Repository navigation
feat(ag2-sparrow): Hatch self-upgrade to the served target_version, with an offline rollback - #5676
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 94%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
packages/ag2-sparrow/ag2_sparrow/adapters/hatch/keepalive.pyLines 279-287 279 """Whether `pid` runs a Sparrow command line (/proc, or sysctl on macOS); False when
280 that cannot be read."""
281 cmd = _proc(pid, "cmdline")
282 if cmd is None:
! 283 return is_sparrow_argv(workspace_lock.proc_argv(pid) or [])
284 return is_sparrow_argv([a.decode(errors="replace") for a in cmd.split(b"\0") if a])
285
286
287 def sparrow_env(pid: int) -> "dict[str, str] | None":Lines 298-306 298 """A pidfd naming the process that has `pid` now; None where the platform has none
299 (macOS). ProcessLookupError when no process has it; any other failure raises."""
300 opener = getattr(os, "pidfd_open", None)
301 if opener is None or not hasattr(signal, "pidfd_send_signal"):
! 302 return None
303 return opener(pid)
304
305
306 def _stop(pid: int, handle: "int | None" = None) -> bool:Lines 321-329 321 if handle is not None:
322 import select
323 if select.select([handle], [], [], max(0.0, deadline - time.monotonic()))[0]:
324 return True # the pidfd turns readable when its process exits
! 325 continue
326 if _cli.process_gone(pid):
327 return True
328 time.sleep(0.2)
329 print(f"error: Sparrow (pid {pid}) did not exit within {STOP_WAIT_S:g}s of SIGTERM",Lines 448-456 448 that could not be opened) nothing is signalled: a bare pid is never trusted."""
449 try:
450 handle = _pidfd_open(pid)
451 except ProcessLookupError:
! 452 return True, None
453 except OSError as e:
454 return False, (f"no stable process handle for pid {pid} (pidfd_open: "
455 f"{diagnostics.describe(e)})")
456 if handle is None:Lines 491-499 491 return False, f"{unit} was not written by install, so its Sparrow is left alone", None
492 if owner == "ours":
493 props = _cli._unit_props()
494 if props is None:
! 495 return False, f"systemctl --user show {_cli.SERVICE_NAME} did not give a well-formed state", None
496 main_pid = props["MainPID"]
497 outside = holder if holder is not None and holder != main_pid else None
498 unproven = unproven_holder(record, outside) if outside is not None else None
499 if unproven:Lines 500-508 500 return False, f"{unproven}, beside {_cli.SERVICE_NAME}; nothing was signalled", None
501 r = _cli._systemctl(("--user", "stop", "ag2-sparrow.service"))
502 how = f"systemctl --user stop {_cli.SERVICE_NAME}"
503 if not _cli._ok(r):
! 504 return False, f"{how} failed ({_cli._why(r)})", "unit"
505 if main_pid and not _wait_gone(main_pid):
506 return False, f"{how}, but its Sparrow (pid {main_pid}) is still running", "unit"
507 if outside is not None:
508 stopped, unproven = stop_proven(record, outside)Lines 508-516 508 stopped, unproven = stop_proven(record, outside)
509 if unproven:
510 return False, f"{how}; {unproven}; it was not signalled", "unit"
511 if not stopped:
! 512 return False, f"the Sparrow outside {_cli.SERVICE_NAME} (pid {outside}) did not stop", "unit"
513 how += f"; stopped the Sparrow outside it (pid {outside})"
514 manager = "unit"
515 elif holder is None:
516 how, manager = "Sparrow was not running", "detached"Lines 694-702 694 return settled or "recovery-blocked"
695 return settled or self_upgrade.maybe_start(cfg, setup)
696 except Exception as e: # noqa: BLE001 - keepawake's own job must still be logged
697 print(f"NOTE served setup: {diagnostics.describe(e)}", file=sys.stderr)
! 698 return settled
699
700
701 def last_keepawake(log: Path) -> float | None:
702 """The time of the log's last keepawake line, or None."""packages/ag2-sparrow/ag2_sparrow/adapters/hatch/self_upgrade.pyLines 353-361 353 failed = load_state(state_dir).get("failed")
354 except JournalError:
355 with contextlib.suppress(OSError):
356 return state_path(state_dir).stat().st_mtime
! 357 return time.time()
358 if isinstance(failed, dict) and installed in (failed.get("from"), failed.get("target")):
359 at = failed.get("at")
360 return float(at) if isinstance(at, (int, float)) else time.time()
361 return NoneLines 444-453 444
445 def _listing(directory: Path) -> "list[str]":
446 try:
447 return sorted(p.name for p in Path(directory).iterdir())
! 448 except OSError:
! 449 return []
450
451
452 def _record_scope(dist) -> dict:
453 """The previous install, read before anything changes: the site dir, the top-levelLines 479-487 479 and console scripts into the scripts dir, and its whole dist-info dir (pip adds files
480 there). ValueError for any other location, which is never installed."""
481 found = sorted(Path(wheels).glob(f"ag2_sparrow-{version}-*.whl"))
482 if len(found) != 1:
! 483 raise ValueError(f"expected one ag2_sparrow-{version} wheel, found {len(found)}")
484 site, scripts = Path(scope["site"]), _scripts_dir(scope)
485 info, data = f"ag2_sparrow-{version}.dist-info", f"ag2_sparrow-{version}.data"
486 owned = {str(site / info)}
487 with zipfile.ZipFile(found[0]) as zf:Lines 492-507 492 entry = ""
493 for row in rows:
494 rel = PurePosixPath(row[0]) if row and row[0] else None
495 if rel is None:
! 496 continue
497 if rel.is_absolute() or ".." in rel.parts:
! 498 raise ValueError(f"the wheel names a path outside its install: {row[0]}")
499 parts = rel.parts
500 if parts[0] != data:
501 owned.add(str(site.joinpath(*parts)))
502 elif len(parts) > 2 and parts[1] in ("purelib", "platlib"):
! 503 owned.add(str(site.joinpath(*parts[2:])))
504 elif len(parts) > 2 and parts[1] == "scripts":
505 owned.add(str(scripts.joinpath(*parts[2:])))
506 else:
507 raise ValueError(f"the wheel installs outside site-packages: {row[0]}")Lines 509-517 509 points.optionxform = str
510 points.read_string(entry)
511 for section in ("console_scripts", "gui_scripts"):
512 if points.has_section(section):
! 513 owned.update(str(scripts / name) for name in points.options(section))
514 return sorted(owned)
515
516
517 def _files_under(path: Path) -> "list[Path]":Lines 515-523 515
516
517 def _files_under(path: Path) -> "list[Path]":
518 if path.is_file():
! 519 return [path]
520 return sorted(p for p in path.rglob("*") if p.is_file() and "__pycache__" not in p.parts) \
521 if path.is_dir() else []
522 Lines 577-585 577 name = f"{i:05d}"
578 shutil.copy2(path, tmp / "files" / name)
579 entries.append({"path": path, "copy": name, "sha256": digest})
580 if not entries:
! 581 raise ValueError("the install's files are missing")
582 dirs = _dirs_before(scope, [e["path"] for e in entries] + scope["owned"])
583 manifest = dict(scope, version=version, files=entries, dirs=dirs)
584 (tmp / "manifest.json").write_text(json.dumps(manifest), encoding="utf-8")
585 shutil.rmtree(dest, ignore_errors=True)Lines 628-636 628 removed = [Path(p) for p in namespace(manifest) if p in ours and p not in want]
629 for path in removed:
630 path.unlink()
631 for pyc in path.parent.glob(f"__pycache__/{glob.escape(path.stem)}.*.pyc"):
! 632 pyc.unlink(missing_ok=True)
633 for entry in manifest["files"]:
634 target = Path(entry["path"])
635 target.parent.mkdir(parents=True, exist_ok=True)
636 shutil.copy2(snap / "files" / entry["copy"], target)Lines 747-755 747 problems, recorded = [], set()
748 # RECORD is parsed here: Distribution.files drops the rows whose file is missing.
749 for row in csv.reader((dist.read_text("RECORD") or "").splitlines()):
750 if not row or not row[0]:
! 751 continue
752 path = Path(os.path.normpath(dist.locate_file(row[0])))
753 mode, _, value = (row[1] if len(row) > 1 else "").partition("=")
754 recorded.add(path)
755 if not value:Lines 753-761 753 mode, _, value = (row[1] if len(row) > 1 else "").partition("=")
754 recorded.add(path)
755 if not value:
756 if path.name != "RECORD" and "__pycache__" not in path.parts:
! 757 problems.append(f"unhashed {path}")
758 elif not path.is_file():
759 problems.append(f"missing {path}")
760 elif mode != "sha256" or base64.urlsafe_b64encode(bytes.fromhex(
761 _sha256(path))).rstrip(b"=").decode() != value:Lines 772-780 772 """For the owner's install: lift a restore-failed hold once the installed files verify
773 against their RECORD (verify_installed). None when Sparrow may start, else why not."""
774 with upgrade_lock(state_dir) as free:
775 if not free:
! 776 return "an upgrade is running"
777 held = recovery_blocked(state_dir)
778 if held is None:
779 return None
780 attempted = load_state(state_dir)["attempted"] if "cannot be read" not in held else {}Lines 887-895 887 namespace or an interrupted restore is restored, in a phase that resumes next run."""
888 state = load_state(state_dir)
889 for target, tried in state["attempted"].items():
890 if not isinstance(tried, dict):
! 891 continue
892 at, phase = tried.get("at"), tried.get("phase")
893 stale = (tried.get("outcome") == "started" and isinstance(at, (int, float))
894 and (phase is not None or now - at > STALE_START_S))
895 if not stale:Lines 906-914 906 "why": why, "at": now}
907 save_state(state_dir, state)
908 held = recovery_blocked(state_dir) if phase == "quiesced" else None
909 if held:
! 910 print(f"error: Sparrow is not started: {held}", file=sys.stderr)
911 elif phase == "quiesced":
912 ok, how = start_sparrow(cfg, tried.get("manager"))
913 print(("started: " if ok else "error: Sparrow did not start: ") + how,
914 file=sys.stderr if not ok else sys.stdout)packages/ag2-sparrow/tests/test_hatch_served_setup.pyLines 3290-3298 3290 if journal["attempted"]["0.6.0"].get("manager") in ("unit", "detached") and \
3291 journal["attempted"]["0.6.0"]["phase"] in ("quiesced", "installing",
3292 "installed") and \
3293 "installed" not in journal and "failed" not in journal:
! 3294 continue # the writer's own variant, kept out of the rejected set
3295 self.assert_held_everywhere(json.dumps(journal), journal)
3296
3297 _writer_saves = NoneLines 3551-3559 3551 for state in saved:
3552 for m in self.mutants(state):
3553 key = self.full_record(m)
3554 if key not in written and self_upgrade._journal_fault(m, latest) is None:
! 3555 loose[key] = m
3556 self.assertEqual(sorted(loose), [], "accepted, but no production writer saves it")
3557
3558 def test_the_one_target_projection_misses_a_mutant_the_full_record_catches(self):
3559 failed_rb = [s for s in self.writer_saves() if len(s["attempted"]) == 1Lines 4167-4178 4167 gate, entered = threading.Event(), threading.Event()
4168 fake = self.fake()
4169
4170 def slow(argv, timeout):
! 4171 if FakeRun.kind(argv) == "pip-install":
! 4172 entered.set()
! 4173 gate.wait(10)
! 4174 return fake(argv, timeout)
4175 outcomes = []
4176 with mock.patch.object(self_upgrade, "_run", slow), \
4177 contextlib.redirect_stdout(io.StringIO()), \
4178 contextlib.redirect_stderr(io.StringIO()):Lines 4612-4620 4612 "-m", "ag2_sparrow"])
4613 self.addCleanup(lambda: (p.poll() is None and p.kill(), p.wait()))
4614 deadline = time.monotonic() + 10
4615 while not keepalive.is_sparrow_pid(p.pid) and time.monotonic() < deadline:
! 4616 time.sleep(0.05) # until exec: a forked child still shows the parent's argv
4617 return p
4618
4619 @unittest.skipUnless(sys.platform in ("linux", "darwin"), "no start-time source")
4620 def test_a_real_holder_is_stopped_only_with_its_own_start_time(self):Lines 4622-4630 4622 start = workspace_lock.proc_start(p.pid)
4623 self.assertIsNotNone(start)
4624 self.assertNotEqual(start, workspace_lock.proc_start(os.getpid()))
4625 if sys.platform == "darwin":
! 4626 self.assertTrue(keepalive.is_sparrow_pid(p.pid), "argv read through sysctl")
4627 with mock.patch.object(keepalive, "is_sparrow_pid", return_value=True), \
4628 contextlib.redirect_stderr(io.StringIO()):
4629 stopped, why = keepalive.stop_proven({"pid": p.pid, "proc_start": start + "0"}, p.pid)
4630 self.assertEqual(stopped, False)Lines 4629-4642 4629 stopped, why = keepalive.stop_proven({"pid": p.pid, "proc_start": start + "0"}, p.pid)
4630 self.assertEqual(stopped, False)
4631 self.assertIsNone(p.poll(), "a mismatched start time is never signalled")
4632 if not hasattr(os, "pidfd_open"): # macOS: no handle, so never signalled
! 4633 self.assertIn("no stable process handle", why)
! 4634 stopped, why = keepalive.stop_proven({"pid": p.pid, "proc_start": start}, p.pid)
! 4635 self.assertEqual(stopped, False)
! 4636 self.assertIn("no stable process handle", why)
! 4637 self.assertIsNone(p.poll())
! 4638 return
4639 self.assertIn("is not proven", why)
4640 # Reaped as it exits, as a detached Sparrow's own parent would.
4641 threading.Thread(target=p.wait, daemon=True).start()
4642 self.assertEqual(keepalive.stop_proven({"pid": p.pid, "proc_start": start}, p.pid),src/workspace_lock.pyLines 160-180 160
161
162 def _darwin_sysctl(mib: list) -> bytes | None:
163 """The raw value of sysctl `mib` on macOS; None when it cannot be read or is empty."""
! 164 import ctypes
! 165 import ctypes.util
! 166 try:
! 167 libc = ctypes.CDLL(ctypes.util.find_library("c"), use_errno=True)
! 168 arr, size = (ctypes.c_int * len(mib))(*mib), ctypes.c_size_t(0)
! 169 if libc.sysctl(arr, len(mib), None, ctypes.byref(size), None, 0) != 0 or not size.value:
! 170 return None
! 171 buf = ctypes.create_string_buffer(size.value)
! 172 if libc.sysctl(arr, len(mib), buf, ctypes.byref(size), None, 0) != 0:
! 173 return None
! 174 return buf.raw[:size.value] or None
! 175 except (OSError, AttributeError, TypeError):
! 176 return None
177
178
179 # CTL_KERN, then KERN_PROC/KERN_PROC_PID, KERN_BOOTTIME and KERN_PROCARGS2 (<sys/sysctl.h>).
180 _KERN, _KERN_PROC_PID, _KERN_BOOTTIME, _KERN_PROCARGS2 = 1, (14, 1), 21, 49Lines 181-197 181
182
183 def proc_argv(pid: int) -> list[str] | None:
184 """The argv of `pid` on macOS (KERN_PROCARGS2); None elsewhere or when unreadable."""
! 185 if sys.platform != "darwin":
! 186 return None
! 187 raw = _darwin_sysctl([_KERN, _KERN_PROCARGS2, pid])
! 188 if raw is None or len(raw) < 4:
! 189 return None
! 190 argc = int.from_bytes(raw[:4], sys.byteorder)
! 191 _exe, _, rest = raw[4:].partition(b"\0")
! 192 args = rest.lstrip(b"\0").split(b"\0")
! 193 return [a.decode(errors="replace") for a in args[:argc]] if 0 < argc <= len(args) else None
194
195
196 def proc_start(pid: int, proc: Path = Path("/proc")) -> str | None:
197 """`<boot id>:<start time>` of `pid`, which no later process with that pid shares:Lines 197-211 197 """`<boot id>:<start time>` of `pid`, which no later process with that pid shares:
198 /proc's start ticks, or on macOS kinfo_proc's p_starttime; None where it cannot be
199 read (or the process is gone)."""
200 if sys.platform == "darwin" and not proc.is_dir():
! 201 boot = _darwin_sysctl([_KERN, _KERN_BOOTTIME])
! 202 info = _darwin_sysctl([_KERN, *_KERN_PROC_PID, pid])
! 203 if boot is None or info is None or len(boot) < 12 or len(info) < 12:
! 204 return None
205 # struct timeval {time_t; suseconds_t} opens both kern.boottime and kinfo_proc.
! 206 (bsec, busec), (sec, usec) = (struct.unpack_from("=qi", b) for b in (boot, info))
! 207 return f"darwin-{bsec}.{busec:06d}:{sec}.{usec:06d}" if sec > 0 else None
208 try:
209 boot = (proc / "sys" / "kernel" / "random" / "boot_id").read_text().strip()
210 stat = (proc / str(pid) / "stat").read_bytes()
211 except (OSError, ValueError): |
146757f to
0216c11
Compare
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact child head 0216c1119865be33b83e00e190f301df1272a324 over #5665 tip cad2299780b0eecf37a1794e6d1875d3b13fb130; ancestry is verified, and current main is already an ancestor of the parent. The rebase-sensitive resolutions are sound: install_service retains mxid and the bool contract while adding may_start, --no-start neither starts nor restarts, _served_setup retains diagnostics.describe, and UNIT_UNREACHABLE fails closed. I still have to request changes for three production failure-path blockers.
P1 — rollback can certify a mixed installation as rolled-back. restore() removes only paths named by whichever RECORD is visible after the failed install (self_upgrade.py:210-219), and _rollback() declares success from the imported version plus reconnect (:428-437). If pip fails after overwriting an old module and adding a target-only module while the old dist-info remains, that old RECORD cannot name the new file. Fault-injecting exactly that state through production run_upgrade() produced:
outcome=rolled-back
old_after=old
target_residue_after=True
recorded_problems=[]
The stale-attempt path has the same blind spot: _reconcile() treats current == previous as proof that no mutation occurred (:364-374). With target bytes and a target-only module beside old 0.5.4 metadata, it returned 0.6.0:crashed, made zero rollback calls, left both target changes in place, and consumed the only attempt for 0.6.0. Removing only the version guard is insufficient because the current restore still cannot remove introduced paths. Persist a pre-mutation phase/inventory that can restore the exact package namespace independently of post-failure metadata, remove additions, and verify that inventory before recording rolled-back.
P1 — the upgrader does not prove Sparrow is quiescent before changing its installed files. stop_sparrow() handles UNIT_UNREACHABLE and owner == "ours", but lets owner == "other" fall through to a raw signal (self_upgrade.py:386-399); start_sparrow() then goes through restart_listener(), which spawns detached for that same hand-written service (:402-403, keepalive.py:244-260). Exact-head probe:
other stop=(True, 'stopped Sparrow (pid 4242)') signals=[4242]
other start=(True, 'spawned')
That bypasses the existing ensure_sparrow() policy which deliberately leaves hand-made services alone (keepalive.py:401-404), and their supervisor may restart the process while pip is writing. Adjacent states fail for the same reason: an owned but inactive unit plus an external holder reports a successful unit stop without stopping the holder, and a still-live holder with a stale heartbeat becomes None/“not running.” On rollback the code proceeds even after an explicit stop failure (self_upgrade.py:411-419):
stop failed -> outcome=restore-failed restore_calls=1 install_calls=1 start_calls=1
No package mutation should occur unless the exact running process is proven stopped. Reuse one ownership/quiescence contract for the exhaustive owner/holder states, refuse unattended upgrade for owner == "other", preserve the manager across restart, and abort restoration when stop is unproven.
P1 — keepawake starts a crashed attempt before reconciling it. keepawake() calls _keep_listener() at keepalive.py:483-485, releases that block, and only then reaches _served_setup() / maybe_start() reconciliation at :488,523-529. With a stale started attempt, target metadata present, and the listener down, the observed order was:
events=['restart-unverified-target', 'reconcile-rollback']
log: listener=down restarted=1 upgrade=0.6.0:crashed
Reconcile the stale attempt under the initial upgrade lock before any listener probe/start; otherwise the unverified or mixed target can process work before rollback.
Verification at this exact head:
python3 packages/ag2-sparrow/tests/test_hatch_served_setup.py -v— 154/154 passedpython3 packages/ag2-sparrow/tests/test_hatch_adapter.py -v— 214/214 passed (2 skipped)- child-only and cumulative
review-checks.sh— PASS;git diff --check— clean - hosted CI and
license/cla— green
The existing stop/start test covers ours, UNIT_UNREACHABLE, and None, but omits other, owned-unit/external-holder, stale-live-holder, and rollback-after-stop-failure. Please add discriminating cases for those inputs and the two partial-install shapes above.
This PR is correctly still draft and explicitly still owes the real post-restart upgrade/delivery witness once setup transport exists; I did not restart anything. Because auto-upgrade is opt-out once a served target appears, the worst case is fleet-wide mixed environments or an unverified process handling work. Fixing these contracts and then supplying that exact-head live witness would change my verdict.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Requesting changes at head 0216c111 (base feat/sparrow-served-wording @ cad22997, this PR's own diff only).
What I verified. The rebase resolutions are right. install_service keeps mxid and the bool return, and adds may_start on top. _served_setup keeps diagnostics.describe. stop_sparrow fails closed on UNIT_UNREACHABLE. With --no-start, a changed unit is stopped for the write and left stopped. One correction to the body: _cli._systemd_run still exists (cli.py:34 re-exports it). Only _service_managed is gone. The switch to _systemctl is correct either way.
A downgrade cannot be forced. _decide returns below-installed/current (self_upgrade.py:300-303), the worker re-reads under the lock (:462), and a min_version above the target is refused. pip is run with --no-deps --only-binary=:all: --require-hashes. Tests drive the real functions against a fake site-packages. They don't grep source, and there are no hardcoded paths. Mutation spot-checks: each of these fails its test when removed: the under-lock <= guard, the _decide downgrade guard, may_start and in start =, the not may_start restart veto, the UNIT_UNREACHABLE stop guard, restore-failed when problems exist, --require-hashes, and reconcile-before-decide.
Suites, run locally on python3 3.12: test_hatch_served_setup 154 OK, test_hatch_adapter 214 OK (skipped=2). The adapter suite only passes through an interpreter path without spaces. Through this host's real path (it contains a space), 41 HookScript tests fail, and 39 of them fail the same way at the base. That is a host artefact, not this PR. CI is green, and license/cla is 1 context, success.
Blocking (incomplete_fix). I independently confirm, by reading the code, the three P1s in keweichen's review at this head:
restore()only removes what the post-failure RECORD names (self_upgrade.py:210-219), and_reconciletreatscurrent == previousas "nothing changed" (:364-374).stop_sparrowSIGTERMs the lock holder under a hand-written (other) unit (:395-399). Also,_rollbackrestores even after the stop failed (:411-419).keepawakeruns_keep_listenerbefore_served_setupreconciles (keepalive.py:483-488), so it can restart an unverified target first.
One more problem on the same path:
- An interrupted crash recovery is never retried, and the unverified target stays installed.
_reconcilesavesoutcome=crashedandfailedbefore_rollbackruns (self_upgrade.py:368-374). That rollback runs inside the keepawake process, which runs undertimeout 120(keepalive.py:51, 92). Its own budget can exceed 120 s: stop (STOP_WAIT_S20 s, or a systemd stop timeout), install (up to 300 s), start plus_await_connected(30 s),installed_version(up to 60 s). That comes after_keep_listenerhas already spent up to 30 s in the same run. Probe at this head: I made_rollbackraiseSystemExitthe waytimeout's SIGTERM would, then ranmaybe_startagain 30 min later. The second run returnedNonewith 0 rollback calls,outcome: crashed, and0.6.0still installed. The health row does showupgrade-failed, so nothing is hidden, but nothing recovers it either. If SIGTERM lands insiderestore()'s unlink/copy loop, the files are left mixed. Asks: write a resumable phase (e.g.restoring) that reconcile retries, and recordcrashedonly after the rollback has settled. Or move the recovery out of the 120 s keepawake budget (detached, like the upgrade worker). Add a test that interrupts the recovery and asserts the next run finishes it.
Not blocking, for the transport PR. target_version and target_sha256 arrive in the same served payload. The hash pin protects against a tampered index or wheel, but not against a forged setup: whoever controls that payload can pick any published version above the installed one. setup_problem does not look at package; only _decide does. Today setup_fetch has no transport, so this is dormant. The transport that lands must be authenticated end to end, and its PR should say how.
Bar. Not met: one CHANGES_REQUESTED (keweichen), no approvals. The live post-restart witness is still owed, which the body already says (named, not blocking). The PR is correctly still a draft.
— Sutando (susans-MacBook-Pro)
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-review at 2c138cd7 (PR is still a draft). Three of the four earlier blockers are fixed and I checked each one myself. One new blocker: the no-unit stop path can SIGTERM a process that is not Sparrow.
Verified at this head
- P1-1 (rolled-back while new-version files remained): fixed for the files in scope. I faked a mixed install: target bytes over
__init__.py, a newag2_sparrow/sub/deep.py, a half-writtenag2_sparrow-0.6.0.dist-infowith a garbage RECORD, and a rewritten console script. Rollback removed the new files, put the old bytes back, and the hash inventory matched the pre-upgrade one.restorenever reads RECORD. Without the removal loop, 15 tests fail. Without the drift check at the end ofrestore, 1 test fails. - 4th finding (interrupted recovery never retried): fixed. I killed a rollback in the middle of
restore(SystemExit on the 2ndshutil.copy2, after the unlinks). The journal readphase: restoring, outcome: started, and drift showed a half-restored tree. On the nextmaybe_startthe result was0.6.0:rolled-back, the inventory matched the original, and the phase wasrolled-back. Removing the_phase(..., "restoring")write fails a test. - P1-3 (keepawake restarting an unverified version): fixed.
settle_attemptruns under the initial lock before_keep_listener. Removing it failstest_keepawake_settles_a_dead_upgrade_before_it_restarts_the_listener. - P1-2, "ours" branch and refusals: fixed. A hand-made unit and an unreachable user manager are refused in
_decideand inquiesce_sparrow, with nothing signalled. An external holder beside our unit is stopped only whenis_sparrow_pidsays it is Sparrow (removing that check fails a test). An unproven stop changes no file (2 tests fail if restore runs anyway). - Test counts match the claim: served-setup ran 175 tests, OK. Adapter ran 214 tests, OK (2 skipped). CI is green and
license/claissuccess(1 context).
Blocking: the no-unit stop SIGTERMs an unverified pid (keepalive.py:400, :448)
resolve_sparrow now returns any alive pid from the lock record, whether or not its heartbeat is fresh (holder = pid if alive else None). When there is no unit, quiesce_sparrow sends that pid SIGTERM via _stop(holder) without calling is_sparrow_pid. The old code went through live_lock_holder, which only returned a pid with a fresh heartbeat. So this push widens the window: a stale lock record left after a crash names a pid the OS may have reused, and the upgrader (or a rollback) will now SIGTERM whatever process holds it and report "stopped Sparrow". Repro with the PR's own Quiesce harness: self.holder(99, fresh=False, sparrow=False) returns (True, 'stopped Sparrow (pid 99)', 'detached') with calls == [('term', 99)]. A fresh-heartbeat non-Sparrow pid gives the same result. That breaks the stated contract ("an external holder is stopped only if proven to be Sparrow"), which is so far enforced only beside our unit.
Ask: before _stop in the detached branch, require is_sparrow_pid(holder). If it is not proven Sparrow, refuse with no manager, the same as the "ours" branch. Add a test for a stale and a fresh non-Sparrow holder with no unit, asserting no term call. test_a_live_holder_with_a_stale_heartbeat_is_still_stopped (tests/test_hatch_served_setup.py:3135) should mark its pid as Sparrow explicitly, so it keeps covering the intended case.
Non-blocking
- The namespace scope comes from the old RECORD's top-level names plus its recorded scripts (
self_upgrade.py:204-236). A target that adds a new top-level package or a new console script leaves it behind, and the outcome is stillrolled-back. In my fake,bin/ag2-sparrow-newcmdandag2_sparrow_newtop/m.pysurvived. Consider also sweepingbin/ag2-sparrow*andag2_sparrow*top-level names insite, or reading the target wheel's RECORD from the downloaded wheel before installing it. - The
or phase == "restoring"clause insettle_attempt(self_upgrade.py:461) is never exercised. Removing it fails no test, because an interrupted restore still hasoutcome: startedwith a phase set, whichstalealready matches. Either drop the clause or add a test where it is the only route. - Still open from last time: the transport that serves
target_sha256must be authenticated end to end.
Kewei's CHANGES_REQUESTED at 0216c111 still stands. No approval exists at this head, so this cannot be approved yet in any case.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact child head 2c138cd74fa7a95e25c306c0640e809622d3b1ce over #5665 tip/base cad2299780b0eecf37a1794e6d1875d3b13fb130; ancestry is verified, and the cumulative main merge-base is 7940f25ff965f51df3273d94f009cb1b771119c0.
REQUEST_CHANGES — not ready to merge. The exact earlier partial-install, interrupted-restore, successful-settlement-order, and manager/refusal cases are fixed, but three P1 safety gaps remain. I independently reproduced Susan's no-unit PID finding and two adjacent recovery failures.
P1 — detached quiescence SIGTERMs an unverified PID. resolve_sparrow() accepts any alive PID from the lock record (keepalive.py:392-401), regardless of heartbeat freshness or identity. The owned-unit branch correctly requires is_sparrow_pid() for an outside holder (:431-434), but the no-unit branch calls _stop(holder) directly (:446-451). With owner=None, a stale live holder, and is_sparrow_pid=False, the production function returned:
(True, 'stopped Sparrow (pid 77)', 'detached')
calls=[('term', 77)]
The positive control beside our unit refused the same unproven holder without signalling it. A stale lock can name a reused unrelated PID, so require proof in the detached branch too and cover both fresh and stale non-Sparrow holders with zero signal calls.
P1 — rollback can still certify residue outside the old RECORD-derived scope. _record_scope() derives roots and extra solely from the pre-upgrade RECORD (self_upgrade.py:204-214), and namespace(), namespace_drift(), and restore() inspect only those roots, matching dist-info directories, and the exact old extra paths (:217-234, :269-308). A target-only top-level package plus a new console command therefore survive restore while the upgrader reports no drift:
reported_drift=[]
independent_added=[.../bin/ag2-sparrow-new-command,
.../site/ag2_sparrow_extra/__init__.py]
Driving the full rollback path records rolled-back with both files still present. The current partial-install tests add a module only under the already-known ag2_sparrow root, so they cannot expose this. This is the original mixed-installation failure one input to the side: make the cleanup/verification scope include target-introduced top-level packages and console scripts (or refuse such a target before mutation), and compare the result with an independent complete distribution inventory.
P1 — failed recovery still launches unverified bytes. _rollback() catches restore/hash failures at self_upgrade.py:535-538 but still calls start_sparrow() at :543-548. Injecting a restore OSError left target 0.6.0 installed and produced:
outcome=0.6.0:restore-failed
started_versions=['0.6.0']
The existing hash-mismatch test explicitly expects that restart (test_hatch_served_setup.py:2736-2751). There is a second route: keepawake() calls _keep_listener() after settlement regardless of its result (keepalive.py:550-555). With a dead mutating attempt, no usable snapshot, and the listener down:
listener=down restarted=1 upgrade=0.6.0:restore-failed
started_versions=['0.6.0']
An exception from _settle_upgrade() also returns None and reaches that restart (:593-600). A failed integrity proof must preserve the stopped state; only a verified restore should allow either the rollback start or the ordinary keepawake restart.
Verified at this head:
test_hatch_served_setup.py -v: 175/175 passedtest_hatch_adapter.py -v: 214/214 passed, 2 skippeddelivery-core-enforcement.test.py: 15/15 passed (thesettle_attemptrename satisfies the reservedreconcilecontract)start-cli-private-socket-foreign-core.test.py: 4/4 passed; this is rerun evidence, not a change in this child diff- child-only and cumulative
review-checks.sh,git diff --check, and package no-drift check: passed - hosted CI and
license/cla: green
The PR is correctly still draft and still owes the exact-head live post-restart upgrade/delivery witness stated in its body; I did not restart a real service. Even after the code blockers are fixed, that remains an approval gate under REVIEW §15.
Worst case once setup transport activates this opt-out feature: an unattended upgrade kills an unrelated process, reports a mixed install as rolled back, or starts corrupt/unverified package bytes to process work. The three fail-closed fixes above plus the real post-restart witness are what would change this verdict.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 70015e7c6a0387021eb9983ab7f5855280fa4995. The fresh/stale non-Sparrow holder case is fixed, and the five named regressions from the last round discriminate correctly (all fail at 2c138cd7, all pass here). I ran the full focused suites too: served setup 179/179, adapter 216/216 (2 skipped), cumulative review checks PASS, and git diff --check is clean.
REQUEST_CHANGES — the same fail-closed contracts still break on adjacent inputs.
P1 — a failed-recovery hold still permits unverified code to run
There are three independent routes:
-
The documented recovery command starts the service before it clears or validates the hold.
_install()callsinstall_service(..., may_start=True)atcli.py:746-749; that path canenable --now/restart at:627-644. Only afterward doesclear_blocked()run at:754-758.clear_blocked()(self_upgrade.py:556-565) merely relabelsrestore-failedasacknowledged; Hatch install does not reinstall or hash-verify the package despite printing “this install put the files in place.” In the production CLI with fake systemd and a realrestore-failedjournal, the observed order was:systemctl --user enable --now ag2-sparrow.service service_state_at_clear=active phase_at_clear=restore-failed final_phase=acknowledged -
A blocked keepawake tick still calls
_served_setup()unconditionally (keepalive.py:564-568), whilemaybe_start()has norecovery_blocked()gate (self_upgrade.py:443-480). With failed target A=0.6.0and newly served B=0.7.0, the exact-head path produced:rc=1 listener_restarts=0 worker_spawns=1 keepawake ... reason=upgrade-failed upgrade=0.7.0:started worker outcome=ok worker events=[pip-check, stop, pip-install, pip-check, install, start, version] installed=0.7.0 recovery_blocked='the upgrade to 0.6.0 left unverified files (restore-failed)'The worker itself also lacks the gate, so it snapshots the already-unverified baseline and completes the install/start.
-
After a restore/hash failure,
_rollback()unconditionally callsinstalled_version()(self_upgrade.py:613), which starts a fresh interpreter and importsag2_sparrow(:403-406). Injectingrestore -> OSErrorwith an unverified target__init__.pythat writes a marker gaveoutcome=restore-failed, zero Sparrow start calls, andmarker_exists=true: the diagnostic still executed the unverified package.
Please gate every service start and upgrade entry point (including the locked worker) while recovery is blocked; make the owner recovery action establish and verify known bytes before lifting the hold; and do not import package code to diagnose a failed integrity recovery.
P1 — rollback ownership is both incomplete and destructive
_record_scope() records only old RECORD roots plus top-level/bin name listings, and _owned_tops()/_owned_scripts() infer ownership from those names (self_upgrade.py:210-249). It never reads an exact target-owned inventory.
- With pre-existing
shared_vendor/foreign.py, the target RECORD addedshared_vendor/sparrow_target.py. A failed reconnect drove the complete rollback path, which returnedrolled-back;sparrow_target.pysurvived andnamespace_drift()returned[]. Because the top-level directory existed before and was not an old RECORD root,namespace()never sees the target-owned nested file. - Conversely, an unrelated
foreign_added_later/keep.pyandbin/foreign-tool-added-latercreated after the snapshot were both deleted byrestore(), again withnamespace_drift=[]. Every post-snapshot top-level/bin name is assumed to be Sparrow-owned, even though the upgrade lock cannot exclude another installer or owner action during a crash-recovery window.
So the new top-level test proves only one favorable shape; the policy can still falsely certify target residue and can remove foreign data. Capture an exact target-owned path inventory (for example from the target wheel/RECORD) and use it together with the old snapshot. Unknown additions must not be deleted or certified away.
P1 — a corrupt existing recovery journal fails open
load_state() maps missing, unreadable, and malformed state to the same empty dictionary (self_upgrade.py:98-106). A truncated existing hatch-upgrade.json containing {"attempted": therefore made settlement see no attempt, recovery_blocked() return false, and keepawake restart a down listener:
rc=0 restarted=1
keepawake listener=down restarted=1
An existing journal that cannot be parsed/read must preserve the stopped state and surface a repairable error; only a genuinely absent journal can mean “no recovery pending.”
P1 — command-class proof is not process ownership proof
The no-unit branch now calls is_sparrow_pid(), but that only matches argv basename/-m (keepalive.py:270-281). A stale record whose PID has been reused by a different Sparrow instance/workspace still passes and is SIGTERMed at :449-455; the docstring’s “a reused pid is never signalled” claim is therefore too strong. Bind destructive authorization to the expected state-dir/instance and process generation (or an equivalently non-reusable identity), not merely “some process whose argv looks like Sparrow.”
Finally, the required live post-restart upgrade + queued-delivery witness is still explicitly absent, so it remains an approval gate. During this review the stacked base advanced to 0776ebb58b471422e4ccc1e37f857b7149f4ef9f and GitHub now reports the PR dirty; after the blockers are fixed, rebase and rerun the cumulative checks/witness on that resulting exact head.
…ith an offline rollback The keepawake run decides and logs whether to upgrade to the served target_version, and starts one detached, hash-pinned upgrade per target: snapshot, stop, install, start, verify, under STATE_DIR/upgrade.lock. Beyond the earlier review fixes: - install --no-start never starts or restarts the service, so the re-run install cannot bring the target up before the upgrade's own start and checks. - A rollback reports what happened: "rolled-back" only when the previous version is installed again and Sparrow reconnects, else "restore-failed" with the version installed. - pip check that cannot run, or times out, is a failure. - The worker re-reads the installed version under the lock and skips a target not above it; keepawake holds the lock across its probe and restart. - An attempt still started after 30 minutes is settled before any decision, and the snapshot restored if it left another version installed. - verify names the upgrade-failed health reason. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Sparrow stopped, and settles a dead attempt before any restart Review round at 0216c11 (three P1s and an interrupted-recovery gap): - The snapshot is the whole package namespace, hashed: the top-level names RECORD lists, every ag2_sparrow-*.dist-info and the console scripts. restore() removes every namespace path the snapshot does not list, copies it back and requires the namespace to hash equal to it, whatever RECORD says afterwards; "rolled-back" needs that and the reconnect, else "restore-failed" with the diff. Target-only files left by a partial install, a half-swapped dist-info, and a stale crashed attempt with target bytes beside old metadata are all restored to the exact pre-upgrade inventory. - One quiescence contract in keepalive (resolve_sparrow, which ensure_sparrow now reads too; quiesce_sparrow): a hand-made unit or an unreachable manager is refused with nothing touched, and the keepawake decision names it (hand-made-service, unit-unreachable); for our unit the unit is stopped and its process, and any proven Sparrow outside it, must be gone within the bounded wait; a lock holder still alive with a stale heartbeat is stopped and proven gone; a holder not proven Sparrow refuses the stop. Sparrow is started only through the manager that was stopped. A rollback whose stop is unproven changes no file. - keepawake settles a dead attempt under the initial lock before its listener probe or restart, so a restart never runs an unverified target. - The attempt record is a journal (snapshot, quiesced, installing, installed, restoring), written before each step and only under the lock, so a dead attempt is settled as soon as the lock is free: nothing installed and no drift means crashed (Sparrow restarted if it had been stopped); a mutating phase, drift, or an interrupted restore is restored, and an interrupted restore resumes on the next run instead of staying "crashed". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…reconcile The repository's structural rule reserves the name `reconcile` for the delivery contract's (attempt) signature; the self-upgrade settler takes (cfg, state_dir, now) and is a different operation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ow is stopped; the rollback namespace covers what the target adds - quiesce_sparrow's no-unit branch requires is_sparrow_pid before SIGTERM, as the unit branch already did for an external holder: a reused pid behind a stale or fresh lock record is refused with nothing signalled and no manager. - The namespace the snapshot and restore cover is every ag2_sparrow* top-level name in the site dir and every ag2-sparrow* script in the recorded scripts' directories, so a top-level package or console script the target introduced is removed by the rollback. - settle_attempt's separate "restoring" clause was unreachable (an interrupted restore still carries outcome started with a phase set); dropped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… and unverified files are never started - The snapshot records the full top-level listing of the site dir and of each scripts directory beside the RECORD roots; namespace, drift and restore treat every name absent from that listing as the target's, so a top-level package or console script the target introduced, prefixed or not, is removed and reported, and a foreign package that was already there is left alone. "rolled-back" needs the listing to equal the snapshot's. - _rollback starts Sparrow only after a verified restore; a restore or hash failure leaves it stopped, records restore-failed and writes the hook's notice. - keepawake settles first and then refuses to restart the listener while the journal holds a mutating or restore-failed attempt, or while settlement raised; the line carries reason=upgrade-failed. The owner's `ag2-sparrow-hatch install` (not --no-start) lifts the hold; the attempt stays recorded, so the target is not retried. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…olds Sparrow stopped load_state() mapped a missing, unreadable and malformed hatch-upgrade.json to the same empty journal, so a truncated file read as "no attempt pending" and keepawake restarted a listener over files a dead upgrade may have left unverified. Only an absent journal now means nothing is pending; any other unreadable shape raises JournalError, which recovery_blocked() and keepawake turn into the upgrade-failed hold, and failed_since() reports as a failure. The file is left in place for the owner to repair. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…art time match the record quiesce_sparrow proved only the command class (argv basename or -m ag2_sparrow), so a stale lock record whose pid had been reused by a different Sparrow instance or workspace passed and was SIGTERMed. The workspace lock now records proc_start (boot id + start ticks from /proc, additive to schema 1), and keepalive.unproven_holder() requires that start time to equal the live pid's before any signal, with no unit or beside our unit. A record without it (written before this change) or an unreadable start time is refused, with nothing signalled. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… install lifts it only over verified bytes
Three routes still ran unverified code while a failed recovery held Sparrow stopped:
- install started the service (enable --now / restart) before clear_blocked(), which
relabelled restore-failed as acknowledged without checking a byte. install now calls
release_hold() first: the hold lifts only when every RECORD row hashes equal on disk
and no unrecorded file sits in an ag2_sparrow* directory (verify_installed, which
parses RECORD itself because Distribution.files drops rows whose file is missing);
otherwise the service is installed --no-start style and install exits 1.
- keepawake still called maybe_start() while blocked, and neither maybe_start() nor the
locked worker checked recovery_blocked(); all three now refuse ("recovery-blocked"),
and a dead quiesced attempt is not restarted while another attempt holds.
- _rollback() imported the package (installed_version) after a failed restore or an
unproven stop; it now reads the metadata version instead unless the restore verified.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t wheel's own inventory Ownership was inferred from names: old RECORD roots, ag2_sparrow* names, and any top-level or scripts-dir name that appeared after the snapshot. So a target file nested in a directory that existed before (and was no old RECORD root) survived a rollback that reported no drift, and a foreign package or script another installer added in the window was deleted. The worker now downloads the pinned wheel first (pip download --require-hashes) and reads from its own RECORD and entry points every path it will install (wheel_inventory: site-relative files, .data/purelib|platlib, .data/scripts and console scripts, its whole dist-info dir; any other .data location refuses the upgrade). The snapshot stores that inventory and the directories that existed; restore removes only files in the old RECORD, the snapshot or the inventory, plus their bytecode and the directories the target created, and installs from the downloaded wheel with --no-index. A file in an ag2_sparrow* directory that is none of those is kept and reported as unknown drift, so a restore never certifies it away. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
70015e7 to
b7b9b95
Compare
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 67b3b908e4b6b86c492daa1817a66101a147786f. The round-4 patches fix the named happy paths, but two fail-closed contracts still have adjacent P1 gaps, so my request for changes remains.
P1 — preserve every recovery hold instead of rewriting the record that creates it
_upgrade_locked() correctly detects a held recovery, but then calls _finish(state_dir, target, "recovery-blocked") (self_upgrade.py:833-838). _finish() overwrites that target's existing phase (:719-723), while recovery_blocked() recognizes only mutating phases and restore-failed (:731-743). If the retried worker names the same target as the failed attempt, the refusal itself erases the hold.
Exact-head reproduction, with the service edge faked:
installed verification: changed __init__.py; unrecorded target-only module
initial outcome: restore-failed
same-target worker: recovery-blocked
hold afterward: None
next keepawake: exit 0, restart calls 1
The different-target control retains the original restore-failed phase, exits 1, and makes zero restart calls. The current test uses A=0.6.0 followed by B=0.7.0, so it misses the overwrite when the worker is also A. The same overwrite clears installing, installed, and restoring holds too.
The malformed-journal validation is also still container-only. These existing files both produced recovery_blocked=None, failed_since=None, and then a keepawake restart (rc=0, one restart):
{"attempted":{"0.6.0":{}}}
{"attempted":{"0.6.0":{"outcome":"started","at":"bad"}}}Neither shape is a journal the writer can produce, so an existing but structurally incomplete attempt must fail closed just like truncated JSON. Please preserve the phase that caused a worker refusal, validate required/known attempt fields and phases, and add same-target plus incomplete-attempt regressions. This is the README's stated invariant that no restart occurs while files may be unverified.
P1 — bind identity validation to the signal, and cover the supported Darwin path
unproven_holder() compares argv and proc_start once (keepalive.py:408-420), but the detached branch later calls _stop(pid) (:470-473), which signals the bare PID with os.kill (:294-310). The owned-unit/external-holder branch has an even wider interval because systemctl stop runs between validation and _stop (:453-465). A deterministic generation-swap at that seam produced:
detached: record boot-A:100; generation at signal boot-A:999
verdict: (True, "stopped Sparrow (pid 4242)", "detached")
signal: SIGTERM 4242 (boot-A:999)
unit/external: unit-stop, then SIGTERM 77 (boot-A:999)
verdict: success
So the new tuple narrows PID reuse but does not establish the promised “a reused pid is never signalled” property. Use a stable process handle from validation through signal where the platform provides one (this tree already has pidfd signaling precedent in connect_common.py:789-800), and fail closed otherwise.
There is also no native Darwin identity path. On this supported macOS host:
platform=darwin
/proc exists=false
proc_start(self)=None
lock payload has proc_start=false
is_sparrow_pid() and proc_start() both depend on /proc. macOS has no systemd user manager, so the documented normal install is detached; every current lock continues to lack the proof after restart. The disclosed 0.6.0 transition is therefore not temporary on macOS: unattended self-upgrade always reaches stop-failed. Add production-probe coverage for the Darwin behavior and either provide safe identity/quiescence there or explicitly prevent claiming/enabling the self-upgrade path on that supported platform.
Verified on this head:
- unreadable JSON/bad top-level types now hold;
- static start-time mismatches and missing identities are not signalled in the fixture matrix;
- install verifies RECORD bytes before clearing
restore-failed; - target-wheel inventory removes target-owned residue while preserving/reporting unknown namespace files;
test_hatch_served_setup.py: 188 passed;test_hatch_adapter.py: 218 passed, 2 skipped;tests/workspace-lock.test.py: 30 passed;- review scanner PASS;
git diff --checkclean; GitHub CI green.
The required real post-restart upgrade with a queued delivery is still absent, so that remains an approval gate after these code blockers are fixed.
…ncomplete attempt record fails closed
A worker refused with recovery-blocked called _finish(), which overwrote that target's
phase. When the refused worker named the same target as the held attempt, the refusal
replaced restore-failed (or installing/installed/restoring) with recovery-blocked, the
hold vanished and the next keepawake restarted unverified files. The refusal is now
read-only.
load_state() also validated only the container, so {"attempted": {"0.6.0": {}}} or an
attempt with a non-numeric "at" read as no hold. Every attempt record must now carry a
known outcome, a numeric at, and a known phase and string from when present; anything
else raises JournalError and holds Sparrow stopped. A worker run that maybe_start did not
record now writes a complete record first, so the writer cannot produce an incomplete one.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and macOS records carry a start time quiesce_sparrow proved a lock holder's argv and start time once, then sent SIGTERM to the bare pid, so a pid reused in between (during the detached stop, or during the unit's systemctl stop beside an outside holder) was signalled. stop_proven() now opens a pidfd first, reads the proof after it, and signals through that pidfd: the signal reaches the proven process or none, and the wait polls the pidfd. Where there is no pidfd (macOS, old kernels) the proof is read immediately before the bare signal. The proof was also /proc-only, so on macOS no lock record carried proc_start and the detached self-upgrade always stopped at stop-failed. workspace_lock.proc_start() now reads kern.boottime and kinfo_proc.p_starttime through sysctl on macOS, and proc_argv() reads KERN_PROCARGS2, which is_sparrow_pid() uses when /proc is absent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Requesting changes at exact head c245119af2c78dd17ed303e37ff4405c6cd4ea21. The round-5 changes fix the two concrete cases from my prior review, but the underlying fail-closed contracts still have two P1 gaps.
Verified fixed on this head:
- A same-target
recovery-blockedrefusal leaves the existing hold byte-identical. - The newly enumerated bad attempt fields fail closed, and an externally launched worker first creates a complete record.
- Linux opens the pidfd before proof, signals through it, and re-proves an out-of-unit holder after
systemctl stop. - The native macOS
sysctlstart-time/argv probes and real-child test pass on this Darwin host. - All current GitHub checks are green after the rerun. Locally: served-setup 193/193, adapter 218/218 (2 skipped), workspace-lock 30/30, vendored-drift/review scans/diff check all pass.
P1 — writer-impossible journal states still clear the recovery hold
load_state() validates each field independently, makes phase optional, and accepts an existing {} even though its contract says only an absent file is empty. recovery_blocked() then consults only phase, not the outcome/state-machine relationship.
Using production recovery_blocked() and keepawake() at this head:
empty-existing-object: blocked=None rc=0 restarts=1
restore-failed-without-phase: blocked=None rc=0 restarts=1
restore-failed-with-ok-phase: blocked=None rc=0 restarts=1
A positive control with {"attempted":{"0.6.0":{}}} is rejected and produces rc=1, restarts=0, so the harness exercises the intended hold path. The missed cases are all states the atomic writer cannot produce; most critically, an explicit outcome: restore-failed can restart files the journal says were never verified.
Please validate the journal as a state machine rather than as independent enums (including requiring the top-level shape), and make a restore-failed outcome fail closed regardless of a missing/inconsistent phase. Add regressions for an existing {}, restore-failed without a phase, and restore-failed paired with a terminal phase.
P1 — the no-pidfd path still has the original check/signal race
stop_proven() falls back to _stop(pid, None), whose os.kill(pid, SIGTERM) is not bound to the generation just proved. A deterministic generation swap after the final proof shows the difference:
pidfd=true: wrong_generation_signalled=false
pidfd=false: stop_proven=(True, None)
signal=(SIGTERM, pid=4242, generation=boot:999)
wrong_generation_signalled=true
This host is macOS and has no pidfd, so it takes the unsafe branch. The native sysctl identity check works, but cannot close the gap between check and signal. The new tests cover a generation swap with pidfd and a no-pidfd ordering assertion; they do not swap the process after the final no-pidfd proof. Also, _pidfd_open() turns every OSError—including resource failures such as EMFILE—into that same bare-PID fallback on Linux.
Answering the requested residual decision: yes, fail closed when there is no stable process handle. Until an identity-bound Darwin mechanism or cooperative stop exists, detached macOS self-upgrades should end stop-failed. Disabling unattended upgrade is safer than SIGTERMing an unrelated process. Unexpected pidfd-open failures must also fail closed, and the regression needs to swap generations at the final proof-to-signal seam.
Remaining approval gate
The PR is still draft and its body correctly says the required live witness is absent. After the code blockers are fixed, I still need an exact-head, real post-restart round trip: upgrade through the live service, one queued delivery surviving the stop, delivery after restart, and no repeat delivery. Please also refresh the PR body: it still says the base is #5665 although the actual base is main, stops at review round 3, and lacks round-4/5 parent-vs-head evidence and the no-pidfd guarantee/limitation.
Worst case as written: recovery can start explicitly unverified bytes, or the no-pidfd stop can send SIGTERM to an unrelated process that inherited a reused PID.
…ate machine
load_state() checked each attempt field on its own, made phase optional and accepted an
existing {} as an empty journal, and recovery_blocked() read only the phase. So {}, an
outcome restore-failed with no phase, or restore-failed beside a terminal phase such as
ok all read as no hold and keepawake restarted bytes the journal calls unverified.
The journal must now be what save_state writes: an attempted map, optional failed and
installed maps and nothing else, and each attempt at a (outcome, phase) pair the writer
produces: started with no phase or a working phase, a terminal outcome as its own phase,
or restore-failed acknowledged. Anything else raises JournalError and holds. A
restore-failed outcome holds whatever its phase unless acknowledged. A worker re-run of
an already settled target now starts a fresh record instead of journaling a working
phase over a terminal outcome.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the stop fails closed Without a pidfd, stop_proven() re-read the proof and then sent SIGTERM to the bare pid, so a pid reused after that final proof was still signalled; and _pidfd_open() turned every OSError, EMFILE included, into that same bare-pid fallback. Now only a pidfd carries the signal: where the platform has none (macOS) or pidfd_open fails, nothing is signalled and quiesce refuses, so a detached macOS self-upgrade ends stop-failed with Sparrow left running the old version. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ecoder recursion is a JournalError Attempt records require from and valid version keys; hook only with ok, manager only from the quiesced phase; failed and installed summaries require every field their writers save. Decoder recursion and any validator failure become JournalError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 5761ea4802404f2e6c8329be81d13fbca0dbbaec over 440eb9d6986561d334eb04a34321b69cd1cd79bc.
The decoder/validator-exception P1 is fixed. The specifically listed schema cases are also fixed: missing from, invalid attempted-map versions, hooks on non-ok attempts, managers in pre-quiescence phases, each required failed-summary field or one-sided outcome/problems pair, and installed summaries without hashes all hold across the consumer matrix. The three new regressions are collected and pass here; against the prior production source they produce one failure and two uncaught errors.
REQUEST_CHANGES — the broader writer-state blocker remains.
[P1] A quiesced attempt with no valid restart manager is still accepted, declared healthy, and restarted generically
_ATTEMPT accepts manager=None or any string, and _well_formed() makes the field optional. That is broader than the writer:
- successful quiescence returns exactly
unitordetached; - the
quiescedwrite always includes that manager; and start_sparrow()accepts only those two values.
For started + phase="quiesced" with manager absent, null, and "bogus", each exact-head production-consumer probe returned:
load_state=accepted
recovery_blocked=None
failed_since=None
health=(healthy, None)
release_hold=None
keepawake=(rc=0, generic_restart_count=1)
journal phase: quiesced -> crashed
The manager-specific restart correctly refuses the bad value, but settlement reports no remaining hold, so the same keepawake tick falls through to _keep_listener() and performs its generic restart. This is the original failure mode: a record no writer can leave opens the restart gate and is rewritten rather than preserved.
The adjacent state variants need to be closed in the same pass. Current positive controls explicitly bless quiesced with manager=None, success with arbitrary manager="m", and rolled-back with nonempty problems; older controls also bless success without the atomically written installed summary and post-quiescence phases without a manager. The validator also accepts rolled-back with nonempty problems and restore-failed with none, although the writer chooses those outcomes from whether problems exist.
Please model the writer's actual variants rather than adding one more isolated predicate: constrain any manager to unit|detached; require it for quiesced, installing, installed, and ok (but not universally for recovery terminals); preserve the atomic success/installed relation; enforce the failure outcome/problems relation; and drive each adjacent rejected shape through the full hold/byte-identity matrix.
Verification on Python 3.13.5:
- served setup: 208/208 pass;
- adapter: 218/218 pass, 2 platform skips;
- three new named regressions: 3/3 pass; prior source: 1 failure + 2 errors;
- independent malformed-record/exception matrix: the intended cases fail closed through load, recovery, health, release, and two keepawake ticks;
- cumulative review checks and
git diff --check: pass; the hardcoded-path scan's positive control fails as expected.
CI is not green: exact-head Python shard 6/9 timed out in tests/hermetic-env-structural.test.py at its 240-second per-file cap on both attempts (rerun). The aggregate gate therefore fails and diff coverage is skipped. This looks like a whole-tree scanner/capacity boundary, but it is not proven independent of a PR that enlarges the scanned tree; it needs a green run before merge.
Worst case after transport activation: a corrupt/interrupted quiesced journal is treated as healthy, its evidence is rewritten, and keepawake starts bytes whose recovery state was not writer-valid. Draft status and the absent transport limit current exposure.
Verdict: changes requested. Fix the remaining writer-state variants and obtain green required CI. The exact-head real post-restart upgrade plus queued-delivery/no-repeat witness remains the separate approval gate already acknowledged in the PR body.
…grade writers save Replace the field-by-field attempt checks with a table of the (outcome, phase) variants the writers leave and the manager each carries: none before quiescence, unit or detached from quiescence to success, optional where a restore may start un-quiesced. A success needs its installed summary, and a failure summary's outcome must agree with its problems. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head f6287e957fe12b7a8641ebb0345de62b45980b1d over 5761ea4802404f2e6c8329be81d13fbca0dbbaec.
The prior manager/phase blocker is fixed: quiesced/installing/installed/ok now require unit|detached; the listed outcome/problems and success/installed cases reject across the consumer matrix. Exact-head CI and CLA are green.
REQUEST_CHANGES — the current head still accepts complete journal records that no writer can leave.
[P1] Recovery history can lose or contradict the summary its writer saves atomically
The local attempt table is not enough. _ATTEMPT_VARIANTS admits the recovery terminals, while _well_formed_journal() validates failed only when it happens to be present. That leaves two independently reproduced holes.
First, this isolated record is accepted:
{"attempted":{"0.6.0":{"at":1.0,"from":"0.5.4","outcome":"restore-failed","phase":"acknowledged"}}}Exact-head production-consumer probe:
load_state=accepted
recovery_blocked=None
failed_since=None
release_hold=None verify_calls=0
keepawake_1=(0, 1)
keepawake_2=(0, 1)
bytes_unchanged=True
Each keepawake tuple is (return_code, generic_restart_count). The same missing-summary shape is accepted for crashed and rolled-back; unacknowledged restore-failed initially holds, but release_hold() verifies then rewrites the malformed record to acknowledged.
No writer can leave the minimal record above. Crash/no-snapshot settlement co-writes the terminal attempt and failed before one save; rollback passes both through the same _finish() save; and clear_blocked() changes only the phase and retains the summary. The only production removal of failed is a successful upgrade, whose same save leaves installed and an ok attempt.
Second, even when failed is present, a same-run summary whose from contradicts its attempt is accepted:
attempt.from=0.5.4 failed.from=9.9.9 outcome=crashed
load_state=accepted
recovery_blocked=None
failed_since(0.5.4)=None
keepawake=(0, 1)
bytes_unchanged=True
Both recovery writers derive those two from fields from the same previous value. A mismatch can be valid only on the retained-summary branch, where a later attempt has an outcome in KEEPS_FAILED; when the attempt itself still has the summarized recovery outcome, the fields must agree.
The tests currently encode only one direction. writer_journal() adds the required companion summary for ok, but synthesizes bare recovery terminals as positive controls, and the real-writer spy proves writer ⇒ accepted, not accepted ⇒ writer-reachable. Two no-edit mutation controls both leave all 210 tests green:
no-download manager rule: NO -> MAYBE
mutant_tests=210 failures=0 errors=0 success=True
restore-failed + acknowledged manager rules: MAYBE -> YES
mutant_tests=210 failures=0 errors=0 success=True
The second mutation rejects real managerless recovery records: a pre-quiescence snapshot with drift can enter _rollback(), which writes restoring before it obtains a manager and never persists the later stopped_by; a failed restore and clear_blocked() therefore legitimately leave managerless restore-failed/acknowledged variants.
Please validate the complete reachable journal structure, not only each attempt in isolation. Preserve historical-terminal cases: multiple failures legitimately leave one current failed, and a later success can remove it while leaving an installed witness. Add both directions to the regression: every real writer state is admitted, and adjacent non-writer states are rejected through load, recovery, health, release, two keepawake ticks, and byte identity.
Verification here: served setup 210/210; adapter 218/218 with 2 platform skips; workspace-lock 30/30; cumulative review checks and git diff --check pass; hardcoded-path positive control fires. Worst case after transport activation is that malformed surviving recovery state is reported healthy and keepawake starts potentially unverified bytes. Draft status and the absent transport limit current exposure.
Verdict: changes requested; this head is not ready to approve. After the code blocker is fixed, the PR still needs the acknowledged exact-head live post-restart upgrade + queued-delivery/no-repeat witness, and the PR body needs actual current-head before/after output (it currently ends at round 6).
…leave it, across records A crashed, rolled-back or restore-failed attempt is saved in the same write as its `failed` summary, and only a later success drops that summary. The journal now requires one of the two: the summary, or an `installed` record whose new `after` list (written by the success: every recovery attempt it followed) names the attempt. A held restore-failed never lacks its summary, and a summary that describes its own attempt carries that attempt's `from`. Tests now check both directions: every journal the production writers save, across each settled history and its reruns, is accepted, and every one-target journal shape the validator accepts is one a writer saves. Adjacent shapes and the cross-record rows are held through load, recovery, health, release, two keepawake ticks and byte identity. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ved, never order or time `installed.after` now records every attempt the journal held when the success was saved, not only the recoveries. A recovery attempt without its summary must be in that lineage, and so must every other `ok` attempt: one outside it came later and would have replaced the summary. A lineage that names the success itself, names a missing attempt, or is not sorted and unique fails closed. Neither wall-clock times, key order nor version order is consulted. Rows added for a success that time, key order and version call later but the lineage does not, a second success outside the lineage, a duplicated lineage, and a rerun that invalidates the installed reference beside an earlier success. The test file drops its itertools import, which the 3.9 stdlib fallback in the hermetic-env scan does not list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…amed fault `success_lineage(state, terminal)` walks from a surviving terminal to the installed success through the one edge that success saved, `installed.after`, and returns the chain it traversed with the fault that breaks it: no success, an equal reference, a cycle, a missing link, a fork (another success outside the lineage), a success outside the terminal's lineage, or a lineage not as its writer saves it. It never reads times, key order or versions, and never falls back to another success when the installed reference fails. The validator now returns the reason it rejects a journal (`_journal_fault`), and the regression rows assert each row's chain, fault and journal reason, then the full hold path. A new test takes a writer-saved crash-then-success journal, invalidates its reference while the installed record stays present, and shows the resolver still finds that record and rejects it as unrelated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s-field writer-impossible records are held; a rerun over a quiesced record keeps it valid Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Round 14 at 3923d4f, mapping each finding to its fix and test (all in test_hatch_served_setup.py; each new test fails at e1b7e39 and passes now).
Local: test_hatch_served_setup 218 and test_hatch_adapter 218 OK. Not done: the test-audit note about the lossy one-target projection and a 3+ attempt positive history is not addressed in this commit. CI: the only red lane is python standalone tests 5/9, a >240s timeout of tests/hermetic-env-structural.test.py (passes locally in about 2 min; the same timeout is red on main runs); rerun once, same result. |
…mpt production histories Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Round 14 follow-up (test audit), head below: the writer corpus now includes production-writer histories of 3+ attempts, a success followed by a download failure, and reruns after a rollback and after a success. test_the_accepted_set_is_the_writers_by_full_record compares every record exactly (only the clock and free text are scrubbed) and requires that no one-field mutation of a written journal in the fields its own record decides (what a rolled-back summary says is installed, the version a success names) is accepted unless a writer saves it. test_the_one_target_projection_misses_a_mutant_the_full_record_catches shows a rolled-back summary with a different installed version has the same one-target projection as the real one, and that with _failed_consistent disabled only the full comparison flags it. Not claimed: fields the record cannot decide alone (a crashed or restore-failed summary's installed version, an attempt's from beyond ok ordering, which in-flight attempts are in installed.after) are not mutation-checked, so global accepted-equals-writer-reachable equivalence is still not asserted. |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 9ceb741c4758d3959bfc9088a780a358ffbb8395 over e1b7e39840aae48cba9b26406ceb08ed5e9574f1.
The requested repairs are real: replacing an attempt now removes its stale installed.after edge; rolled-back summaries require installed == from; the quiesced-rerun paths remove a manager they did not acquire; and equal/descending ok.from values are rejected. The four named regressions pass at HEAD and fail against the exact parent implementation (2 failures, 2 errors). The expanded writer corpus and one-target mutant demonstrate the claimed full-record distinction within their stated mutation scope. Focused suites pass (test_hatch_served_setup: 220/220; test_hatch_adapter: 218/218, 2 platform skips), and review checks plus git diff --check are clean.
REQUEST_CHANGES — one fail-closed P1 remains.
[P1] An unparseable success origin is ordered as lower than every target
_journal_fault() uses (parse_version(t["from"]) or ()) < parse_version(k). A parse failure therefore becomes the empty tuple and passes the new ordering check. At exact HEAD, an ok/ok attempt for target 0.6.0 with from: "not-a-version", manager unit, and its matching installed summary produced:
fault=None
load_state=accepted
recovery_blocked=None
failed_since=None
release_hold=None
health=('healthy', None)
keepawake=(0, 1)
bytes_unchanged=True
The keepawake tuple is (return_code, generic_restart_count). Empty and 65-character invalid strings behave the same; valid equal and higher origins are correctly rejected. This case is inside the claimed ok ordering invariant, not the disclosed residual “attempt.from beyond ok ordering.”
No success writer can leave it. _upgrade_locked() parses both target and installed origin and writes not-above-installed when either is unparseable; only after that gate and a successful install does it save the ok attempt and installed summary. A writer-impossible journal is thus reported healthy and opens the restart gate.
Please reject a missing parse result explicitly and add invalid/empty/overlong origins to the full-consumer regression and full-record mutation audit. The adjacent audit also found invalid or non-lower origins accepted in variants that prove the version gate was crossed (started phases snapshot|quiesced|installing|installed, no-pip-check, and stop-failed); preserve the legitimate pre-gate unsupported-install / not-above-installed cases while applying the invariant wherever the record proves it.
CLA is green. Hosted CI is currently red after two attempts at the same reap-orphans.test.py positive-control assertion (different cwd/exec subcases); the cumulative diff does not touch process hygiene, the focused test passes locally, and the aggregate job is red only because of that lane. It appears unrelated and timing-sensitive, but current CI is still not green. After the code blocker is fixed, the exact-head live post-restart upgrade plus queued-delivery/no-repeat witness remains the separate approval gate already recorded on this draft.
Verdict: changes requested; this head is not ready to approve.
…d-back summary is a journal fault Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 7c971c863a647c5b31bca5f4f921d9c464312447 over 9ceb741c4758d3959bfc9088a780a358ffbb8395.
The two changed predicates are real: the invalid ok.from and currently summarized invalid rolled-back from now fail closed through the consumer matrix. The new named test is collected and passes; reverting either production hunk makes its corresponding subcase fail. The full served-setup suite is 221/221, child review checks and git diff --check pass, and current hosted CI/CLA are green after the unrelated 5/9 rerun.
REQUEST_CHANGES — the invalid-origin P1 is only partially fixed.
[P1] Validate origins on the attempt variants that prove the version gate was crossed, including historical rollbacks
_journal_fault() checks the attempt origin only for ok; _failed_consistent() checks a rolled-back origin only while the failed summary is still present. A later success legitimately removes that summary while retaining the earlier attempt in installed.after, so an impossible invalid rolled-back origin then escapes both checks.
Exact-head no-edit repro, with a historical 0.6.0 rollback followed by a valid 0.7.0 success:
historical_rolled_back_from=not-a-version
fault=null
load=accepted
recovery_blocked=null
failed_since=null
release_hold=null
health=[healthy,null]
keepawake=[[0,1],[0,1]]
bytes_unchanged=true
The production-writer corpus generated 16 historical rolled-back records; every origin was parseable (0.5.4). A successful rollback cannot originate from an unparseable version because it needs a snapshot, and snapshot is reached only after both versions parse. This malformed journal is therefore one no writer can leave, yet it is reported healthy and opens the restart gate, contrary to the fail-closed contract in README.md:655-667.
The adjacent scope named in the previous review is still open as well. At this head _journal_fault() returns None for invalid, empty, overlong, equal, and higher origins in all six variants:
started/snapshot
started/quiesced
started/installing
started/installed
no-pip-check
stop-failed
Those variants are downstream of the strict version check and the snapshot phase's from=previous overwrite. This does not imply globally requiring from < target: a production-writer control proves a stale pre-gate attempt can legitimately roll back from a higher installed version when a retained snapshot exists. Nor should legitimate pre-gate started / unsupported-install / not-above-installed / crashed records be tightened; writer controls show that their origin can be empty or otherwise unparseable.
Please validate parseability/order according to the attempt variant that proves the gate was crossed, validate rolled-back attempts even after their summary is removed, and extend the full-consumer/full-record mutation audit beyond its current failed.installed and installed.version mutations.
Worst case after transport activation: a present journal no writer could leave is classified healthy and keepawake starts Sparrow instead of preserving an owner-visible recovery hold. This draft is not ready to approve. After the code fix, the acknowledged exact-head post-restart upgrade plus queued-delivery/no-repeat witness remains the separate approval gate.
… origin, historical ones included Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head 4193723badee491cee84deb70dc01d6c65c8e022 (the one-commit child of 7c971c863a647c5b31bca5f4f921d9c464312447). The two new named tests pass at this head, the gated-origin test fails when run against the parent implementation, the full focused suites pass (223/223; adapter 218/218, 2 skipped), and both child/cumulative review scans are clean.
There are still blocking writer-provenance gaps:
-
[P1] Make the origin rule variant-aware for
crashedandnot-started.self_upgrade.py:173-178excludes allcrashedandnot-startedrecords from_PARSED_ORIGIN, andtest_hatch_served_setup.py:3783-3794codifies arbitrary origins for both. But acrashed/crashedrecord carrying a manager can only descend fromstarted/quiesced:settle_attempt()preserves that manager atself_upgrade.py:897-901, so this variant has crossed the snapshot/version gate. At this head, a manager-bearing crash with target0.6.0andfrom: "not-a-version"loads without a journal fault, without a recovery hold, and keepawake attempts two starts. Separately, the onlynot-startedwriter first passes_decide()'s parsed, strictly-increasing version check (self_upgrade.py:803-809) and copies that same installed value intofrombefore_finish()(861-870). Anot-startedrecord with the same malformed origin is nevertheless accepted. The early phase-less crash without a manager can remain the pre-gate exception; the writer-observable variants cannot. -
[P1] Validate a retained
restore-failedsummary independently of the replacement attempt. A later direct rerun may legitimately retain an older failure summary throughKEEPS_FAILED, but_failed_consistent()only parsesfailed.fromforrolled-back(self_upgrade.py:280-283), while the equality check at215-217is skipped once the current attempt has a different outcome. I loaded a journal whose current0.6.0attempt is validno-download/from 0.5.4, but whose retainedrestore-failedsummary hasfrom: "not-a-version"; it is accepted unchanged, produces no journal/recovery fault, and keepawake attempts two starts. That summary can only have been written after rollback began, so its own origin still needs the post-gate rule even when a later attempt replaces the keyed record. -
[P1] Enforce the ordering half of the prior gate rule for forward-only variants. The new matrix varies only non-parseable strings (
test_hatch_served_setup.py:3753-3781), and_journal_fault()still checksorigin < targetonly forok(self_upgrade.py:197-202). Equal/higher parseable origins are accepted forstarted/snapshot,started/quiesced,started/installing,started/installed,no-pip-check, andstop-failed, although each writer reaches these only afterwant > haveat1079-1083. For example,snapshotwith0.6.0 -> 0.6.0andstop-failedwith0.7.0 -> 0.6.0both load and restart here. Keep the documented higher-origin rollback case valid; this needs a forward-variant rule, not a blanket comparison.
Current hosted checks also remain red in the worker-config-directory job. The touched files and helper are byte-identical to current main and that test passed locally (4/4), so it looks unrelated, but the required hosted gate is not green. The requested real post-restart live witness is also still outstanding (the transport companion is now on main/synthetic merge, while this branch is behind it).
Verdict: REQUEST_CHANGES. Please cover the variant-aware crash/not-started cases, retained-summary provenance, and equal/higher forward-origin mutations; rerun hosted CI; then provide the live witness. I will re-review the resulting exact head.
…rget; retained restore-failed summaries and not-started and manager crashes need a version origin Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 54b163981cb36c4e8ec2163e02449d5dd2d8cb08 against direct parent 4193723badee491cee84deb70dc01d6c65c8e022 and the current-main synthetic merge.
The three code findings from my parent-head review are resolved. The validator now:
- requires a parsed, strictly lower origin for all eight forward-only forms (
startedsnapshot/quiesced/installing/installed;no-pip-check;stop-failed;not-started; manager-bearingcrashed), including historical attempts; - independently validates the origin in retained/historical
restore-failedsummaries; and - requires a parsed origin, without imposing the wrong ordering constraint, for rollback/restoring/acknowledged-restore states where a higher origin is a legitimate pre-gate recovery path.
Evidence I reproduced:
- Exact head:
test_hatch_served_setup.pyran 226/226 green;test_hatch_adapter.pyran 218 green with 2 skipped (444 tests total). - Current-
mainsynthetic merge: the 226-test served-setup suite is green. - Overlaying the exact-head tests on the direct parent fails on forward ordering and retained-summary provenance, while the positive higher-origin rollback control remains valid.
- An independent 98-case adversarial origin matrix has 0 mismatches at HEAD versus 34 on the parent; a 1,590-state production-writer corpus remains accepted.
- Child-only and cumulative
review-checks.shpass,git diff --checkis clean, and the added-line path scan found no production host path. - Exact-head hosted status is green: 35 successful Actions checks plus successful SHA-bound CLA, with 3 intentional skips. The two first-attempt failures were unrelated untouched-test/time-cap failures and passed on the same-SHA rerun.
Worst case now fails closed: a malformed or impossible journal is faulted and restart recovery remains blocked instead of silently accepting an unsafe retry. I found no remaining code-logic blocker in this head.
The remaining blocker is the required live-path evidence. The PR body still explicitly leaves the exact-head real post-restart witness unclaimed. Because this changes startup/restart and queued delivery behavior, approval still requires a real round trip at this head showing the upgrade/startup result delivered to the owner exactly once, followed by a second restart with no repeat delivery. Code review is clear to proceed to the Chi-go/live-witness step, but this PR is not merge-ready until that evidence is posted.
Verdict: REQUEST_CHANGES — code-clear; live witness only.
Live restart witness at
|
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Rechecked exact head 54b163981cb36c4e8ec2163e02449d5dd2d8cb08 and the posted live evidence. It satisfies the exact witness requested after my prior code-clear review:
- Phase 0 shows the owner-directed item queued while the gateway was down, with no receipt or archive.
- Boot 1 at this head delivered it once: one delivery log, a terminal
DELIVEREDrecord, an archive whose hash equals the queued file, and exactly one provider event whose body digest matches. - Boot 2 at the same head then drained for 79 seconds (with a 1-second scan) without naming or replaying the item: the receipt stayed byte-identical with its original mtime, the archive count stayed one, and the provider still showed the same sole event.
- The process path plus per-boot
git rev-parseassertions bind both runs to the frozen worktree; the lock records carry this PR-onlyproc_startfield, and the restored main process does not. The live gateway was restored afterward.
I independently recomputed the disclosed message hashes: the 186-byte stripped body is 307c42639999ea16b39d1aed2f94dfc00f6f06fe5ce1708b7e94885aa55d603c, and the 187-byte newline-terminated queued file is aeea91913eea954a45553ecf17b6006feb4e9a16d0b92a3db5501846b5a15ccd, matching the witness.
This closes the remaining worst-case concern from my review: an owner result being duplicated across a real restart. The witness explicitly does not exercise Hatch self-upgrade; that path is dormant on this non-Hatch host until setup transport exists. That limitation was disclosed and is outside the precise restart/delivery witness I requested, so it is not a new blocker at this unchanged head.
Current exact-head status is 35 successful Actions checks plus successful SHA-bound CLA, with 3 intentional skips and no pending or failed checks. My code verdict remains clear.
Verdict: APPROVE. This replaces my stale live-witness-only change request. The PR is still a draft, and another maintainer’s independent CHANGES_REQUESTED remains; the complete merge gate still applies.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-review at 54b16398 (PR is still a draft). The blocker from my change request at 2c138cd7 is fixed, and I checked it myself. Approving.
The no-unit stop path no longer signals an unproven pid (keepalive.py:517-523)
- The detached branch of
quiesce_sparrownow goes throughstop_proveninstead of a bare_stop(holder).stop_provenopens a pidfd first. It then requiresis_sparrow_pidand aproc_startmatching the lock record (unproven_holder,keepalive.py:429-441), and signals only through that pidfd. With no pidfd (macOS) it signals nothing. This is stricter than what I asked for. - My earlier repro, re-run with the PR's own
Quiesceharness:holder(99, fresh=False, sparrow=False)andholder(99, fresh=True, sparrow=False)both now return(False, "pid 99 holds Sparrow's lock but is not proven to be Sparrow; nothing was signalled", None)withcalls == []. A Sparrow argv with a mismatched start time is refused the same way. - The asked-for test exists:
test_a_lock_holder_not_proven_sparrow_is_never_signalled_with_no_unitcovers stale and fresh.test_a_live_holder_with_a_stale_heartbeat_is_still_stoppednow passessparrow=Trueexplicitly. - Mutations: putting
_stop(holder)back in the detached branch fails 5Quiescetests. Removing theis_sparrow_pidcheck fromunproven_holderfails 2. tests/test_hatch_served_setup.pyat this head: 225 tests OK, in an isolated HOME with theRealHolderclass excluded (I signal no process). CI: 36 success, 3 skipped, 0 failing.license/cla=success(1 context). No unresolved review threads.
Earlier non-blocking notes
- New top-level names: now covered. The scope reads the target wheel's own RECORD and sweeps
ag2_sparrow*entries (self_upgrade.py:476,:524). - Still open, not blocking: the transport that serves
target_sha256must be authenticated end to end.
Approve bar: @keweichen approved at this exact head after the live restart witness. With this review, both maintainers' change requests are cleared.
Not verified live: the witness covers delivery across two real restarts. It does not cover a Hatch self-upgrade or rollback, which is dormant on that host. That round trip is still owed once a Hatch install with setup transport exists.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approve at 54b1639 as code owner for adapters/hatch/ — agent-assisted review by Mars, Qingyun's Sutando seat, on Qingyun's instruction to review and approve Chi's PRs.
I focused on the security-relevant part of self_upgrade.py: what gets installed, and on whose word.
Trust model (two keys, sound):
- The served setup supplies
package.target_versionandtarget_sha256(a{wheel filename: sha256}map;valid_hasheschecks the names and requires 64-hex digests). _pinnedwritesag2-sparrow==<version> --hash=sha256:…to a temporary requirements file and runs pip with--no-deps --only-binary=:all: --require-hashes. The fetch downloads only that pinned wheel, and the install is--no-index --find-linksfrom that directory.- So a compromised backend can only move a host up to an already-published ag2-sparrow wheel, and a compromised index can't serve anything whose hash the backend didn't pin. No source builds, no dependency changes, and no package name chosen by the server.
Injection: version reaches both the requirements line and wheels/<version>, but parse_version is a fullmatch on a strict PEP 440 subset (at most 64 characters, digits, dots, a/b/rc, .post, .dev), and _decide refuses anything it can't parse before pip is involved. So there's no newline, option or path traversal.
Gates: the upgrade only goes up; it's refused when min_version is above the target, when the hash map is missing, with AUTO_UPGRADE=0, for an editable or URL install, and for a hand-made or unreachable unit. One attempt per target, under upgrade.lock, re-checking the installed version.
Failure handling: a sha256 snapshot of every package file is taken before anything changes. The stop is proven (quiesce_sparrow), pip check must not newly name ag2-sparrow, and the version is verified after start. Any failure restores the snapshot offline and must match it by hash. The phase journal settles a crashed attempt, and restore-failed blocks both Sparrow and any upgrade until a RECORD-verified install lifts it.
Kewei's and Susan's approvals at this head, Kewei's live exactly-once witness across two restarts (with the dormant self-upgrade path scoped out, as stated), and green CI (39 checks) cover the runtime side. I did not run the 2,452-line served-setup suite locally.
Conflict resolution: workspace_lock._payload keeps both the holder's declares and main's proc_start (both copies identical). keepalive keeps main's _stop(pid, handle) and stop_proven for the upgrader; this branch's lock-proving stop becomes stop_holder(cfg, pid), which proves the holder then calls _stop. keepawake keeps main's upgrade lock and line, and this branch's best-effort _log_line; the health row carries both the held-task and upgrade-failed problems. A served-setup test mocks open_tasks, the name this branch gave oldest_open_task. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Hatch held-task line (#5828), Hatch self-upgrade to the served target_version (#5676), the served-setup fetch from the broker (#5855), served Hatch wording (#5665), seat-bridge env and token fixes (#5893, #5859), and the claude-local/codex-local connect commands (#5860, #5761). Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Priority
Level: medium
Reason: lets a Hatch install follow the backend's target version with an offline rollback. It stays dormant until the backend has a production transport for the served setup.
Closes
N/A (proposal, no issue).
Base
main. #5665 (served wording, the setup cache and the heartbeat'spackage_version) has merged as0776ebb58; this PR was rebased ontomainin round 4 andmainwas merged in again at67b3b908. Nothing is stacked on another PR any more.Summary
The self-upgrade moved out of #5665 into this PR. The fixes for both review rounds on #5665 are in.
upgrade=off,current,below-installed,min-above-target,no-hashes,unsupported-install,running,<v>:started,<v>:attemptedor<v>:crashed.min_versionis above the target, without a validtarget_sha256map, or for an editable or URL install (direct_url.json).startedafter 30 minutes is markedcrashed, even if its target is now installed. If it left another version installed, the snapshot is restored.STATE_DIR/upgrade.lock; a second run logs "upgrade already running" and skips. Keepawake holds the same lock across its listener probe and restart, so neither can interleave with an upgrade.pip download --require-hashesof the pinned wheel; its own RECORD and entry points name every path the target installs.ag2_sparrow*entries, and whatever sits at a target path) toSTATE_DIR/rollback/<version>/, with that inventory.python -m pip install --no-index --find-links <fetched> --no-deps --only-binary=:all: --require-hashes -r <req>, with every sha256 in the map in the requirements file (pip accepts--hashonly there). Thenpip checkmust run and report no new problem naming ag2-sparrow, andinstall --no-startis re-run.install --no-startnever starts the service. It enables the unit without--nowand does not restart a changed unit. So the re-run install cannot bring the target up before the upgrade's own start and checks.pip checkmust run. One that exits with neither 0 nor 1, or times out, is a failure: before the stop, nothing is changed; after the install, the upgrade rolls back.rolled-backonly when the previous version is installed again and Sparrow reconnects.restore-failed, recording the problems and the version actually installed. Causes include a stop, restore, install or start failure, no reconnect, or the wrong version.upgrade-failed, whichverifynow names with a hint.REQUIRED hook:line from the re-run install is not applied. The hook wakes the agent once with it.This needs a live post-restart witness. It can only come once the backend has a production transport for
agent.setup.steps. Until thensetup_fetchreturns None, so the keepawake run decides nothing and never upgrades. It is not claimed here.Files to read first
adapters/hatch/self_upgrade.py, then thekeepawakeand_keep_listenerhunks inkeepalive.py, theinstall_servicemay_starthunk incli.py, and the notice branch inag2-sparrow-hatch.sh.Docs
The package README's "Served wording and self-upgrade" section describes it.
docs/is N/A for the same reason as #5665.Before / after evidence
At #5665's head, no self-upgrade exists. The review repros now each reproduce as a test that passes at this head and fails when its fix is reverted:
install --no-startstarted the unit (enable --now)rolled-backpip checkexit 127 or a timeout passedcurrentverifyraisedKeyError: 'upgrade-failed'Test plan
tests/test_hatch_served_setup.pycovers 100% ofself_upgrade.pyandruntime_text.py. pip and every subprocess are faked against a fake site-packages holding a RECORD-listed ag2-sparrow, so there is no network and no real pip. Tests for this round:Service.test_no_start_never_starts_or_restarts_the_service,Service.test_install_hands_no_start_to_the_service_steptest_a_failed_restore_is_recorded_as_restore_failed_with_what_is_installed,test_each_failed_recovery_step_is_restore_failed_never_rolled_back(stop, install, start, reconnect, wrong version)test_a_pip_check_that_cannot_run_is_a_failure(exit 127, exit 2, timeout; before and after the install)test_a_queued_target_at_or_below_what_is_installed_now_is_skipped,test_keepawake_holds_the_upgrade_lock_across_its_probe_and_restarttest_an_interrupted_upgrade_is_settled_before_the_current_check,test_a_crash_with_nothing_changed_is_marked_without_a_restoreKeepalive.test_a_failed_upgrade_is_abnormal_and_verify_names_itThese tests from the earlier round still pass:
--no-deps,--only-binary=:all:and--require-hashes, with every hash passedMutation check for this round: 19 single-line mutations of the new fixes, and every one fails its test. The earlier round's mutations were rerun: 23 still apply and every one fails its test; 4 target lines this round rewrote, and the rewritten lines were mutation-checked again.
Run locally on python3 3.14:
test_hatch_served_setup.py: 69 OKtest_hatch_adapter.py: 174 OK (skipped=2)tests/gateway-disposal-delegation.test.py: OKtools/test_no_drift.py: PASSscripts/review-checks.sh: PASSHardcoded paths
None in production code. The test fixtures use placeholder paths.
Rebased onto main (2026-10-24)
Rebased the one self-upgrade commit from
d2a0c10donto the current stack basefeat/sparrow-served-wording@cad22997(#5665's tip, which already contains all oforigin/main; 76 main commits and #5665's later review rounds come in through it). Old head146757f4, new head0216c111. The stack is unchanged: base stays #5665, merge order #5665 then this.Conflicts and how each was resolved:
adapters/hatch/cli.pyinstall_servicesignature: kept feat(ag2-sparrow): serve the Hatch worker's wording from the backend, with a built-in fallback #5665'smxidparameter andboolreturn ("left stopped" fails the install), and added this PR'smay_start; the call site passes bothmxid=args.mxidandmay_start=not args.no_start.adapters/hatch/cli.pyenable/restart: keptreturn Falseon a failed enable and feat(ag2-sparrow): serve the Hatch worker's wording from the backend, with a built-in fallback #5665's_systemctl((...))call form for the restart, with this PR'smay_startguard on it.adapters/hatch/keepalive.py_served_setup: kept feat(ag2-sparrow): serve the Hatch worker's wording from the backend, with a built-in fallback #5665'sdiagnostics.describe(e)in the NOTE, added this PR'sreturn None.tests/test_hatch_served_setup.pyimports: union of both sides (threading,time;diagnostics,gateway_credentials,outbox).Semantic follow-ups the auto-merge did not catch:
self_upgrade.stop_sparrowusedkeepalive._service_managed()and_cli._systemd_run([...]), the first gone since feat(credentials): one owner for gateway credential precedence and masking (prerequisite for #5665) #5698 reached the base (_systemd_runis still re-exported by cli.py; the switch to_systemctlmatchesrestart_listenereither way). It now followsrestart_listener:_unit_owner(), fail-closed onUNIT_UNREACHABLE(not stopped, says why),_systemctl(("--user", "stop", ...))for our unit. The test covers the unreachable case.--no-startthat service is now left stopped withnot started (--no-start)(keepawake starts it on its next run, as the flag's help says); this PR's "the unit changed but was not restarted (--no-start)" branch remains for the stop-failed path, and the base's "it is restarted after the install instead" line says "it keeps running the old unit (--no-start)" there instead.test_no_start_never_starts_or_restarts_the_serviceasserts both paths; nothing was dropped as moot.Run at
0216c111(python3 3.14):test_hatch_served_setup.py154 OK;test_hatch_adapter.py214 OK (skipped=2);scripts/review-checks.shPASS;git diff --checkclean.Review round 2 (2026-10-24):
0216c111->6cb77fc5->2c138cd7One commit on
0216c111(plus2c138cd7, a rename only: the settler issettle_attempt, since the repository reserves the namereconcilefor the delivery contract's(attempt)signature), answering keweichen's three P1s and liususan091219's interrupted-recovery finding. Still a draft: the live post-restart upgrade/delivery witness is owed once the backend servesagent.setup.steps, and is not claimed here.P1 rollback certified a mixed install. The snapshot is now a hashed inventory of the whole package namespace, taken before anything changes: the top-level names RECORD lists, every
ag2_sparrow-*.dist-infowhatever its version, and the console scripts (self_upgrade.py:217namespace,:237snapshot).restore()(:283) never reads RECORD or_distribution(): it removes every namespace path the snapshot does not list, copies the snapshot in, and requires the namespace to hash equal to it (namespace_drift,:269); a mismatch raises, so_rollbackrecordsrestore-failedwith the diff and neverrolled-back. Tests compare the restored namespace to an independently hashed pre-upgrade inventory and prove the target-only files are gone:test_a_partial_install_that_added_target_only_files_is_removed_exactly(nonzero pip after target bytes + a target-only module, old dist-info kept),test_a_partial_install_with_a_half_swapped_dist_info_is_removed_exactly,test_a_stale_attempt_that_left_target_residue_beside_old_metadata_is_restored(Kewei's shape: the version still reads0.5.4, the stale RECORD cannot name the module;settle_attemptrestores on drift, not on the version), andtest_a_restore_that_does_not_hash_equal_to_the_snapshot_is_restore_failed.P1 no proof of quiescence. One contract in keepalive, read by
ensure_sparrowas well (keepalive.py:392resolve_sparrow: owner, the lock holder's pid if still alive whatever its heartbeat, and whether the heartbeat is fresh;:463):quiesce_sparrow(:413) enumerates owner in {ours, other, UNIT_UNREACHABLE, None} x holder in {none, live, stale-heartbeat-but-live}.otherandUNIT_UNREACHABLEare refused with nothing signalled, and_deciderefuses them before any worker is spawned (self_upgrade.py:400,:403:hand-made-service,unit-unreachableon the keepawake line). Forours, the unit is stopped and itsMainPIDmust be gone within the bounded wait (_wait_gone,:404); a lock holder beside the unit is stopped only when proven Sparrow, else the whole stop is refused; a stale-heartbeat holder that is still alive is SIGTERMed and must be gone; a final re-read of the lock must show no live holder.start_sparrow(cfg, manager)(self_upgrade.py:512) starts only through the manager that was stopped (unitneeds the unit still ours,detachedneeds no unit). A rollback whose stop is unproven changes no file (_rollback,:522). Tests: classQuiesce(hand-made unit, unreachable manager, our unit stopped and proven gone / still running, inactive unit beside an external holder, external holder not proven Sparrow, stale-heartbeat live holder, SIGTERM ignored, holder still in the lock,ensure_sparrowreads the same resolution fresh-only),test_stop_is_keepalive_quiescence_and_start_keeps_the_manager,test_a_rollback_whose_stop_is_unproven_changes_no_file,test_a_hand_made_or_unreachable_unit_is_never_upgraded_unattended.P1 keepawake restarted before reconciling.
keepawakesettles a dead attempt under the initial lock before_keep_listenerprobes or restarts anything (keepalive.py:553,_settle_upgrade:593); that verdict is the line'supgrade=value.test_keepawake_settles_a_dead_upgrade_before_it_restarts_the_listeneruses the exact state (stale started attempt, target files in place, listener down) and asserts the event orderstop, install, start, version, restart@0.5.4with the log linelistener=down restarted=1 upgrade=0.6.0:rolled-back.Interrupted recovery (Susan). The attempt record is a journal:
_phase(self_upgrade.py:492) writessnapshot,quiesced,installing,installed(:607,:611) andrestoring(:528) before each step, under the lock only.settle_attempt(:448) therefore settles an attempt with any phase as soon as the lock is free, and a phase-less one after 30 minutes: nothing installed and no drift iscrashed(Sparrow restarted through its manager if the attempt had stopped it); a mutating phase, drift, orrestoringis restored, andcrashed/rolled-back/restore-failedare written only when settled.test_an_interrupted_restore_resumes_on_the_next_runkills the restore withSystemExitthe way the keepawake timeout does, asserts the phase isrestoringand the target still installed, and the next run restores the exact inventory. Alsotest_a_crash_after_the_stop_restarts_sparrow_through_its_manager_only,test_a_dead_attempt_in_a_mutating_phase_is_settled_at_once,test_a_mutating_crash_with_no_snapshot_is_restore_failed_not_rolled_back,test_the_attempt_journal_names_each_phase_and_the_manager.Mutation check for this round, one test each: removing the dist-info name match, the drift check in
settle_attempt, the post-restore hash verification, theotherrefusal, the reconcile-before-probe order, the unproven-stop abort, and therestoringphase write each fail their test.Run at
2c138cd7(python3 3.14):test_hatch_served_setup.py175 OK;test_hatch_adapter.py214 OK (skipped=2);tests/delivery-core-enforcement.test.py15 OK;scripts/review-checks.shchild-only and cumulative PASS;git diff --checkclean. CI at6cb77fc5was red on two shards: 5/9 was thereconcilename rule (fixed by the rename); 2/9 wastests/start-cli-private-socket-foreign-core.test.pyfailing in its own tempdir cleanup (Directory not empty), a test that drivessrc/agent/start-cli.shwith stubs and never reaches this package; this PR changes nosrc/file, and the test passes 4/4 locally at this head.Transport note (non-blocking, for the transport PR).
target_sha256only guards against a tampered index or wheel;target_versionand the hashes arrive in the same served payload, so the eventual transport foragent.setup.stepsmust be authenticated end to end, and its PR should say how.Review round 3 (2026-10-24):
2c138cd7->2c8aa473->70015e7cTwo commits on
2c138cd7(the first landed before keweichen's round-3 review arrived, so the rest went into a second; nothing amended). Still a draft; the live post-restart upgrade/delivery witness is still owed and not claimed.P1 no-unit stop SIGTERMed an unverified pid (Susan, Kewei).
quiesce_sparrow's detached branch now requiresis_sparrow_pid(holder)before_stop(keepalive.py:449), exactly as the unit branch does for an outside holder: a live pid behind a stale or fresh lock record that is not proven Sparrow is refused with nothing signalled and no manager.Quiesce.test_a_lock_holder_not_proven_sparrow_is_never_signalled_with_no_unitcovers both heartbeats with zero signal calls;test_a_live_holder_with_a_stale_heartbeat_is_still_stoppednow marks its pid Sparrow explicitly.P1 rollback scope came from the old RECORD (Kewei; Susan non-blocking).
_record_scope(self_upgrade.py:210) records, before any mutation, the full top-level listing of the site dir and of each scripts directory beside the RECORD roots._owned_tops/_owned_scripts(:227,:240) treat every name absent from that listing, everyag2_sparrow*name and the RECORD roots as the package's;namespace,namespace_driftandrestore(:252,:297,:315) all read that scope, so a top-level package or console script the target introduced, prefixed or not, is reported asaddedand removed, while a foreign package already present is left alone.test_a_target_that_adds_a_top_level_package_and_a_script_is_rolled_back_wholeinstalls a target addingag2_sparrow_newtop/,sparrow_vendor/,bin/ag2-sparrow-newcmdandbin/sprw-toolbeside a pre-existingother_pkg/, and asserts both the independent inventory and the site listing equal the snapshot's after the rollback (full path and stale-attempt path). Removing either listing diff fails it.P1 failed recovery still launched unverified bytes (Kewei).
_rollback(:584) starts Sparrow only after a verified restore; a restore or hash failure recordsrestore-failedwith "Sparrow is left stopped" (:602) and_finishwrites the hook's wake notice (:539).keepawakesettles first and then refuses_keep_listenerwhilerecovery_blocked(:546: any attempt in a mutating phase orrestore-failed) or a settlement that raised says so (keepalive.py:557-564,_settle_upgrade:603); the line carriesrestarted=0 reason=upgrade-failed, rc 1. The owner'sag2-sparrow-hatch install(not--no-start) lifts the hold throughclear_blocked(self_upgrade.py:556,cli.py:756); the attempt stays recorded, so the target is never retried. Tests:test_a_failed_restore_is_recorded_as_restore_failed_with_what_is_installed(no start after the failed restore),test_a_restore_that_does_not_hash_equal_to_the_snapshot_is_restore_failed(now asserts no start),test_a_failed_restore_holds_keepawake_restarts_until_the_owner_installs,test_keepawake_never_restarts_on_unverified_files_or_a_failed_settlement(dead mutating attempt with no snapshot; settlement raising), and in the adapter suitetest_an_install_that_starts_clears_a_failed_upgrades_hold_and_no_start_does_not.Dead clause (Susan).
settle_attempt's separatephase == "restoring"test was unreachable (an interrupted restore still carriesoutcome: startedwith a phase set); dropped.Mutation check, one test each: the no-unit
is_sparrow_pidgate, the site-listing diff, the bin-listing diff, the no-start-after-failed-restore branch, the keepawake hold, and the raise-blocks path each fail their test when removed.Run at
70015e7c(python3 3.14):test_hatch_served_setup.py179 OK;test_hatch_adapter.py216 OK (skipped=2);tests/delivery-core-enforcement.test.py15 OK;scripts/review-checks.shchild-only and cumulative PASS;git diff --checkclean.Still open, non-blocking: the transport that serves
target_sha256must be authenticated end to end (for the transport PR).Review round 4 (2026-10-24):
70015e7c->b7b9b950Rebased onto
main(3f38f181f) withgit rebase --onto origin/main cad229978: #5665's squash0776ebb58has the same tree ascad229978, so only this PR's 5 commits replayed, with no conflicts. Four commits answer keweichen's round-4 review; each test failed on the parent code and passes at the head.95a78559b). Only an absenthatch-upgrade.jsonmeans nothing is pending; an unreadable one raisesJournalErrorand holds. Parent: a truncated{"attempted":gave(rc, restarts, spawns) = (0, 1, 0); head:(1, 0, 0),reason=upgrade-failed. Testtest_an_unreadable_journal_holds_restarts_and_only_an_absent_one_means_none.9740c60cb). The workspace lock recordsproc_start(additive to schema 1); a holder is signalled only when its live start time equals it. Parent: a reused pid was SIGTERMed ((True, 'detached', [('term', 4242)])); head: refused. TestQuiesce.test_a_reused_pid_running_another_sparrow_is_never_signalled.ca60d2123). Install callsrelease_hold()beforeinstall_service: arestore-failedhold lifts only when every RECORD row hashes equal on disk (RECORD parsed directly, sinceDistribution.filesdrops rows whose file is missing) and no unrecorded file sits in anag2_sparrow*directory. Keepawake,maybe_startand the worker refuse while held; a failed restore never imports the package. Parent:enable --nowran with the phase stillrestore-failed; spawns 1 for a newly served target;installed_version()imported after the failure. Head: none of these. TestsService.test_a_failed_recovery_hold_is_lifted_only_by_verified_files_and_before_any_start,test_a_held_recovery_starts_no_upgrade_and_the_worker_refuses_one,test_a_failed_restore_never_imports_the_unverified_package,test_the_installed_files_verify_only_when_every_recorded_hash_matches.b7b9b950b). Parent: a target file nested in a pre-existing foreign directory survived with no drift, and a foreign package and script added after the snapshot were deleted. Head: the former is removed, the latter kept and uncounted; an unknown file inag2_sparrow*is kept and reported, so the restore isrestore-failed. Teststest_a_target_file_nested_in_a_foreign_directory_is_rolled_back,test_a_foreign_addition_after_the_snapshot_is_neither_deleted_nor_counted,test_the_target_inventory_is_read_from_its_wheel_before_anything_changes,test_an_unknown_file_in_the_package_directory_is_kept_and_never_certified. Inventory checked against a real built 0.6.0 wheel (115 paths, all 5 console scripts).Review round 5 (2026-10-24):
67b3b908->c245119a089dc124a). The worker'srecovery-blockedrefusal is read-only. Parent: a same-target refusal replacedrestore-failedwithrecovery-blocked, so the next keepawake restarted. Head: the journal is byte-identical and keepawake gives(1, 0)forrestore-failed,installing,installedandrestoring. Testtest_a_same_target_refusal_leaves_the_hold_exactly_as_it_was; alsotest_an_attempt_record_the_writer_cannot_produce_holds_restarts.c245119af).stop_proven()opens a pidfd, then proves, then signals through it. On macOSproc_start()readskern.boottime+kinfo_proc.p_starttimeandproc_argv()readsKERN_PROCARGS2through sysctl. Parent: a pid swapped after the proof was signalled (('term', 4242, 'boot:999')); on macOSproc_startwas None. TestsQuiesce.test_a_pid_reused_between_the_proof_and_the_signal_is_never_signalled,test_the_holder_is_signalled_through_a_handle_opened_before_its_proof,RealHolder, and the workspace-lock start-time test.Review round 6 (2026-10-24):
c245119a->551cc8c6fe704bb13). The file must be whatsave_statewrites (anattemptedmap, optionalfailed/installedmaps, nothing else), and each attempt a legal(outcome, phase):startedwith no phase or a working phase (snapshot,quiesced,installing,installed,restoring); a terminal outcome as its own phase; orrestore-failedacknowledged. Anything else holds;restore-failedholds whatever its phase unless acknowledged. A worker re-run of an already settled target starts a fresh record, so the writer never journals a working phase over a terminal outcome. Parent: an existing{}read as no hold. Testtest_a_journal_state_the_writer_cannot_reach_holds_restarts({}, restore-failed with no phase /ok/rolled-back/installing, and eight more impossible pairs, plus legal controls).551cc8c6d). Only a pidfd carries the signal. Where the platform has none (macOS) orpidfd_openfails (EMFILE, EPERM, ...), nothing is signalled and quiesce refuses, so a detached macOS self-upgrade endsstop-failedwith Sparrow left running the old version. Guarantee: on Linux with pidfds, the SIGTERM reaches the process whose argv and start time were proven, or none. Limitation: on macOS (and Linux without pidfds) unattended self-upgrade of a detached Sparrow does not stop it, and so never upgrades it, until an identity-bound or cooperative stop exists. Parent: with no pidfd, a pid swapped after the final proof was signalled ((True, [('term', 4242, 'boot:999')])); head:(False, []). TestQuiesce.test_with_no_stable_handle_nothing_is_signalled_and_the_stop_fails(no pidfd, EMFILE, EPERM; detached and beside our unit; the upgrade's stop has no manager) andRealHolderon macOS.Run at
551cc8c6on macOS (python3 3.14):test_hatch_served_setup.py195 OK;test_hatch_adapter.py218 OK (skipped=2);scripts/review-checks.shPASS;git diff --checkclean.The live post-restart round trip (upgrade through the live service, one queued delivery surviving the stop, delivered after restart, no repeat) is not claimed here.
Review rounds 7-12 (2026-10-25), rebuilt from the commits and review threads
The body last recorded round 6. These sections were rebuilt from each commit message and the review it answered. The output quoted at the time was not kept, so none is reproduced here.
551cc8c6->67ab29c8. Afailedorinstalledsummary must name an attempt saved with it. Before,{"attempted": {}, "failed": {... restore-failed ...}}read as no hold, so keepawake restarted andrelease_hold()returned None.67ab29c8->92018c0b. A newer attempt keeps a summary only when its writer does.failedsurvives only a started rerun or one ending in a non-summary outcome;installedsurvives anything butokandnot-started. Everyatmust be finite: NaN, inf and out-of-range ints held nothing before.92018c0b->4a4cd735. Validity no longer rests on wall-clock order. Every time must lie in 0 .. now +CLOCK_SKEW_S, and a summary's own attempt is told from a rerun by outcome alone. Duplicate member names are rejected while parsing. The spawn-failurenot-startedwrite moved under the lock, and only over the recordmaybe_startwrote.4a4cd735->440eb9d6. Every field of every record is type-checked as its writer saves it, before any membership test. Any validator exception is aJournalError, so"outcome": []holds instead of raisingTypeError.440eb9d6->5761ea48. Attempts needfromand a version key;hookonly withok;manageronly from quiescence; summaries need every field their writers save. Decoder recursion is aJournalError.5761ea48->f6287e95. The field checks became a table of the(outcome, phase)variants the writers leave, with the manager each carries. A success needs itsinstalledsummary, and a failure summary's outcome must agree with itsproblems.Review round 13 (2026-10-25):
f6287e95->228cc369->ae93e998->e1b7e398Three commits, none amended. The first adds the cross-record rules, the second makes provenance the whole attempt lineage, and the third resolves that lineage as a chain with a named fault.
P1: cross-record recovery history. The table validated each attempt alone. A crashed, rolled-back or restore-failed attempt is saved in the same write as its
failedsummary, and only a later success drops that summary.installed.after: every attempt the journal held when it was saved, sorted. That is the only "ordered after" evidence. Wall-clock times, key order and version order are never read, and neither is a matching target or version.success_lineage(state, terminal)walks from the terminal to the installed success through that edge. It returns the chain it traversed and the fault that breaks it:no success,an equal reference,a cycle,a missing link,a fork(another success outside the lineage),a success outside its lineage, ora lineage not as its writer saves it. When the installed reference fails, it never falls back to another success.failedsummary needs a fault-free chain to the installed success. A held (unacknowledged)restore-failednever lacks its summary, since nothing can succeed while it holds. Whenfaileddescribes the same attempt (same target, same outcome), itsfrommust equal the attempt'sfrom; an unrelated historical terminal is not forced to match._journal_fault).load_statestill raisesJournalErrorfor any fault, and for any validator exception.Tests (
test_hatch_served_setup.py, classUpgrade):test_every_journal_the_real_writers_save_is_one_variant: writer -> accepted. The production writers run every ending of a run on 0.6.0: success, each failure step, and each settlement of a dead attempt (before quiescence, quiesced, mutating; with and without a snapshot or drift; restore ok or failed; acknowledged). Each ending runs after each settled history (none; a success; a crash; a rollback; a held then acknowledged restore, with and without a manager; each after a success), with an owner reinstall of the same or another version between. Second-target histories (sequential failures, failure -> success -> rerun, two successes in sequence) are included. Every journal they save is accepted.test_a_journal_is_accepted_exactly_when_a_real_writer_saves_its_shape: accepted -> reachable, and the reverse, over every one-target shape: outcome x phase x manager x hook x summary kind with matched or mismatchedfromxinstalled, 21,280 shapes.test_a_journal_one_step_from_a_writers_holds_everywhere: each written shape with its manager flipped, its summary dropped or itsfrommismatched, where no writer saves the result. Each is held throughload_state,recovery_blocked,failed_since, the health row,release_hold, two keepawake ticks, and byte identity.test_recovery_history_is_accepted_only_as_its_writers_leave_it. Positive rows, each located among the production writers' own saves: a historical terminal beside the current matched summary, the managerlessrestore-failedand acknowledged variants, failure -> success, failure -> success -> direct rerun, and two successes in sequence. Thirteen negative rows, counted in the test, each asserting its traversed chain, its lineage fault and the journal's reason, then the full hold path above.test_an_invalidated_reference_never_falls_through_to_another_success. It takes a writer-saved crash -> success journal, whose chain["0.6.0", "0.7.0"]has no fault. It drops the edge while the installed record stays present: the resolver still reaches that record and rejects it asa success outside its lineage. It then moves the reference onto a crashed rerun beside the earlier success:["0.8.0", "0.8.0"],an equal reference, with no fallback to0.7.0. Both are held everywhere.Reviewer's two shapes, parent vs HEAD (
keepawaketuples are(return_code, generic_restart_count)):Negative rows: parent result -> HEAD result | row | HEAD reason. At the parent,
installed.afteris stripped, since the parent schema has no such field.Named tests, raw output (temp paths redacted to
<worktree>/). Command, run at each SHA with these tests:python3 packages/ag2-sparrow/tests/test_hatch_served_setup.py -v Upgrade.test_a_journal_is_accepted_exactly_when_a_real_writer_saves_its_shape Upgrade.test_a_journal_one_step_from_a_writers_holds_everywhere Upgrade.test_recovery_history_is_accepted_only_as_its_writers_leave_it Upgrade.test_an_invalidated_reference_never_falls_through_to_another_success Upgrade.test_every_journal_the_real_writers_save_is_one_variantParent
f6287e957fe12b7a8641ebb0345de62b45980b1d:HEAD
e1b7e39840aae48cba9b26406ceb08ed5e9574f1:The writer -> accepted test passes at the parent as well, as it should: the parent rejects none of the writers' journals. Its gap was accepting journals no writer saves.
Whole file, collected counts (
python3 packages/ag2-sparrow/tests/test_hatch_served_setup.py). At the parent, the two older tests fail only because their fixtures now carryinstalled.afterandself_upgrade.RECOVERY_OUTCOMES, which the parent lacks.Mutation controls. Each mutation was applied to
self_upgrade.pyat HEAD, the five named tests were run, and the file was restored:In-process, every entry of the variant table was also set to each other manager rule, and removed: 57 mutants. Each makes the accepted and written shape sets differ, or makes a writer's journal rejected. All 57 were caught.
Run at
e1b7e398on macOS (python3 3.14):test_hatch_served_setup.py: 214 OKtest_hatch_adapter.py: 218 OK (skipped=2)tests/workspace-lock.test.py: 30 OKtests/hermetic-env-structural.test.py: 26 OKtools/test_no_drift.py: PASSscripts/review-checks.shon this round's diff and on the cumulative diff: PASSgit diff --check: cleanCI at
e1b7e398: 35 pass, 3 skipped. The first run failed intests/skills/process-hygiene/reap-orphans.test.py, which this PR does not touch and which also fails onmainCI; it passed on the rerun. At the two intermediate heads, the same kind of unrelated failure appeared intests/skills/worker-pool/worker-config-directory.test.py, which also fails onmain. At228cc369,tests/hermetic-env-structural.test.pyalso caught a real problem: the test file importeditertools, which is missing from its Python 3.9 stdlib fallback list. That import was removed inae93e998.🤖 Generated with Claude Code
hermetic-env-structural timing (not caused by this PR). Run side by side on one machine,
tests/hermetic-env-structural.test.pytakes 106 s on this head and 109 s on origin/main (26 tests, OK). Two whole-tree scan tests take about 48 s each on both, so the 240 s per-file cap is a runner-capacity boundary, not a cost this PR adds.