Skip to content

Self-update: startup gate, post-restart rollback, pinned deps, release channel - #47

Merged
Showdown76py merged 3 commits into
mainfrom
claude/audit-fixes-ycn7ph
Oct 11, 2026
Merged

Showdown76py merged 3 commits into
mainfrom
claude/audit-fixes-ycn7ph

Conversation

@Showdown76py

@Showdown76py Showdown76py commented Oct 11, 2026 •

Copy link
Copy Markdown
Owner

Audit points 1 and 2 (one PR, as agreed).

Before: beaconmcp_self_update pulled the tip of main unsigned, ran an unpinned pip install -e ., and its only gate was validate-config, which imports config.py alone. A broken import anywhere else passed the gate, the service restarted, and the server stayed down until someone fixed it by hand.

After: the new code must actually start before the restart (beaconmcp check-startup), and a watchdog rolls the update back when the restarted service does not answer /health. Dependencies are pinned by constraints.txt. An operator can follow release tags instead of the branch and require a trusted signature.

How

  • beaconmcp check-startup (new CLI): loads the config, imports every module, builds the HTTP app and calls /health through its lifespan, against scratch copies of the token store, dashboard DB and clients file. apply_update runs it in a subprocess after pip install; on failure it resets the checkout, restores the dependencies and does not restart. CI runs it on beaconmcp.yaml.example.
  • Post-restart watchdog: the restart goes through systemd-run as its own transient unit (a child of the service would be killed by systemctl restart with the cgroup). It restarts, waits up to 90 s for /health (and checks it again 10 s later), otherwise does git reset --hard <previous>, reinstalls and restarts again. The outcome (healthy, rolled_back, rollback_unhealthy, rollback_failed) goes to .beaconmcp-update.json (gitignored) and is reported by beaconmcp_check_update as last_update. Without systemd-run the old unwatched restart is kept. All values travel as argv, the script is a constant.
  • Pinned dependencies: constraints.txt (68 pins, resolved from a clean .[wizard,dev] install; identical on 3.11, 3.12 and 3.13). Used by the self-update, deploy/install.sh, the Dockerfile and every CI install.
  • Channels and signatures: features.updates.channel: branch | release and features.updates.require_signed. release follows the newest vX.Y.Z tag (pre-release tags skipped); require_signed makes git verify-tag (release) or git verify-commit (branch tip) a hard gate before anything changes. The update fast-forwards to the exact sha that was resolved and verified (git merge --ff-only <sha>), not to whatever the remote points at a moment later. A diverged checkout is now reported as a blocker by the check.
  • Docs: docs/updates.md, docs/configuration.md, beaconmcp.yaml.example, and the MCP tool docstrings.

branch stays the default: git ls-remote --tags origin returns no tag today, so release would only say "no release tag". Switching the default makes sense once releases are tagged (signed).

Verified on a real server

Fake local remote, a real beaconmcp serve started by a fake systemctl, a fake systemd-run that runs the watchdog detached, real git and real pip. Update triggered through the beaconmcp_self_update MCP tool.

Scenario main this PR
Upstream commit adds import beaconmcp_module_that_does_not_exist to dashboard/app.py validate-config passes, restart, server down (ModuleNotFoundError) check-startup fails, checkout restored, not restarted, server stays up on the old revision
Upstream commit passes every check but serve exits at start (same as above: down) restart, /health down for 90 s, watchdog resets to the old sha, reinstalls, restarts: up again on the old revision, status rolled_back, then reported by beaconmcp_check_update
Good update restart, up on the new sha, status healthy
channel: release, require_signed: true, unsigned v2.1.0 check reports the blocker, apply stops at verify-signature, HEAD unchanged
Same, GPG-signed v2.2.0 (key trusted by the host) plus a later v2.3.0-rc1 updated to exactly v2.2.0, rc skipped, status healthy

Not exercised: a real systemd (no systemd in the test container), so systemd-run --collect --unit=... itself was checked only for its argv.

Tests: 532 pass. New ones cover the release channel (version sort, rc skipped, no tags), signatures with a throwaway GPG key (unsigned, signed and trusted, signed by an unknown key, signed branch tip), the diverged blocker, -c constraints.txt, the watchdog argv and its fallback, and the real watchdog script under /bin/sh with fake systemctl/pip (healthy, rolled back, rollback that does not help). tests/test_check_startup.py runs the CLI, including a case validate-config accepts and check-startup rejects (dashboard enabled without BEACONMCP_SESSION_KEY). The update tests now ignore the developer's global git config.

