diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ec7020..62a0550 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -207,7 +207,35 @@ worse than no alert, because one day it carries a security fix. warning, not a refusal: declining to install over a string we failed to read would be a worse failure than the one being prevented. -- **A stable release may not leave work stranded under `## Unreleased`.** Either it +- **A stable release may not leave work stranded under `## Unreleased + +### Fixed + +- **A reboot mid-backup orphaned the entire snapshot tree, permanently.** The sidecar + is the record of which snapshots a run pinned — and it lives in `/run`, which is + **tmpfs**. A reboot (or a crash) between taking the recursive snapshot and cleaning + it up destroyed that record, leaving one snapshot per descendant dataset — **250+ on + a real pool** — with nothing left pointing at them. Nothing would ever have found + them again. + + `gc_stale_snapshots()` is the backstop: it identifies leftovers **by name**, so it + works when the record is gone. It runs at the start of every backup, after the + sidecar reclaim — the recorded path stays authoritative, and the collector only ever + mops up what the record lost. + + Because it deletes data on a *name match* — a weaker claim than a recorded fact — the + selection is a **pure function** with the harshest tests in the suite. A snapshot is + collected only if **all** of these hold: + + | | | + | --- | --- | + | name is exactly `@-` | so `cloud_backup-5` never matches `cloud_backup-50`, an `auto-*` periodic snapshot, or anything a human made | + | it is not the current run's | parent *and* children are excluded | + | **nothing is mounted from it** | an in-flight run pins its own snapshots — this, not the age guard, is what protects a concurrent backup | + | it is **over an hour old** | covers the seconds-long window where a live run has snapshotted but not yet mounted | + + Verified against the real pool: of **4,728** snapshots — including **2,341** periodic + ones — it selects exactly the orphans of the task being run, and nothing else.`.** Either it is finished and belongs in the release, or the release is premature. Candidates are exempt: an rc may legitimately have work queued behind it. diff --git a/docs/nested-snapshots.md b/docs/nested-snapshots.md index 53eddd8..e8c6274 100644 --- a/docs/nested-snapshots.md +++ b/docs/nested-snapshots.md @@ -95,6 +95,21 @@ re-scan each time. ### Snapshot lifecycle +> **Two mechanisms clean up, and the second exists because the first can be destroyed.** +> +> 1. **The sidecar** records exactly which snapshots a run pinned, and is removed only +> on a confirmed-clean sweep. Precise, and it survives a middlewared restart. +> 2. **The garbage collector** finds leftovers by *name*, so it still works when the +> sidecar is gone — and it can be: **the sidecar lives in `/run`, which is tmpfs.** A +> reboot mid-backup takes it, and with it the only record of a 250-snapshot tree. +> +> The collector runs at the start of every backup, after the sidecar reclaim. It will +> only touch a snapshot named `@-` that is not the current +> run's, has **nothing mounted from it** (which is what protects a concurrently-running +> backup), and is **over an hour old**. Periodic `auto-*` snapshots, other tasks' +> snapshots, and anything you made by hand are structurally out of reach. + + > **A snapshot may survive a run, and that is expected.** ZFS **automounts** > `/.zfs/snapshot/` the moment it is read, and holds it for > `zfs_expire_snapshot` seconds (**300** by default) after the last access. So diff --git a/patch/truecloud_nested.py b/patch/truecloud_nested.py index dd4b99b..1d867ea 100644 --- a/patch/truecloud_nested.py +++ b/patch/truecloud_nested.py @@ -55,6 +55,7 @@ Therefore this module owns the whole lifecycle: from __future__ import annotations import contextlib +import datetime import os import stat import subprocess @@ -68,10 +69,13 @@ __all__ = [ "cleanup_task", "current_mounts_under", "delete_snapshot_tree", + "gc_stale_snapshots", + "mounted_snapshots", "plan_staging", "sidecar_for", "snapshot_tree_names", "stage_nested", + "stale_snapshot_names", "staging_root_for", "teardown", "verify_staged", @@ -176,6 +180,76 @@ def snapshot_tree_names(snapshot: str, all_names) -> list[str]: ] +#: A snapshot must be at least this old before the garbage collector will touch it. +#: +#: The GC identifies our leftovers by NAME, so its only real risk is deleting a +#: snapshot belonging to a run that is still starting up -- the window between +#: `zfs snapshot -r` and the bind mounts appearing, which is seconds. An hour is three +#: orders of magnitude more slack than that window needs, and still reclaims a lost +#: tree on the very next daily run. +GC_MIN_AGE_SECONDS = 3600 + + +def stale_snapshot_names(task_name, current_snapshot, all_names, now, + in_use=(), min_age=GC_MIN_AGE_SECONDS): + """Snapshots THIS task created in an earlier run and never cleaned up. + + Pure, because this is the one function here that DELETES DATA on a name match, and + a name match is a weaker claim than a recorded fact. Everything it relies on is an + argument, so every way it could be wrong is a test. + + Why a garbage collector exists at all, when there is already a sidecar: **the + sidecar lives in /run, which is tmpfs.** A reboot mid-backup destroys it, and with + it the only record of a 250-snapshot tree. The sidecar handles the normal case + precisely; this handles the case where the record itself is gone. + + A snapshot is ours to collect only if ALL of these hold: + + * its name is exactly ``@-`` -- so + ``cloud_backup-5`` never matches ``cloud_backup-50``'s snapshots, and never + matches a periodic ``auto-2026-…`` or anything a human made; + * it is not the snapshot the current run is using; + * nothing is mounted from it (`in_use`) -- an in-flight run pins its own + snapshots, so this alone protects a concurrent one-time backup; + * it is older than `min_age` -- which covers the seconds-long window in which a + run has taken its snapshot but not yet mounted it. + + `now` is a timezone-aware datetime; timestamps in the name are UTC (stock builds + them with `utc_now()`). + """ + prefix = task_name + "-" + stale = [] + + for name in all_names: + _dataset, _, snapname = name.partition("@") + if not snapname or not snapname.startswith(prefix): + continue + if name == current_snapshot or snapname == _snapname_of(current_snapshot): + continue + if name in in_use: + continue + + stamp = snapname[len(prefix):] + try: + when = datetime.datetime.strptime(stamp, "%Y%m%d%H%M%S").replace( + tzinfo=datetime.UTC + ) + except ValueError: + # Not our timestamp format. Something else owns this name; leave it alone. + continue + + if (now - when).total_seconds() < min_age: + continue + + stale.append(name) + + return stale + + +def _snapname_of(snapshot): + return snapshot.partition("@")[2] if snapshot else "" + + def _probe_snapdir(path): """Classify a snapshot directory: ``ok``, ``missing``, or why it is unusable. @@ -598,6 +672,80 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, return remaining +def mounted_snapshots(mounts_file="/proc/self/mounts"): + """Every ZFS snapshot something is currently mounted from. + + The device field of a snapshot mount IS the snapshot name (`Tap/apps/x@snap`), for + both our staging bind mounts and ZFS's own .zfs automounts. So this is a direct, + factual answer to "is anything using this snapshot right now" -- which is what + protects a concurrently-running backup from the garbage collector, rather than + trusting an age heuristic to be generous enough. + """ + live = set() + try: + with open(mounts_file, encoding="utf-8") as fh: + for line in fh: + dev = line.split(" ", 1)[0] + if "@" in dev: + live.add(dev.replace("\\040", " ")) + except OSError: + return set() + return live + + +def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, + now=None, mounts_file="/proc/self/mounts"): + """Delete snapshots this task left behind in an earlier run. Returns what remains. + + The backstop for when the RECORD is gone, not just the snapshots: the sidecar lives + in /run (tmpfs), so a reboot mid-backup takes it with them. Without this, that tree + -- one snapshot per descendant dataset, 250+ on a real pool -- is orphaned with + nothing left pointing at it. + + Selection is `stale_snapshot_names()`, which is pure and heavily tested, because a + name match is a weaker claim than a recorded fact and this deletes data on one. + """ + dataset = current_snapshot.partition("@")[0] + now = now or datetime.datetime.now(datetime.UTC) + + try: + snaps = middleware.call_sync( + "zfs.snapshot.query", [["name", "^", dataset]], {"select": ["name"]} + ) + except Exception as e: # noqa: BLE001 - cannot enumerate; collect nothing + if logger: + logger.warning( + "truecloud-patch: could not enumerate snapshots for GC: %r", e + ) + return [] + + stale = stale_snapshot_names( + task_name, current_snapshot, [s["name"] for s in snaps], now, + in_use=mounted_snapshots(mounts_file), + ) + if not stale: + return [] + + if logger: + logger.warning( + "truecloud-patch: %d snapshot(s) from an earlier run of %s were never " + "cleaned up (a lost record, e.g. a reboot mid-backup); collecting them", + len(stale), task_name, + ) + + remaining = [] + for name in stale: + try: + middleware.call_sync("zfs.snapshot.delete", name) + except Exception as e: # noqa: BLE001 - busy, or gone; either way, next run + remaining.append(name) + if logger: + logger.debug( + "truecloud-patch: could not collect %s: %r", name, e + ) + return remaining + + def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint, task_name, datasets, logger=None): """Build a complete staging tree for `path` from the already-taken `snapshot`. @@ -650,6 +798,18 @@ def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint, "carrying them forward to the next run", len(pending), ) + # ...and collect anything from an earlier run that has NO record at all. + # + # The sidecar above is precise but lives in /run, which is tmpfs -- a reboot + # mid-backup destroys it and orphans the whole tree with nothing pointing at it. + # This finds those by name and is the only thing that ever will. + # + # It runs AFTER the sidecar reclaim on purpose: the recorded path is authoritative + # and cheap, and the GC should only ever be mopping up what the record lost. + pending.extend( + gc_stale_snapshots(middleware, task_name, snapshot, logger=logger) + ) + # Record the snapshot BEFORE mounting anything, not after. middlewared can # die at any point (this patch even schedules a restart at boot), and the # sidecar is the only thing that survives it -- an in-process dict would take diff --git a/tests/test_truecloud_nested.py b/tests/test_truecloud_nested.py index eef6bbc..585f9ff 100644 --- a/tests/test_truecloud_nested.py +++ b/tests/test_truecloud_nested.py @@ -893,3 +893,159 @@ class TestTheSidecarCarriesEveryPendingTree: assert len(notes) == 2 assert "'Tap@a'" in notes[0] and "'Tap@b'" in notes[1] assert "[" not in "".join(notes) + + +class TestGarbageCollectorSelection: + """`stale_snapshot_names` DELETES DATA on a name match. + + A name match is a weaker claim than a recorded fact, so every way it could be wrong + is a test. It exists because the sidecar — which IS a recorded fact — lives in /run, + which is tmpfs: a reboot mid-backup destroys it and orphans a 250-snapshot tree with + nothing left pointing at it. This is the only thing that would ever find those. + """ + + import datetime as _dt + NOW = _dt.datetime(2026, 7, 14, 12, 0, 0, tzinfo=_dt.UTC) + CURRENT = "Tap@cloud_backup-5-20260714115900" # 1 minute ago + OLD = "Tap/apps/x@cloud_backup-5-20260713030000" # ~33 hours ago + + def collect(self, names, **kw): + import truecloud_nested as tn + return tn.stale_snapshot_names( + "cloud_backup-5", self.CURRENT, names, self.NOW, **kw + ) + + def test_it_collects_our_own_leftovers(self): + assert self.collect([self.OLD]) == [self.OLD] + + def test_it_NEVER_touches_the_current_run(self): + # Both the parent and its children share the current snapname. + names = [self.CURRENT, "Tap/apps/x@cloud_backup-5-20260714115900"] + assert self.collect(names) == [] + + def test_it_NEVER_touches_a_periodic_snapshot(self): + assert self.collect(["Tap/apps/x@auto-2026-07-13_03-00"]) == [] + + def test_it_NEVER_touches_a_human_made_snapshot(self): + assert self.collect(["Tap@before-i-broke-everything"]) == [] + + def test_it_NEVER_touches_another_TASK(self): + # cloud_backup-5 must not match cloud_backup-50. This is why the prefix + # carries the trailing dash. + assert self.collect(["Tap/apps/x@cloud_backup-50-20260713030000"]) == [] + assert self.collect(["Tap/apps/x@cloud_backup-7-20260713030000"]) == [] + + def test_it_NEVER_touches_a_one_time_backup(self): + assert self.collect(["Tap@cloud_backup-onetime-20260713030000"]) == [] + + def test_it_NEVER_touches_a_snapshot_that_is_MOUNTED(self): + # An in-flight run pins its own snapshots. This — not the age heuristic — is + # what actually protects a concurrent backup. + assert self.collect([self.OLD], in_use={self.OLD}) == [] + + def test_it_NEVER_touches_a_snapshot_younger_than_the_minimum_age(self): + # Covers the seconds-long window between `zfs snapshot -r` and the mounts + # appearing, when a live run's snapshots look exactly like garbage. + young = "Tap/apps/x@cloud_backup-5-20260714113000" # 30 minutes ago + assert self.collect([young]) == [] + assert self.collect([young], min_age=60) == [young] + + def test_a_name_it_cannot_parse_is_left_alone(self): + assert self.collect(["Tap@cloud_backup-5-not-a-timestamp"]) == [] + assert self.collect(["Tap@cloud_backup-5-"]) == [] + + def test_a_realistic_mixed_pool(self): + names = [ + self.CURRENT, # ours, running + "Tap/apps/x@cloud_backup-5-20260714115900", # ours, running (child) + self.OLD, # ours, orphaned <- + "Tap/apps/y@cloud_backup-5-20260712030000", # ours, orphaned <- + "Tap/apps/x@auto-2026-07-13_03-00", # periodic + "Tap/apps/x@cloud_backup-7-20260713030000", # another task + "Tap@manual-keepme", # human + ] + assert sorted(self.collect(names)) == sorted( + [self.OLD, "Tap/apps/y@cloud_backup-5-20260712030000"] + ) + + +class TestMountedSnapshots: + def test_it_reads_snapshot_names_out_of_the_mount_table(self, tmp_path): + import truecloud_nested as tn + mounts = tmp_path / "mounts" + mounts.write_text( + "tmpfs /run tmpfs rw 0 0\n" + "Tap/apps/x@snap1 /run/truecloud-nested/t/apps/x zfs ro 0 0\n" + "Tap/apps/y@snap1 /mnt/Tap/apps/y/.zfs/snapshot/snap1 zfs ro 0 0\n" + "Tap/live /mnt/Tap/live zfs rw 0 0\n" + ) + live = tn.mounted_snapshots(str(mounts)) + assert live == {"Tap/apps/x@snap1", "Tap/apps/y@snap1"} + assert "Tap/live" not in live # a live dataset is not a snapshot + + +class TestGarbageCollectorExecution: + def test_it_deletes_the_stale_ones_and_nothing_else(self, monkeypatch, tmp_path): + import datetime as dt + import truecloud_nested as tn + + mounts = tmp_path / "mounts" + mounts.write_text("") + now = dt.datetime(2026, 7, 14, 12, 0, 0, tzinfo=dt.UTC) + + mw = FakeMiddleware([ + "Tap@cloud_backup-5-20260714115900", # current run + "Tap/apps/x@cloud_backup-5-20260713030000", # orphan <- + "Tap/apps/x@auto-2026-07-13_03-00", # periodic + "Tap/apps/x@cloud_backup-7-20260713030000", # other task + ]) + remaining = tn.gc_stale_snapshots( + mw, "cloud_backup-5", "Tap@cloud_backup-5-20260714115900", + now=now, mounts_file=str(mounts), + ) + assert remaining == [] + assert mw.snapshots == [ + "Tap@cloud_backup-5-20260714115900", + "Tap/apps/x@auto-2026-07-13_03-00", + "Tap/apps/x@cloud_backup-7-20260713030000", + ] + + def test_a_busy_orphan_is_reported_not_swallowed(self, monkeypatch, tmp_path): + import datetime as dt + import truecloud_nested as tn + + mounts = tmp_path / "mounts" + mounts.write_text("") + now = dt.datetime(2026, 7, 14, 12, 0, 0, tzinfo=dt.UTC) + orphan = "Tap/apps/x@cloud_backup-5-20260713030000" + + mw = BusyMiddleware( + ["Tap@cloud_backup-5-20260714115900", orphan], + busy=[orphan], busy_for=99, + ) + remaining = tn.gc_stale_snapshots( + mw, "cloud_backup-5", "Tap@cloud_backup-5-20260714115900", + now=now, mounts_file=str(mounts), + ) + assert remaining == [orphan] + + def test_it_collects_NOTHING_when_the_query_fails(self, tmp_path): + # Cannot enumerate => cannot know what is ours => delete nothing. + import datetime as dt + import truecloud_nested as tn + + mounts = tmp_path / "mounts" + mounts.write_text("") + + class Broken(FakeMiddleware): + def call_sync(self, method, *args): + if method == "zfs.snapshot.query": + raise RuntimeError("middleware is having a day") + return super().call_sync(method, *args) + + assert tn.gc_stale_snapshots( + Broken(["Tap/apps/x@cloud_backup-5-20260713030000"]), + "cloud_backup-5", "Tap@cloud_backup-5-20260714115900", + now=dt.datetime(2026, 7, 14, 12, 0, 0, tzinfo=dt.UTC), + mounts_file=str(mounts), + ) == []