The sidecar must carry EVERY pending tree, not just the newest

Found live, in the code written to prevent exactly this.

The sidecar held ONE snapshot. So a run that reclaimed an older tree, failed to
finish reclaiming it, and then recorded its own snapshot OVERWROTE the only record of
the survivor -- orphaning it permanently.

Observed: job 24 left one snapshot busy and kept the sidecar (correct). Job 46
reclaimed it, hit ZFS's 300s automount window (the runs were minutes apart), left it
behind again, and then wrote its own snapshot over the record. Permanent orphan,
created by the safety net.

The sidecar is now a list. stage_nested carries forward whatever a reclaim could not
delete; cleanup_task sweeps every pending tree and writes back only the survivors.
cleanup_all reports them one per line instead of formatting a list into an f-string
at the user during uninstall.

Job 46 also confirms the automount fix itself: it swept all 256 of its own snapshots
with no straggler.

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

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**
+> `<dataset>/.zfs/snapshot/<snap>` 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.

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}'")
 

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)