Not in this PR

Running the service as a non-root user, also raised by the audit, is a deployment migration (file ownership under /opt/beaconmcp, and the watchdog then needs a polkit rule or sudo for systemctl restart). Proposed as a separate change rather than imposed here.

…gate and post-restart rollback

Self-update now:
- follows either the default branch (default) or the newest vX.Y.Z tag
  (features.updates.channel), and can refuse unsigned targets
  (features.updates.require_signed: git verify-tag / verify-commit);
- fast-forwards to the exact commit it resolved and verified;
- installs dependencies pinned by constraints.txt (also used by
  install.sh, the Dockerfile and CI);
- runs the new 'beaconmcp check-startup' before restarting: the new code
  loads the real config, imports every module, builds the HTTP app and
  answers /health against scratch state; on failure it rolls back and
  does not restart;
- restarts through a watchdog in its own transient unit (systemd-run) that
  rolls back to the previous revision when /health does not answer, and
  records the outcome in .beaconmcp-update.json (reported as last_update).

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Lxm6BaCA2kUHH1f6xvh8kS
@Showdown76py Showdown76py added the bug Something isn't working label Oct 11, 2026 — with Claude
@Showdown76py Showdown76py self-assigned this Oct 11, 2026
@Ailcope
Ailcope self-requested a review October 11, 2026 15:11

@Ailcope Ailcope left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Showdown76py the startup gate, pinned SHA and watchdog design look solid, and the tests that run the real shell watchdog are good. I found a few gaps:

  1. No guard across processes. _apply_lock only works within one process, and the restart replaces that process. If a second update starts while the first watchdog is still waiting on /health, the first watchdog can time out and reset --hard to its from, which wipes out the second update.
  2. systemd-run result unchecked (_schedule_restart). Popen succeeding only means the process spawned. If D-Bus refuses the unit, or two calls land in the same second (int(time.time()) unit name), nothing restarts, yet the result says "watched" and the status stays pending forever.
  3. Rollback ignores pip's exit code (_rollback). It reports "rolled back" even when reinstalling the old dependencies failed. The shell watchdog ignores it too.
  4. _ahead_behind returns (0, 0) when git fails, so apply says "Already up to date" instead of reporting an error.
  5. Defaults trust any upstream push. With channel: branch + require_signed: false and User=root in the unit, the docs should say this plainly.

Fix commit 2e2e443 (pushing right after this comment):

  • A new update is refused while the last status is pending and younger than the watchdog's worst case (_WATCHDOG_WINDOW_SECONDS). A stale pending no longer blocks.
  • systemd-run now runs via _run and its exit code is checked. On failure it falls back to an unwatched restart. Unit names use time_ns(). _schedule_restart returns "watched" / "unwatched" / None.
  • _rollback adds a rollback-pip-install step and says clearly when the reinstall failed. The watchdog logs it.
  • _ahead_behind returns None on failure, and check/apply report an error.
  • docs/updates.md gets a warning about the defaults plus a note on the pending guard.

Tests updated and new ones added: 77 passed for updates/check-startup, full suite 535 passed, ruff clean.


Generated by Claude Code

Ailcope commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator

Correction to my review: the push of 2e2e443 was rejected by the repo ruleset ("Changes must be made through a pull request"), so here's the fix as a patch. To apply it, save the block below as fix.patch, then on claude/audit-fixes-ycn7ph run git am fix.patch.

From 2e2e443b3c626970047755bd63399f5fbde6fa15 Mon Sep 17 00:00:00 2001
From: Claude <[email protected]>
Date: Sun, 11 Oct 2026 15:25:46 +0000
Subject: [PATCH] fix(updates): guard pending watchdogs, check systemd-run and
 rollback pip

- Refuse a new update while the last one is still "pending" (its
  watchdog may roll back and wipe the new update). _apply_lock is
  per-process and the restart replaced that process.
- Check systemd-run's exit code and fall back to an unwatched restart
  if the unit is refused; unit names use time_ns() so two calls in one
  second don't collide. _schedule_restart now says which mode ran.
- The pre-restart rollback reports a failed dependency reinstall
  instead of claiming it restored them; the watchdog logs it.
