diff --git a/patch/apply.sh b/patch/apply.sh index d1774b5..139633c 100755 --- a/patch/apply.sh +++ b/patch/apply.sh @@ -535,8 +535,8 @@ if _tc_nested is not None: # snapshot=true, not just ours. Two consequences, and the second is worse: # # * everything below is a NEW failure mode for tasks that worked before we - # were installed. A `pool.dataset.query` that errors would break a - # CloudSync job we have no business touching. + # were installed. A `zfs list` that errors would break a CloudSync job + # we have no business touching. # * if a CloudSync task ever were staged, nothing would ever tear it down: # the teardown is wired into cloud_backup's restic_backup finally, and # CRUD_BLOCK deliberately leaves CloudSync's nesting guard intact. The @@ -568,9 +568,16 @@ if _tc_nested is not None: dataset, nested = _tc_nested.get_dataset_recursive(datasets, path) if not nested: - # No children: stock behaviour, untouched. Stock's `finally` owns - # the snapshot from here (its non-recursive delete is correct, - # because a non-nested snapshot has no children). + # Nothing to STAGE -- but we still own the SWEEP, and that is not a + # formality. Stock decides `recursive` by its own rule, and on 26 that + # rule is no longer ours: it snapshots recursively whenever the backup + # path IS the dataset's mountpoint (filesystem.statfs), while + # get_dataset_recursive() sees nothing to stage when the only + # descendants are ZVOLs or legacy/none-mountpoint datasets. Stock then + # deletes the PARENT ONLY. Without this, one snapshot per descendant is + # orphaned on every run, forever, with no sidecar and no GC to find it -- + # and the backup still reports success. + _tc_nested.own_snapshot(middleware, name, snapshot, logger=_logger) return snapshot, snap_path staging_root = _tc_nested.stage_nested( @@ -584,7 +591,19 @@ if _tc_nested is not None: # stays None and its `finally` deletes NOTHING. Sweep the tree ourselves # or leak the parent plus one snapshot per descendant dataset (160+ here) # on every failed run. - _tc_nested.delete_snapshot_tree(middleware, snapshot, logger=_logger) + # + # The sweep is itself wrapped: a cleanup that raises would REPLACE the + # original exception with its own, hiding why the backup actually failed. + # An error handler must not be able to lose the error. + try: + _tc_nested.delete_snapshot_tree(middleware, snapshot, logger=_logger) + except Exception as _tc_sweep_err: + if _logger: + _logger.error( + "truecloud-patch: could not sweep %s after a staging failure " + "(%r) -- it is orphaned and must be deleted by hand", + snapshot, _tc_sweep_err, + ) raise return snapshot, staging_root diff --git a/patch/truecloud_nested.py b/patch/truecloud_nested.py index 17c4c47..f5ef6b7 100644 --- a/patch/truecloud_nested.py +++ b/patch/truecloud_nested.py @@ -62,6 +62,7 @@ import subprocess import time __all__ = [ + "DELETE_METHODS", "SNAPSHOT_SERVICES", "STAGING_BASE", "StagingError", @@ -74,7 +75,7 @@ __all__ = [ "gc_stale_snapshots", "list_snapshot_names", "mounted_snapshots", - "normalise_dataset", + "own_snapshot", "pick_snapshot_service", "plan_staging", "query_filesystems", @@ -123,106 +124,109 @@ __all__ = [ # 26 `pool.snapshot` only -- `plugins/zfs_/` is gone # #: Snapshot CRUD namespaces, best first. `tools/compat.py` checks this exact list -#: (MiddlewareCall.also), so what CI verifies and what runs cannot drift apart. +#: (MiddlewareCall.also) with the same predicate the runtime uses -- the namespace +#: exists AND it defines `delete`/`do_delete` -- and a test binds the two lists +#: together, so what CI verifies and what runs cannot drift apart. SNAPSHOT_SERVICES = ("pool.snapshot", "zfs.snapshot") -def pick_snapshot_service(has_service): - """First namespace in SNAPSHOT_SERVICES that this middleware exposes. +#: The CRUDService method spellings that answer to `.delete`. A +#: CRUDService exposes `delete` from a method NAMED `do_delete`; both are live +#: across the matrix. `tools/compat.py` accepts exactly this pair. +DELETE_METHODS = ("delete", "do_delete") - Pure: `has_service(name) -> bool`. Returns None if middleware has none of - them, which is a middleware we have never seen and must not guess about. + +def pick_snapshot_service(can_delete): + """First namespace in SNAPSHOT_SERVICES that can actually DELETE for us. + + Pure: `can_delete(namespace) -> bool`. Returns None if no namespace can, + which is a middleware we have never seen and must not guess about. + + The predicate is "can delete", NOT "the service is registered", and the + difference is the whole point. `get_service()` only proves the namespace is + in the registry; it says nothing about whether `delete` still exists on it. + `tools/compat.py` checks namespace AND method, so if the runtime settled for + the weaker test the two could disagree — and would, in the one way that + matters: iX guts a method while keeping its service (they have already done + exactly that to `pool.snapshot.do_update` on master). compat would try + `pool.snapshot`, find `delete` gone, fall through to `zfs.snapshot`, and + report **ok**; the runtime would take `pool.snapshot` because the service is + still registered, and then fail on every single delete — orphaning the whole + tree while the backup reports success. + + Same predicate on both sides, so they cannot drift. """ for name in SNAPSHOT_SERVICES: - if has_service(name): + if can_delete(name): return name return None -def _has_service(middleware, name): +def _can_delete(middleware, namespace): + """Is `.delete` actually callable on this middleware?""" try: - middleware.get_service(name) + service = middleware.get_service(namespace) except Exception: - # get_service raises KeyError for an unregistered namespace. Anything - # else here is equally a "cannot use it", and guessing YES on a service - # that is not really there would fail later, mid-backup, holding a - # snapshot -- the worst possible moment. + # KeyError for an unregistered namespace; AttributeError if `get_service` + # itself ever goes away. Both mean "cannot use it", and guessing YES on a + # service that is not really there fails later, mid-backup, holding a + # snapshot -- the worst possible moment to find out. return False - return True + return any(callable(getattr(service, m, None)) for m in DELETE_METHODS) def snapshot_service(middleware): - """The snapshot CRUD namespace this middleware actually has.""" - name = pick_snapshot_service(lambda n: _has_service(middleware, n)) + """The snapshot namespace this middleware can actually delete through.""" + name = pick_snapshot_service(lambda n: _can_delete(middleware, n)) if name is None: raise StagingError( - "middleware exposes neither " + " nor ".join(SNAPSHOT_SERVICES) - + ". Refusing to stage a nested backup, because the snapshot it " + "middleware exposes no usable snapshot delete (" + + " / ".join(f"{n}.delete" for n in SNAPSHOT_SERVICES) + + "). Refusing to stage a nested backup, because the snapshot it " "creates could not then be swept." ) return name -def normalise_dataset(row): - """A `pool.dataset.query` row -> the shape this module's planner speaks. - - The planner does not consume this any more -- :func:`query_filesystems` reads - ZFS directly, because the middleware query is filtered (see the note above). - It is kept because it is the safe way to consume a middleware dataset row if - anything ever needs to, and because the two traps below are not obvious and - cost real debugging to find: - - * `mountpoint` is a plain string there, not ``{"value": ...}``. - * `mounted` is still a property dict, but its ``value`` is ``"YES"``/``"NO"`` - -- UPPERCASE, where the old API said ``"yes"``. The planner tests - ``== "no"``, so an unmounted dataset would read as mounted and the planner - would try to stage a snapdir that is not there. Read ``parsed``, which is a - real bool, and only fall back to the string. - """ - mounted = row.get("mounted") - if isinstance(mounted, dict): - parsed = mounted.get("parsed") - if isinstance(parsed, bool): - is_mounted = parsed - else: - is_mounted = str(mounted.get("value", "yes")).lower() != "no" - elif isinstance(mounted, bool): - is_mounted = mounted - else: - # Absent means the caller did not ask for the property. Assume mounted: - # the planner's own snapdir probe is the real check, and assuming - # UNmounted would silently drop datasets that hold data. - is_mounted = True - - mountpoint = row.get("mountpoint") - if isinstance(mountpoint, dict): # tolerate the old shape too - mountpoint = mountpoint.get("value", "") - - return { - "name": row["name"], - "properties": { - "mountpoint": {"value": mountpoint or ""}, - "mounted": {"value": "yes" if is_mounted else "no"}, - }, - } - - class ZfsError(Exception): """`zfs list` failed. Enumeration is unreliable, so the caller must not guess.""" -def _zfs_lines(args, runner=None): +def _zfs_lines(args, runner=None, fields=None): """`zfs ` as a list of tab-split rows. Raises ZfsError if it fails. Never returns a partial or empty list on failure: a caller that cannot tell "no datasets" from "the command broke" will happily stage nothing, or sweep nothing, and report success. + + `fields`, if given, is the exact number of tab-separated columns every row must + have. A row that does not is an ERROR, not something to skip. `zfs list -H` + neither quotes nor escapes, so a mountpoint containing a tab or a newline would + split wrong -- and quietly dropping that row would remove a dataset from the + staging plan without it appearing in `skipped` either. Silent omission is the + one thing this module may never do, so it raises instead. """ runner = runner or _run - r = runner(["zfs", *args]) + try: + r = runner(["zfs", *args]) + except OSError as e: + # `zfs` missing from middlewared's PATH raises FileNotFoundError, which is + # not a ZfsError and would sail past callers that only expect one. + raise ZfsError(f"could not run zfs: {e}") from e + if r.returncode != 0: raise ZfsError((r.stderr or "").strip() or f"zfs {' '.join(args)} failed") - return [ln.split("\t") for ln in r.stdout.splitlines() if ln.strip()] + + rows = [ln.split("\t") for ln in r.stdout.splitlines() if ln.strip()] + if fields is not None: + bad = [r for r in rows if len(r) != fields] + if bad: + raise ZfsError( + f"zfs {' '.join(args)} returned {len(bad)} row(s) that do not have " + f"{fields} tab-separated fields (first: {bad[0]!r}). Refusing to " + f"guess -- a dropped row is a dataset silently missing from the backup." + ) + return rows def query_filesystems(middleware=None, runner=None): @@ -254,7 +258,7 @@ def query_filesystems(middleware=None, runner=None): """ rows = _zfs_lines( ["list", "-H", "-p", "-o", "name,mountpoint,mounted", "-t", "filesystem"], - runner=runner, + runner=runner, fields=3, ) return [ { @@ -265,7 +269,7 @@ def query_filesystems(middleware=None, runner=None): "mounted": {"value": mounted}, }, } - for name, mountpoint, mounted in (r for r in rows if len(r) == 3) + for name, mountpoint, mounted in rows ] @@ -282,7 +286,7 @@ def list_snapshot_names(dataset, runner=None): """ rows = _zfs_lines( ["list", "-H", "-o", "name", "-t", "snapshot", "-r", dataset], - runner=runner, + runner=runner, fields=1, ) return [r[0] for r in rows] @@ -476,7 +480,7 @@ def plan_staging(base_dataset, base_mountpoint, path, snapshot_name, datasets, staging_root, probe=_probe_snapdir): """Compute the bind-mount plan for staging a nested tree. Pure function. - ``datasets`` is a list of dicts shaped like ``zfs.dataset.query`` results: + ``datasets`` is what :func:`query_filesystems` returns: ``{"name": str, "properties": {"mountpoint": {"value": str}, "mounted": {"value": "yes"|"no"}}}``. @@ -801,12 +805,15 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, middleware.call_sync(f"{svc}.delete", snapshot, {"recursive": True}) return [] except Exception as e: # noqa: BLE001 - fall through to the explicit sweep - # Usually just "parent already gone" (stock's finally won the race once our - # mounts were released), which the sweep below handles. Log it rather than - # swallow it: if the real cause is something else, this is the only place - # it is visible -- the sweep would report a different, downstream failure. + # "Parent already gone" is the EXPECTED race (stock's finally won, once our + # mounts were released) and happens on every clean run, so it is debug. + # Anything else is a real fault -- a namespace that cannot delete, a schema + # change, a permission error -- and this is the only place it is visible, + # because the sweep below will report a different, downstream failure. At + # debug it would never reach disk on stock middlewared, which logs at INFO. if logger: - logger.debug( + expected = "does not exist" in str(e).lower() + (logger.debug if expected else logger.warning)( "truecloud-patch: recursive delete of %s failed (%r); sweeping " "the tree by name instead", snapshot, e, ) @@ -849,12 +856,19 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, return [n for n in failed if n in live] remaining = list(names) + last_error = {} for attempt in range(attempts): failed = [] for name in remaining: try: middleware.call_sync(f"{svc}.delete", name) - except Exception: # noqa: BLE001 - busy, or already gone; sorted out below + except Exception as e: # noqa: BLE001 - busy, or already gone; sorted below + # KEEP the reason. This used to discard it and then report every + # survivor as "(still busy?)" -- which names the one cause that is + # benign and self-healing, and hides the ones that are permanent (a + # namespace that cannot delete, a permission error, a schema change). + # A misleading diagnosis is worse than none: it tells you to wait. + last_error[name] = e failed.append(name) remaining = confirm_gone(failed) @@ -871,8 +885,8 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, if logger: logger.warning( "truecloud-patch: could not delete snapshot %s after %d attempts " - "(still busy?) -- it will be reclaimed on the next run", - name, attempts, + "(last error: %r) -- it stays recorded and the next run reclaims it", + name, attempts, last_error.get(name), ) return remaining @@ -953,27 +967,43 @@ def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, 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`. +def own_snapshot(middleware, task_name, snapshot, logger=None, list_snapshots=None): + """Take ownership of `snapshot`'s whole tree: reclaim, collect, and record it. - `snapshot` is a full ZFS snapshot name ("Tap@cloud_backup-5-2026..."). + Call this on EVERY ``snapshot = true`` cloud_backup run — **whether or not the + tree gets staged**. That unconditionality is the fix for a real leak, so do not + make it conditional again. - `datasets` is the FILESYSTEM dataset list. **It MUST have been enumerated - AFTER `snapshot` was taken.** A list read beforehand can miss a dataset - created in the gap: the recursive snapshot would capture it, but the staging - plan would not, and its data would be silently omitted from the backup. - Enumerated afterwards, an unsnapshotted dataset instead trips the isdir() - check in plan_staging and fails the run loudly. + Stock decides whether to take a RECURSIVE snapshot by its own rule, and that + rule is not ours: - Returns the staging root to hand to the backup tool. + ``<= 25.10`` + stock's ``create_snapshot`` calls ``get_dataset_recursive()`` — the very + function this module vendors. "Stock went recursive" and "we have something + to stage" were therefore the *same question*, and a non-staged snapshot + provably had no children. Stock's non-recursive delete was correct. - Raises StagingError if the tree cannot be staged completely -- the caller - must let that propagate so the backup fails instead of silently uploading a - partial tree. The caller is responsible for deleting `snapshot` in that case - (see SNAPSHOT_BLOCK in apply.sh). + ``26`` + stock uses ``filesystem.statfs``: ``recursive = (path == the dataset's + mountpoint)``. Now the two rules disagree. A dataset whose only descendants + are **ZVOLs** or **legacy/none-mountpoint** datasets gets a RECURSIVE + snapshot — while ``get_dataset_recursive()`` reports nothing to stage, + because neither kind is a mounted filesystem under ``path``. + + In that gap stock takes one snapshot per descendant and then deletes only the + parent (its ``finally`` destroys ``path=snapshot``, non-recursively). Nothing + would ever have found the children: no staging tree, so no sidecar, and the GC + only ever ran from :func:`stage_nested`. One orphan per zvol/legacy descendant, + on every run, forever — while the backup reports SUCCESS. That is the exact + failure this module exists to prevent, reintroduced by a gate. + + So ownership of the sweep is no longer conditional on staging. It is cheap: + :func:`delete_snapshot_tree` is idempotent, and on a genuinely childless + snapshot it is one recursive destroy of a snapshot stock has usually already + removed. + + Returns the staging root; the sidecar sits beside it. """ - snapshot_name = snapshot.split("@", 1)[1] staging_root = staging_root_for(task_name) # A previous run may have crashed mid-flight; never build on top of that. @@ -997,7 +1027,8 @@ def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint, "truecloud-patch: reclaiming snapshot tree from an earlier " "run: %s", stale, ) - pending.extend(delete_snapshot_tree(middleware, stale, logger=logger)) + pending.extend(delete_snapshot_tree( + middleware, stale, logger=logger, list_snapshots=list_snapshots)) if pending and logger: logger.warning( @@ -1013,9 +1044,10 @@ def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint, # # 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) - ) + pending.extend(gc_stale_snapshots( + middleware, task_name, snapshot, logger=logger, + list_snapshots=list_snapshots, + )) # Record the snapshot BEFORE mounting anything, not after. middlewared can # die at any point (this patch even schedules a restart at boot), and the @@ -1023,6 +1055,34 @@ def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint, # 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, [*pending, snapshot]) + return staging_root + + +def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint, + task_name, datasets, logger=None, list_snapshots=None): + """Build a complete staging tree for `path` from the already-taken `snapshot`. + + `snapshot` is a full ZFS snapshot name ("Tap@cloud_backup-5-2026..."). + + `datasets` is the FILESYSTEM dataset list. **It MUST have been enumerated + AFTER `snapshot` was taken.** A list read beforehand can miss a dataset + created in the gap: the recursive snapshot would capture it, but the staging + plan would not, and its data would be silently omitted from the backup. + Enumerated afterwards, an unsnapshotted dataset instead trips the isdir() + check in plan_staging and fails the run loudly. + + Returns the staging root to hand to the backup tool. + + Raises StagingError if the tree cannot be staged completely -- the caller + must let that propagate so the backup fails instead of silently uploading a + partial tree. The caller is responsible for deleting `snapshot` in that case + (see SNAPSHOT_BLOCK in apply.sh). + """ + snapshot_name = snapshot.split("@", 1)[1] + staging_root = own_snapshot( + middleware, task_name, snapshot, logger=logger, + list_snapshots=list_snapshots, + ) try: mounts, skipped = plan_staging( @@ -1060,7 +1120,7 @@ def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint, return staging_root -def cleanup_task(middleware, task_name, logger=None): +def cleanup_task(middleware, task_name, logger=None, list_snapshots=None): """Tear down a task's staging tree and delete the snapshot it pinned. Safe to call unconditionally: a no-op when the task was never staged. @@ -1084,7 +1144,8 @@ def cleanup_task(middleware, task_name, logger=None): # finish reclaiming. survivors = [] for snapshot in pinned: - survivors.extend(delete_snapshot_tree(middleware, snapshot, logger=logger)) + survivors.extend(delete_snapshot_tree( + middleware, snapshot, logger=logger, list_snapshots=list_snapshots)) # KEEP the sidecar if anything survived. It is the only record that those # snapshots exist, and removing it orphans them permanently. diff --git a/tests/test_compat.py b/tests/test_compat.py index f6ca055..450b3c1 100644 --- a/tests/test_compat.py +++ b/tests/test_compat.py @@ -57,6 +57,13 @@ GOOD = { # # This default tree is a MODERN box (25.10/26): it has pool.snapshot and no # zfs.snapshot. The older shape is built explicitly where it is tested. + # Not a plugin: a method on the middleware OBJECT. `snapshot_service()` resolves + # the snapshot namespace through it, so if it vanishes the module cannot sweep the + # snapshot it just took. + "utils/plugins.py": ( + "class LoadPluginsMixin:\n" + " def get_service(self, name):\n pass\n" + ), "plugins/pool_/dataset.py": ( "class PoolDatasetService(CRUDService):\n" " class Config:\n" diff --git a/tests/test_truecloud_nested.py b/tests/test_truecloud_nested.py index cf79f5b..af4dcd7 100644 --- a/tests/test_truecloud_nested.py +++ b/tests/test_truecloud_nested.py @@ -19,6 +19,7 @@ import pytest sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "patch")) +import truecloud_nested as tn # noqa: E402 from truecloud_nested import ( # noqa: E402 StagingError, apply_plan, @@ -34,6 +35,41 @@ from truecloud_nested import ( # noqa: E402 verify_staged, ) + +@pytest.fixture(autouse=True) +def never_touch_the_real_system(monkeypatch): + """A unit test must never shell out to the real box. This makes it impossible. + + Five tests silently did. `gc_stale_snapshots`'s DEFAULT lister runs + `zfs list -t snapshot -r Tap` -- and on the NAS, `Tap` is the real pool, with + 2148 snapshots and a genuine leaked `cloud_backup-5` snapshot in it. Those tests + passed on the dev box only because it has no `zfs` binary (FileNotFoundError, + swallowed by a broad except), and would have gone RED on the one machine the + release process requires them green on -- for reasons having nothing to do with + the code. The same code path calls `umount`. + + Tests that need a system command inject one (`runner=` / `list_snapshots=`). + + It RECORDS and asserts at teardown rather than raising, because raising would be + swallowed: `gc_stale_snapshots` catches broad `Exception` around its enumeration + (deliberately — it must fail toward collecting nothing). That swallow is exactly + what let five tests shell out unnoticed, so the check must survive it. + """ + attempted = [] + + def forbidden(cmd): + attempted.append(cmd) + raise AssertionError(f"real system command in a unit test: {cmd!r}") + + monkeypatch.setattr(tn, "_run", forbidden) + yield + assert not attempted, ( + "this test ran real system commands: " + + "; ".join(repr(c) for c in attempted) + + ". Inject a fake (runner= / list_snapshots=) -- on the NAS these hit the " + "REAL pool, and the suite must not depend on the machine it runs on." + ) + SNAP = "cloud_backup-5-20260712030000" ROOT = "/run/truecloud-nested/cloud_backup-5" @@ -197,6 +233,22 @@ class TestSnapshotTreeNames: assert snapshot_tree_names("Tap", self.ALL) == [] +class _FakeSnapshotService: + """What `middleware.get_service("")` hands back. + + It carries a `delete` (or `do_delete`) attribute and nothing else, because that + is the ONLY thing the runtime inspects: `snapshot_service()` asks "can this + namespace delete for me?", not "is this namespace registered?". A service object + with no delete must be REFUSED — iX has already gutted a method while keeping its + service (`pool.snapshot.do_update` on master), and settling for the weaker + question is how the runtime and `tools/compat.py` would come to disagree. + """ + + def __init__(self, delete_method): + if delete_method: + setattr(self, delete_method, lambda *a, **kw: None) + + class FakeMiddleware: """middlewared as this module actually uses it: `call_sync`, from a thread. @@ -217,16 +269,22 @@ class FakeMiddleware: snapshots on somebody's NAS. """ - def __init__(self, snapshots=None, snapshot_ns="pool.snapshot"): + def __init__(self, snapshots=None, snapshot_ns="pool.snapshot", + delete_method="delete"): self.snapshots = list(snapshots or []) self.calls = [] self.logger = None self.snapshot_ns = snapshot_ns + #: A CRUDService exposes `delete` from a method NAMED `do_delete`. Both + #: spellings are live across the matrix, and the runtime must accept either + #: -- it resolves the namespace by asking whether it can DELETE, not merely + #: whether the service is registered. + self.delete_method = delete_method def get_service(self, name): if name != self.snapshot_ns: raise KeyError(name) # middleware raises KeyError for an unknown ns - return object() + return _FakeSnapshotService(self.delete_method) def list_snapshots(self, dataset): """Stands in for `zfs list -t snapshot -r `. @@ -312,27 +370,37 @@ class TestDeleteSnapshotTree: # 252 sequential deletes are slow AND not atomic: a run killed part-way # through leaves exactly the orphans this function exists to prevent. mw = FakeMiddleware(["Tap@snap", "Tap/apps@snap", "Tap/apps/lidarr@snap"]) - delete_snapshot_tree(mw, "Tap@snap", list_snapshots=mw.list_snapshots) + listed = [] + + def counting_lister(dataset): + listed.append(dataset) + return mw.list_snapshots(dataset) + + delete_snapshot_tree(mw, "Tap@snap", list_snapshots=counting_lister) assert mw.snapshots == [] deletes = [a for m, a in mw.calls if m.endswith(".delete")] assert len(deletes) == 1, "should be ONE recursive call, not one per snapshot" assert deletes[0][1] == {"recursive": True} - assert not [m for m, _a in mw.calls if m.endswith(".query")], ( - "no enumeration needed on the fast path" + assert listed == [], ( + "the fast path enumerated. On the real pool that is a `zfs list` over 2148 " + "snapshots on every clean run, for nothing." ) - def test_survives_recursive_and_query_failure_by_deleting_the_parent(self): - class Broken(FakeMiddleware): + def test_survives_recursive_and_enumeration_failure_by_deleting_the_parent(self): + # Both the recursive delete AND the ZFS enumeration fail. The sweep must still + # remove the parent rather than give up entirely. + class NoRecursive(FakeMiddleware): def call_sync(self, method, *args): - if method.endswith(".query"): - raise RuntimeError("boom") if method.endswith(".delete") and len(args) > 1: raise RuntimeError("recursive delete unavailable") return super().call_sync(method, *args) - mw = Broken(["Tap@snap"]) - delete_snapshot_tree(mw, "Tap@snap", list_snapshots=mw.list_snapshots) - assert mw.snapshots == [] + def broken_lister(_dataset): + raise tn.ZfsError("boom") + + mw = NoRecursive(["Tap@snap"]) + delete_snapshot_tree(mw, "Tap@snap", list_snapshots=broken_lister) + assert mw.snapshots == [], "must fall back to at least deleting the parent" def test_leaves_unrelated_snapshots_alone_when_the_tree_is_gone(self): mw = FakeMiddleware(["Tap@unrelated"]) @@ -352,9 +420,10 @@ class TestStageNestedOrdering: stub_core(monkeypatch, tn, order=order, plan=([("/src", str(tmp_path / "cloud_backup-5"))], [])) + _mw = FakeMiddleware() tn.stage_nested( - FakeMiddleware(), "/mnt/Tap", "Tap@snap", "Tap", "/mnt/Tap", - "cloud_backup-5", DATASETS, + _mw, "/mnt/Tap", "Tap@snap", "Tap", "/mnt/Tap", + "cloud_backup-5", DATASETS, list_snapshots=_mw.list_snapshots, ) assert order.index("_write_sidecar") < order.index("apply_plan") @@ -377,7 +446,7 @@ class TestStageNestedOrdering: tn.stage_nested( mw, "/mnt/Tap", "Tap@new", "Tap", "/mnt/Tap", - "cloud_backup-5", DATASETS, + "cloud_backup-5", DATASETS, list_snapshots=mw.list_snapshots, ) assert mw.snapshots == [], "the crashed run's snapshot tree must be reclaimed" @@ -397,9 +466,10 @@ class TestStageNestedOrdering: stub_core(monkeypatch, tn, plan_raises=StagingError("boom")) with pytest.raises(StagingError): + _mw = FakeMiddleware() tn.stage_nested( - FakeMiddleware(), "/mnt/Tap", "Tap@snap", "Tap", "/mnt/Tap", - "cloud_backup-5", DATASETS, + _mw, "/mnt/Tap", "Tap@snap", "Tap", "/mnt/Tap", + "cloud_backup-5", DATASETS, list_snapshots=_mw.list_snapshots, ) assert os.path.exists(sidecar_for(root)), ( @@ -426,7 +496,7 @@ class TestCleanupTask: mw = FakeMiddleware(["Tap@snap", "Tap/apps@snap"]) monkeypatch.setattr(tn, "teardown", lambda *_a, **_k: []) - cleanup_task(mw, "cloud_backup-5") + cleanup_task(mw, "cloud_backup-5", list_snapshots=mw.list_snapshots) assert mw.snapshots == [] assert not os.path.exists(sidecar_for(root)) @@ -436,7 +506,7 @@ class TestCleanupTask: monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path / "nope")) mw = FakeMiddleware(["Tap@snap"]) - cleanup_task(mw, "cloud_backup-5") + cleanup_task(mw, "cloud_backup-5", list_snapshots=mw.list_snapshots) assert mw.calls == [] assert mw.snapshots == ["Tap@snap"] @@ -798,9 +868,9 @@ class TestSidecarSurvivesAnIncompleteSweep: mw = BusyMiddleware(["Tap@snap", "Tap/apps/prometheus@snap"], busy=["Tap/apps/prometheus@snap"], busy_for=99) monkeypatch.setattr(tn, "delete_snapshot_tree", - lambda m, s, logger=None: ["Tap/apps/prometheus@snap"]) + lambda m, s, logger=None, **kw: ["Tap/apps/prometheus@snap"]) - tn.cleanup_task(mw, "cloud_backup-5") + tn.cleanup_task(mw, "cloud_backup-5", list_snapshots=mw.list_snapshots) assert os.path.exists(sidecar_for(root)), ( "sidecar removed despite survivors — they are now orphaned forever" ) @@ -814,8 +884,9 @@ class TestSidecarSurvivesAnIncompleteSweep: with open(sidecar_for(root), "w", encoding="utf-8") as fh: fh.write("Tap@snap") - monkeypatch.setattr(tn, "delete_snapshot_tree", lambda m, s, logger=None: []) - tn.cleanup_task(FakeMiddleware(), "cloud_backup-5") + monkeypatch.setattr(tn, "delete_snapshot_tree", lambda m, s, logger=None, **kw: []) + _mw = FakeMiddleware() + tn.cleanup_task(_mw, "cloud_backup-5", list_snapshots=_mw.list_snapshots) assert not os.path.exists(sidecar_for(root)) @@ -862,12 +933,14 @@ class TestTheSidecarCarriesEveryPendingTree: # 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 [], + lambda m, s, logger=None, **kw: ["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) + _mw = FakeMiddleware() + tn.stage_nested(_mw, "/mnt/Tap", "Tap@new", "Tap", "/mnt/Tap", + "cloud_backup-5", DATASETS, + list_snapshots=_mw.list_snapshots) recorded = tn._read_sidecar(root) assert "Tap/apps/x@old" in recorded, ( @@ -887,12 +960,13 @@ class TestTheSidecarCarriesEveryPendingTree: swept = [] - def fake_delete(m, s, logger=None): + def fake_delete(m, s, logger=None, **kw): 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") + _mw = FakeMiddleware() + tn.cleanup_task(_mw, "cloud_backup-5", list_snapshots=_mw.list_snapshots) 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 @@ -906,9 +980,10 @@ class TestTheSidecarCarriesEveryPendingTree: 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: []) + monkeypatch.setattr(tn, "delete_snapshot_tree", lambda m, s, logger=None, **kw: []) - tn.cleanup_task(FakeMiddleware(), "cloud_backup-5") + _mw = FakeMiddleware() + tn.cleanup_task(_mw, "cloud_backup-5", list_snapshots=_mw.list_snapshots) assert not os.path.exists(sidecar_for(root)) def test_cleanup_all_reports_each_pending_snapshot_on_its_own_line(self, tmp_path): @@ -1073,18 +1148,23 @@ class TestGarbageCollectorExecution: mounts = tmp_path / "mounts" mounts.write_text("") - class Broken(FakeMiddleware): - def call_sync(self, method, *args): - if method.endswith(".query"): - raise RuntimeError("middleware is having a day") - return super().call_sync(method, *args) + # The ENUMERATION fails -- which is now a `zfs list` that cannot run, not a + # middleware query. (This test used to fake a failure of `.query`, a call + # production no longer makes, so it passed no matter what the code did.) + def broken_lister(_dataset): + raise tn.ZfsError("cannot open 'Tap': pool I/O is currently suspended") + mw = FakeMiddleware(["Tap/apps/x@cloud_backup-5-20260713030000"]) assert tn.gc_stale_snapshots( - Broken(["Tap/apps/x@cloud_backup-5-20260713030000"]), + mw, "cloud_backup-5", "Tap@cloud_backup-5-20260714115900", now=dt.datetime(2026, 7, 14, 12, 0, 0, tzinfo=dt.UTC), mounts_file=str(mounts), + list_snapshots=broken_lister, ) == [] + assert mw.snapshots, ( + "cannot enumerate => cannot know what is ours => must delete NOTHING" + ) class TestEnumerationComesFromZfsNotMiddleware: @@ -1178,3 +1258,224 @@ class TestEnumerationComesFromZfsNotMiddleware: "pool.snapshot.query hides this; a sweep that cannot see it orphans it " "on every single run, forever" ) + + +class TestTheProductionWiringIsWhatWeThinkItIs: + """Tests of a seam prove nothing if production stops using the seam. + + An audit mutation-tested this suite and found two regressions that reinstate the + exact bug this module exists to prevent, while all 293 tests still passed: + + * swap `delete_snapshot_tree`/`gc_stale_snapshots`'s DEFAULT enumerator for one + that returns [] (which is what middleware's filtered query does for the 84 + hidden datasets) -- green, because every test injected its own. + * put `pool.dataset.query` back into apply.sh's injected block -- green, because + nothing asserted what that block enumerates with. + + Both are pinned here. These tests are about the WIRING, not the logic. + """ + + def test_the_default_snapshot_enumerator_is_the_ZFS_one(self, monkeypatch, tmp_path): + # Called with no `list_snapshots=`, exactly as production calls it. + called = [] + monkeypatch.setattr(tn, "list_snapshot_names", + lambda ds, **kw: called.append(ds) or []) + + mw = FakeMiddleware(["Tap@snap"]) + + class NoRecursive(FakeMiddleware): + def call_sync(self, method, *args): + if method.endswith(".delete") and len(args) > 1: + raise RuntimeError("recursive delete unavailable") + return super().call_sync(method, *args) + + tn.delete_snapshot_tree(NoRecursive(["Tap@snap"]), "Tap@snap") + assert called == ["Tap"], ( + "delete_snapshot_tree's fallback sweep must enumerate from ZFS by default. " + "If it defaults to a middleware query, it cannot see the internal datasets " + "and orphans one snapshot per hidden dataset on every run." + ) + + called.clear() + mounts = tmp_path / "mounts" + mounts.write_text("") + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + tn.gc_stale_snapshots(mw, "cloud_backup-5", "Tap@cloud_backup-5-2026", + mounts_file=str(mounts)) + assert called == ["Tap"], "the GC must enumerate from ZFS by default too" + + def test_the_injected_block_enumerates_from_ZFS_not_middleware(self): + # apply.sh is a shell file holding the Python that gets injected into + # middlewared. Assert on what that block actually says. + with open(os.path.join(os.path.dirname(__file__), "..", "patch", "apply.sh"), + encoding="utf-8") as fh: + src = fh.read() + + assert "_tc_nested.query_filesystems(" in src, ( + "the staging plan must be built from query_filesystems() (which reads ZFS)" + ) + for filtered in ("pool.dataset.query", "pool.snapshot.query", + "zfs.dataset.query", "zfs.snapshot.query"): + assert f'"{filtered}"' not in src, ( + f"apply.sh calls {filtered}. Middleware's queries apply a visibility " + f"policy and hide ix-apps/*, .system/*, .ix-virt/* -- 84 of 270 " + f"datasets on a real pool, including live app data. Enumerating from " + f"them omits those datasets from the backup SILENTLY." + ) + + +class TestTheSnapshotNamespaceIsResolvedNotAssumed: + """24.10/25.04 have only `zfs.snapshot`; 26 has only `pool.snapshot`. + + Every one of these mutations used to pass the whole suite, because no test ever + built a non-default middleware generation: + + * `_can_delete` -> always True (breaks 24.10: picks a namespace that isn't there) + * SNAPSHOT_SERVICES reversed (breaks 26) + * `snapshot_service` guessing instead of raising + """ + + def test_a_24_10_box_deletes_through_zfs_snapshot(self): + mw = FakeMiddleware(["Tap@snap", "Tap/apps@snap"], snapshot_ns="zfs.snapshot") + assert tn.delete_snapshot_tree(mw, "Tap@snap", + list_snapshots=mw.list_snapshots) == [] + assert mw.snapshots == [] + methods = {m for m, _a in mw.calls} + assert methods == {"zfs.snapshot.delete"}, ( + f"a 24.10 box has no pool.snapshot; called {methods}" + ) + + def test_a_26_box_deletes_through_pool_snapshot(self): + mw = FakeMiddleware(["Tap@snap", "Tap/apps@snap"], snapshot_ns="pool.snapshot") + assert tn.delete_snapshot_tree(mw, "Tap@snap", + list_snapshots=mw.list_snapshots) == [] + assert {m for m, _a in mw.calls} == {"pool.snapshot.delete"} + + def test_a_CRUDService_that_only_defines_do_delete_is_usable(self): + # `delete` is exposed FROM a method named `do_delete`. compat.py accepts both, + # so the runtime must too, or they disagree about the same box. + mw = FakeMiddleware(["Tap@snap"], delete_method="do_delete") + assert tn.snapshot_service(mw) == "pool.snapshot" + + def test_a_registered_service_that_CANNOT_delete_is_not_chosen(self): + # The subtle one. `get_service()` only proves the namespace is registered. + # iX has already gutted a method while keeping its service + # (`pool.snapshot.do_update` on master). If the runtime settled for "the + # service exists", it would pick pool.snapshot, fail every delete, and orphan + # the whole tree -- while compat.py, which checks the METHOD, fell through to + # zfs.snapshot and reported the box healthy. + class GuttedPoolSnapshot(FakeMiddleware): + def get_service(self, name): + if name == "pool.snapshot": + return object() # registered, but no delete on it + if name == "zfs.snapshot": + return super().get_service("zfs.snapshot") + raise KeyError(name) + + mw = GuttedPoolSnapshot(["Tap@snap"], snapshot_ns="zfs.snapshot") + assert tn.snapshot_service(mw) == "zfs.snapshot", ( + "must fall through to a namespace that can actually delete" + ) + + def test_no_usable_namespace_REFUSES_rather_than_guessing(self): + class Neither(FakeMiddleware): + def get_service(self, name): + raise KeyError(name) + + with pytest.raises(StagingError, match="no usable snapshot delete"): + tn.snapshot_service(Neither()) + + def test_the_runtime_list_and_the_compat_manifest_cannot_drift(self): + # tools/compat.py claims "the runtime picks the same way ... so what this + # checks and what the patch does cannot drift apart." Nothing enforced that, + # and reordering SNAPSHOT_SERVICES silently broke the claim. Now it is bound. + sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "tools")) + import compat + + call = next(c for c in compat.MIDDLEWARE_CALLS if c.id == "call-snapshot-delete") + checked = [compat.MiddlewareCall.namespace_of(m) for m, _p in call.options] + assert checked == list(tn.SNAPSHOT_SERVICES), ( + f"compat checks {checked} but the runtime tries {list(tn.SNAPSHOT_SERVICES)} " + f"-- in THIS order. They must agree, or CI blesses a box that fails at run " + f"time." + ) + assert set(compat.DELETE_NAMES) == set(tn.DELETE_METHODS), ( + "compat and the runtime must accept the same delete spellings" + ) + + +class TestWeOwnTheSweepEvenWhenWeDoNotStage: + """TrueNAS 26 leak: stock's `recursive` rule is not the patch's `nested` rule. + + <= 25.10 stock's create_snapshot calls get_dataset_recursive() -- the same + function this module vendors. "Stock went recursive" and "we have + something to stage" were the SAME question, so a non-staged snapshot + provably had no children and stock's non-recursive delete was correct. + + 26 stock uses filesystem.statfs: recursive = (path == the dataset's + mountpoint). Now the rules disagree. A dataset whose only descendants + are ZVOLs or legacy/none-mountpoint datasets gets a RECURSIVE snapshot, + while get_dataset_recursive() reports nothing to stage -- neither kind + is a mounted filesystem under `path`. + + Stock then destroys the PARENT ONLY. With no staging tree there was no sidecar, + and the GC only ever ran from stage_nested -- so nothing on the box would ever + have found the children. One orphan per zvol/legacy descendant, on every run, + forever, while the backup reports SUCCESS. + + Ownership of the sweep is therefore NOT conditional on staging. + """ + + def test_own_snapshot_records_a_snapshot_it_did_not_stage(self, tmp_path, monkeypatch): + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + mounts = tmp_path / "mounts" + mounts.write_text("") + + mw = FakeMiddleware(["Tap@cloud_backup-5-2026", "Tap/vm-zvol@cloud_backup-5-2026"]) + root = tn.own_snapshot(mw, "cloud_backup-5", "Tap@cloud_backup-5-2026", + list_snapshots=mw.list_snapshots) + + assert tn._read_sidecar(root) == ["Tap@cloud_backup-5-2026"], ( + "the snapshot must be RECORDED even though nothing was staged -- the " + "sidecar is the only thing that makes the sweep happen" + ) + + def test_the_recorded_snapshot_is_then_actually_swept(self, tmp_path, monkeypatch): + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + mounts = tmp_path / "mounts" + mounts.write_text("") + + # A recursive snapshot of a dataset whose only child is a ZVOL: exactly the + # 26 case. Nothing to stage, but the children are real. + mw = FakeMiddleware([ + "Tap@cloud_backup-5-2026", + "Tap/vm-zvol@cloud_backup-5-2026", + "Tap/legacy-ds@cloud_backup-5-2026", + ]) + tn.own_snapshot(mw, "cloud_backup-5", "Tap@cloud_backup-5-2026", + list_snapshots=mw.list_snapshots) + + # ...then the run finishes and cleanup fires, exactly as restic_backup's + # `finally` does. + tn.cleanup_task(mw, "cloud_backup-5", list_snapshots=mw.list_snapshots) + + assert mw.snapshots == [], ( + "the zvol/legacy children of an unstaged recursive snapshot were orphaned. " + "Stock deletes only the parent; if we do not own the sweep, nothing does." + ) + + def test_apply_sh_owns_the_snapshot_on_the_not_nested_path(self): + # The gate itself. It used to `return snapshot, snap_path` and hand the + # snapshot back to stock, whose delete is non-recursive. + with open(os.path.join(os.path.dirname(__file__), "..", "patch", "apply.sh"), + encoding="utf-8") as fh: + src = fh.read() + + gate = src.index("if not nested:") + ret = src.index("return snapshot, snap_path", gate) + assert "_tc_nested.own_snapshot(" in src[gate:ret], ( + "the not-nested path returns to stock without recording the snapshot. On " + "26 stock may have taken a RECURSIVE snapshot (its rule is statfs-based, " + "not ours) and deletes only the parent -- so every child is orphaned, with " + "no sidecar and no GC, on every run." + ) diff --git a/tools/compat.py b/tools/compat.py index 48aa692..7a991d0 100644 --- a/tools/compat.py +++ b/tools/compat.py @@ -133,9 +133,43 @@ ASSUMPTIONS = [ params=["middleware", "job", "cloud_backup"], why="SYNC_BLOCK wraps it to tear down bind mounts in a finally", ), + Assumption( + # Not a plugin method -- a method on the middleware OBJECT itself, which the + # manifest had no way to express and therefore never checked. + # + # The nested module calls `middleware.get_service()` to decide whether to + # sweep snapshots through `pool.snapshot` or `zfs.snapshot` (see + # SNAPSHOT_SERVICES). If it ever disappears, `_can_delete()` catches the + # AttributeError, reports BOTH namespaces unusable, and every nested backup + # fails -- loudly, but only at RUN time, on a box the preflight had already + # declared healthy. Checking it costs one file read. + "get-service", NESTED, "utils/plugins.py", + "LoadPluginsMixin.get_service", kind="method", + params=["self", "name"], + why="snapshot_service() resolves the snapshot namespace through it; without " + "it the module cannot sweep the snapshot it just took", + ), ] +def accepted_spellings(name): + """The method names that satisfy a call to `.`. + + A CRUDService exposes `create`/`update`/`delete` from methods NAMED + `do_create`/`do_update`/`do_delete`. Both are live across the matrix: 24.10 and + 25.04 declare `do_delete`, 25.10 renamed it to `delete`, and all of them answer + to `.delete`. Accepting only the literal name reported working releases as + BROKEN and would have switched nested snapshots off on boxes where they work. + """ + return (name, f"do_{name}") + + +#: The spellings that satisfy `.delete`. A test binds this to the runtime's +#: `truecloud_nested.DELETE_METHODS`, so the checker and the patch cannot come to +#: disagree about what "can delete" means on the same box. +DELETE_NAMES = accepted_spellings("delete") + + class MiddlewareCall: """A middlewared METHOD the injected code calls at runtime. @@ -195,14 +229,6 @@ class MiddlewareCall: def name_of(method): return method.rsplit(".", 1)[1] - @property - def namespace(self): - return self.namespace_of(self.method) - - @property - def name(self): - return self.name_of(self.method) - #: Every middlewared method the nested module calls at runtime. #: The middleware methods the nested module CALLS. @@ -220,9 +246,8 @@ class MiddlewareCall: #: cannot be deleted from under us the way `zfs.*` just was. #: * The same methods, in the same files, exist on 24.10 through 26. One code #: path, no version conditionals. -#: * `pool.snapshot.delete` takes `recursive`, which the private call did not. -#: The old sweep had to enumerate ~250 snapshots and delete them one at a -#: time, and any it missed leaked forever. +#: * Both spellings take `recursive`, so ONE call sweeps the whole tree instead +#: of ~250 individual deletes, any of which could be missed. MIDDLEWARE_CALLS = [ MiddlewareCall( "call-snapshot-delete", NESTED, "pool.snapshot.delete", @@ -303,7 +328,7 @@ def check_call(c: MiddlewareCall, src: str | None, n.name for n in ast.walk(tree) if isinstance(n, ast.FunctionDef | ast.AsyncFunctionDef) } - if name not in defined and f"do_{name}" not in defined: + if not any(sp in defined for sp in accepted_spellings(name)): return "broken", f"{path} no longer defines `{method}`" return "ok", None