diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d88b42..9ec7020 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,6 +90,14 @@ worse than no alert, because one day it carries a security fix. one no-op delete on the next run, while a sidecar removed while the tree still exists is unrecoverable. Survivors are reclaimed by the next run. + **Expect the occasional straggler, and expect it to clean itself up.** On a + 256-snapshot tree this reliably sweeps ~255 immediately and may leave **one**: it is + whatever restic read last, so its 300-second window has barely opened. That one is + logged, its sidecar is kept, and the next run reclaims it before doing anything else. + The leak is bounded at a single cycle rather than growing without limit — which is + the property that actually matters. Blocking a backup job for five minutes to chase + the last snapshot would be a worse trade, so it is not made. + - **Installing the patch permanently blocked updating it.** `install.sh` does `chmod +x update.sh`, and git recorded `update.sh` as `100644` — so the chmod was a *tracked modification*, and `update.sh` refuses to run over a dirty tree. Install diff --git a/docs/nested-snapshots.md b/docs/nested-snapshots.md index 3c6213c..53eddd8 100644 --- a/docs/nested-snapshots.md +++ b/docs/nested-snapshots.md @@ -95,6 +95,25 @@ re-scan each time. ### Snapshot lifecycle +> **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 +> whatever restic read *last* is still pinned when we try to destroy it, and +> `zfs destroy` refuses with `dataset is busy`. +> +> The patch unmounts those automounts itself and retries, which clears ~255 of 256 on +> a real pool. The one that remains is **logged, its sidecar is kept, and the next run +> reclaims it before doing anything else** — so the leak is bounded at a single cycle +> instead of growing forever. Seeing one `could not delete snapshot … it will be +> reclaimed on the next run` in the log is normal. Seeing the count *grow* run over run +> is not, and would be a bug. +> +> This is why the sidecar is removed **only on a confirmed-clean sweep**: it is the +> only record those snapshots exist, and a run that dropped it while they were still +> around would orphan them permanently. That is precisely what happened before this was +> fixed. + + `zfs.snapshot.delete` defaults to **`recursive=False`**, and stock `restic_backup()` calls it with no options. Stock is safe only because its validation means a *recursive* snapshot never actually happens in the field. diff --git a/patch/truecloud_nested.py b/patch/truecloud_nested.py index 1662f73..dd4b99b 100644 --- a/patch/truecloud_nested.py +++ b/patch/truecloud_nested.py @@ -114,21 +114,39 @@ def sidecar_for(staging_root: str) -> str: return staging_root + ".snapshot" -def _write_sidecar(staging_root: str, snapshot: str) -> None: - """Record the pinned snapshot on disk. Blocking; call via run_in_thread.""" +def _write_sidecar(staging_root: str, snapshots) -> None: + """Record every snapshot tree this task still owns. One per line. + + A LIST, not a single name -- and that is not over-engineering, it is a bug fix. + + The sidecar used to hold one snapshot, so a run that reclaimed an older tree, + FAILED to finish reclaiming it, and then recorded its own snapshot would + **overwrite the only record of the survivor** -- orphaning it permanently, which is + exactly the outcome the sidecar exists to prevent. Observed live: a snapshot + survived one run, the next run's reclaim also failed (ZFS's 300s automount window + had not elapsed, because the runs were minutes apart), and the record was + destroyed anyway. + + Now every still-pending tree is carried forward until it is actually gone. + """ + if isinstance(snapshots, str): + snapshots = [snapshots] with contextlib.suppress(OSError): os.makedirs(os.path.dirname(staging_root), exist_ok=True) with open(sidecar_for(staging_root), "w", encoding="utf-8") as fh: - fh.write(snapshot) + fh.write("\n".join(dict.fromkeys(snapshots))) # de-duped, order kept -def _read_sidecar(staging_root: str) -> str | None: - """The snapshot a previous run recorded here, if any.""" +def _read_sidecar(staging_root: str): + """Every snapshot tree a previous run recorded here. [] if none. + + Tolerates the old single-line format, which is just a one-element list. + """ try: with open(sidecar_for(staging_root), encoding="utf-8") as fh: - return fh.read().strip() or None + return [ln.strip() for ln in fh if ln.strip()] except OSError: - return None + return [] def _remove_sidecar(staging_root: str) -> None: @@ -606,24 +624,38 @@ def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint, # A previous run may have crashed mid-flight; never build on top of that. teardown(staging_root) - # ...and if it left a sidecar behind, that snapshot tree is still on disk and - # nothing else will ever reclaim it. Sweep it before we overwrite the record, - # or a single crashed run orphans 160+ snapshots permanently. - stale = _read_sidecar(staging_root) - if stale and stale != snapshot: + # ...and if it left snapshot trees behind, they are still on disk and nothing else + # will ever reclaim them. Sweep them before recording our own, or a single crashed + # run orphans 160+ snapshots permanently. + # + # Anything a reclaim FAILS to delete is carried forward, not dropped. Overwriting + # the sidecar with only our own snapshot is what destroyed the record of a survivor + # once already: the reclaim ran, hit ZFS's 300-second automount window (the runs + # were minutes apart), left one snapshot behind, and then the record of it was + # overwritten -- a permanent orphan, created by the very code meant to prevent one. + pending = [] + for stale in _read_sidecar(staging_root): + if stale == snapshot: + continue if logger: logger.warning( "truecloud-patch: reclaiming snapshot tree from an earlier " - "interrupted run: %s", stale, + "run: %s", stale, ) - delete_snapshot_tree(middleware, stale, logger=logger) + pending.extend(delete_snapshot_tree(middleware, stale, logger=logger)) + + if pending and logger: + logger.warning( + "truecloud-patch: %d snapshot(s) from an earlier run are still busy; " + "carrying them forward to the next run", len(pending), + ) # 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 # the sole record of a 160-snapshot tree with it. Writing it after apply_plan # would leave exactly the crash window the sidecar exists to close. - _write_sidecar(staging_root, snapshot) + _write_sidecar(staging_root, [*pending, snapshot]) try: mounts, skipped = plan_staging( @@ -667,9 +699,9 @@ def cleanup_task(middleware, task_name, logger=None): Safe to call unconditionally: a no-op when the task was never staged. """ staging_root = staging_root_for(task_name) - snapshot = _read_sidecar(staging_root) + pinned = _read_sidecar(staging_root) - if snapshot is None and not os.path.isdir(staging_root): + if not pinned and not os.path.isdir(staging_root): return # never staged; nothing to do errors = teardown(staging_root) @@ -677,11 +709,15 @@ def cleanup_task(middleware, task_name, logger=None): for err in errors: logger.warning("truecloud-patch: staging teardown: %s", err) - if snapshot is None: + if not pinned: _remove_sidecar(staging_root) return - survivors = delete_snapshot_tree(middleware, snapshot, logger=logger) + # Every tree this task still owns -- ours, plus anything an earlier run could not + # finish reclaiming. + survivors = [] + for snapshot in pinned: + survivors.extend(delete_snapshot_tree(middleware, snapshot, logger=logger)) # KEEP the sidecar if anything survived. It is the only record that those # snapshots exist, and removing it orphans them permanently. @@ -698,10 +734,13 @@ def cleanup_task(middleware, task_name, logger=None): if survivors: if logger: logger.warning( - "truecloud-patch: %d snapshot(s) from %s could not be deleted; " - "keeping the sidecar so the next run reclaims them", - len(survivors), snapshot, + "truecloud-patch: %d snapshot(s) could not be deleted (still busy); " + "recording them so the next run reclaims them: %s", + len(survivors), ", ".join(survivors), ) + # The SURVIVORS, not the trees we asked to delete. Writing the original list + # back would keep re-sweeping trees that are already gone. + _write_sidecar(staging_root, survivors) return _remove_sidecar(staging_root) @@ -731,8 +770,7 @@ def cleanup_all(base=None, runner=_run, mounts_file="/proc/self/mounts", # a sidecar is the only record that an interrupted run's snapshot tree (one # snapshot per descendant dataset) is still on disk. for sc in sorted(glob_fn(os.path.join(base, "*.snapshot"))): - snap = read_sidecar(sc[: -len(".snapshot")]) - if snap: + for snap in read_sidecar(sc[: -len(".snapshot")]): lines.append(f" NOTE: an interrupted backup left snapshot '{snap}' behind.") lines.append(f" Remove it and its children: zfs destroy -r '{snap}'") diff --git a/tests/test_truecloud_nested.py b/tests/test_truecloud_nested.py index 4f18149..eef6bbc 100644 --- a/tests/test_truecloud_nested.py +++ b/tests/test_truecloud_nested.py @@ -783,3 +783,113 @@ class TestSidecarSurvivesAnIncompleteSweep: monkeypatch.setattr(tn, "delete_snapshot_tree", lambda m, s, logger=None: []) tn.cleanup_task(FakeMiddleware(), "cloud_backup-5") assert not os.path.exists(sidecar_for(root)) + + +class TestTheSidecarCarriesEveryPendingTree: + """The sidecar holds a LIST, and that is a bug fix, not a generalisation. + + It used to hold ONE snapshot. So a run that reclaimed an older tree, FAILED to + finish reclaiming it, and then recorded its own snapshot would **overwrite the only + record of the survivor** — orphaning it permanently, via the exact code written to + prevent orphans. + + Observed live: a snapshot survived one run; the next run's reclaim also failed + (ZFS's 300s automount window had not elapsed, because the two runs were minutes + apart); the record was overwritten; the snapshot was orphaned for good. + """ + + def test_round_trips_a_list(self, tmp_path): + import truecloud_nested as tn + root = str(tmp_path / "cloud_backup-5") + tn._write_sidecar(root, ["Tap@a", "Tap@b"]) + assert tn._read_sidecar(root) == ["Tap@a", "Tap@b"] + + def test_reads_the_old_single_line_format(self, tmp_path): + # Boxes upgrading from an older version have a one-line sidecar on disk. + import truecloud_nested as tn + root = str(tmp_path / "cloud_backup-5") + os.makedirs(os.path.dirname(sidecar_for(root)), exist_ok=True) + with open(sidecar_for(root), "w", encoding="utf-8") as fh: + fh.write("Tap@legacy") + assert tn._read_sidecar(root) == ["Tap@legacy"] + + def test_a_failed_reclaim_is_carried_forward_not_overwritten( + self, tmp_path, monkeypatch + ): + # THE bug. stage_nested reclaims an old tree, cannot finish, then records its + # own snapshot -- the survivor must still be in the sidecar afterwards. + import truecloud_nested as tn + + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + root = tn.staging_root_for("cloud_backup-5") + os.makedirs(os.path.dirname(root), exist_ok=True) + tn._write_sidecar(root, ["Tap@old"]) + + # The reclaim of Tap@old leaves one snapshot behind (still busy). + monkeypatch.setattr( + tn, "delete_snapshot_tree", + lambda m, s, logger=None: ["Tap/apps/x@old"] if s == "Tap@old" else [], + ) + stub_core(monkeypatch, tn, plan=([("/src", root)], [])) + + tn.stage_nested(FakeMiddleware(), "/mnt/Tap", "Tap@new", "Tap", "/mnt/Tap", + "cloud_backup-5", DATASETS) + + recorded = tn._read_sidecar(root) + assert "Tap/apps/x@old" in recorded, ( + "the failed reclaim's survivor was dropped — orphaned forever" + ) + assert "Tap@new" in recorded, "our own snapshot must also be recorded" + + def test_cleanup_sweeps_every_pending_tree_and_records_only_survivors( + self, tmp_path, monkeypatch + ): + import truecloud_nested as tn + + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + root = tn.staging_root_for("cloud_backup-5") + os.makedirs(root, exist_ok=True) + tn._write_sidecar(root, ["Tap@old", "Tap@new"]) + + swept = [] + + def fake_delete(m, s, logger=None): + swept.append(s) + return ["Tap/apps/x@new"] if s == "Tap@new" else [] + + monkeypatch.setattr(tn, "delete_snapshot_tree", fake_delete) + tn.cleanup_task(FakeMiddleware(), "cloud_backup-5") + + assert swept == ["Tap@old", "Tap@new"], "both pending trees must be swept" + # Only the SURVIVOR is written back -- re-recording Tap@old would make every + # future run re-sweep a tree that is already gone. + assert tn._read_sidecar(root) == ["Tap/apps/x@new"] + + def test_a_fully_clean_sweep_removes_the_sidecar(self, tmp_path, monkeypatch): + import truecloud_nested as tn + + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + root = tn.staging_root_for("cloud_backup-5") + os.makedirs(root, exist_ok=True) + tn._write_sidecar(root, ["Tap@a", "Tap@b"]) + monkeypatch.setattr(tn, "delete_snapshot_tree", lambda m, s, logger=None: []) + + tn.cleanup_task(FakeMiddleware(), "cloud_backup-5") + assert not os.path.exists(sidecar_for(root)) + + def test_cleanup_all_reports_each_pending_snapshot_on_its_own_line(self, tmp_path): + # It formats them for a human during uninstall. A list rendered into an + # f-string would print "['Tap@a', 'Tap@b']" at them. + import truecloud_nested as tn + root = str(tmp_path / "cloud_backup-5") + tn._write_sidecar(root, ["Tap@a", "Tap@b"]) + + lines, _errors = tn.cleanup_all( + base=str(tmp_path), + glob_fn=lambda _p: [sidecar_for(root)], + mounts_file=os.devnull, + ) + notes = [ln for ln in lines if "left snapshot" in ln] + assert len(notes) == 2 + assert "'Tap@a'" in notes[0] and "'Tap@b'" in notes[1] + assert "[" not in "".join(notes)