- _ahead_behind returns None when git can't compare, so apply/check
  report an error instead of "already up to date".
- docs: say plainly that the defaults trust any upstream push.
---
 docs/updates.md          |   9 +++-
 src/beaconmcp/updates.py | 114 +++++++++++++++++++++++++++++----------
 tests/test_updates.py    |  67 +++++++++++++++++++----
 3 files changed, 150 insertions(+), 40 deletions(-)

diff --git a/docs/updates.md b/docs/updates.md
index 9d04d88..03bc90f 100644
--- a/docs/updates.md
+++ b/docs/updates.md
@@ -111,9 +111,16 @@ features:
   On the branch channel this needs a signed tip commit; merges made through GitHub's web UI are
   signed by GitHub's key, which you then have to trust.
 
+> **The defaults trust whatever lands upstream.** With `channel: branch` and
+> `require_signed: false`, any commit pushed to the upstream default branch is one confirmation
+> away from running on this host, and the shipped unit runs as `root`. On a server that matters,
+> use `channel: release` with `require_signed: true`, or keep updates manual.
+
 Only one update runs at a time. The dashboard button and the MCP tool reach the same code, and two
 `git pull` / `pip install` runs in one checkout would fight over `index.lock`. A second caller is
-told one is already in progress rather than queued behind a pip that may take minutes.
+told one is already in progress rather than queued behind a pip that may take minutes. That also
+holds across the restart: while the last update's status is still `pending` (its watchdog has not
+reported yet), a new update is refused.
 
 ### From the dashboard
 
diff --git a/src/beaconmcp/updates.py b/src/beaconmcp/updates.py
index 0d3b76d..61331ce 100644
--- a/src/beaconmcp/updates.py
+++ b/src/beaconmcp/updates.py
@@ -39,7 +39,7 @@ import threading
 import time
 from dataclasses import dataclass, field
 from pathlib import Path
-from typing import Any
+from typing import Any, Literal
 
 from .config import UpdatesConfig
 
@@ -70,6 +70,10 @@ _GIT_TIMEOUT = 60
 _PIP_TIMEOUT = 900
 #: ``check-startup`` imports the whole server and builds the HTTP app.
 _STARTUP_CHECK_TIMEOUT = 180
+#: How long a "pending" status blocks a new update: the watchdog's two health
+#: waits plus a dependency reinstall, with margin. Past that, a pending status
+#: is stale (the watchdog died) and no longer blocks.
+_WATCHDOG_WINDOW_SECONDS = 2 * (HEALTH_WAIT_SECONDS + 60) + _PIP_TIMEOUT
 
 
 @dataclass
@@ -623,12 +627,16 @@ def _verify_signature(root: Path, target: _Target) -> tuple[bool, str]:
     return code == 0, detail
 
 
-def _ahead_behind(root: Path, sha: str) -> tuple[int, int]:
-    """(commits HEAD has that ``sha`` lacks, commits ``sha`` has that HEAD lacks)."""
+def _ahead_behind(root: Path, sha: str) -> tuple[int, int] | None:
+    """(commits HEAD has that ``sha`` lacks, commits ``sha`` has that HEAD lacks).
+
+    ``None`` when git cannot compare them, so callers report an error rather
+    than "up to date".
+    """
     code, out, _ = _git(root, "rev-list", "--left-right", "--count", f"HEAD...{sha}")
     parts = out.split()
     if code != 0 or len(parts) != 2 or not all(p.isdigit() for p in parts):
-        return 0, 0
+        return None
     return int(parts[0]), int(parts[1])
 
 
