diff --git a/.gitignore b/.gitignore index 2e760983..a704b4b4 100644 --- a/.gitignore +++ b/.gitignore @@ -17,6 +17,8 @@ dist/ /data/ # Self-edit rollback marker — runtime state, survives restart, never committed. /.selfedit-pending.json +# The config a failed boot was rolled back from — holds owner_handles, like config.yaml. +/.config-failed.yaml # Secrets — keep dir structure, never the real secret files /secrets/* !/secrets/README.md diff --git a/docs/CONFIG.md b/docs/CONFIG.md index 12bc2b4c..3b586745 100644 --- a/docs/CONFIG.md +++ b/docs/CONFIG.md @@ -151,12 +151,13 @@ matters because a bare `owner_handles: +15551234567` parses as the integer and lists work. It exists so `install.sh` scripts set keys byte-exactly instead of the model retyping YAML (#185). -**`config.yaml` is gitignored, so the self-edit seatbelt cannot roll back a bad -config write** — git only restores what it tracks, and tracking this file would -push `owner_handles` to a published repo. The pre-restart config gate stops a -config that will not *load*; `data/config-history/` is what a config that loads -but is wrong gets restored from. Never hand-edit config from a package install; -use `config_apply`. +**`config.yaml` is gitignored, so git is not what rolls back a bad config +write** — git only restores what it tracks, and tracking this file would push +`owner_handles` to a published repo. `data/config-history/` is the seatbelt +instead: on a boot that fails after a restart, the newest snapshot is put back +automatically (see below). The pre-restart config gate still stops a config +that will not *load*. Never hand-edit config from a package install; use +`config_apply`. ### Recovering a bad config @@ -172,11 +173,19 @@ diff data/config-history/.yaml config.yaml # what this change did cp data/config-history/.yaml config.yaml # put it back, then restart ``` -The newest entry is what is live now, so the one to restore is usually the -second-to-last. The last 20 changes are kept. A config too broken to boot never -enters the history — every entry is one that *booted*, which is not the same as -one that was right: the config you most often need to restore *from* is a bad -one that came up fine (`gate: {approved: ["*"]}` parses and boots). +Restoring by hand, the newest entry is usually what is live now, so the one to +copy back is the second-to-last. The last 20 changes are kept. A config too +broken to boot never enters the history — every entry is one that *booted*, +which is not the same as one that was right: the config you most often need to +restore *from* is a bad one that came up fine (`gate: {approved: ["*"]}` parses +and boots). + +**Automatic restore.** When a restart's boot fails, the recovery puts the +**newest** snapshot back on its own — newest, not second-to-last, because the +config that just failed never booted and so never got snapshotted. The config +it replaces is kept at `.config-failed.yaml` in the repo root (gitignored, same +handles as `config.yaml`), so a hand edit is never silently discarded, and the +restart notice on the requesting thread names both. ## On-disk layout @@ -197,7 +206,8 @@ one that came up fine (`gate: {approved: ["*"]}` parses and boots). | `restart_notice.json` | the thread that asked for a restart | | `chief.log` | daemon stdout/stderr on macOS / `--no-service` (systemd uses journald) | -Also gitignored at repo root: `.selfedit-pending.json` — the rollback marker. +Also gitignored at repo root: `.selfedit-pending.json` (the rollback marker) and +`.config-failed.yaml` (the config a failed boot was rolled back *from*). Runtime state that must survive restart and must never be committed. ### `secrets/` — `secrets/*` ignored, `README.md` tracked diff --git a/src/chief/adapters/imessage.py b/src/chief/adapters/imessage.py index 511b2e91..11489871 100644 --- a/src/chief/adapters/imessage.py +++ b/src/chief/adapters/imessage.py @@ -32,7 +32,7 @@ run_jxa_subprocess, ) from chief.adapters.imessage_store import RecentDedup -from chief.selfedit.recovery import RestartBoundary +from chief.selfedit.restart import RestartBoundary logger = logging.getLogger(__name__) diff --git a/src/chief/adapters/imessage_fifo.py b/src/chief/adapters/imessage_fifo.py index 3d071046..243cd387 100644 --- a/src/chief/adapters/imessage_fifo.py +++ b/src/chief/adapters/imessage_fifo.py @@ -11,7 +11,7 @@ from collections.abc import Awaitable, Callable from chief.adapters.base import Message -from chief.selfedit.recovery import RestartBoundary +from chief.selfedit.restart import RestartBoundary logger = logging.getLogger(__name__) diff --git a/src/chief/agent/restart_gate.py b/src/chief/agent/restart_gate.py index 51439ec8..1af82764 100644 --- a/src/chief/agent/restart_gate.py +++ b/src/chief/agent/restart_gate.py @@ -2,7 +2,7 @@ it before exec. Kept apart from the session so the turn path stays focused on assembling and -running turns. ``chief.selfedit.recovery.RestartController`` is the production +running turns. ``chief.selfedit.restart.RestartController`` is the production implementation; ``_NullGate`` is the no-op used by tests and non-daemon runs. """ diff --git a/src/chief/app.py b/src/chief/app.py index 7bbe9f58..b67a80c3 100644 --- a/src/chief/app.py +++ b/src/chief/app.py @@ -33,7 +33,7 @@ from chief.persistence.store import MessageStore from chief.provider.base import Provider from chief.selfedit.pipeline import SelfEditPipeline -from chief.selfedit.recovery import RestartController +from chief.selfedit.restart import RestartController from chief.skills import SkillLibrary from chief.strangers import StrangerLog from chief.tools import ToolContext, ToolDispatcher, ToolRegistry diff --git a/src/chief/config/history.py b/src/chief/config/history.py index ad66954e..9af8094b 100644 --- a/src/chief/config/history.py +++ b/src/chief/config/history.py @@ -18,6 +18,7 @@ record of every *change* rather than of every restart. """ +import os from datetime import datetime from pathlib import Path @@ -31,13 +32,70 @@ def snapshots(history_dir: Path) -> list[Path]: - """Every snapshot, oldest first. The last entry is what is live now, so - the previous config is the second-to-last.""" + """Every snapshot, oldest first. + + Name order is write order (see :data:`STAMP_FORMAT`), and only a config + that booted is ever written here — so the last entry is the last config + known to come up, which is what :func:`restore_newest` puts back. + """ if not history_dir.is_dir(): return [] return sorted(history_dir.glob("*.yaml")) +def _write_private(path: Path, data: bytes, mode: int) -> None: + """Write ``data`` to ``path`` at ``mode``, never wider and never partial. + + Created at 0600 rather than written and chmod'd after: these files hold + ``owner_handles``, and the plain write would leave one world-readable at + the umask default for the breath in between. Renamed into place because + the restore overwrites the live config at the moment the rollback marker + is already gone — a crash mid-write there would leave a truncated config + and nothing left to undo it with. + """ + tmp = path.with_name(path.name + ".tmp") + fd = os.open(tmp, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600) + with os.fdopen(fd, "wb") as handle: + handle.write(data) + tmp.chmod(mode) + os.replace(tmp, path) + + +def restore_newest( + history_dir: Path, config_path: Path, failed_copy: Path +) -> Path | None: + """Put the newest snapshot back, keeping the config it replaced. + + The boot-failure undo for a config change, since git cannot roll back a + file it does not track. The **newest** entry is right here, not the + second-to-last: a config that failed to boot never got snapshotted, so + the last entry is still the last one that came up. + + The config being replaced is saved to ``failed_copy`` first — it may hold + an edit the owner still wants, and a failed boot is not a reason to + discard it silently. Returns the snapshot restored, or ``None`` when + there is no history or the config already matches it (nothing to undo, + and the caller must not reboot into an unchanged state). + """ + kept = snapshots(history_dir) + if not kept: + return None + newest = kept[-1] + mode = newest.stat().st_mode & 0o777 + if config_path.is_file(): + current = config_path.read_bytes() + if current == newest.read_bytes(): + return None + live = config_path.stat().st_mode & 0o777 + failed_copy.parent.mkdir(parents=True, exist_ok=True) + _write_private(failed_copy, current, live) + # The narrower of the two: an owner who ran `chmod 600 config.yaml` + # must not have it widened again by a snapshot taken before they did. + mode &= live + _write_private(config_path, newest.read_bytes(), mode) + return newest + + def snapshot(config_path: Path, history_dir: Path, now: datetime) -> Path | None: """Copy ``config_path`` into the history, unless it is already the newest. @@ -64,8 +122,7 @@ def snapshot(config_path: Path, history_dir: Path, now: datetime) -> Path | None # ``copyfile``'s mode handling — which drops the source's bits, so a # ``chmod 600 config.yaml`` would have yielded a 0644 copy of the handles # the owner had just narrowed. - written.write_bytes(current) - written.chmod(config_path.stat().st_mode & 0o777) + _write_private(written, current, config_path.stat().st_mode & 0o777) # Skip the file just written: a box that boots with its clock set ahead # leaves a future-stamped entry sorting last forever, and once KEEP of # them exist this loop would otherwise unlink the live snapshot. diff --git a/src/chief/dispatch.py b/src/chief/dispatch.py index eaaf99a3..7ee9798e 100644 --- a/src/chief/dispatch.py +++ b/src/chief/dispatch.py @@ -31,7 +31,7 @@ from chief.hub import ObserverHub from chief.policy import StreamPolicy, delta_frame, resolve, result_frame, tool_frame from chief.provider.base import ProviderError, ToolCall -from chief.selfedit.recovery import RestartBoundary +from chief.selfedit.restart import RestartBoundary from chief.strangers import StrangerLog diff --git a/src/chief/entrypoint.py b/src/chief/entrypoint.py index cf1558d7..1469e0e8 100644 --- a/src/chief/entrypoint.py +++ b/src/chief/entrypoint.py @@ -12,7 +12,8 @@ from chief.install.migrate import migrate_instance from chief.instance_lock import AlreadyRunning, acquire_instance_lock from chief.selfedit.notice import mark_notice_rolled_back, take_restart_notice -from chief.selfedit.recovery import clear_marker, restart_daemon, rollback_if_marked +from chief.selfedit.recovery import clear_marker, rollback_if_marked +from chief.selfedit.restart import restart_daemon if TYPE_CHECKING: # the runtime import stays inside the seatbelt below. from chief.daemon import App @@ -64,6 +65,10 @@ async def amain() -> None: level=logging.INFO, format="%(asctime)s %(name)s %(levelname)s %(message)s" ) repo_root = Path.cwd() + # Where the rollback looks for the last config that booted. Rebound below + # once the real config is loaded; the default covers a boot that dies + # before that, when db_path is unknown anyway. + history = repo_root / "data" / "config-history" try: # Imported inside the seatbelt: a self-edit that breaks chief.app's # import (yet somehow passes the done-check) still rolls back instead @@ -77,6 +82,7 @@ async def amain() -> None: for step in migrate_instance(repo_root): logger.info("update migration: %s", step) config = load_config() + history = config.db_path.parent / "config-history" # One daemon per data dir: a stray second instance would double-poll # chat.db and answer every iMessage twice. Held for the whole process. _lock = acquire_instance_lock(config.db_path.parent / "chief.lock") @@ -86,11 +92,14 @@ async def amain() -> None: logger.error("another chief instance is already running — refusing to start") raise SystemExit(1) from None except Exception: - # A failed boot right after a self-edit rolls back and re-execs. - if rollback_if_marked(repo_root): + # A failed boot right after a restart undoes it and reboots — the + # commit via git, the config from its newest snapshot. + if undone := rollback_if_marked(repo_root, history): # The next boot reports the rollback on the requesting thread - # instead of a "restart success" that never happened. - mark_notice_rolled_back(repo_root) + # instead of a "restart success" that never happened — naming + # what moved, since a restart changes the commit, the config, or + # both, and only the recovery knows which. + mark_notice_rolled_back(repo_root, undone) restart_daemon() raise clear_marker(repo_root) diff --git a/src/chief/selfedit/notice.py b/src/chief/selfedit/notice.py index 6c7f247d..95b4969c 100644 --- a/src/chief/selfedit/notice.py +++ b/src/chief/selfedit/notice.py @@ -42,14 +42,16 @@ class RestartNotice: thread_key: str rationale: str = "" rolled_back: bool = False + #: What the rollback actually undid, from ``rollback_if_marked``. A + #: restart moves the commit, the config, or both, so the report cannot + #: assume — it used to claim a previous commit even when none had moved. + undone: str = "" def text(self) -> str: """What the fresh daemon says on the requesting thread.""" if self.rolled_back: - return ( - "⚠️ restart failed — the new code did not boot, so it was " - "rolled back. Back up on the previous commit." - ) + what = self.undone or "it was rolled back" + return f"⚠️ restart failed — the new code did not boot, so {what}." detail = f" ({self.rationale})" if self.rationale else "" return f"✅ restart success — back up{detail}" @@ -65,6 +67,7 @@ def write_restart_notice(repo_root: Path, notice: RestartNotice) -> None: "thread_key": notice.thread_key, "rationale": notice.rationale, "rolled_back": notice.rolled_back, + "undone": notice.undone, "written_at": time.time(), } ) @@ -92,6 +95,7 @@ def take_restart_notice(repo_root: Path) -> RestartNotice | None: thread_key=str(data["thread_key"]), rationale=str(data.get("rationale", "")), rolled_back=bool(data.get("rolled_back", False)), + undone=str(data.get("undone", "")), ) except Exception: # A corrupt notice is dropped, not raised: it must never block a boot. @@ -100,8 +104,12 @@ def take_restart_notice(repo_root: Path) -> RestartNotice | None: return notice -def mark_notice_rolled_back(repo_root: Path) -> None: - """A failed boot: the pending notice now reports the rollback instead.""" +def mark_notice_rolled_back(repo_root: Path, undone: str = "") -> None: + """A failed boot: the pending notice now reports the rollback instead. + + ``undone`` is what the recovery actually put back, carried through so the + owner is told which half moved — and where the config that failed went. + """ notice = take_restart_notice(repo_root) if notice is None: return @@ -112,5 +120,6 @@ def mark_notice_rolled_back(repo_root: Path) -> None: thread_key=notice.thread_key, rationale=notice.rationale, rolled_back=True, + undone=undone, ), ) diff --git a/src/chief/selfedit/pipeline.py b/src/chief/selfedit/pipeline.py index 10e468b8..43033a54 100644 --- a/src/chief/selfedit/pipeline.py +++ b/src/chief/selfedit/pipeline.py @@ -4,9 +4,10 @@ until ``restart``: dirty-tree file-list confirmation, the full done-check, then a live ``config.yaml`` load (fixture configs alone would let a bad real value boot-loop launchd). Only on all green does it commit, write the -rollback marker, and reboot into the new code (a boot failure rolls back — -``recovery.py``). On red the edits are **kept** and the failure returned so -the agent fixes forward. Restarts serialize behind a lock. +rollback marker (always — a config-only restart still needs an undo), and +reboot into the new code (a boot failure rolls back — ``recovery.py``). On red +the edits are **kept** and the failure returned so the agent fixes forward. +Restarts serialize behind a lock. Subprocess plumbing (checks, git, argv-only — no shell) lives in ``gitops.py``. @@ -146,16 +147,22 @@ async def _commit_if_dirty(self, rationale: str, base: str) -> bool: """Commit tracked working-tree changes and drop a rollback marker. Returns whether anything was committed. Gitignored writes (``data``, - ``secrets``, ``config.yaml``, off-repo) never enter the commit, so they - are unversioned — the rollback marker only rewinds repo files (#198). + ``secrets``, ``config.yaml``, off-repo) never enter the commit, so git + cannot rewind them (#198) — hence the recorded config-history path, + and the marker being written **even when nothing was committed**: a + config-only restart used to leave none, so it had no recovery at all. """ - if not await self._dirty_files(): - return False - await self._git("add", "-A") - await self._git("commit", "-m", f"self-edit: {rationale}") - marker = {"rollback_to": base, "rationale": rationale} + committed = bool(await self._dirty_files()) + if committed: + await self._git("add", "-A") + await self._git("commit", "-m", f"self-edit: {rationale}") + marker = { + "rollback_to": base, + "rationale": rationale, + "committed": committed, + } (self._root / MARKER_NAME).write_text(json.dumps(marker)) - return True + return committed async def revert_edits(self) -> str: """Discard uncommitted changes to tracked repo files (back to HEAD). diff --git a/src/chief/selfedit/recovery.py b/src/chief/selfedit/recovery.py index da8c7ec3..c19ba973 100644 --- a/src/chief/selfedit/recovery.py +++ b/src/chief/selfedit/recovery.py @@ -1,115 +1,30 @@ -"""Boot-side half of the self-edit seatbelt. +"""Boot-side half of the self-edit seatbelt: undoing a restart that failed. The pipeline leaves a marker file before restarting. If the new code boots, -the marker is cleared (healthcheck passed). If boot raises, the repo is -reset to the recorded commit and the daemon re-execs into the old code. +the marker is cleared (healthcheck passed). If boot raises, whatever the +restart changed is undone and the daemon restarts into the old state — the +repo resets to the recorded commit, and the config is put back from its +newest history snapshot, which git cannot do because config.yaml is ignored. + +The other half — deciding when the restart is allowed to fire at all — lives +in ``chief.selfedit.restart``. """ -import asyncio import json import logging -import os import subprocess -import sys from collections.abc import Callable from pathlib import Path -from typing import Protocol -from chief.selfedit.notice import RestartNotice, write_restart_notice +from chief.config.history import restore_newest logger = logging.getLogger(__name__) MARKER_NAME = ".selfedit-pending.json" -# How long to let in-flight turns commit before restarting anyway. A stuck -# turn must not wedge the restart forever; a self-edit already merged. -DRAIN_TIMEOUT_SECONDS = 30.0 - - -class RestartBoundary(Protocol): - """The outermost side-effect boundary a caller fires after a turn's reply - (and any cursor) is durable, so a pending self-edit execs last. Satisfied - by ``RestartController``; a no-op elsewhere.""" - - async def fire_if_requested(self) -> None: ... - - -class RestartController: - """Restarts the daemon only once in-flight turns have committed. - - A self-edit / install runs *inside* a turn. If the pipeline called os.execv - the instant the check went green, the process would vanish before any turn - persisted — the exchange that asked for the edit would be lost (the daemon - reboots amnesiac and re-asks in a loop), and any *other* turn running - concurrently would be killed uncommitted too. Instead the pipeline calls - ``request`` mid-turn; the session brackets every turn with ``enter_turn`` / - ``leave_turn`` and calls ``fire_if_requested`` after it commits. On a pending - restart, new turns are held at ``enter_turn`` and the restart waits for the - active turns to drain (bounded by a timeout) before exec'ing, so their - transcripts reach disk first. - """ - - def __init__( - self, - restart: Callable[[], None] | None = None, - drain_timeout: float = DRAIN_TIMEOUT_SECONDS, - repo_root: Path = Path("."), - ) -> None: - self._restart = restart if restart is not None else restart_daemon - self._drain_timeout = drain_timeout - self._repo_root = repo_root - self._notice: RestartNotice | None = None - self._requested = False - self._active = 0 - self._admitting = asyncio.Event() - self._admitting.set() # open until a restart is requested - self._idle = asyncio.Event() - self._idle.set() # set whenever no turn is active - - def request(self, notice: RestartNotice | None = None) -> None: - """Mark a restart due and stop admitting new turns (pipeline side). - - ``notice`` is the thread to report back to; it is held in memory and - only written at the exec — the request and the exec are a whole turn - plus the drain apart, and a notice on disk in between would be - claimed by any unrelated reboot that beat this one to it. - """ - self._requested = True - self._notice = notice - self._admitting.clear() - - async def enter_turn(self) -> None: - """Admission gate: hold new turns once a restart is pending, then - register as active so the drain waits for this turn to commit.""" - await self._admitting.wait() - self._active += 1 - self._idle.clear() - - def leave_turn(self) -> None: - """Deregister a turn that has finished (and committed).""" - self._active -= 1 - if self._active == 0: - self._idle.set() - - async def fire_if_requested(self) -> None: - """If a restart is pending, wait for other turns to drain, then exec. - - Never returns when it restarts (os.execv). The drain is bounded: a turn - that outlasts the timeout is left behind rather than blocking forever. - """ - if not self._requested: - return - try: - await asyncio.wait_for(self._idle.wait(), self._drain_timeout) - except TimeoutError: - logger.warning( - "restart drain timed out after %.0fs; %d turn(s) still in flight", - self._drain_timeout, - self._active, - ) - if self._notice is not None: - write_restart_notice(self._repo_root, self._notice) - self._restart() +#: The config a failed boot was rolled back *from*, kept beside the repo so a +#: hand edit is never silently discarded by the recovery. +FAILED_CONFIG_NAME = ".config-failed.yaml" def clear_marker(repo_root: Path) -> None: @@ -120,23 +35,81 @@ def clear_marker(repo_root: Path) -> None: marker.unlink() -def rollback_if_marked(repo_root: Path) -> bool: - """After a failed boot: reset to the pre-edit commit if one is recorded. +def rollback_if_marked( + repo_root: Path, + config_history: Path | None = None, + run: Callable[[list[str]], object] | None = None, +) -> str: + """After a failed boot: undo whatever the restart changed. + + Two halves, because a restart changes two things the seatbelt covers + differently. Committed repo files rewind with ``git reset --hard``. The + config does not — it is gitignored, so git steps straight past it, and a + config-only restart commits nothing at all, which is why it used to leave + no marker and so had no boot-failure recovery for anything. Its undo is + the newest config-history snapshot. + + Returns what was undone, empty when nothing was — the caller reads that + as "do not reboot", since an identical tree and config reproduce the same + failed boot forever. The text also reaches the owner's restart notice, so + the report names the half that moved instead of assuming both did. + + Neither half may raise: the marker is consumed before either runs, so an + escape would spend the seatbelt and undo nothing — and a git failure would + skip the config half, which may be the undo this boot needs. + + ``config_history`` comes from the caller rather than the marker: the boot + that writes the marker and the boot that reads it resolve the data dir the + same way, so recording it would only let the two drift. - Returns True when a rollback happened (caller should re-exec). + ``run`` is the git runner, injected by tests so they need no real repo. """ marker = repo_root / MARKER_NAME if not marker.exists(): - return False - target = str(json.loads(marker.read_text())["rollback_to"]) + return "" + record = json.loads(marker.read_text()) marker.unlink() - logger.error("boot failed after self-edit; rolling back to %s", target) - subprocess.run( - ["git", "reset", "--hard", target], cwd=repo_root, check=True - ) - return True + undone = [] + # ``committed`` is absent from a marker written by the previous version, + # and those were only ever written when a commit had been made. + if record.get("committed", True) and record.get("rollback_to"): + target = str(record["rollback_to"]) + logger.error("boot failed after self-edit; rolling back to %s", target) + argv = ["git", "reset", "--hard", target] + try: + if run is None: + subprocess.run(argv, cwd=repo_root, check=True) + else: + run(argv) + undone.append(f"the code is back on {target}") + except (subprocess.CalledProcessError, OSError): + logger.exception("could not roll the repo back to %s", target) + if config_history is not None: + try: + restored = restore_newest( + config_history, + repo_root / "config.yaml", + repo_root / FAILED_CONFIG_NAME, + ) + except OSError: + # Same reasoning as the snapshot side: a full disk must not turn a + # recoverable boot failure into an unrecoverable one. + logger.exception("could not put the config back from %s", config_history) + restored = None + if restored is not None: + logger.error("boot failed; put the config back from %s", restored) + undone.append( + f"config.yaml is back from {restored.name}, and the one that " + f"failed is kept at {FAILED_CONFIG_NAME}" + ) + if not undone: + # Naming the history dir here is the one signal that a relocated + # ``db_path`` left the config half looking somewhere empty. + logger.error( + "boot failed, but nothing was left to undo (config history: %s); " + "not restarting", + config_history, + ) + return "; ".join(undone) -def restart_daemon() -> None: - """Replace this process with a fresh daemon (works under any supervisor).""" - os.execv(sys.executable, [sys.executable, "-m", "chief.entrypoint"]) diff --git a/src/chief/selfedit/restart.py b/src/chief/selfedit/restart.py new file mode 100644 index 00000000..8ad2848d --- /dev/null +++ b/src/chief/selfedit/restart.py @@ -0,0 +1,118 @@ +"""Restart side of the self-edit seatbelt: when the daemon is allowed to go. + +A self-edit runs *inside* a turn, so the process may not be replaced the +instant the done-check goes green — the exchange that asked for the edit, and +any turn running beside it, would be lost uncommitted. The pipeline requests a +restart here; the session fires it at the outermost boundary, once replies and +cursors are durable. + +The other half of the seatbelt — what happens when the new code fails to boot +— lives in ``chief.selfedit.recovery``. +""" + +import asyncio +import logging +import os +import sys +from collections.abc import Callable +from pathlib import Path +from typing import Protocol + +from chief.selfedit.notice import RestartNotice, write_restart_notice + +logger = logging.getLogger(__name__) + +# How long to let in-flight turns commit before restarting anyway. A stuck +# turn must not wedge the restart forever; a self-edit already merged. +DRAIN_TIMEOUT_SECONDS = 30.0 + + +class RestartBoundary(Protocol): + """The outermost side-effect boundary a caller fires after a turn's reply + (and any cursor) is durable, so a pending self-edit execs last. Satisfied + by ``RestartController``; a no-op elsewhere.""" + + async def fire_if_requested(self) -> None: ... + + +class RestartController: + """Restarts the daemon only once in-flight turns have committed. + + A self-edit / install runs *inside* a turn. If the pipeline called os.execv + the instant the check went green, the process would vanish before any turn + persisted — the exchange that asked for the edit would be lost (the daemon + reboots amnesiac and re-asks in a loop), and any *other* turn running + concurrently would be killed uncommitted too. Instead the pipeline calls + ``request`` mid-turn; the session brackets every turn with ``enter_turn`` / + ``leave_turn`` and calls ``fire_if_requested`` after it commits. On a pending + restart, new turns are held at ``enter_turn`` and the restart waits for the + active turns to drain (bounded by a timeout) before exec'ing, so their + transcripts reach disk first. + """ + + def __init__( + self, + restart: Callable[[], None] | None = None, + drain_timeout: float = DRAIN_TIMEOUT_SECONDS, + repo_root: Path = Path("."), + ) -> None: + self._restart = restart if restart is not None else restart_daemon + self._drain_timeout = drain_timeout + self._repo_root = repo_root + self._notice: RestartNotice | None = None + self._requested = False + self._active = 0 + self._admitting = asyncio.Event() + self._admitting.set() # open until a restart is requested + self._idle = asyncio.Event() + self._idle.set() # set whenever no turn is active + + def request(self, notice: RestartNotice | None = None) -> None: + """Mark a restart due and stop admitting new turns (pipeline side). + + ``notice`` is the thread to report back to; it is held in memory and + only written at the exec — the request and the exec are a whole turn + plus the drain apart, and a notice on disk in between would be + claimed by any unrelated reboot that beat this one to it. + """ + self._requested = True + self._notice = notice + self._admitting.clear() + + async def enter_turn(self) -> None: + """Admission gate: hold new turns once a restart is pending, then + register as active so the drain waits for this turn to commit.""" + await self._admitting.wait() + self._active += 1 + self._idle.clear() + + def leave_turn(self) -> None: + """Deregister a turn that has finished (and committed).""" + self._active -= 1 + if self._active == 0: + self._idle.set() + + async def fire_if_requested(self) -> None: + """If a restart is pending, wait for other turns to drain, then exec. + + Never returns when it restarts (os.execv). The drain is bounded: a turn + that outlasts the timeout is left behind rather than blocking forever. + """ + if not self._requested: + return + try: + await asyncio.wait_for(self._idle.wait(), self._drain_timeout) + except TimeoutError: + logger.warning( + "restart drain timed out after %.0fs; %d turn(s) still in flight", + self._drain_timeout, + self._active, + ) + if self._notice is not None: + write_restart_notice(self._repo_root, self._notice) + self._restart() + + +def restart_daemon() -> None: + """Replace this process with a fresh daemon (works under any supervisor).""" + os.execv(sys.executable, [sys.executable, "-m", "chief.entrypoint"]) diff --git a/src/chief/wiring.py b/src/chief/wiring.py index 36a4f9ec..17519832 100644 --- a/src/chief/wiring.py +++ b/src/chief/wiring.py @@ -41,7 +41,7 @@ from chief.provider.base import Provider from chief.provider.openrouter import OpenRouterProvider from chief.provider.router import RouterProvider -from chief.selfedit.recovery import RestartController +from chief.selfedit.restart import RestartController from chief.tools import ToolRegistry from chief.web.adapter import WebAdapter from chief.web.app import build_web_app diff --git a/tests/test_config_history.py b/tests/test_config_history.py index 8e5c1c2d..9c2203f0 100644 --- a/tests/test_config_history.py +++ b/tests/test_config_history.py @@ -180,8 +180,8 @@ def test_record_config_writes_under_the_data_dir(tmp_path: Path) -> None: def test_snapshots_are_returned_oldest_first(tmp_path: Path) -> None: - """Recovery reads this list, so the order is the contract: the previous - config is the second-to-last entry.""" + """Order is the contract: name order is write order, so the last entry is + the last config that booted — which is what the recovery restores.""" config = write_config(tmp_path, "one\n") history = tmp_path / "history" snapshot(config, history, at(1)) diff --git a/tests/test_config_rollback.py b/tests/test_config_rollback.py new file mode 100644 index 00000000..229c5c9d --- /dev/null +++ b/tests/test_config_rollback.py @@ -0,0 +1,192 @@ +"""A config-only restart must leave an undo behind. + +`config.yaml` is gitignored, so `git reset --hard` steps past it. And a +config-only restart commits nothing, so before this it wrote no rollback +marker either — meaning that restart had no boot-failure recovery for +*anything*. The undo it needs is the newest config-history snapshot, which by +construction is the last config that booted (#301). +""" + +import inspect +import json +import subprocess +from pathlib import Path + +import pytest + +from chief import entrypoint +from chief.config.history import restore_newest, snapshot +from chief.selfedit.recovery import ( + FAILED_CONFIG_NAME, + MARKER_NAME, + rollback_if_marked, +) +from tests.test_config_history import at, write_config + + +def marker(repo: Path, **fields: object) -> None: + (repo / MARKER_NAME).write_text(json.dumps(fields)) + + +def test_restore_newest_puts_the_last_booted_config_back(tmp_path: Path) -> None: + config = write_config(tmp_path, "good\n") + history = tmp_path / "history" + snapshot(config, history, at(1)) + config.write_text("bad\n") + + restored = restore_newest(history, config, tmp_path / "failed.yaml") + + assert restored is not None + assert config.read_text() == "good\n" + + +def test_restore_newest_keeps_the_config_it_replaced(tmp_path: Path) -> None: + """The config being rolled back may hold a hand edit the owner still + wants; the boot failed, so it is replaced, but never simply discarded.""" + config = write_config(tmp_path, "good\n") + history = tmp_path / "history" + snapshot(config, history, at(1)) + config.write_text("hand edited but broken\n") + failed = tmp_path / "failed.yaml" + + restore_newest(history, config, failed) + + assert failed.read_text() == "hand edited but broken\n" + + +def test_restore_newest_is_a_noop_when_config_already_matches( + tmp_path: Path, +) -> None: + config = write_config(tmp_path, "same\n") + history = tmp_path / "history" + snapshot(config, history, at(1)) + failed = tmp_path / "failed.yaml" + + assert restore_newest(history, config, failed) is None + assert not failed.exists() + + +def test_restore_newest_with_no_history_does_nothing(tmp_path: Path) -> None: + config = write_config(tmp_path, "only\n") + + assert restore_newest(tmp_path / "nope", config, tmp_path / "f.yaml") is None + assert config.read_text() == "only\n" + + +def test_config_only_marker_rolls_the_config_back(tmp_path: Path) -> None: + """The whole point: nothing was committed, so there is no git undo — the + recovery is the snapshot, and the caller must still be told to reboot.""" + config = write_config(tmp_path, "good\n") + history = tmp_path / "history" + snapshot(config, history, at(1)) + config.write_text("bad\n") + marker(tmp_path, committed=False) + + undone = rollback_if_marked(tmp_path, history) + + assert "config.yaml is back" in undone + assert config.read_text() == "good\n" + assert not (tmp_path / MARKER_NAME).exists() + + +def test_a_marker_with_nothing_to_undo_does_not_reboot(tmp_path: Path) -> None: + """Returning True here would crash-loop: restarting into the identical + tree and identical config reproduces the same failed boot forever.""" + config = write_config(tmp_path, "same\n") + history = tmp_path / "history" + snapshot(config, history, at(1)) + marker(tmp_path, committed=False) + + assert rollback_if_marked(tmp_path, history) == "" + assert not (tmp_path / MARKER_NAME).exists() + + +def test_absent_marker_undoes_nothing(tmp_path: Path) -> None: + assert rollback_if_marked(tmp_path, tmp_path / "history") == "" + + +def test_legacy_marker_without_the_new_fields_still_resets(tmp_path: Path) -> None: + """A marker written by the previous version is on disk across exactly the + upgrade this ships in — it must not KeyError on the boot-failure path.""" + calls: list[list[str]] = [] + marker(tmp_path, rollback_to="deadbeef", rationale="old") + + assert rollback_if_marked(tmp_path, None, calls.append) + assert calls == [["git", "reset", "--hard", "deadbeef"]] + + +def test_both_halves_undo_and_are_both_reported(tmp_path: Path) -> None: + """A restart that changed code *and* config must undo both, and the text + the owner is shown must name both — it used to claim only the commit.""" + config = write_config(tmp_path, "good\n") + history = tmp_path / "history" + snapshot(config, history, at(1)) + config.write_text("bad\n") + marker(tmp_path, rollback_to="cafe1234", committed=True) + + undone = rollback_if_marked(tmp_path, history, lambda argv: None) + + assert "cafe1234" in undone and "config.yaml is back" in undone + assert config.read_text() == "good\n" + + +def test_a_failed_git_reset_still_lets_the_config_half_run(tmp_path: Path) -> None: + """The marker is already consumed, so an exception here would spend the + seatbelt and undo nothing — while the config may be the actual culprit.""" + config = write_config(tmp_path, "good\n") + history = tmp_path / "history" + snapshot(config, history, at(1)) + config.write_text("bad\n") + marker(tmp_path, rollback_to="cafe1234", committed=True) + + def explode(argv: list[str]) -> None: + raise subprocess.CalledProcessError(1, argv) + + undone = rollback_if_marked(tmp_path, history, explode) + + assert "cafe1234" not in undone + assert "config.yaml is back" in undone + assert config.read_text() == "good\n" + + +def test_a_restore_that_cannot_write_does_not_replace_the_boot_failure( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A full disk must not turn a recoverable boot failure into a crash the + caller cannot even report — same reasoning as the snapshot side.""" + config = write_config(tmp_path, "good\n") + history = tmp_path / "history" + snapshot(config, history, at(1)) + config.write_text("bad\n") + marker(tmp_path, committed=False) + + def full_disk(*args: object) -> Path: + raise OSError("no space left on device") + + monkeypatch.setattr("chief.selfedit.recovery.restore_newest", full_disk) + + assert rollback_if_marked(tmp_path, history) == "" + + +def test_the_config_that_failed_is_kept_where_the_owner_is_told( + tmp_path: Path, +) -> None: + config = write_config(tmp_path, "good\n") + history = tmp_path / "history" + snapshot(config, history, at(1)) + config.write_text("hand edited but broken\n") + marker(tmp_path, committed=False) + + undone = rollback_if_marked(tmp_path, history) + + assert FAILED_CONFIG_NAME in undone + assert (tmp_path / FAILED_CONFIG_NAME).read_text() == "hand edited but broken\n" + + +def test_the_history_only_holds_configs_that_booted(tmp_path: Path) -> None: + """The whole basis for restoring the *newest* entry rather than the + second-to-last: entrypoint snapshots strictly after the healthcheck, so a + config that failed to boot never reaches the directory at all.""" + source = inspect.getsource(entrypoint.amain) + assert source.index("clear_marker(repo_root)") < source.index("record_config(") + assert "rollback_if_marked" in source[: source.index("clear_marker(repo_root)")] diff --git a/tests/test_dispatch.py b/tests/test_dispatch.py index 6b5fbc12..b7a81a2c 100644 --- a/tests/test_dispatch.py +++ b/tests/test_dispatch.py @@ -16,7 +16,7 @@ from chief.persistence.store import MessageStore from chief.policy import RICH, StreamPolicy from chief.provider.base import ProviderError, ProviderEvent, ToolSpec -from chief.selfedit.recovery import RestartController +from chief.selfedit.restart import RestartController from chief.strangers import StrangerLog from chief.tools import Tool, ToolRegistry diff --git a/tests/test_imessage.py b/tests/test_imessage.py index f9a7ee91..930b5a37 100644 --- a/tests/test_imessage.py +++ b/tests/test_imessage.py @@ -10,7 +10,7 @@ from chief.adapters.base import Message from chief.adapters.imessage import BOT_PREFIX, IMessageAdapter from chief.adapters.imessage_send import owner_send_guard -from chief.selfedit.recovery import RestartBoundary, RestartController +from chief.selfedit.restart import RestartBoundary, RestartController OWNER = "+15550001111" CHIEF = "chief@example.com" # chief's own Apple ID handle (dedicated mode) diff --git a/tests/test_selfedit.py b/tests/test_selfedit.py index e348e1e3..9429d0eb 100644 --- a/tests/test_selfedit.py +++ b/tests/test_selfedit.py @@ -23,12 +23,8 @@ write_restart_notice, ) from chief.selfedit.pipeline import SelfEditPipeline -from chief.selfedit.recovery import ( - MARKER_NAME, - RestartController, - clear_marker, - rollback_if_marked, -) +from chief.selfedit.recovery import MARKER_NAME, clear_marker, rollback_if_marked +from chief.selfedit.restart import RestartController from chief.selfedit.tools import register_restart_tool from chief.tools import ToolContext, ToolRegistry from chief.tools.files import register_file_tools @@ -120,8 +116,12 @@ async def test_noop_restart_is_allowed(repo: Path, tmp_path: Path) -> None: result = await pipeline.restart("just reload") assert "restarting" in result assert restart.called - assert not (repo / MARKER_NAME).exists() assert "self-edit" not in git(repo, "log", "--oneline") + # A marker is written even with nothing committed: this restart can still + # have changed the gitignored config, and leaving none meant a failed boot + # afterwards had no recovery for anything at all. + marker = json.loads((repo / MARKER_NAME).read_text()) + assert marker["committed"] is False async def test_bad_config_aborts_restart_and_keeps_edits( @@ -448,11 +448,25 @@ def test_rolled_back_notice_reports_the_failure(repo: Path) -> None: write_restart_notice( repo, RestartNotice(channel="cli", thread_key="cli:t", rationale="risky") ) - mark_notice_rolled_back(repo) + mark_notice_rolled_back(repo, "config.yaml is back from 2026-01-01T00-00-00Z") notice = take_restart_notice(repo) assert notice is not None assert notice.rolled_back assert notice.text().startswith("⚠️ restart failed") + # Names what actually moved: this restart changed no commit, and the old + # text claimed one anyway ("back up on the previous commit"). + assert "config.yaml is back from" in notice.text() + assert "previous commit" not in notice.text() + + +def test_a_rollback_with_no_detail_still_reads_as_a_rollback(repo: Path) -> None: + write_restart_notice(repo, RestartNotice("cli", "cli:t", "risky")) + mark_notice_rolled_back(repo) + notice = take_restart_notice(repo) + assert notice is not None + assert notice.text() == ( + "⚠️ restart failed — the new code did not boot, so it was rolled back." + ) def test_unreadable_notice_is_consumed_not_raised(repo: Path) -> None: @@ -472,10 +486,10 @@ def test_rollback_if_marked_resets_and_reexecs(repo: Path) -> None: git(repo, "add", "-A") git(repo, "commit", "-m", "self-edit: bad") (repo / MARKER_NAME).write_text(json.dumps({"rollback_to": base})) - assert rollback_if_marked(repo) is True + assert base in rollback_if_marked(repo) assert (repo / "greeting.txt").read_text() == "hello\n" assert not (repo / MARKER_NAME).exists() - assert rollback_if_marked(repo) is False + assert rollback_if_marked(repo) == "" async def test_boot_failure_after_selfedit_rolls_back( diff --git a/tests/test_session.py b/tests/test_session.py index f8d7cd25..eb569dab 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -9,7 +9,7 @@ from chief.agent.manager import SessionManager from chief.persistence.store import MessageStore from chief.provider.base import ProviderEvent, ToolSpec -from chief.selfedit.recovery import RestartController +from chief.selfedit.restart import RestartController from chief.tools import Tool, ToolRegistry from .fakes import FakeProvider, text_turn, tool_turn