feat(doctor): count the update backups nobody was ever told about (#681)

keel/commands/doctor.py

@@ -852,6 +852,122 @@ def orphan_bracket_findings(records: dict[str, Any]) -> list[Finding]:
     ]
 
 
+@dataclass(frozen=True)
+class BackupFootprint:
+    """What `keel update` has left behind in one launch folder (#681)."""
+
+    #: `<db>.bak-before-...` files, grouped by the database they were taken from.
+    per_database: dict[str, int]
+    total_files: int
+    total_bytes: int
+    #: The version stamp of the oldest backup, for the operator's sense of how far back this
+    #: goes -- "0.4.0" says more about whether to act than a byte count does.
+    oldest_version: str | None
+
+
+def backup_footprint_findings(footprint: BackupFootprint, *, keep: int = 3) -> list[Finding]:
+    """Superseded update backups, counted rather than deleted (#681).
+
+    `keel update` copies every database before it installs and NEVER removes one. That is
+    correct -- `update.py` names them as the data-recovery path, and an updater that pruned its
+    own rollback would be an updater you cannot roll back from. So this reports and does not
+    act, and `test_nothing_in_keel_deletes_an_update_backup` is the pin that keeps it that way.
+
+    **The COUNT is the operator-actionable number, not the bytes.** "23 superseded copies of
+    keel.db, oldest from 0.4.0" tells someone what to do; "7 GB" tells them only that something
+    is large. The size rides along because a disk filling during an update is the failure mode,
+    and it is the one moment a rollback path matters most.
+
+    WARN, not FAIL, and never below `keep`: a handful of recent backups is the design working.
+    What is worth a human's attention is a launch folder still holding the rollback for a
+    version nobody could install any more.
+    """
+    superseded = {db: n for db, n in footprint.per_database.items() if n > keep}
+    if not superseded:
+        return [
+            Finding(
+                "backups.footprint",
+                OK,
+                f"{footprint.total_files} update backup(s) retained",
+                f"{_human_bytes(footprint.total_bytes)}; nothing beyond the {keep} most recent "
+                "per database",
+                "-",
+            )
+        ]
+    described = ", ".join(
+        f"{db}: {count}" for db, count in sorted(superseded.items(), key=lambda kv: -kv[1])
+    )
+    since = f" going back to {footprint.oldest_version}" if footprint.oldest_version else ""
+    return [
+        Finding(
+            "backups.footprint",
+            WARN,
+            f"{footprint.total_files} update backups, {_human_bytes(footprint.total_bytes)}",
+            f"{described}{since} -- `keel update` never deletes one, by design, so they "
+            "accumulate one set per release",
+            "review and prune BY HAND: `ls -lhS <launch>/*.bak-before-*`. keel will not delete "
+            "a backup for you -- the release you need is the one before the release that broke",
+        )
+    ]
+
+
+def _human_bytes(count: int) -> str:
+    """A size an operator reads at a glance. Binary units, one decimal, never scientific."""
+    size = float(count)
+    for unit in ("B", "KiB", "MiB", "GiB"):
+        if size < 1024 or unit == "GiB":
+            return f"{size:.1f} {unit}" if unit != "B" else f"{int(size)} B"
+        size /= 1024
+    return f"{size:.1f} GiB"  # pragma: no cover - the loop always returns
+
+
+def read_backup_footprint(launch_dir: Path) -> BackupFootprint:
+    """Measure `launch_dir`'s `.bak-before-*` files. Never raises.
+
+    `doctor` is what an operator runs when something is already wrong, so a launch folder it
+    cannot read must produce an empty measurement rather than an exception -- a diagnostic that
+    dies on the state it exists to describe is worse than no diagnostic.
+
+    **No `try` around the glob, and that is a correction rather than an omission.** The first
+    version wrapped it, on the assumption that a missing or unreadable directory raises. It does
+    not: `Path.glob` returns an empty iterator for a path that does not exist AND for one with
+    mode 000, so the handler was unreachable and a mutation deleting it changed nothing --
+    which is how it was found. What genuinely races is `stat` on a file that vanished between
+    the glob and the read, and that one is guarded below where it can actually happen.
+    """
+    per_database: dict[str, int] = {}
+    total_bytes = 0
+    total_files = 0
+    versions: list[str] = []
+    for path in sorted(launch_dir.glob("*.bak-before-*")):
+        try:
+            total_bytes += path.stat().st_size
+        except OSError:
+            continue
+        total_files += 1
+        database, _, stamp = path.name.partition(".bak-before-")
+        per_database[database] = per_database.get(database, 0) + 1
+        version = stamp.rsplit("-", 2)[0] if "-" in stamp else stamp
+        if version:
+            versions.append(version)
+    return BackupFootprint(
+        per_database=per_database,
+        total_files=total_files,
+        total_bytes=total_bytes,
+        oldest_version=min(versions, key=_version_key) if versions else None,
+    )
+
+
+def _version_key(stamp: str) -> tuple[int, ...]:
+    """Sort `0.4.0` before `0.13.2`, and anything unparseable last -- a hand-named backup
+    (`keel.db.bak-before-recordflow-...`) is not a release and must not claim to be the oldest
+    one."""
+    parts = stamp.split(".")
+    if len(parts) == 3 and all(p.isdigit() for p in parts):
+        return (0, *(int(p) for p in parts))
+    return (1,)
+
+
 def unbooked_exit_findings(
     open_positions: list[dict[str, Any]], orders: list[dict[str, Any]]
 ) -> list[Finding]:
@@ -1021,6 +1137,7 @@ def gather_findings(repo: Any, config: Any, log_lines: Iterable[str], now_ts: in
     """
     from keel import agent
     from keel.commands import fetch
+    from keel.commands import update as update_mod
     from keel.commands._products import _default_sim_products
     from keel.execution import executor as executor_mod
     from keel.execution import guards
@@ -1083,6 +1200,10 @@ def gather_findings(repo: Any, config: Any, log_lines: Iterable[str], now_ts: in
             for key in repo.get_state_keys(reconcile_mod.ORPHAN_BRACKET_PREFIX)
         }
     )
+    # #681. The LAUNCH FOLDER, resolved the way `keel update` resolves it -- the same directory
+    # the runbook's four commands run from -- because that is where the updater writes and
+    # therefore the only place the count means anything.
+    findings += backup_footprint_findings(read_backup_footprint(update_mod._launch_dir()))
     # #639: modes are POOLED here, unlike the partial-fill sweep above -- the ledger
     # invariant belongs to `agent._open_tranche`, which writes it for paper and live alike.
     findings += unbooked_exit_findings(repo.get_open_positions(), repo.get_orders())

keel/commands/update.py

@@ -1125,6 +1125,20 @@ def render_plan_lines(plan: UpdatePlan) -> list[str]:
     for name in plan.wheel_names:
         lines.append(f"  wheel: {name}")
     lines.append(f"  download to: {plan.release_dir}")
+    # #681: what is ALREADY there, named at the one moment the operator is thinking about
+    # backups anyway -- immediately before another set is written. Reported, never acted on:
+    # `keel update` does not delete a backup, and this line is not the beginning of one that
+    # does. A launch folder it cannot read simply contributes nothing.
+    existing = sorted(plan.launch_dir.glob("*.bak-before-*"))
+    if existing:
+        try:
+            total = sum(path.stat().st_size for path in existing)
+        except OSError:  # pragma: no cover - a file that vanished between glob and stat
+            total = 0
+        lines.append(
+            f"  already kept from earlier updates: {len(existing)} file(s), "
+            f"{total / (1024 * 1024):.0f} MiB (never deleted by keel -- prune by hand)"
+        )
     if plan.db_paths:
         names = ", ".join(path.name for path in plan.db_paths)
         lines.append(f"  then back up (never deleted): {names}")

tests/commands/test_doctor.py

@@ -24,6 +24,7 @@
     admissibility_findings,
     allowance_findings,
     attestation_findings,
+    backup_footprint_findings,
     balance_drift_findings,
     data_health_findings,
     doctor_exit_code,
@@ -32,6 +33,7 @@
     orphan_bracket_findings,
     partial_fill_findings,
     rail_state_findings,
+    read_backup_footprint,
     render_json,
     trade_scope_findings,
     unbooked_exit_findings,
@@ -584,6 +586,7 @@ def test_gather_findings_covers_every_check_over_a_seeded_db(tmp_path, valid_con
         "fill.partial",
         "balance.drift",
         "bracket.orphan",
+        "backups.footprint",
         "ledger.unbooked_exit",
         "data.missing",
         "data.stale",
@@ -883,3 +886,159 @@ def test_gather_findings_surfaces_a_swept_orphan(tmp_path, valid_config_path) ->
     (orphan,) = [f for f in findings if f.name == "bracket.orphan"]
     assert orphan.status == "warn"
     assert "BTC-USD" in orphan.detail
+
+
+# -- update backups: counted, never deleted (#681) ------------------------------------------------
+
+
+def _bak(launch: Path, name: str, size: int = 1024) -> None:
+    (launch / name).write_bytes(b"x" * size)
+
+
+def test_a_launch_folder_with_no_backups_says_so_calmly(tmp_path) -> None:
+    """A fresh deployment must not be told it has a problem it does not have."""
+    (finding,) = backup_footprint_findings(read_backup_footprint(tmp_path))
+
+    assert finding.name == "backups.footprint"
+    assert finding.status == "ok"
+
+
+def test_a_handful_of_recent_backups_is_the_design_working(tmp_path) -> None:
+    """`keel update` is SUPPOSED to leave these. Warning about three would train the finding
+    to be ignored by the time it matters."""
+    for version in ("0.13.0", "0.13.1", "0.13.2"):
+        _bak(tmp_path, f"keel.db.bak-before-{version}-20260901-120000")
+
+    (finding,) = backup_footprint_findings(read_backup_footprint(tmp_path))
+
+    assert finding.status == "ok"
+
+
+def test_backups_beyond_the_keep_count_are_surfaced_per_database(tmp_path) -> None:
+    """The COUNT is the operator-actionable number. "23 copies of keel.db, oldest 0.4.0" says
+    what to do; a byte total says only that something is large."""
+    for version in ("0.4.0", "0.9.1", "0.12.2", "0.13.1", "0.13.2"):
+        _bak(tmp_path, f"keel.db.bak-before-{version}-20260901-120000")
+    _bak(tmp_path, "keel-live.db.bak-before-0.13.2-20260901-120000")
+
+    (finding,) = backup_footprint_findings(read_backup_footprint(tmp_path))
+
+    assert finding.status == "warn"
+    assert "keel.db: 5" in finding.detail
+    assert "0.4.0" in finding.detail, "the oldest version is what says how far back this goes"
+    assert "keel-live.db" not in finding.detail, "one backup is not an accumulation"
+
+
+def test_the_fix_never_tells_keel_to_delete_anything(tmp_path) -> None:
+    """**The load-bearing test of this finding.** These files are the data-recovery path, and
+    the release you need is the one before the release that broke. A fix line that offered to
+    prune them would be the updater deleting its own rollback with extra steps."""
+    for version in ("0.4.0", "0.9.1", "0.12.2", "0.13.2"):
+        _bak(tmp_path, f"keel.db.bak-before-{version}-20260901-120000")
+
+    (finding,) = backup_footprint_findings(read_backup_footprint(tmp_path))
+
+    assert "BY HAND" in finding.fix
+    assert "keel will not delete" in finding.fix
+    for forbidden in ("keel backups prune", "--prune", "rm -rf"):
+        assert forbidden not in finding.fix
+
+
+def test_nothing_in_keel_deletes_an_update_backup() -> None:
+    """The pin that outlives this finding.
+
+    `update.py` states that the `.bak-before-*` files are never removed, and every recovery
+    procedure in the runbook rests on it. A future change that added a prune would be a change
+    to the rollback guarantee, and it should have to delete this test to make it.
+    """
+    import keel
+
+    root = Path(keel.__file__).resolve().parent
+    offenders: list[str] = []
+    for path in sorted(root.rglob("*.py")):
+        source = path.read_text(encoding="utf-8")
+        if "bak-before" not in source:
+            continue
+        for line in source.splitlines():
+            if "bak-before" not in line:
+                continue
+            if any(verb in line for verb in ("unlink", "rmtree", "os.remove", "shutil.move")):
+                offenders.append(f"{path.name}: {line.strip()}")
+    assert not offenders, (
+        f"something now deletes an update backup: {offenders}. These are the data-recovery "
+        "path; the release you need is the one before the release that broke."
+    )
+
+
+def test_an_unreadable_launch_folder_does_not_break_doctor(tmp_path) -> None:
+    """`doctor` is what an operator runs when something is already wrong. A diagnostic that
+    dies on the state it exists to describe is worse than no diagnostic."""
+    missing = tmp_path / "not-a-directory"
+
+    footprint = read_backup_footprint(missing)
+
+    assert footprint.total_files == 0
+    assert backup_footprint_findings(footprint)[0].status == "ok"
+
+
+def test_a_hand_named_backup_never_claims_to_be_the_oldest_release(tmp_path) -> None:
+    """`keel.db.bak-before-recordflow-...` exists in the live deployment and is not a version.
+
+    Sorting it as one would report the oldest release as "recordflow", which is both wrong and
+    unactionable -- the operator cannot decide whether to keep a release they cannot name.
+    """
+    _bak(tmp_path, "keel.db.bak-before-recordflow-20260820T075747")
+    for version in ("0.9.1", "0.12.2", "0.13.1", "0.13.2"):
+        _bak(tmp_path, f"keel.db.bak-before-{version}-20260901-120000")
+
+    footprint = read_backup_footprint(tmp_path)
+
+    assert footprint.oldest_version == "0.9.1"
+
+
+def test_gather_findings_reads_the_real_launch_folder(tmp_path, valid_config_path, monkeypatch):
+    """The wiring, not just the finding.
+
+    A mutation replacing the launch-folder read with an empty footprint passed every test above,
+    because the seeded deployment has no backups and both paths then report `ok`. The finding
+    has to be shown reading somewhere real.
+
+    Resolved through `update._launch_dir`, the same seam `keel update` uses, so doctor counts
+    the folder the updater actually writes to rather than the process's cwd.
+    """
+    from keel.commands import update as update_mod
+
+    launch = tmp_path / "launch"
+    launch.mkdir()
+    for version in ("0.4.0", "0.9.1", "0.12.2", "0.13.2"):
+        (launch / f"keel.db.bak-before-{version}-20260901-120000").write_bytes(b"x" * 4096)
+    monkeypatch.setattr(update_mod, "_launch_dir", lambda: launch)
+
+    repo = _seeded_repo(tmp_path / "keel.db")
+    findings = gather_findings(repo, load_config(valid_config_path), [], NOW)
+
+    (footprint,) = [f for f in findings if f.name == "backups.footprint"]
+    assert footprint.status == "warn", (
+        "doctor reported no backups for a folder holding four -- it is not reading the launch "
+        "folder the updater writes to"
+    )
+    assert "keel.db: 4" in footprint.detail
+
+
+def test_a_folder_keel_cannot_read_measures_as_empty(tmp_path) -> None:
+    """`Path.glob` returns nothing for a missing directory and for one with mode 000 alike, so
+    this is a statement about behaviour rather than about a handler -- see the note in
+    `read_backup_footprint` about the guard that was removed for being unreachable."""
+    import os
+
+    unreadable = tmp_path / "sealed"
+    unreadable.mkdir()
+    (unreadable / "keel.db.bak-before-0.1.0-1").write_bytes(b"x")
+    os.chmod(unreadable, 0o000)
+    try:
+        footprint = read_backup_footprint(unreadable)
+    finally:
+        os.chmod(unreadable, 0o755)
+
+    assert footprint.total_files == 0
+    assert read_backup_footprint(tmp_path / "missing").total_files == 0

tests/commands/test_update.py

@@ -1531,3 +1531,31 @@ def test_a_failed_download_does_not_claim_backups_left_by_an_EARLIER_run(tmp_pat
         "an earlier update and this run took none"
     )
     assert stale.is_file(), "an earlier run's backup must never be touched"
+
+
+def test_the_plan_names_the_backups_already_kept(tmp_path: Path) -> None:
+    """#681: surfaced at the one moment the operator is already thinking about backups —
+    immediately before another set is written.
+
+    Reported, never acted on. `keel update` does not delete a backup, and this line is not the
+    beginning of one that does; `tests/commands/test_doctor.py` carries the pin that nothing in
+    keel ever removes one.
+    """
+    launch = _deployment(tmp_path)
+    for version in ("0.12.2", "0.13.1"):
+        (launch / f"keel.db.bak-before-{version}-20260901-120000").write_bytes(b"x" * 2048)
+    plan = _plan(launch)
+
+    lines = up.render_plan_lines(plan)
+
+    kept = [line for line in lines if "already kept" in line]
+    assert kept, f"the plan did not name the existing backups: {lines}"
+    assert "2 file(s)" in kept[0]
+    assert "never deleted by keel" in kept[0]
+
+
+def test_a_launch_folder_with_no_backups_adds_no_line(tmp_path: Path) -> None:
+    """A first update must not be told about a footprint it does not have."""
+    plan = _plan(_deployment(tmp_path))
+
+    assert not [line for line in up.render_plan_lines(plan) if "already kept" in line]