@@ -690,7 +698,11 @@ def _check_uncached(
         info.release_tag = target.name
         info.instructions = manual_instructions(install, release_tag=target.name)
 
-    ahead, behind = _ahead_behind(root, target.sha)
+    counts = _ahead_behind(root, target.sha)
+    if counts is None:
+        info.error = f"could not compare HEAD with {target.name}"
+        return info
+    ahead, behind = counts
     info.behind = behind
     info.available = behind > 0
 
@@ -857,7 +869,8 @@ fi
 log "no healthy answer from $url within ${wait}s; rolling back to $from"
 if git -C "$root" reset --hard "$from"; then
   cd "$root" || exit 1
-  if [ -f constraints.txt ]; then "$@" install -e . -c constraints.txt; else "$@" install -e .; fi
+  if [ -f constraints.txt ]; then "$@" install -e . -c constraints.txt; else "$@" install -e .; fi \
+    || log "dependency reinstall for $from failed; it may run against newer libraries"
   systemctl restart "$service"
   if wait_healthy; then
     write_status rolled_back
@@ -897,7 +910,9 @@ def _write_status(root: Path, payload: dict[str, Any]) -> None:
         pass
 
 
-def _schedule_restart(service: str, delay: int, watchdog: _Watchdog | None = None) -> bool:
+def _schedule_restart(
+    service: str, delay: int, watchdog: _Watchdog | None = None,
+) -> Literal["watched", "unwatched"] | None:
     """Restart the unit after ``delay`` seconds, detached from this process.
 
     A direct ``systemctl restart`` would kill us mid-response, so the caller
@@ -906,14 +921,18 @@ def _schedule_restart(service: str, delay: int, watchdog: _Watchdog | None = Non
 
     With a ``watchdog`` and ``systemd-run`` available, the restart is done by
     :data:`_WATCHDOG_SCRIPT` in its own transient unit, which rolls the
-    update back if the new process never becomes healthy.
+    update back if the new process never becomes healthy. If systemd refuses
+    that unit, the restart falls back to an unwatched one.
+
+    Returns ``"watched"``, ``"unwatched"``, or ``None`` when no restart could
+    be scheduled.
     """
     if not shutil.which("systemctl"):
-        return False
+        return None
     if watchdog is not None and shutil.which("systemd-run"):
         cmd = [
             "systemd-run", "--quiet", "--collect",
-            f"--unit=beaconmcp-update-{int(time.time())}",
+            f"--unit=beaconmcp-update-{time.time_ns()}",
             "--setenv=GIT_TERMINAL_PROMPT=0",
             "/bin/sh", "-c", _WATCHDOG_SCRIPT, "beaconmcp-update",
             str(int(delay)), service, watchdog.health_url, str(watchdog.root),
@@ -921,14 +940,18 @@ def _schedule_restart(service: str, delay: int, watchdog: _Watchdog | None = Non
             str(watchdog.root / STATUS_FILE), str(int(watchdog.wait_seconds)),
             str(int(watchdog.settle_seconds)), *watchdog.pip,
         ]
-    else:
-        cmd = [
-            # Values go through argv, never interpolated into the script:
-            # `service` is a literal today, but a future change that made it
-            # configurable must not turn this into a shell injection.
-            "sh", "-c", 'sleep "$1"; systemctl restart "$2"',
-            "sh", str(int(delay)), service,
-        ]
+        # Without --wait, systemd-run returns once the unit has started, so
+        # its exit code says whether the watchdog is actually running.
+        code, _ = _run(cmd, watchdog.root, 30)
+        if code == 0:
+            return "watched"
+    cmd = [
+        # Values go through argv, never interpolated into the script:
+        # `service` is a literal today, but a future change that made it
+        # configurable must not turn this into a shell injection.
+        "sh", "-c", 'sleep "$1"; systemctl restart "$2"',
+        "sh", str(int(delay)), service,
+    ]
     try:
         subprocess.Popen(
             cmd,
@@ -936,9 +959,9 @@ def _schedule_restart(service: str, delay: int, watchdog: _Watchdog | None = Non
             stderr=subprocess.DEVNULL,
             start_new_session=True,
         )
-        return True
+        return "unwatched"
     except OSError:
-        return False
+        return None
 
 
 def apply_update(
@@ -996,6 +1019,21 @@ def _apply_update_locked(
         result.message = "Refusing to update: " + "; ".join(blockers)
         result.steps.append(UpdateStep("preflight", False, result.message))
         return result
+    previous = last_update_status(root)
+    if (
+        previous
+        and previous.get("outcome") == "pending"
+        and time.time() - float(previous.get("at") or 0) < _WATCHDOG_WINDOW_SECONDS
+    ):
+        # The process that ran the last update is gone (it was restarted), so
+        # _apply_lock can't see it. Its watchdog may still roll back to its
+        # own "from" revision and would wipe out this update with it.
+        result.message = (
+            "The previous update is still being checked after its restart. "
+            "Wait a few minutes for it to settle, then try again."
+        )
+        result.steps.append(UpdateStep("preflight", False, result.message))
+        return result
     result.steps.append(UpdateStep("preflight", True, "git checkout is clean"))
 
     code, from_ref, _ = _git(root, "rev-parse", "HEAD")
@@ -1012,7 +1050,12 @@ def _apply_update_locked(
         return result
     result.steps.append(UpdateStep("fetch", True, f"{target.name} is {target.sha[:12]}"))
 
-    ahead, behind = _ahead_behind(root, target.sha)
+    counts = _ahead_behind(root, target.sha)
+    if counts is None:
+        result.message = f"Could not compare HEAD with {target.name}. Nothing was changed."
+        result.steps.append(UpdateStep("fetch", False, result.message))
+        return result
+    ahead, behind = counts
     if behind == 0:
         result.ok = True
         result.to_ref = result.from_ref
@@ -1047,10 +1090,16 @@ def _apply_update_locked(
     def _rollback(reason: str) -> UpdateResult:
         code, out, err = _git(root, "reset", "--hard", from_ref)
         rolled = code == 0
+        deps_ok = False
         if rolled:
             # Put the dependency set back too, so a half-applied update
             # doesn't leave newer libraries against older code.
-            _run(_pip_install_command(install, root), root, _PIP_TIMEOUT)
+            pip_code, pip_out = _run(_pip_install_command(install, root), root, _PIP_TIMEOUT)
+            deps_ok = pip_code == 0
+            result.steps.append(UpdateStep(
+                "rollback-pip-install", deps_ok,
+                "dependencies restored" if deps_ok else _tail(pip_out),
+            ))
         result.rolled_back = rolled
         result.steps.append(
             UpdateStep(
@@ -1059,11 +1108,17 @@ def _apply_update_locked(
             )
         )
         result.ok = False
-        result.message = reason + (
-            " The checkout was rolled back and the server was NOT restarted."
-            if rolled
-            else " ROLLBACK FAILED -- fix the checkout by hand before restarting."
-        )
+        if not rolled:
+            tail = " ROLLBACK FAILED -- fix the checkout by hand before restarting."
+        elif not deps_ok:
+            tail = (
+                f" The checkout was rolled back to {result.from_ref} but reinstalling its "
+                "dependencies FAILED; rerun the pip install by hand before restarting. "
+                "The server was NOT restarted."
+            )
+        else:
+            tail = " The checkout was rolled back and the server was NOT restarted."
+        result.message = reason + tail
         return result
 
     code, out = _run(_pip_install_command(install, root), root, _PIP_TIMEOUT)
@@ -1102,8 +1157,9 @@ def _apply_update_locked(
                 from_sha=from_ref, to_sha=to_sha, python=install.python,
                 pip=_pip_command(install),
             )
-        scheduled = _schedule_restart(install.service, restart_delay, watchdog)
-        watched = scheduled and watchdog is not None and bool(shutil.which("systemd-run"))
+        mode = _schedule_restart(install.service, restart_delay, watchdog)
+        scheduled = mode is not None
+        watched = mode == "watched"
         result.restart_scheduled = scheduled
         result.restart_in_seconds = restart_delay if scheduled else 0
         if scheduled:
diff --git a/tests/test_updates.py b/tests/test_updates.py
index 664ba86..9d076b2 100644
--- a/tests/test_updates.py
+++ b/tests/test_updates.py
@@ -338,7 +338,7 @@ def stub_side_effects(monkeypatch):
         return 0, ""
 
     monkeypatch.setattr(updates, "_run", fake_run)
-    monkeypatch.setattr(updates, "_schedule_restart", lambda service, delay, watchdog=None: True)
+    monkeypatch.setattr(updates, "_schedule_restart", lambda service, delay, watchdog=None: "unwatched")
     return recorded, outcomes
 
 
@@ -436,6 +436,9 @@ def test_apply_rolls_back_when_dependencies_fail(
     assert result.ok is False
     assert result.rolled_back is True
     assert "Dependency install failed" in result.message
+    # Every pip run fails here, the rollback's reinstall included: say so.
+    assert "reinstalling its dependencies FAILED" in result.message
+    assert any(s.name == "rollback-pip-install" and not s.ok for s in result.steps)
     after = subprocess.run(
         ["git", "rev-parse", "HEAD"], cwd=str(checkout),
         capture_output=True, text=True,
@@ -494,7 +497,7 @@ def test_a_second_update_is_refused_while_one_runs(checkout, upstream, monkeypat
         return real_run(cmd, cwd, timeout)
 
     monkeypatch.setattr(updates, "_run", blocking_run)
-    monkeypatch.setattr(updates, "_schedule_restart", lambda service, delay, watchdog=None: True)
+    monkeypatch.setattr(updates, "_schedule_restart", lambda service, delay, watchdog=None: "unwatched")
 
     install = _install(checkout)
     first: dict[str, object] = {}
@@ -570,7 +573,7 @@ def test_restart_command_passes_values_through_argv(monkeypatch):
         updates.shutil, "which", lambda name: "/bin/systemctl" if name == "systemctl" else None,
     )
     monkeypatch.setattr(updates.subprocess, "Popen", FakePopen)
-    assert updates._schedule_restart("beaconmcp; rm -rf /", 5) is True
+    assert updates._schedule_restart("beaconmcp; rm -rf /", 5) == "unwatched"
     cmd = seen["cmd"]
     # The dangerous string is an argument, never part of the script.
     assert "beaconmcp; rm -rf /" in cmd
@@ -582,18 +585,18 @@ def test_restart_with_a_watchdog_runs_as_its_own_transient_unit(monkeypatch, tmp
     """systemctl restart kills the service's cgroup, children included."""
     seen = {}
 
-    class FakePopen:
-        def __init__(self, cmd, **kwargs):
-            seen["cmd"] = cmd
+    def fake_run(cmd, cwd, timeout):
+        seen["cmd"] = cmd
+        return 0, ""
 
     monkeypatch.setattr(updates.shutil, "which", lambda name: f"/usr/bin/{name}")
-    monkeypatch.setattr(updates.subprocess, "Popen", FakePopen)
+    monkeypatch.setattr(updates, "_run", fake_run)
     watchdog = updates._Watchdog(
         health_url="http://127.0.0.1:8420/health", root=tmp_path,
         from_sha="a" * 40, to_sha="b" * 40, python="/opt/venv/bin/python",
         pip=["/opt/venv/bin/pip"],
     )
-    assert updates._schedule_restart("beaconmcp", 5, watchdog) is True
+    assert updates._schedule_restart("beaconmcp", 5, watchdog) == "watched"
     cmd = seen["cmd"]
     assert cmd[0] == "systemd-run"
     script_at = cmd.index("-c") + 1
@@ -622,7 +625,26 @@ def test_restart_without_systemd_run_falls_back_to_an_unwatched_restart(monkeypa
         health_url="http://127.0.0.1:8420/health", root=tmp_path,
         from_sha="a" * 40, to_sha="b" * 40, python=sys.executable, pip=["pip"],
     )
-    assert updates._schedule_restart("beaconmcp", 5, watchdog) is True
+    assert updates._schedule_restart("beaconmcp", 5, watchdog) == "unwatched"
+    assert seen["cmd"][:3] == ["sh", "-c", 'sleep "$1"; systemctl restart "$2"']
+
+
+def test_restart_falls_back_to_unwatched_when_systemd_run_fails(monkeypatch, tmp_path):
+    """A refused transient unit must not be reported as a watched restart."""
+    seen = {}
+
+    class FakePopen:
+        def __init__(self, cmd, **kwargs):
+            seen["cmd"] = cmd
+
+    monkeypatch.setattr(updates.shutil, "which", lambda name: f"/usr/bin/{name}")
+    monkeypatch.setattr(updates, "_run", lambda cmd, cwd, timeout: (1, "unit exists"))
+    monkeypatch.setattr(updates.subprocess, "Popen", FakePopen)
+    watchdog = updates._Watchdog(
+        health_url="http://127.0.0.1:8420/health", root=tmp_path,
+        from_sha="a" * 40, to_sha="b" * 40, python=sys.executable, pip=["pip"],
+    )
+    assert updates._schedule_restart("beaconmcp", 5, watchdog) == "unwatched"
     assert seen["cmd"][:3] == ["sh", "-c", 'sleep "$1"; systemctl restart "$2"']
 
 
@@ -630,7 +652,7 @@ def test_apply_hands_the_watchdog_both_revisions(checkout, upstream, monkeypatch
     calls = []
     monkeypatch.setattr(
         updates, "_schedule_restart",
-        lambda service, delay, watchdog=None: calls.append(watchdog) or True,
+        lambda service, delay, watchdog=None: calls.append(watchdog) or "watched",
     )
     monkeypatch.setattr(updates.shutil, "which", lambda name: f"/usr/bin/{name}")
     updates.configure(UpdatesConfig(), health_url="http://127.0.0.1:8420/health")
@@ -651,6 +673,31 @@ def test_apply_hands_the_watchdog_both_revisions(checkout, upstream, monkeypatch
     assert status["outcome"] == "pending" and status["from"] == before
 
 
+def test_apply_refuses_while_the_previous_update_is_pending(checkout, upstream, monkeypatch, stub_side_effects):
+    """The previous process is gone, so only the status file shows its watchdog."""
+    updates._write_status(checkout, {"at": int(time.time()), "outcome": "pending"})
+    _advance(upstream, "feat: thing")
+    before = _head(checkout)
+
+    result = updates.apply_update(install=_install(checkout))
+
+    assert result.ok is False
+    assert "still being checked" in result.message
+    assert _head(checkout) == before
+
+
+def test_a_stale_pending_status_does_not_block(checkout, upstream, monkeypatch, stub_side_effects):
+    stale = time.time() - updates._WATCHDOG_WINDOW_SECONDS - 1
+    updates._write_status(checkout, {"at": int(stale), "outcome": "pending"})
+    _advance(upstream, "feat: thing")
+    before = _head(checkout)
+
+    result = updates.apply_update(install=_install(checkout))
+
+    assert result.ok is True
+    assert _head(checkout) != before
+
+
 def test_pip_install_is_pinned_when_the_tree_has_constraints(tmp_path):
     install = _install(tmp_path)
     assert "-c" not in updates._pip_install_command(install, tmp_path)
-- 
2.43.0

Generated by Claude Code

… reinstalls

- Refuse a new update while the last status is a 'pending' younger than
  the watchdog's longest run, and make the watchdog leave the checkout
  alone (status 'superseded') if HEAD is no longer the revision it
  watches. A watchdog still waiting can no longer undo a newer update.
- Run systemd-run synchronously and check its exit status; on failure fall
  back to the unwatched restart instead of reporting 'watched'. Unit names
  use time_ns(). _schedule_restart returns 'watched' / 'unwatched' / None.
- The pre-restart rollback checks pip's exit code (new
  'rollback-pip-install' step) and says when the old dependencies could
  not be reinstalled; the watchdog logs the same failure.
- _ahead_behind returns None when git fails, and check/apply report an
  error instead of 'Already up to date'.
- docs/updates.md: what the default channel and the root unit trust.

Reported by Ailcope in review.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Lxm6BaCA2kUHH1f6xvh8kS

Copy link
Copy Markdown
Owner Author

@Ailcope all five confirmed. 2e2e443 never reached this branch, so I applied the fixes in 30e2e42, following your outline:

  1. A new update is refused while the last status is pending and younger than _WATCHDOG_WINDOW_SECONDS (two health waits, a pip run and slack); a stale pending doesn't block. The watchdog also refuses to roll back if HEAD is no longer the revision it watches, and records superseded instead.
  2. systemd-run goes through _run and its exit code is checked. On failure it falls back to the unwatched restart. Units use time_ns(), and _schedule_restart returns "watched" / "unwatched" / None.
  3. _rollback adds a rollback-pip-install step, and the message says when reinstalling the old dependencies failed. The watchdog logs the same failure.
  4. _ahead_behind returns None on failure, and check/apply report an error.
  5. docs/updates.md now has a warning on what the defaults trust, plus notes on the pending guard and superseded.

New tests cover each point, including the real shell watchdog leaving a newer update alone. Full suite: 539 passed, ruff clean. I also reran the real-server scenario on this commit: an update that fails at runtime is still rolled back by the watchdog and comes back up on the old revision.


Generated by Claude Code

@Ailcope Ailcope left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, 30e2e42 covers all five, and the superseded check in the watchdog is a nice extra safety net.


Generated by Claude Code

… guard ignores it

The AST guard from the non-blocking file tools change flags any
client.get() in an async function. This one is an awaited httpx call.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Lxm6BaCA2kUHH1f6xvh8kS
@Showdown76py
Showdown76py merged commit 69a1aa5 into main Oct 11, 2026
6 checks passed
@Showdown76py
Showdown76py deleted the claude/audit-fixes-ycn7ph branch October 11, 2026 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants