From 605231b39f60d9151e54dc2d44fa9631126bd5e8 Mon Sep 17 00:00:00 2001 From: sudolulo Date: Mon, 13 Jul 2026 22:43:20 +0000 Subject: [PATCH 1/9] feat: TrueNAS 26 support; enumerate datasets and snapshots from ZFS, not middleware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TrueNAS 26 deletes plugins/zfs_/ outright, taking the private zfs.dataset.query, zfs.snapshot.query and zfs.snapshot.delete with it. All three were on the nested module's critical path, so nested snapshots were BROKEN on 26. Snapshot deletion now resolves its namespace at runtime: pool.snapshot on 25.10 and 26, zfs.snapshot on 24.10 and 25.04. No single namespace spans every supported release. tools/compat.py checks the same list the runtime uses, so what CI verifies and what runs cannot drift apart. Enumeration does NOT move to pool.dataset.query / pool.snapshot.query, and that is the point of this commit. Those methods exist, are documented, and are covered by iX's deprecation policy — and they are not like-for-like replacements. They apply a visibility policy that hides ix-apps/*, .system/* and .ix-virt/*: 84 of 270 datasets on a real pool, including live application data. Staging from that view omits them silently, and plan_staging never sees them, so they do not even reach the skipped list. The snapshot query hides the same datasets' snapshots, so the sweep orphans one per hidden dataset on every run. So: read the truth from ZFS, make changes through middleware. zfs list cannot be filtered by policy and behaves identically on every release. A failing zfs list raises rather than returning an empty list — "no datasets" and "the command broke" must never look the same. No shipped release is affected: v0.6.1 and earlier use the private zfs.dataset.query, which returns all 270 datasets. The bug existed only in this port. Verified on a real TrueNAS 26.0.0-BETA.1 install: 274-snapshot recursive backup of a 292-dataset pool, zero orphaned snapshots, zero leaked mounts, and a byte-identical restore of a four-level-deep child dataset that pool.dataset.query hides. --- CHANGELOG.md | 48 +++++++ README.md | 8 +- patch/apply.sh | 11 +- patch/truecloud_nested.py | 243 ++++++++++++++++++++++++++++++--- tests/test_apply_blocks.py | 18 ++- tests/test_compat.py | 126 +++++++++++------ tests/test_truecloud_nested.py | 163 +++++++++++++++++++--- tools/compat.py | 167 ++++++++++++++++------ 8 files changed, 664 insertions(+), 120 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1287d2..417903f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,54 @@ is deliberate: see [Releasing](docs/releasing.md). Twelve releases were cut on live, every one of those interrupts every user. An alert people learn to ignore is worse than no alert, because one day it carries a security fix. +## Unreleased +### Added + +- **TrueNAS 26 support, verified on a real TrueNAS 26 install.** 26 deletes + `plugins/zfs_/` outright, taking the private `zfs.dataset.query`, + `zfs.snapshot.query` and `zfs.snapshot.delete` with it. Every one of those was on + the nested module's critical path, so nested snapshots were **BROKEN** on 26 and + `apply.sh` correctly refused to apply the module there. + + Snapshot **deletion** now resolves its namespace at runtime — `pool.snapshot` on + 25.10 and 26, `zfs.snapshot` on 24.10 and 25.04, because no single namespace spans + every supported release. `tools/compat.py` checks the same list the runtime uses, + so what CI verifies and what runs cannot drift apart. + + Hardware-verified on TrueNAS 26.0.0-BETA.1: a 274-snapshot recursive backup of a + 292-dataset pool, then a **byte-identical restore of a four-level-deep child + dataset**. + +### Fixed + +- **Enumeration no longer trusts middleware's dataset and snapshot queries — they + are filtered.** This is the important one, and it is the bug that a test VM caught + and no amount of source analysis ever could have. + + The obvious port of the deleted private `zfs.dataset.query` was the public + `pool.dataset.query`. It exists, it is documented, it is covered by iX's + deprecation policy — and it is **not a like-for-like replacement**. It applies a + *visibility policy*: it hides the datasets TrueNAS considers its own — `ix-apps/*`, + `.system/*`, `.ix-virt/*`. On a real pool that is **84 of 270 datasets**, and + `ix-apps` holds **live application data**. + + Staging from that view would have silently omitted every one of them. Worse, + `plan_staging()` would never have seen them, so they would not have appeared in its + `skipped` list either — no warning, no failure, just a green backup quietly missing + data. That is precisely the failure this module exists to prevent. The snapshot + query lies the same way (205 of 274), so the sweep would have orphaned one snapshot + per hidden dataset, on every run, forever. + + The module now **reads the truth from ZFS and makes changes through middleware**: + enumeration is `zfs list`, which no policy can filter and which behaves identically + on every release; mutation stays a middleware call, so TrueNAS's own bookkeeping + stays consistent. A failing `zfs list` raises rather than returning an empty list — + "no datasets" and "the command broke" must never look the same. + + **No shipped release is affected.** v0.6.1 and earlier call the *private* + `zfs.dataset.query`, which returns all 270 datasets. The bug existed only in the + unreleased TrueNAS 26 port. + ## v0.6.1 — 2026-07-13 ### Fixed diff --git a/README.md b/README.md index 03e6934..a991ab9 100644 --- a/README.md +++ b/README.md @@ -60,7 +60,7 @@ If something is wrong, the reason is in `apply.log` — start at | 24.10.2.4 | ok | ok | — | | 25.04.2.6 | ok | ok | — | | 25.10.4 | ok | ok | nested + providers; 252-snapshot recursive backup of /mnt/Tap, 18m | -| 26.0.0-BETA.3 _(unreleased)_ | ok | **BROKEN** | — | +| 26.0.0-BETA.3 _(unreleased)_ | ok | ok | — | | master _(unreleased)_ | **BROKEN** | **BROKEN** | — | | verdict | meaning | @@ -77,6 +77,12 @@ source. It does not mean a human ran a backup on it — that is the The table is **regenerated daily by CI** against iXsystems' actual middleware source — it is not a claim somebody typed once and forgot. +**On TrueNAS 26:** the patch was run on a real TrueNAS **26.0.0-BETA.1** install — a +274-snapshot recursive backup of a 292-dataset pool, followed by a byte-identical +restore of a four-level-deep child dataset. The *Hardware-verified* column tracks the +newest beta iX has tagged (currently BETA.3), so it does not carry that mark: a build +nobody has actually run a backup on does not get credit for one. + **TrueNAS 26: nested snapshots are not supported yet, and upgrading will not break you.** 26 rewrites `cloud_backup` and deletes the ZFS methods this module calls. On 26 `apply.sh` finds that the assumptions no longer hold and **does not apply the diff --git a/patch/apply.sh b/patch/apply.sh index 2723ccd..d1774b5 100755 --- a/patch/apply.sh +++ b/patch/apply.sh @@ -535,7 +535,7 @@ 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 `zfs.dataset.query` that errors would break a + # were installed. A `pool.dataset.query` 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 @@ -556,9 +556,12 @@ if _tc_nested is not None: # our staging plan would not -- silently omitting it from the backup. # Read afterwards, an unsnapshotted dataset instead trips the isdir() # check in plan_staging and fails the run loudly. Loud beats silent. - datasets = middleware.call_sync( - "zfs.dataset.query", [["type", "=", "FILESYSTEM"]] - ) + # query_filesystems() reads ZFS directly. It deliberately does NOT use + # pool.dataset.query: that applies a visibility policy and hides + # TrueNAS-internal datasets (ix-apps/*, .system/*, .ix-virt/*) -- 84 of + # 270 on a real pool, including live app data. Staging from the filtered + # view omits them silently, which is the one thing this must never do. + datasets = _tc_nested.query_filesystems(middleware) # OUR copy of get_dataset_recursive, not the host module's: TrueNAS 26 # deleted that helper (create_snapshot uses filesystem.statfs now), so # calling it out of the module namespace is a NameError there. diff --git a/patch/truecloud_nested.py b/patch/truecloud_nested.py index 1d867ea..17c4c47 100644 --- a/patch/truecloud_nested.py +++ b/patch/truecloud_nested.py @@ -34,7 +34,7 @@ feature exists to prevent, and it would be worse than not having the feature. Snapshot lifecycle -- read this before changing anything -------------------------------------------------------- -``zfs.snapshot.delete`` defaults to ``recursive=False``, and stock +The snapshot delete call defaults to ``recursive=False``, and stock ``restic_backup()`` calls it with no options. Stock gets away with that because its validation means ``recursive`` is never actually True in the field. Enabling nested datasets makes recursive snapshots real, so the parent @@ -62,17 +62,24 @@ import subprocess import time __all__ = [ + "SNAPSHOT_SERVICES", "STAGING_BASE", "StagingError", + "ZfsError", "apply_plan", "cleanup_all", "cleanup_task", "current_mounts_under", "delete_snapshot_tree", "gc_stale_snapshots", + "list_snapshot_names", "mounted_snapshots", + "normalise_dataset", + "pick_snapshot_service", "plan_staging", + "query_filesystems", "sidecar_for", + "snapshot_service", "snapshot_tree_names", "stage_nested", "stale_snapshot_names", @@ -81,6 +88,204 @@ __all__ = [ "verify_staged", ] + +# ── how this module talks to the system ────────────────────────────────────── +# +# READ the truth from ZFS. MAKE CHANGES through middleware. +# +# That split is not stylistic. It was forced by finding, on a real TrueNAS 26 box, +# that middleware's query APIs apply a VISIBILITY POLICY: +# +# zfs list 274 datasets 205 from pool.dataset.query +# zfs list -t snapshot 274 snapshots 205 from pool.snapshot.query +# +# The missing 69 are the datasets TrueNAS considers its own -- `ix-apps/*`, +# `.system/*`, `.ix-virt/*` -- and on the real pool that is 84 of 270, including +# `ix-apps`, which holds live application data. Enumerating from that view would +# have silently omitted every one of them from the staging plan and from the +# snapshot sweep: a green backup missing data, and one orphaned snapshot per +# hidden dataset on every run. Both are exactly what this module exists to +# prevent. +# +# This went unnoticed because the patch used to call the PRIVATE `zfs.dataset.query` +# and `zfs.snapshot.query`, which return everything. TrueNAS 26 deleted them, and +# the public replacements are NOT like-for-like -- they are filtered. So +# enumeration now reads ZFS directly, which no policy can filter and which behaves +# identically on every release. +# +# MUTATION still goes through middleware, so TrueNAS's own bookkeeping stays +# consistent -- and an exact-name delete works fine even on a dataset the query +# hides. The one wrinkle is that no single snapshot namespace spans every +# supported release, so it is resolved at runtime rather than pinned: +# +# 24.10, 25.04 `zfs.snapshot` (public back then; `pool.snapshot` does not exist) +# 25.10 both -- `pool.snapshot` public, `zfs.snapshot` demoted to private +# 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. +SNAPSHOT_SERVICES = ("pool.snapshot", "zfs.snapshot") + + +def pick_snapshot_service(has_service): + """First namespace in SNAPSHOT_SERVICES that this middleware exposes. + + 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. + """ + for name in SNAPSHOT_SERVICES: + if has_service(name): + return name + return None + + +def _has_service(middleware, name): + try: + middleware.get_service(name) + 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. + return False + return True + + +def snapshot_service(middleware): + """The snapshot CRUD namespace this middleware actually has.""" + name = pick_snapshot_service(lambda n: _has_service(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 " + "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): + """`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. + """ + runner = runner or _run + r = runner(["zfs", *args]) + 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()] + + +def query_filesystems(middleware=None, runner=None): + """Every FILESYSTEM dataset, in the shape the planner speaks -- read from ZFS. + + NOT from `pool.dataset.query`, and this is the single most important decision + in this file. + + middleware's dataset query applies a VISIBILITY POLICY: it hides the datasets + TrueNAS considers its own -- `ix-apps/*`, `.system/*`, `.ix-virt/*`. That is + **84 of 270 datasets** on the real pool, and `ix-apps` holds live application + data. Building the staging plan from that view would silently omit every one of + them. Worse, `plan_staging()` would never even SEE them, so they would not turn + up in its `skipped` list either -- no warning, no failure, just a green backup + quietly missing data. That is exactly the failure this whole module exists to + prevent, and it is the failure the cardinal rule at the top of this file is + about. + + It worked before only because the patch called the PRIVATE `zfs.dataset.query`, + which returned everything. TrueNAS 26 deleted it. The public replacement is not + a like-for-like: it is a filtered view. + + So: **read the truth from ZFS, make changes through middleware.** ZFS cannot + apply a policy to what it reports, and `zfs list` behaves identically on every + release -- which also means one code path instead of a version conditional. + + `middleware` is accepted and ignored, so callers need not care where the data + comes from. + """ + rows = _zfs_lines( + ["list", "-H", "-p", "-o", "name,mountpoint,mounted", "-t", "filesystem"], + runner=runner, + ) + return [ + { + "name": name, + "properties": { + "mountpoint": {"value": mountpoint}, + # `zfs list` prints yes/no; the planner already speaks that. + "mounted": {"value": mounted}, + }, + } + for name, mountpoint, mounted in (r for r in rows if len(r) == 3) + ] + + +def list_snapshot_names(dataset, runner=None): + """Every snapshot at or under `dataset` -- read from ZFS, for the same reason. + + `pool.snapshot.query` filters exactly like the dataset query does: on this box + it returned 205 of 274 snapshots, hiding the internal datasets' snapshots. A + sweep built on that view leaves one orphan per hidden dataset, on every run, + forever -- which is the bug this module was written to fix in the first place. + + (An EXACT-name delete still works on a hidden dataset, so mutations may keep + going through middleware. It is only enumeration that lies.) + """ + rows = _zfs_lines( + ["list", "-H", "-o", "name", "-t", "snapshot", "-r", dataset], + runner=runner, + ) + return [r[0] for r in rows] + #: Where staging trees are assembled. tmpfs; bind mounts consume no space. STAGING_BASE = "/run/truecloud-nested" @@ -560,7 +765,7 @@ def get_dataset_recursive(datasets, directory): def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, - sleep=time.sleep): + sleep=time.sleep, list_snapshots=None): """Delete the parent snapshot AND every child created by ``zfs snapshot -r``. Returns the snapshots it could NOT delete -- callers must not throw that away. @@ -578,6 +783,8 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, snapshots hit this, and before the fix they were orphaned permanently. """ dataset, _, snapname = snapshot.partition("@") + svc = snapshot_service(middleware) + list_snapshots = list_snapshots or list_snapshot_names # Release ZFS's own automounts first, or `zfs destroy` refuses with EBUSY on # everything restic read in the last few minutes. @@ -591,7 +798,7 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, # through 252 sequential deletes leaves exactly the orphans this function # exists to prevent. try: - middleware.call_sync("zfs.snapshot.delete", snapshot, {"recursive": True}) + 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 @@ -608,13 +815,14 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, # our mounts are released -- which fails the recursive delete while the # children survive. Sweep them by name. try: - snaps = middleware.call_sync( - "zfs.snapshot.query", [["name", "^", dataset]], {"select": ["name"]} - ) + # From ZFS, not middleware: the snapshot query hides internal datasets' + # snapshots (205 of 274 on the test box), and a sweep that cannot see them + # orphans one per hidden dataset on every run. See list_snapshot_names(). + # # An empty result means the tree is already gone -- delete nothing, and # do not fall back to the parent, which would only log a spurious # "does not exist" warning on every clean run. - names = snapshot_tree_names(snapshot, [s["name"] for s in snaps]) + names = snapshot_tree_names(snapshot, list_snapshots(dataset)) except Exception as e: # noqa: BLE001 - fall back to at least the parent if logger: logger.warning( @@ -635,12 +843,9 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, if not failed: return [] try: - live = middleware.call_sync( - "zfs.snapshot.query", [["name", "^", dataset]], {"select": ["name"]} - ) + live = set(list_snapshots(dataset)) except Exception: # noqa: BLE001 - cannot refine; trust the delete's verdict return list(failed) - live = {s["name"] for s in live} return [n for n in failed if n in live] remaining = list(names) @@ -648,7 +853,7 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, failed = [] for name in remaining: try: - middleware.call_sync("zfs.snapshot.delete", name) + middleware.call_sync(f"{svc}.delete", name) except Exception: # noqa: BLE001 - busy, or already gone; sorted out below failed.append(name) @@ -694,7 +899,7 @@ def mounted_snapshots(mounts_file="/proc/self/mounts"): def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, - now=None, mounts_file="/proc/self/mounts"): + now=None, mounts_file="/proc/self/mounts", list_snapshots=None): """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 @@ -707,11 +912,13 @@ def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, """ dataset = current_snapshot.partition("@")[0] now = now or datetime.datetime.now(datetime.UTC) + svc = snapshot_service(middleware) + list_snapshots = list_snapshots or list_snapshot_names try: - snaps = middleware.call_sync( - "zfs.snapshot.query", [["name", "^", dataset]], {"select": ["name"]} - ) + # From ZFS: middleware's snapshot query hides internal datasets, and an + # orphan it cannot see is an orphan nothing will ever collect. + all_names = list_snapshots(dataset) except Exception as e: # noqa: BLE001 - cannot enumerate; collect nothing if logger: logger.warning( @@ -720,7 +927,7 @@ def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, return [] stale = stale_snapshot_names( - task_name, current_snapshot, [s["name"] for s in snaps], now, + task_name, current_snapshot, all_names, now, in_use=mounted_snapshots(mounts_file), ) if not stale: @@ -736,7 +943,7 @@ def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, remaining = [] for name in stale: try: - middleware.call_sync("zfs.snapshot.delete", name) + middleware.call_sync(f"{svc}.delete", name) except Exception as e: # noqa: BLE001 - busy, or gone; either way, next run remaining.append(name) if logger: diff --git a/tests/test_apply_blocks.py b/tests/test_apply_blocks.py index 44f716b..d2ec7e0 100644 --- a/tests/test_apply_blocks.py +++ b/tests/test_apply_blocks.py @@ -438,8 +438,22 @@ class TestOnlyOurOwnTasksAreTouched: # The point is to add NO new failure mode to a CloudSync task. If any # middleware call happened before the bail-out, we would already have broken # the thing we are trying not to touch. + # + # Checked against whichever interactions the block ACTUALLY contains, not a + # fixed list: the dataset query moved behind `_tc_nested.query_filesystems()` + # when it switched to the public pool.* API, and a hardcoded + # `middleware.call_sync(` simply stopped being found -- a test that silently + # stops testing is worse than no test. block = extract_blocks()[name] gate = block.index('if not name.startswith("cloud_backup"):') - for call in ("middleware.call_sync(", "_tc_nested.stage_nested(", - "_tc_nested.delete_snapshot_tree("): + + interactions = [ + "middleware.call_sync(", + "_tc_nested.query_filesystems(", + "_tc_nested.stage_nested(", + "_tc_nested.delete_snapshot_tree(", + ] + present = [c for c in interactions if c in block] + assert present, "found no middleware interaction at all -- the test is vacuous" + for call in present: assert gate < block.index(call), f"{call} runs before the cloud_backup gate" diff --git a/tests/test_compat.py b/tests/test_compat.py index 4d405a5..f6ca055 100644 --- a/tests/test_compat.py +++ b/tests/test_compat.py @@ -51,24 +51,37 @@ GOOD = { "async def restic_backup(middleware, job, cloud_backup, dry_run=False, " "rate_limit=None):\n pass\n" ), - # The middlewared METHODS the injected code calls. TrueNAS 26 deleted both of - # these files, taking zfs.dataset.query / zfs.snapshot.query / zfs.snapshot.delete - # with them -- see TestMiddlewareMethodsWeCall. - "plugins/zfs_/dataset.py": ( - "class ZFSDataset(CRUDService):\n" + # The middlewared METHODS the injected code calls. These now go through the + # PUBLIC pool.* API: TrueNAS 26 deleted plugins/zfs_/ outright, taking the whole + # private zfs.* service with it -- see TestMiddlewareMethodsWeCall. + # + # 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. + "plugins/pool_/dataset.py": ( + "class PoolDatasetService(CRUDService):\n" " class Config:\n" - " namespace = 'zfs.dataset'\n" + " namespace = 'pool.dataset'\n" " def query(self, filters, options):\n pass\n" ), - "plugins/zfs_/snapshot.py": ( - "class ZFSSnapshot(CRUDService):\n" + "plugins/pool_/snapshot.py": ( + "class PoolSnapshotService(CRUDService):\n" " class Config:\n" - " namespace = 'zfs.snapshot'\n" + " namespace = 'pool.snapshot'\n" " def query(self, filters, options):\n pass\n" " def delete(self, id_, options={}):\n pass\n" ), } +#: A 24.10/25.04 box: `pool.snapshot` does not exist yet and the snapshot CRUD +#: service still answers to the (then-public) `zfs.snapshot`. +ZFS_ERA_SNAPSHOT = ( + "class ZFSSnapshot(CRUDService):\n" + " class Config:\n" + " namespace = 'zfs.snapshot'\n" + " def query(self, filters, options):\n pass\n" + " def do_delete(self, id_, options={}):\n pass\n" +) + def loader(files): def load(path): @@ -303,52 +316,86 @@ class TestMiddlewareMethodsWeCall: per descendant dataset (250 on a real pool) on every run, forever. """ - ZFS_SNAPSHOT = ( - "class ZFSSnapshot(CRUDService):\n" - " class Config:\n" - " namespace = 'zfs.snapshot'\n" - " def query(self, filters, options):\n pass\n" - " def delete(self, id_, options={}):\n pass\n" - ) - ZFS_DATASET = ( - "class ZFSDataset(CRUDService):\n" - " class Config:\n" - " namespace = 'zfs.dataset'\n" - " def query(self, filters, options):\n pass\n" - ) + POOL_SNAPSHOT = GOOD["plugins/pool_/snapshot.py"] def _tree(self, **over): files = dict(GOOD) - files["plugins/zfs_/snapshot.py"] = self.ZFS_SNAPSHOT - files["plugins/zfs_/dataset.py"] = self.ZFS_DATASET files.update(over) return files + #: A 26 box: pool.snapshot only. + def _modern(self, **over): + return self._tree(**over) + + #: A 24.10/25.04 box: zfs.snapshot only -- plugins/pool_/snapshot.py does not + #: exist yet. + def _zfs_era(self, **over): + return self._tree(**{ + "plugins/pool_/snapshot.py": None, + "plugins/zfs_/snapshot.py": ZFS_ERA_SNAPSHOT, + **over, + }) + def test_present_methods_are_ok(self): - r = check_files(self._tree()) + r = check_files(self._modern()) assert r[NESTED]["ok"], r[NESTED]["problems"] - def test_a_deleted_plugin_file_is_broken(self): - # Literally TrueNAS 26: plugins/zfs_/snapshot.py does not exist. - r = check_files(self._tree(**{"plugins/zfs_/snapshot.py": None})) + def test_the_OLD_zfs_era_snapshot_service_also_satisfies_the_call(self): + # 24.10 and 25.04 have no `pool.snapshot` at all -- the CRUD service is the + # then-public `zfs.snapshot`. Pinning only the modern spelling marked both of + # those releases BROKEN and would have switched nested snapshots OFF on boxes + # where they work perfectly. The runtime picks the same way; see + # pick_snapshot_service(). + r = check_files(self._zfs_era()) + assert r[NESTED]["ok"], r[NESTED]["problems"] + + def test_it_is_broken_only_when_NEITHER_namespace_exists(self): + # The real failure: middleware drops the last spelling we know how to call. + r = check_files(self._tree(**{ + "plugins/pool_/snapshot.py": None, + "plugins/zfs_/snapshot.py": None, + })) assert is_broken(r[NESTED]) details = " ".join(p["detail"] for p in r[NESTED]["problems"]) - assert "zfs.snapshot.delete" in details + assert "pool.snapshot.delete" in details + assert "zfs.snapshot.delete" in details, ( + "the report must say BOTH spellings were tried, or whoever reads it will " + "think we simply never looked for the one their box has" + ) + + def test_we_do_NOT_depend_on_a_middleware_dataset_query_at_all(self): + # iX could delete plugins/pool_/dataset.py tomorrow and the patch would not + # care, because the staging plan is enumerated from ZFS, not from middleware. + # + # That is deliberate, and it was expensive to learn. `pool.dataset.query` + # exists and is correctly shaped -- and it LIES: it applies a visibility + # policy that hides ix-apps/*, .system/* and .ix-virt/* (84 of 270 datasets + # on the real pool, including live app data). No source check could ever + # have caught that; only running it could. So there is no assumption here + # left to break. + r = check_files(self._modern(**{"plugins/pool_/dataset.py": None})) + assert r[NESTED]["ok"], r[NESTED]["problems"] + + ids = {c.id for c in compat.MIDDLEWARE_CALLS} + assert not any("dataset" in i or "query" in i for i in ids), ( + "a dataset/snapshot QUERY assumption crept back into the manifest -- " + "middleware's queries are filtered; enumerate from ZFS" + ) def test_a_renamed_namespace_is_broken(self): r = check_files(self._tree(**{ - "plugins/zfs_/snapshot.py": self.ZFS_SNAPSHOT.replace( - "'zfs.snapshot'", "'zfs.resource.snapshot'"), + "plugins/pool_/snapshot.py": self.POOL_SNAPSHOT.replace( + "'pool.snapshot'", "'zfs.resource.snapshot'"), + "plugins/zfs_/snapshot.py": None, })) assert is_broken(r[NESTED]) def test_the_CRUDService_do_prefix_is_accepted(self): - # 24.10 and 25.04 declare `do_delete`; 25.10 renamed it to `delete`. BOTH - # answer to zfs.snapshot.delete. Accepting only the literal name reported the - # two older releases as broken -- a false BROKEN that would have switched off - # nested snapshots on boxes where they work perfectly. - r = check_files(self._tree(**{ - "plugins/zfs_/snapshot.py": self.ZFS_SNAPSHOT.replace( + # A CRUDService exposes `delete` from a method NAMED `do_delete`. Both + # spellings are live across the matrix. Accepting only the literal name + # reported working releases as broken. + r = check_files(self._modern(**{ + "plugins/pool_/snapshot.py": self.POOL_SNAPSHOT.replace( "def delete(", "def do_delete("), })) assert r[NESTED]["ok"], r[NESTED]["problems"] @@ -356,6 +403,9 @@ class TestMiddlewareMethodsWeCall: def test_the_snapshot_delete_reason_names_the_orphan_risk(self): # If this ever regresses, whoever reads the bug report must understand that # it is not a cosmetic failure. - r = check_files(self._tree(**{"plugins/zfs_/snapshot.py": None})) + r = check_files(self._tree(**{ + "plugins/pool_/snapshot.py": None, + "plugins/zfs_/snapshot.py": None, + })) whys = " ".join(p["why"] for p in r[NESTED]["problems"]) assert "orphan" in whys diff --git a/tests/test_truecloud_nested.py b/tests/test_truecloud_nested.py index 585f9ff..cf79f5b 100644 --- a/tests/test_truecloud_nested.py +++ b/tests/test_truecloud_nested.py @@ -204,18 +204,52 @@ class FakeMiddleware: truecloud_nested.py. TrueNAS <= 25.10 reaches it through `await middleware.run_in_thread(...)` and TrueNAS 26 calls it directly, but the logic below the boundary is the same code either way, so it is tested once. + + `snapshot_ns` picks which middleware GENERATION this is, because they do not + agree on what the snapshot service is called: + + pool.snapshot 25.10 and 26 (26 has ONLY this -- plugins/zfs_/ is gone) + zfs.snapshot 24.10 and 25.04 (pool.snapshot does not exist yet) + + A method in the namespace this box does NOT have raises, exactly as middleware + does ("Method does not exist"). That is what makes the runtime picker testable: + a module that guessed wrong would blow up here instead of silently orphaning + snapshots on somebody's NAS. """ - def __init__(self, snapshots=None): + def __init__(self, snapshots=None, snapshot_ns="pool.snapshot"): self.snapshots = list(snapshots or []) self.calls = [] self.logger = None + self.snapshot_ns = snapshot_ns + + def get_service(self, name): + if name != self.snapshot_ns: + raise KeyError(name) # middleware raises KeyError for an unknown ns + return object() + + def list_snapshots(self, dataset): + """Stands in for `zfs list -t snapshot -r `. + + Enumeration comes from ZFS now, NOT from middleware -- middleware's query + hides internal datasets, and a sweep that cannot see a snapshot can never + collect it. Passed in as `list_snapshots=` so the seam is explicit. + """ + return [ + n for n in self.snapshots + if n.split("@")[0] == dataset or n.startswith(dataset + "/") + ] def call_sync(self, method, *args): self.calls.append((method, args)) - if method == "zfs.snapshot.query": + namespace, _, op = method.rpartition(".") + + if namespace != self.snapshot_ns: + raise RuntimeError("Method does not exist") + + if op == "query": return [{"name": n} for n in self.snapshots] - if method == "zfs.snapshot.delete": + if op == "delete": name = args[0] opts = args[1] if len(args) > 1 else {} if name not in self.snapshots: @@ -265,44 +299,44 @@ class TestDeleteSnapshotTree: mw = FakeMiddleware([ "Tap@snap", "Tap/apps@snap", "Tap/apps/lidarr@snap", "Tap@keepme", ]) - delete_snapshot_tree(mw, "Tap@snap") + delete_snapshot_tree(mw, "Tap@snap", list_snapshots=mw.list_snapshots) assert mw.snapshots == ["Tap@keepme"] def test_is_idempotent_when_stock_already_removed_the_parent(self): # Stock's finally can win the race once our mounts are released. mw = FakeMiddleware(["Tap/apps@snap", "Tap/apps/lidarr@snap"]) - delete_snapshot_tree(mw, "Tap@snap") + delete_snapshot_tree(mw, "Tap@snap", list_snapshots=mw.list_snapshots) assert mw.snapshots == [] def test_uses_a_single_recursive_delete_not_252_individual_ones(self): # 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") + delete_snapshot_tree(mw, "Tap@snap", list_snapshots=mw.list_snapshots) assert mw.snapshots == [] - deletes = [a for m, a in mw.calls if m == "zfs.snapshot.delete"] + 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 == "zfs.snapshot.query"], ( + assert not [m for m, _a in mw.calls if m.endswith(".query")], ( "no enumeration needed on the fast path" ) def test_survives_recursive_and_query_failure_by_deleting_the_parent(self): class Broken(FakeMiddleware): def call_sync(self, method, *args): - if method == "zfs.snapshot.query": + if method.endswith(".query"): raise RuntimeError("boom") - if method == "zfs.snapshot.delete" and len(args) > 1: + 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") + delete_snapshot_tree(mw, "Tap@snap", list_snapshots=mw.list_snapshots) assert mw.snapshots == [] def test_leaves_unrelated_snapshots_alone_when_the_tree_is_gone(self): mw = FakeMiddleware(["Tap@unrelated"]) - delete_snapshot_tree(mw, "Tap@snap") + delete_snapshot_tree(mw, "Tap@snap", list_snapshots=mw.list_snapshots) assert mw.snapshots == ["Tap@unrelated"] @@ -692,7 +726,7 @@ class BusyMiddleware(FakeMiddleware): self.attempts = 0 def call_sync(self, method, *args): - if method == "zfs.snapshot.delete": + if method.endswith(".delete"): name = args[0] opts = args[1] if len(args) > 1 else {} if opts.get("recursive"): @@ -713,7 +747,7 @@ class TestDeleteRetriesAndReportsSurvivors: ["Tap@snap", "Tap/apps@snap", "Tap/apps/prometheus@snap"], busy=["Tap/apps/prometheus@snap"], busy_for=1, ) - survivors = tn.delete_snapshot_tree(mw, "Tap@snap", sleep=lambda _s: None) + survivors = tn.delete_snapshot_tree(mw, "Tap@snap", sleep=lambda _s: None, list_snapshots=mw.list_snapshots) assert survivors == [] assert mw.snapshots == [] @@ -725,7 +759,7 @@ class TestDeleteRetriesAndReportsSurvivors: ["Tap@snap", "Tap/apps/prometheus@snap"], busy=["Tap/apps/prometheus@snap"], busy_for=99, ) - survivors = tn.delete_snapshot_tree(mw, "Tap@snap", sleep=lambda _s: None) + survivors = tn.delete_snapshot_tree(mw, "Tap@snap", sleep=lambda _s: None, list_snapshots=mw.list_snapshots) assert survivors == ["Tap/apps/prometheus@snap"] assert mw.snapshots == ["Tap/apps/prometheus@snap"] @@ -742,7 +776,7 @@ class TestDeleteRetriesAndReportsSurvivors: return real(method, *args) mw.call_sync = spy - tn.delete_snapshot_tree(mw, "Tap@snap", sleep=lambda _s: None) + tn.delete_snapshot_tree(mw, "Tap@snap", sleep=lambda _s: None, list_snapshots=mw.list_snapshots) assert order[0] == ("release", "snap"), order @@ -1002,6 +1036,7 @@ class TestGarbageCollectorExecution: remaining = tn.gc_stale_snapshots( mw, "cloud_backup-5", "Tap@cloud_backup-5-20260714115900", now=now, mounts_file=str(mounts), + list_snapshots=mw.list_snapshots, ) assert remaining == [] assert mw.snapshots == [ @@ -1026,6 +1061,7 @@ class TestGarbageCollectorExecution: remaining = tn.gc_stale_snapshots( mw, "cloud_backup-5", "Tap@cloud_backup-5-20260714115900", now=now, mounts_file=str(mounts), + list_snapshots=mw.list_snapshots, ) assert remaining == [orphan] @@ -1039,7 +1075,7 @@ class TestGarbageCollectorExecution: class Broken(FakeMiddleware): def call_sync(self, method, *args): - if method == "zfs.snapshot.query": + if method.endswith(".query"): raise RuntimeError("middleware is having a day") return super().call_sync(method, *args) @@ -1049,3 +1085,96 @@ class TestGarbageCollectorExecution: now=dt.datetime(2026, 7, 14, 12, 0, 0, tzinfo=dt.UTC), mounts_file=str(mounts), ) == [] + + +class TestEnumerationComesFromZfsNotMiddleware: + """The bug that a source check can never catch, found only by running it. + + Porting the deleted private `zfs.dataset.query` to the public + `pool.dataset.query` looked obviously right: the method exists, it is + documented, iX will not delete it. Every test passed and `compat.py` went green + on TrueNAS 26. + + It was wrong. The public query applies a VISIBILITY POLICY -- on a real box it + returns 205 of 274 datasets, hiding `ix-apps/*`, `.system/*` and `.ix-virt/*`. + On the production pool that is 84 of 270, and `ix-apps` holds LIVE APPLICATION + DATA. The staging plan would have omitted every one of them, and `plan_staging` + would never have seen them, so they would not even appear in `skipped`. A green + backup, silently missing data -- the precise failure this module exists to + prevent. + + The snapshot query lies the same way, so the sweep would orphan one snapshot per + hidden dataset, forever. + + Hence: READ from ZFS, MUTATE through middleware. These tests hold that line. + """ + + def test_query_filesystems_shells_out_to_zfs(self): + import truecloud_nested as tn + + seen = [] + + class R: + returncode = 0 + stdout = ( + "scratch\t/mnt/scratch\tyes\n" + "scratch/ix-apps\t/mnt/scratch/ix-apps\tyes\n" # middleware HIDES this one + "scratch/.system\tlegacy\tno\n" + ) + stderr = "" + + def runner(cmd): + seen.append(cmd) + return R() + + rows = tn.query_filesystems(runner=runner) + + assert seen and seen[0][0] == "zfs", "must read ZFS, not call middleware" + names = [r["name"] for r in rows] + assert "scratch/ix-apps" in names, ( + "ix-apps is exactly what pool.dataset.query hides, and exactly what " + "holds live app data. If it is not here the backup omits it silently." + ) + # ...and it still speaks the shape the planner expects. + row = next(r for r in rows if r["name"] == "scratch/.system") + assert row["properties"]["mountpoint"]["value"] == "legacy" + assert row["properties"]["mounted"]["value"] == "no" + + def test_a_failing_zfs_raises_rather_than_returning_an_empty_list(self): + # The whole failure class in one assertion. "No datasets" and "the command + # broke" must never look the same: a caller that cannot tell them apart + # stages nothing, sweeps nothing, and reports success. + import truecloud_nested as tn + + class R: + returncode = 1 + stdout = "" + stderr = "cannot open 'scratch': no such pool" + + with pytest.raises(tn.ZfsError, match="no such pool"): + tn.query_filesystems(runner=lambda cmd: R()) + + with pytest.raises(tn.ZfsError): + tn.list_snapshot_names("scratch", runner=lambda cmd: R()) + + def test_list_snapshot_names_reads_the_whole_tree_from_zfs(self): + import truecloud_nested as tn + + seen = [] + + class R: + returncode = 0 + stdout = "scratch@s\nscratch/ix-apps@s\n" + stderr = "" + + def runner(cmd): + seen.append(cmd) + return R() + + names = tn.list_snapshot_names("scratch", runner=runner) + assert names == ["scratch@s", "scratch/ix-apps@s"] + assert "-t" in seen[0] and "snapshot" in seen[0] and "-r" in seen[0] + assert "scratch/ix-apps@s" in names, ( + "pool.snapshot.query hides this; a sweep that cannot see it orphans it " + "on every single run, forever" + ) diff --git a/tools/compat.py b/tools/compat.py index f190c0b..48aa692 100644 --- a/tools/compat.py +++ b/tools/compat.py @@ -162,54 +162,119 @@ class MiddlewareCall: the whole design: declining is always the cheaper mistake. """ - def __init__(self, ident, module, method, path, why=""): + def __init__(self, ident, module, method, path, why="", also=()): self.id = ident self.module = module - self.method = method # "zfs.snapshot.delete" + self.method = method # "pool.snapshot.delete" self.path = path # plugin file that declares it self.why = why + #: Equally acceptable spellings of the SAME call, as (method, path) pairs. + #: + #: No single snapshot namespace spans every supported release. 24.10 and + #: 25.04 expose the CRUD service as the public `zfs.snapshot`; 25.10 + #: promoted it to `pool.snapshot` and demoted `zfs.snapshot` to private; + #: 26 deleted `plugins/zfs_/` entirely. Pinning either one alone marks + #: half the matrix BROKEN and declines to apply on versions that work + #: perfectly well. + #: + #: The call is satisfied if ANY option is present. The runtime picks the + #: same way -- see `pick_snapshot_service()` in the nested module -- so + #: what this checks and what the patch does cannot drift apart. + self.also = tuple(also) + + @property + def options(self): + """Every (method, path) that would satisfy this call, best first.""" + return ((self.method, self.path), *self.also) + + @staticmethod + def namespace_of(method): + return method.rsplit(".", 1)[0] + + @staticmethod + def name_of(method): + return method.rsplit(".", 1)[1] @property def namespace(self): - return self.method.rsplit(".", 1)[0] + return self.namespace_of(self.method) @property def name(self): - return self.method.rsplit(".", 1)[1] + return self.name_of(self.method) #: Every middlewared method the nested module calls at runtime. +#: The middleware methods the nested module CALLS. +#: +#: These used to be the PRIVATE `zfs.*` service (`zfs.dataset.query`, +#: `zfs.snapshot.delete`, `zfs.snapshot.query`). TrueNAS 26 deleted +#: `plugins/zfs_/` outright and every one of them vanished -- silently, because a +#: private service carries no stability contract and nothing warned us. The patch +#: would have applied cleanly and then failed on the first backup. +#: +#: The replacements are the PUBLIC `pool.*` API, and switching to it is not merely +#: a TrueNAS 26 fix -- it is the correct call on every version: +#: +#: * It is public, documented, and covered by iX's deprecation policy, so it +#: 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. MIDDLEWARE_CALLS = [ MiddlewareCall( - "call-zfs-dataset-query", NESTED, "zfs.dataset.query", - "plugins/zfs_/dataset.py", - why="SNAPSHOT_BLOCK enumerates FILESYSTEM datasets to build the staging plan", - ), - MiddlewareCall( - "call-zfs-snapshot-delete", NESTED, "zfs.snapshot.delete", - "plugins/zfs_/snapshot.py", + "call-snapshot-delete", NESTED, "pool.snapshot.delete", + "plugins/pool_/snapshot.py", + also=[("zfs.snapshot.delete", "plugins/zfs_/snapshot.py")], why="delete_snapshot_tree() sweeps the recursive snapshot. Without it every " "run orphans one snapshot per descendant dataset (250 on a real pool)", ), - MiddlewareCall( - "call-zfs-snapshot-query", NESTED, "zfs.snapshot.query", - "plugins/zfs_/snapshot.py", - why="delete_snapshot_tree()'s fallback sweep enumerates the tree by name", - ), ] +# There is deliberately NO entry here for a dataset or snapshot QUERY. +# +# The patch used to call `zfs.dataset.query` / `zfs.snapshot.query` (private, and +# deleted in TrueNAS 26). The obvious port was to the public `pool.dataset.query` / +# `pool.snapshot.query` -- and that port was WRONG in a way no source check could +# ever have caught, because the methods are all present and correctly shaped. +# +# They are simply filtered. On a real box they return 205 of 274 datasets and 205 +# of 274 snapshots, hiding `ix-apps/*`, `.system/*` and `.ix-virt/*` -- 84 of 270 +# on the production pool, including live application data. Staging from that view +# silently omits them; sweeping from it orphans one snapshot per hidden dataset, +# forever. +# +# So the module enumerates from ZFS itself and there is no middleware assumption +# left to check. That is the point: the fewer things we assume about middleware, +# the less there is for iX to break. Only the MUTATION is still a middleware call, +# and that is the one entry above. + + +def check_call(c: MiddlewareCall, src: str | None, + method: str | None = None, path: str | None = None, + ) -> tuple[str, str | None]: + """Is `method` still registered by middlewared? + + `method`/`path` name WHICH spelling of the call is being tried -- a call may + have several equally acceptable ones (see MiddlewareCall.also). They default + to the preferred spelling. + """ + method = method or c.method + path = path or c.path + namespace = MiddlewareCall.namespace_of(method) + name = MiddlewareCall.name_of(method) -def check_call(c: MiddlewareCall, src: str | None) -> tuple[str, str | None]: - """Is `c.method` still registered by middlewared?""" if src is None: return "broken", ( - f"{c.path} no longer exists, so `{c.method}` is gone" + f"{path} no longer exists, so `{method}` is gone" ) try: tree = ast.parse(_stock(src)) except SyntaxError as e: - return "unknown", f"{c.path} does not parse: {e}" + return "unknown", f"{path} does not parse: {e}" # namespace = 'zfs.snapshot' on some Service class in this file... namespaces = { @@ -220,10 +285,10 @@ def check_call(c: MiddlewareCall, src: str | None) -> tuple[str, str | None]: and isinstance(n.value.value, str) and any(isinstance(t, ast.Name) and t.id == "namespace" for t in n.targets) } - if c.namespace not in namespaces: + if namespace not in namespaces: return "broken", ( - f"{c.path} no longer declares namespace {c.namespace!r} " - f"(found: {sorted(namespaces) or 'none'}), so `{c.method}` is gone" + f"{path} no longer declares namespace {namespace!r} " + f"(found: {sorted(namespaces) or 'none'}), so `{method}` is gone" ) # ...and it defines the method. @@ -238,8 +303,8 @@ def check_call(c: MiddlewareCall, src: str | None) -> tuple[str, str | None]: n.name for n in ast.walk(tree) if isinstance(n, ast.FunctionDef | ast.AsyncFunctionDef) } - if c.name not in defined and f"do_{c.name}" not in defined: - return "broken", f"{c.path} no longer defines `{c.method}`" + if name not in defined and f"do_{name}" not in defined: + return "broken", f"{path} no longer defines `{method}`" return "ok", None @@ -555,28 +620,46 @@ def check(loader, modules=None) -> dict: }) # The methods the injected code CALLS, not just the symbols it wraps. + # + # A call may have several equally acceptable spellings, because no single + # snapshot namespace spans every supported release (24.10 has `zfs.snapshot`, + # 26 has only `pool.snapshot`). It is satisfied if ANY of them is present -- + # exactly as the runtime resolves it -- and BROKEN only when they all vanish. for c in MIDDLEWARE_CALLS: if c.module not in out: continue - try: - text = src(c.path) - except Unreadable as e: - out[c.module]["unknown"] = True - out[c.module]["problems"].append({ - "id": c.id, "detail": f"could not read {c.path}: {e}", "why": c.why, - }) + + satisfied, unknown, details = False, False, [] + for method, path in c.options: + try: + text = src(path) + except Unreadable as e: + unknown = True + details.append(f"could not read {path}: {e}") + continue + + status, detail = check_call(c, text, method, path) + if status == "ok": + satisfied = True + break + if status == "unknown": + unknown = True + details.append(detail) + + if satisfied: continue - status, detail = check_call(c, text) - if status == "broken": - out[c.module]["ok"] = False - out[c.module]["problems"].append({ - "id": c.id, "detail": detail, "why": c.why, - }) - elif status == "unknown": + # Every spelling failed. If we could not READ one of them we do not know + # that it is broken -- a rate-limited fetch is not a regression. + if unknown: out[c.module]["unknown"] = True out[c.module]["problems"].append({ - "id": c.id, "detail": detail, "why": c.why, + "id": c.id, "detail": "; ".join(details), "why": c.why, + }) + else: + out[c.module]["ok"] = False + out[c.module]["problems"].append({ + "id": c.id, "detail": "; ".join(details), "why": c.why, }) for module, (path, phrase, native_when_present) in NATIVE_PROBES.items(): @@ -804,6 +887,10 @@ def is_broken(r: dict) -> bool: #: a strictly weaker claim than "a restore worked". Add a row only after doing it. HARDWARE_VERIFIED = { "25.10.4": "nested + providers; 252-snapshot recursive backup of /mnt/Tap, 18m", + "26.0.0-BETA.1": ( + "nested + providers; 274-snapshot recursive backup of a 292-dataset pool, " + "restored a 4-deep child dataset byte-identical" + ), } _LEGEND = """ From 413cd60ed412a79ff2579a5009d93db93846412b Mon Sep 17 00:00:00 2001 From: sudolulo Date: Mon, 13 Jul 2026 23:30:48 +0000 Subject: [PATCH 2/9] fix: own the snapshot sweep unconditionally; align the runtime and the manifest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four audits of the TrueNAS 26 branch. The findings, in severity order. 1. TrueNAS 26 orphaned a snapshot on every run, with no backstop. Stock decides `recursive` by its own rule, and on 26 that rule is no longer ours. <= 25.10 its create_snapshot called get_dataset_recursive() — the same function this module vendors — so "stock went recursive" and "we have something to stage" were the same question. 26 uses filesystem.statfs: recursive = (path == the dataset's mountpoint). A dataset whose only descendants are ZVOLs or legacy/none-mountpoint datasets now gets a RECURSIVE snapshot while the patch sees nothing to stage. The patch then handed the snapshot back to stock, which destroys the parent only. No staging tree meant no sidecar, and the GC only ever ran from stage_nested — so nothing on the box would ever have found the children. Reproduced on the VM: one orphan per zvol, every run, forever, backup green. Ownership of the sweep is no longer conditional on staging (own_snapshot()). 2. The runtime resolved a NAMESPACE; compat.py verified a METHOD. get_service() only proves a namespace is registered. compat checks the namespace AND that it defines delete/do_delete. So if iX guts the method but keeps the service — which they have already done to pool.snapshot.do_update on master — compat falls through to zfs.snapshot and reports the box healthy, while the runtime picks pool.snapshot and fails every delete. Both sides now ask "can this namespace delete?", and a test binds the two lists together. 3. query_filesystems() silently dropped malformed rows — the one remaining silent-omission path, and a direct contradiction of the cardinal rule. It raises now. A missing `zfs` binary raised FileNotFoundError rather than ZfsError; also fixed. 4. The retry loop discarded the delete error and reported every survivor as "(still busy?)" — naming the one cause that is benign and hiding the ones that are permanent. It keeps and reports the real error. Also: the staging-failure handler could lose the original exception if its own sweep raised; get_service is now a checked assumption; normalise_dataset and two dead MiddlewareCall properties removed; stale comments corrected. Tests: five of them were shelling out to the REAL pool (`zfs list -r Tap`, 2148 snapshots) and passed here only because this box has no zfs binary — they would have gone red on the NAS, which is the one machine the release process requires them green on. An autouse fixture now makes that impossible. Mutation-tested: reverting any of the five fixes above now fails the suite; before, all 293 passed. Verified on TrueNAS 26.0.0-BETA.1 (zvol leak reproduced, then closed; 292-dataset backup, 0 orphans, byte-identical restore of a 4-deep hidden dataset) and on 25.10.4 (pool.snapshot.delete honours recursive=True). --- patch/apply.sh | 31 ++- patch/truecloud_nested.py | 259 ++++++++++++++--------- tests/test_compat.py | 7 + tests/test_truecloud_nested.py | 373 +++++++++++++++++++++++++++++---- tools/compat.py | 49 +++-- 5 files changed, 566 insertions(+), 153 deletions(-) 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 From 928d0d197378eca0a49710d589f8d7ec2555c94d Mon Sep 17 00:00:00 2001 From: sudolulo Date: Mon, 13 Jul 2026 23:33:30 +0000 Subject: [PATCH 3/9] =?UTF-8?q?refactor:=20one=20seam=20for=20snapshots=20?= =?UTF-8?q?=E2=80=94=20read=20from=20ZFS,=20mutate=20through=20middleware?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `middleware` and the ZFS reader were being threaded through five functions as a pair, and the snapshot namespace was re-resolved in each of them. That is one collaborator, not two. _Snapshots owns both, resolves the namespace lazily (so the read-only paths do not raise over a mutation they never make), and makes the rule the module rests on structural instead of a comment people have to remember. Deliberately internal: apply.sh injects calls to the public functions into middlewared itself, so their signatures are a boot-time contract with a live NAS and are not worth churning for tidiness. Verified on TrueNAS 26.0.0-BETA.1 after the change: 292-dataset backup, 0 orphans, 0 leaked mounts, byte-identical restore of a 4-deep child dataset. --- patch/truecloud_nested.py | 67 +++++++++++++++++++++++++++++++++------ 1 file changed, 57 insertions(+), 10 deletions(-) diff --git a/patch/truecloud_nested.py b/patch/truecloud_nested.py index f5ef6b7..652b444 100644 --- a/patch/truecloud_nested.py +++ b/patch/truecloud_nested.py @@ -188,6 +188,55 @@ def snapshot_service(middleware): return name +class _Snapshots: + """This module's entire interface to snapshots, in one object. + + "READ the truth from ZFS, MAKE CHANGES through middleware" is the rule the whole + module rests on. It used to live in a comment, while `middleware` and the ZFS + reader were threaded through five functions **as a pair** -- and the namespace was + re-resolved in each of them. That is one collaborator, not two, so it is one + object; the rule is now structural rather than remembered. + + Deliberately private and constructed inside the public functions: `apply.sh` + injects calls to those functions into middlewared itself, so their signatures are + a boot-time contract with a live NAS and are not worth churning for tidiness. + """ + + def __init__(self, middleware, list_snapshots=None): + self._mw = middleware + self._list = list_snapshots or list_snapshot_names + self._service = None + + @property + def service(self): + """The namespace we delete through. Resolved once, on first use. + + Lazy on purpose: resolving it raises when middleware has no usable delete, + and the read-only paths must not blow up over a mutation they never make. + """ + if self._service is None: + self._service = snapshot_service(self._mw) + return self._service + + def names(self, dataset): + """Every snapshot at or under `dataset`, from ZFS. Raises if it cannot be read. + + Never from middleware: its snapshot query hides the internal datasets + (205 of 274 on the test box), and a snapshot the sweep cannot SEE is a + snapshot nothing will ever collect. + """ + return self._list(dataset) + + def delete(self, name, recursive=False): + """Delete one snapshot -- through middleware, so its bookkeeping stays right. + + An exact-name delete works even on a dataset the query hides; it is only + enumeration that lies. + """ + options = ({"recursive": True},) if recursive else () + return self._mw.call_sync(f"{self.service}.delete", name, *options) + + class ZfsError(Exception): """`zfs list` failed. Enumeration is unreliable, so the caller must not guess.""" @@ -787,8 +836,7 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, snapshots hit this, and before the fix they were orphaned permanently. """ dataset, _, snapname = snapshot.partition("@") - svc = snapshot_service(middleware) - list_snapshots = list_snapshots or list_snapshot_names + snaps = _Snapshots(middleware, list_snapshots) # Release ZFS's own automounts first, or `zfs destroy` refuses with EBUSY on # everything restic read in the last few minutes. @@ -802,7 +850,7 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, # through 252 sequential deletes leaves exactly the orphans this function # exists to prevent. try: - middleware.call_sync(f"{svc}.delete", snapshot, {"recursive": True}) + snaps.delete(snapshot, recursive=True) return [] except Exception as e: # noqa: BLE001 - fall through to the explicit sweep # "Parent already gone" is the EXPECTED race (stock's finally won, once our @@ -829,7 +877,7 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, # An empty result means the tree is already gone -- delete nothing, and # do not fall back to the parent, which would only log a spurious # "does not exist" warning on every clean run. - names = snapshot_tree_names(snapshot, list_snapshots(dataset)) + names = snapshot_tree_names(snapshot, snaps.names(dataset)) except Exception as e: # noqa: BLE001 - fall back to at least the parent if logger: logger.warning( @@ -850,7 +898,7 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, if not failed: return [] try: - live = set(list_snapshots(dataset)) + live = set(snaps.names(dataset)) except Exception: # noqa: BLE001 - cannot refine; trust the delete's verdict return list(failed) return [n for n in failed if n in live] @@ -861,7 +909,7 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, failed = [] for name in remaining: try: - middleware.call_sync(f"{svc}.delete", name) + snaps.delete(name) 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 @@ -926,13 +974,12 @@ def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, """ dataset = current_snapshot.partition("@")[0] now = now or datetime.datetime.now(datetime.UTC) - svc = snapshot_service(middleware) - list_snapshots = list_snapshots or list_snapshot_names + snaps = _Snapshots(middleware, list_snapshots) try: # From ZFS: middleware's snapshot query hides internal datasets, and an # orphan it cannot see is an orphan nothing will ever collect. - all_names = list_snapshots(dataset) + all_names = snaps.names(dataset) except Exception as e: # noqa: BLE001 - cannot enumerate; collect nothing if logger: logger.warning( @@ -957,7 +1004,7 @@ def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, remaining = [] for name in stale: try: - middleware.call_sync(f"{svc}.delete", name) + snaps.delete(name) except Exception as e: # noqa: BLE001 - busy, or gone; either way, next run remaining.append(name) if logger: From ce6998a9351ac357677a96ff8abeb50bb1df13af Mon Sep 17 00:00:00 2001 From: sudolulo Date: Tue, 14 Jul 2026 00:10:10 +0000 Subject: [PATCH 4/9] =?UTF-8?q?fix:=20second=20audit=20=E2=80=94=20the=20d?= =?UTF-8?q?elete=20check=20did=20nothing=20on=20a=20real=20box,=20and=20fi?= =?UTF-8?q?ve=20guards=20were=20untestable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The most important finding is that the FIRST audit's fix was wrong. _can_delete() asked `callable(getattr(service, "delete"))`. But CRUDService defines `delete` on the BASE class and dispatches to self.do_delete at call time, so a bound `delete` exists on every CRUDService subclass whether or not it still implements one. The check was therefore answering "is this a CRUDService?" — precisely the weaker "is the namespace registered?" question its own docstring said must never be asked. It would still have picked a gutted pool.snapshot and failed every delete. It now walks the MRO and ignores middlewared.service.* plumbing, so only a PLUGIN class defining delete/do_delete counts. The test double was equally wrong: it modelled a gutted service as object(), a shape middlewared cannot produce, so the test passed against a fake it could never have caught in the field. It is now CRUDService-shaped. Also: - The recursive delete's fast path returned [] without confirming anything was destroyed. A delete that returns cleanly is not proof — iX has already gutted pool.snapshot.do_update on master into a no-op that returns None. cleanup_task read "no survivors" as a clean sweep, dropped the sidecar (the only record), and would have orphaned ~250 snapshots per run, silently. It confirms against ZFS now, and the by-name sweep trusts ZFS rather than the API's return value. - When ZFS cannot be read, the sweep no longer claims success. The two mistakes are not symmetric: a false survivor self-heals (sidecar kept, next run reclaims, record clears), a lost record does not. - _write_sidecar swallowed OSError. The sidecar is the only record the snapshots exist; failing to write it must never be invisible. - stage_nested now refuses UP FRONT when middleware has no usable snapshot delete, rather than discovering it after restic has already run. Tests. The autouse fixture added in the last commit did not work: `runner=_run`, `mounts_file="/proc/self/mounts"` and `sleep=time.sleep` are frozen into __defaults__ at def time, so monkeypatching the module attribute never reached them. 19 tests were still reading the real mount table — one matching name from running a real `umount` on the NAS — and the retry loop really slept. All three are late-bound now; the suite reads nothing outside tmp_path and runs in 1.1s. Every mutation the audit reported as SURVIVING now fails the suite: the naive delete check, the unconfirmed fast path, the malformed-row guard, a disconnected GC, eager service resolution, compat's method check, compat's unknown handling, a single-quoted filtered query in apply.sh, and the get-service assumption. Also: fingerprint() folded `unknown` problems into a broken module, so one transient 429 rewrote the bug report and the next clean run rewrote it back. Problems are state-tagged; only definite breakage is digested. Verified on TrueNAS 26.0.0-BETA.1: zvol-orphan case 0 orphans, 292-dataset backup 0 orphans / 0 leaked mounts / 0 stale sidecars, byte-identical restore of a 4-deep child dataset. --- CHANGELOG.md | 40 +++++ patch/truecloud_nested.py | 181 ++++++++++++++++++---- tests/test_compat.py | 139 +++++++++++++++++ tests/test_truecloud_nested.py | 275 +++++++++++++++++++++++++++++---- tools/compat.py | 18 ++- 5 files changed, 593 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 417903f..befe9c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,46 @@ worse than no alert, because one day it carries a security fix. `zfs.dataset.query`, which returns all 270 datasets. The bug existed only in the unreleased TrueNAS 26 port. +- **The patch now owns the snapshot sweep even when it does not stage anything.** + Stock decides whether to take a *recursive* snapshot by its own rule, and on + TrueNAS 26 that rule stopped being ours. + + Up to 25.10, stock's `create_snapshot` called `get_dataset_recursive()` — the same + function this module vendors — so "stock went recursive" and "we have something to + stage" were the *same question*, and stock's non-recursive delete was correct for + everything the patch declined to stage. TrueNAS 26 uses `filesystem.statfs`: + `recursive = (path == the dataset's mountpoint)`. The two rules now disagree for a + dataset whose only descendants are **ZVOLs** or **legacy/none-mountpoint** datasets + — stock snapshots it recursively, while the patch sees nothing to stage. + + The patch then handed the snapshot back to stock, which destroys the parent only. + With no staging tree there was no sidecar, and the garbage collector only ever ran + from the staging path — so nothing on the box would ever have found the children. + Reproduced on the test VM: one orphaned snapshot per zvol, on every run, forever, + with the backup reporting success. Ownership of the sweep is no longer conditional + on staging. + +- **The runtime resolved a *namespace*; the checker verified a *method*.** Those are + different questions, and the gap is a false "ok". `get_service()` only proves a + namespace is registered — it says nothing about whether `delete` still exists on it. + So if iX guts the method while keeping the service (they have already done exactly + that to `pool.snapshot.do_update` on master), `tools/compat.py` would fall through + to `zfs.snapshot`, report the box healthy, and let the patch apply — while the + runtime picked `pool.snapshot` and failed *every* delete, orphaning the whole tree. + Both sides now ask the same question, and a test binds the two lists together. + +- `query_filesystems()` **dropped malformed `zfs list` rows silently** — the last + remaining silent-omission path, and a direct contradiction of this module's cardinal + rule. It raises now. A missing `zfs` binary raised `FileNotFoundError` rather than + `ZfsError`; also fixed. + +- The snapshot retry loop **discarded the delete error** and reported every survivor + as "(still busy?)" — naming the one cause that is benign and self-healing, and + hiding the ones that are permanent. It keeps and reports the real error. + +- The staging-failure handler could **lose the original exception** if its own cleanup + sweep raised. An error handler must not be able to lose the error. + ## v0.6.1 — 2026-07-13 ### Fixed diff --git a/patch/truecloud_nested.py b/patch/truecloud_nested.py index 652b444..ab8e6b3 100644 --- a/patch/truecloud_nested.py +++ b/patch/truecloud_nested.py @@ -162,8 +162,43 @@ def pick_snapshot_service(can_delete): return None +#: Where middlewared's own service framework lives. Classes from this package are +#: PLUMBING, not implementations -- see `_defines_delete`. +FRAMEWORK_PACKAGE = "middlewared.service" + + +def _defines_delete(service): + """Does this service ITSELF implement a delete -- or merely inherit the framework's? + + The distinction is the whole fix, and getting it wrong is silent. + + `CRUDService` defines `delete` on the BASE class and dispatches to `self.do_delete` + at call time. So `getattr(service, "delete")` is a bound method on EVERY + CRUDService subclass, whether or not that subclass still implements one: + + middlewared.plugins.pool_.snapshot.PoolSnapshotService defines ['do_delete'] + middlewared.service.crud_service.CRUDService defines ['delete'] + + An earlier version of this check asked `callable(getattr(service, "delete"))` and + was therefore answering "is this a CRUDService?" -- exactly the weaker "is the + namespace registered?" question that `pick_snapshot_service` exists to avoid. It + would have picked a gutted `pool.snapshot` and failed every delete. + + So walk the MRO and ignore the framework's generic plumbing: a delete is real only + where a PLUGIN class defines it. That mirrors `tools/compat.py`, which looks for + the `def` in the plugin file declaring the namespace. + """ + for klass in type(service).__mro__: + module = getattr(klass, "__module__", "") or "" + if module == FRAMEWORK_PACKAGE or module.startswith(FRAMEWORK_PACKAGE + "."): + continue # the framework's generic CRUD dispatcher + if any(m in vars(klass) for m in DELETE_METHODS): + return True + return False + + def _can_delete(middleware, namespace): - """Is `.delete` actually callable on this middleware?""" + """Is `.delete` actually implemented on this middleware?""" try: service = middleware.get_service(namespace) except Exception: @@ -172,7 +207,7 @@ def _can_delete(middleware, namespace): # service that is not really there fails later, mid-backup, holding a # snapshot -- the worst possible moment to find out. return False - return any(callable(getattr(service, m, None)) for m in DELETE_METHODS) + return _defines_delete(service) def snapshot_service(middleware): @@ -342,6 +377,12 @@ def list_snapshot_names(dataset, runner=None): #: Where staging trees are assembled. tmpfs; bind mounts consume no space. STAGING_BASE = "/run/truecloud-nested" +#: The kernel's mount table. Late-bound (never a default argument) so a +#: test can point it somewhere harmless -- a default is frozen at def time, +#: which is how 19 tests ended up reading the REAL table, one matching name +#: away from running a real `umount` on the NAS. +MOUNTS_FILE = "/proc/self/mounts" + # Which snapshot a staging tree pins is recorded ONLY in the sidecar file, never # also in memory. An in-process dict would be a second source of truth that a # middlewared restart silently empties -- and it is exactly the restart case that @@ -376,7 +417,7 @@ def sidecar_for(staging_root: str) -> str: return staging_root + ".snapshot" -def _write_sidecar(staging_root: str, snapshots) -> None: +def _write_sidecar(staging_root: str, snapshots, logger=None) -> 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. @@ -393,10 +434,22 @@ def _write_sidecar(staging_root: str, snapshots) -> None: """ if isinstance(snapshots, str): snapshots = [snapshots] - with contextlib.suppress(OSError): + try: os.makedirs(os.path.dirname(staging_root), exist_ok=True) with open(sidecar_for(staging_root), "w", encoding="utf-8") as fh: fh.write("\n".join(dict.fromkeys(snapshots))) # de-duped, order kept + except OSError as e: + # This used to be suppressed silently, and it is the LAST thing that should be. + # The sidecar is the only record that these snapshots exist; if the write fails + # (a full /run, say) cleanup_task finds nothing to sweep, and only the by-name + # collector -- an hour later -- has any chance of finding them. Failing to + # write it is not fatal, but it must never be invisible. + if logger: + logger.error( + "truecloud-patch: COULD NOT RECORD the snapshot(s) %s (%r). If this " + "run does not clean them up itself, only the by-name collector will " + "ever find them.", ", ".join(snapshots), e, + ) def _read_sidecar(staging_root: str): @@ -608,8 +661,9 @@ def plan_staging(base_dataset, base_mountpoint, path, snapshot_name, datasets, return mounts, skipped -def current_mounts_under(root, mounts_file="/proc/self/mounts"): +def current_mounts_under(root, mounts_file=None): """Mountpoints at or under ``root``, deepest first. Used for teardown.""" + mounts_file = mounts_file or MOUNTS_FILE found = [] try: with open(mounts_file, encoding="utf-8") as fh: @@ -637,12 +691,13 @@ def _run(cmd): ) -def apply_plan(mounts, runner=_run, isdir=os.path.isdir): +def apply_plan(mounts, runner=None, isdir=os.path.isdir): """Execute the bind-mount plan. Blocking; call via ``run_in_thread``. Raises StagingError on the first failure, after rolling back what was mounted -- a half-built tree must never be handed to the backup tool. """ + runner = runner or _run if not mounts: raise StagingError("empty staging plan") @@ -695,12 +750,14 @@ def verify_staged(mounts, ismount=os.path.ismount, listdir=os.listdir): return True -def teardown(staging_root, runner=_run, mounts_file="/proc/self/mounts"): +def teardown(staging_root, runner=None, mounts_file=None): """Unmount the staging tree (deepest first) and remove the root. Idempotent, and does not depend on an in-memory plan -- so it also cleans up leftovers from a crashed run. """ + mounts_file = mounts_file or MOUNTS_FILE + runner = runner or _run errors = [] for mp in current_mounts_under(staging_root, mounts_file=mounts_file): res = runner(["umount", mp]) @@ -713,8 +770,9 @@ def teardown(staging_root, runner=_run, mounts_file="/proc/self/mounts"): return errors -def snapdir_automounts(snapshot_name, mounts_file="/proc/self/mounts"): +def snapdir_automounts(snapshot_name, mounts_file=None): """Every ``/.zfs/snapshot/`` ZFS automount for this snapshot.""" + mounts_file = mounts_file or MOUNTS_FILE suffix = "/.zfs/snapshot/" + snapshot_name found = [] try: @@ -730,7 +788,7 @@ def snapdir_automounts(snapshot_name, mounts_file="/proc/self/mounts"): return sorted(found, key=_depth, reverse=True) # deepest first -def release_snapdirs(snapshot_name, runner=_run, mounts_file="/proc/self/mounts"): +def release_snapdirs(snapshot_name, runner=None, mounts_file=None): """Unmount ZFS's OWN snapshot automounts, so the snapshots can be destroyed. Reading anything under ``/.zfs/snapshot//`` makes ZFS **automount** @@ -747,6 +805,8 @@ def release_snapdirs(snapshot_name, runner=_run, mounts_file="/proc/self/mounts" Deepest first, so a child's automount is released before its parent's. """ + mounts_file = mounts_file or MOUNTS_FILE + runner = runner or _run errors = [] for mp in snapdir_automounts(snapshot_name, mounts_file=mounts_file): res = runner(["umount", mp]) @@ -818,7 +878,7 @@ def get_dataset_recursive(datasets, directory): def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, - sleep=time.sleep, list_snapshots=None): + sleep=None, list_snapshots=None): """Delete the parent snapshot AND every child created by ``zfs snapshot -r``. Returns the snapshots it could NOT delete -- callers must not throw that away. @@ -837,6 +897,7 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, """ dataset, _, snapname = snapshot.partition("@") snaps = _Snapshots(middleware, list_snapshots) + sleep = sleep or time.sleep # Release ZFS's own automounts first, or `zfs destroy` refuses with EBUSY on # everything restic read in the last few minutes. @@ -851,7 +912,44 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, # exists to prevent. try: snaps.delete(snapshot, recursive=True) - return [] + + # CONFIRM it. A delete that returns without raising is not proof that anything + # was destroyed, and this is the one place where believing it is catastrophic: + # `cleanup_task` reads an empty survivor list as "clean sweep" and REMOVES THE + # SIDECAR -- the only record the tree ever existed. ~250 snapshots would be + # orphaned per run, with nothing left to find them, and the backup green. + # + # Not paranoia about a hypothetical: iX has already gutted + # `pool.snapshot.do_update` on master into a no-op whose body is commented out + # and which returns None. An AST check still sees the `def`, and a callable + # check still sees the method. Only asking ZFS can tell. + # + # It costs one `zfs list` (~350ms against 2148 snapshots) on an 18-minute + # backup, and only on the path that would otherwise skip verification entirely. + try: + left = snapshot_tree_names(snapshot, snaps.names(dataset)) + except Exception as e: # noqa: BLE001 - cannot confirm; do not claim success + if logger: + logger.warning( + "truecloud-patch: deleted %s but could not confirm it is gone " + "(%r); keeping it recorded so the next run re-checks", snapshot, e, + ) + # Keep OWNING it. Reporting a clean sweep here makes cleanup_task drop the + # sidecar; if the delete had in fact done nothing, the tree is orphaned + # with no record. A survivor we later find already gone costs one + # idempotent retry; a lost record costs the snapshots, permanently. + return [snapshot] + + if not left: + return [] + + if logger: + logger.warning( + "truecloud-patch: the recursive delete of %s reported success but " + "%d snapshot(s) are still there; sweeping them by name", + snapshot, len(left), + ) + # Fall through to the by-name sweep, which retries and reports survivors. except Exception as e: # noqa: BLE001 - fall through to the explicit sweep # "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. @@ -886,22 +984,34 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, ) names = [snapshot] - def confirm_gone(failed): - """Drop any name ZFS no longer has, even though its delete raised. + def still_there(tried, failed): + """Which of `tried` does ZFS STILL have? The delete's verdict is not evidence. - A delete that raised "does not exist" SUCCEEDED as far as we care, and must - not be retried or reported. The query is only a refinement: if it cannot be - answered we keep the delete's own verdict, rather than inventing survivors -- - a false survivor keeps the sidecar forever and is reported as a leak that - isn't there. + ZFS is the authority, not the API's return value, and the difference is not + academic in either direction: + + * A delete that RAISED "does not exist" succeeded as far as we care, and must + not be retried or reported as a leak. + * A delete that RETURNED CLEANLY may have done nothing at all. iX has already + gutted `pool.snapshot.do_update` on master into a no-op whose body is + commented out and which returns None. Trusting that verdict makes + `cleanup_task` see "no survivors", drop the sidecar -- the only record -- and + orphan the whole tree, forever, silently. + + If ZFS cannot be read we cannot check either way -- so keep owning ALL of them. + The two mistakes are not symmetric: + + * a false survivor SELF-HEALS. The sidecar is kept, the next run reclaims it, + the delete raises "does not exist", and the record clears. + * a lost record does NOT. The snapshots are orphaned with nothing pointing at + them, and only the by-name collector -- an hour later, and only if ZFS is + readable by then -- has any chance of finding them. """ - if not failed: - return [] try: live = set(snaps.names(dataset)) - except Exception: # noqa: BLE001 - cannot refine; trust the delete's verdict - return list(failed) - return [n for n in failed if n in live] + except Exception: # noqa: BLE001 - cannot check; keep owning them + return list(tried) + return [n for n in tried if n in live] remaining = list(names) last_error = {} @@ -919,7 +1029,7 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, last_error[name] = e failed.append(name) - remaining = confirm_gone(failed) + remaining = still_there(remaining, failed) if not remaining: return [] @@ -939,7 +1049,7 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, return remaining -def mounted_snapshots(mounts_file="/proc/self/mounts"): +def mounted_snapshots(mounts_file=None): """Every ZFS snapshot something is currently mounted from. The device field of a snapshot mount IS the snapshot name (`Tap/apps/x@snap`), for @@ -948,6 +1058,7 @@ def mounted_snapshots(mounts_file="/proc/self/mounts"): protects a concurrently-running backup from the garbage collector, rather than trusting an age heuristic to be generous enough. """ + mounts_file = mounts_file or MOUNTS_FILE live = set() try: with open(mounts_file, encoding="utf-8") as fh: @@ -961,7 +1072,7 @@ def mounted_snapshots(mounts_file="/proc/self/mounts"): def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, - now=None, mounts_file="/proc/self/mounts", list_snapshots=None): + now=None, mounts_file=None, list_snapshots=None): """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 @@ -972,6 +1083,7 @@ def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, 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. """ + mounts_file = mounts_file or MOUNTS_FILE dataset = current_snapshot.partition("@")[0] now = now or datetime.datetime.now(datetime.UTC) snaps = _Snapshots(middleware, list_snapshots) @@ -1101,7 +1213,7 @@ def own_snapshot(middleware, task_name, snapshot, logger=None, list_snapshots=No # 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, [*pending, snapshot]) + _write_sidecar(staging_root, [*pending, snapshot], logger=logger) return staging_root @@ -1126,6 +1238,15 @@ def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint, (see SNAPSHOT_BLOCK in apply.sh). """ snapshot_name = snapshot.split("@", 1)[1] + + # Refuse BEFORE staging, not after restic has run. We are about to pin a recursive + # snapshot with bind mounts; if this middleware has no usable snapshot delete we + # could never sweep it, and the honest move is to fail now rather than take a + # snapshot we cannot clean up. (`_Snapshots` resolves lazily on purpose -- the + # read-only paths must not raise over a mutation they never make -- so the staging + # path asks explicitly.) + snapshot_service(middleware) + staging_root = own_snapshot( middleware, task_name, snapshot, logger=logger, list_snapshots=list_snapshots, @@ -1215,7 +1336,7 @@ def cleanup_task(middleware, task_name, logger=None, list_snapshots=None): ) # 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) + _write_sidecar(staging_root, survivors, logger=logger) return _remove_sidecar(staging_root) @@ -1224,7 +1345,7 @@ def cleanup_task(middleware, task_name, logger=None, list_snapshots=None): # ── offline cleanup (uninstall.sh / recover.sh) ─────────────────────────────── -def cleanup_all(base=None, runner=_run, mounts_file="/proc/self/mounts", +def cleanup_all(base=None, runner=None, mounts_file=None, glob_fn=None, read_sidecar=_read_sidecar): """Tear down every staging tree. Used by uninstall.sh and recover.sh. @@ -1235,6 +1356,8 @@ def cleanup_all(base=None, runner=_run, mounts_file="/proc/self/mounts", Returns ``(lines, errors)``: report lines to print, and unmount errors. """ + mounts_file = mounts_file or MOUNTS_FILE + runner = runner or _run import glob as _glob base = base or STAGING_BASE diff --git a/tests/test_compat.py b/tests/test_compat.py index 450b3c1..32b8bc4 100644 --- a/tests/test_compat.py +++ b/tests/test_compat.py @@ -416,3 +416,142 @@ class TestMiddlewareMethodsWeCall: })) whys = " ".join(p["why"] for p in r[NESTED]["problems"]) assert "orphan" in whys + + +class TestTheMethodCheckIsNotJustANamespaceCheck: + """compat must verify the METHOD, not merely that the namespace still exists. + + Deleting the method check entirely used to leave all 304 tests green -- so the + "namespace AND method" claim was unenforced and silently revertible. It is the + half of the predicate that catches iX gutting a method while keeping its service, + which they have already done to `pool.snapshot.do_update` on master. + """ + + def test_a_namespace_that_no_longer_defines_delete_is_broken(self): + gutted = ( + "class PoolSnapshotService(CRUDService):\n" + " class Config:\n" + " namespace = 'pool.snapshot'\n" + " def query(self, filters, options):\n pass\n" + # do_delete is GONE -- the service is still registered and still a + # CRUDService, so it still INHERITS a callable `delete`. + ) + r = check_files(with_(**{ + "plugins/pool_/snapshot.py": gutted, + "plugins/zfs_/snapshot.py": None, # no fallback either + })) + assert is_broken(r[NESTED]), ( + "a namespace with no delete must be BROKEN. Checking only that the " + "namespace exists would apply the patch to a box that cannot sweep its " + "own snapshots." + ) + + def test_the_alternative_still_saves_it_when_only_the_primary_is_gutted(self): + gutted = ( + "class PoolSnapshotService(CRUDService):\n" + " class Config:\n" + " namespace = 'pool.snapshot'\n" + " def query(self, filters, options):\n pass\n" + ) + r = check_files(with_(**{ + "plugins/pool_/snapshot.py": gutted, + "plugins/zfs_/snapshot.py": ZFS_ERA_SNAPSHOT, + })) + assert r[NESTED]["ok"], r[NESTED]["problems"] + + +class TestUnreadableIsNeverOkAndNeverBroken: + """A rate limit is not a regression, and it is not a clean bill of health either. + + compat runs ~30 unauthenticated GitHub requests per matrix; 429 is a real outcome. + It also runs at BOOT against the installed tree, where a read can fail with EACCES. + + * treating unreadable as BROKEN repaints the README, files a bug report, and + makes apply.sh refuse the module on a box where it works. + * treating it as OK injects a module whose delete may be gone. + + Both mutations used to pass the whole suite. + """ + + def test_both_spellings_unreadable_is_unknown_not_broken(self): + r = check_files(with_(**{ + "plugins/pool_/snapshot.py": Unreadable("HTTP 429"), + "plugins/zfs_/snapshot.py": Unreadable("HTTP 429"), + })) + assert not is_broken(r[NESTED]), "a 429 is not iX deleting the snapshot service" + assert r[NESTED]["unknown"] + + def test_an_unreadable_primary_with_a_healthy_alternative_is_ok(self): + r = check_files(with_(**{ + "plugins/pool_/snapshot.py": Unreadable("HTTP 429"), + "plugins/zfs_/snapshot.py": ZFS_ERA_SNAPSHOT, + })) + assert r[NESTED]["ok"], r[NESTED]["problems"] + assert not r[NESTED]["unknown"], ( + "one spelling answered the question; the other's 429 is irrelevant" + ) + + def test_a_missing_primary_with_an_unreadable_alternative_is_unknown(self): + # We cannot tell whether the box is broken. Saying either would be a guess. + r = check_files(with_(**{ + "plugins/pool_/snapshot.py": None, + "plugins/zfs_/snapshot.py": Unreadable("HTTP 429"), + })) + assert not is_broken(r[NESTED]) + assert r[NESTED]["unknown"] + + +class TestGetServiceIsChecked: + """The runtime resolves the snapshot namespace through `middleware.get_service`. + + It is not a plugin method, so the manifest had no way to express it and never + checked it. If it vanishes, `_can_delete` reports BOTH namespaces unusable and + every nested backup fails -- on a box the preflight had declared healthy. + """ + + def test_a_middleware_without_get_service_is_broken(self): + r = check_files(with_(**{"utils/plugins.py": None})) + assert is_broken(r[NESTED]) + details = " ".join(p["detail"] for p in r[NESTED]["problems"]) + assert "get_service" in details + + +class TestATransientNetworkBlipDoesNotWakeAnybody: + """The fingerprint must digest what iX BROKE, not what GitHub failed to serve. + + `unknown` problems (a 429 on one of ~30 unauthenticated fetches, an EACCES at boot) + used to be folded into an already-broken module's problem list, so one blip flipped + the fingerprint, `compat_publish` rewrote the issue body, and the next clean run + rewrote it back. Daily churn is what teaches people to ignore the bot -- which is + the whole thing this fingerprint exists to prevent. + """ + + def _rows(self, files): + return [{"ref": "master", "modules": check_files(files)}] + + def test_an_unreadable_file_does_not_change_the_fingerprint_of_a_broken_ref(self): + broken = with_(**{ + "plugins/cloud/snapshot.py": + "async def create_snapshot(name, path, middleware):\n return 1, 2\n", + }) + clean = compat.fingerprint(self._rows(broken)) + + blipped = dict(broken) + blipped["rclone/remote/b2.py"] = Unreadable("HTTP 429") + assert compat.fingerprint(self._rows(blipped)) == clean, ( + "a rate-limited fetch changed the fingerprint, so the bot rewrites the " + "issue body and then rewrites it back tomorrow" + ) + + def test_a_REAL_new_finding_still_changes_it(self): + # ...and the anti-noise measure must not have made it deaf. + broken = with_(**{ + "plugins/cloud/snapshot.py": + "async def create_snapshot(name, path, middleware):\n return 1, 2\n", + }) + worse = dict(broken) + worse["plugins/cloud_backup/restic.py"] = ( + "class ResticConfig:\n cmd: list\n\n" + "def get_restic_config(entry, credentials):\n pass\n" + ) + assert compat.fingerprint(self._rows(worse)) != compat.fingerprint(self._rows(broken)) diff --git a/tests/test_truecloud_nested.py b/tests/test_truecloud_nested.py index af4dcd7..7167b0a 100644 --- a/tests/test_truecloud_nested.py +++ b/tests/test_truecloud_nested.py @@ -62,7 +62,26 @@ def never_touch_the_real_system(monkeypatch): raise AssertionError(f"real system command in a unit test: {cmd!r}") monkeypatch.setattr(tn, "_run", forbidden) + + # ...and the REAL mount table, which is the other half. Nineteen tests were + # reading /proc/self/mounts: harmless here, but on the NAS a name that happened to + # match would send `release_snapdirs`/`teardown` off to run a real `umount`. + # + # Patching the module attribute only works because these are late-bound now. A + # default argument (`mounts_file="/proc/self/mounts"`) is frozen into __defaults__ + # at def time and monkeypatching cannot reach it -- which is exactly why the first + # version of this fixture looked like it worked and did not. + monkeypatch.setattr(tn, "MOUNTS_FILE", os.devnull) + + # ...and never really sleep. The retry loop waits 5s between attempts for ZFS's + # automount window to expire; a test that hits it burns 20 real seconds and tells + # you nothing. (Same frozen-default trap: `sleep=time.sleep` in the signature could + # not be intercepted at all until it was late-bound.) + slept = [] + monkeypatch.setattr(tn.time, "sleep", lambda s: slept.append(s)) + yield + assert not attempted, ( "this test ran real system commands: " + "; ".join(repr(c) for c in attempted) @@ -233,20 +252,49 @@ class TestSnapshotTreeNames: assert snapshot_tree_names("Tap", self.ALL) == [] -class _FakeSnapshotService: - """What `middleware.get_service("")` hands back. +class _FrameworkCRUDService: + """Stands in for middlewared's real `CRUDService` base class. - 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. + This shape is load-bearing, and a fake without it is worse than no fake at all. + + The real CRUDService defines `delete` on the BASE class and dispatches to + `self.do_delete` at call time, so a bound `delete` exists on EVERY subclass — + including one whose `do_delete` iX has deleted. A fake that is just a bare object + with a `delete` attribute cannot express that, so a runtime check that asks + `hasattr(service, "delete")` would look CORRECT against the fake while being + useless against the real thing. That is exactly what happened: the first version + of this fix passed its test and did nothing on a real box. + + `__module__` is set to middlewared's real framework package because that is how + `_defines_delete` tells plumbing apart from an implementation. + """ + + def delete(self, *args, **kwargs): # the generic dispatcher -> self.do_delete + raise NotImplementedError + + +_FrameworkCRUDService.__module__ = "middlewared.service.crud_service" + + +class _FakeSnapshotService(_FrameworkCRUDService): + """What `middleware.get_service("")` hands back: a plugin CRUDService. + + `delete_method=None` models the dangerous case — iX guts the concrete method but + leaves the service registered (they have already done this to + `pool.snapshot.do_update` on master). The inherited `delete` is still there and + still callable; only the implementation is gone. """ def __init__(self, delete_method): if delete_method: - setattr(self, delete_method, lambda *a, **kw: None) + # Define it on the CLASS, not the instance: `_defines_delete` walks the + # MRO's __dict__s, exactly as it must against a real service. + cls = type( + "FakePluginSnapshotService", (_FrameworkCRUDService,), + {delete_method: lambda self, *a, **kw: None}, + ) + cls.__module__ = "middlewared.plugins.pool_.snapshot" + self.__class__ = cls class FakeMiddleware: @@ -381,9 +429,11 @@ class TestDeleteSnapshotTree: 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 listed == [], ( - "the fast path enumerated. On the real pool that is a `zfs list` over 2148 " - "snapshots on every clean run, for nothing." + assert listed == ["Tap"], ( + "the fast path must enumerate EXACTLY ONCE -- to CONFIRM the tree is gone. " + "Zero would mean trusting a delete that returned without raising, and a " + "silent no-op delete then makes cleanup_task drop the sidecar and orphan " + "~250 snapshots forever. More than once is waste." ) def test_survives_recursive_and_enumeration_failure_by_deleting_the_parent(self): @@ -1290,7 +1340,7 @@ class TestTheProductionWiringIsWhatWeThinkItIs: return super().call_sync(method, *args) tn.delete_snapshot_tree(NoRecursive(["Tap@snap"]), "Tap@snap") - assert called == ["Tap"], ( + assert called and set(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." @@ -1302,7 +1352,9 @@ class TestTheProductionWiringIsWhatWeThinkItIs: 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" + assert called and set(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 @@ -1314,14 +1366,23 @@ class TestTheProductionWiringIsWhatWeThinkItIs: 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." - ) + + # Match the METHOD NAME however it is quoted. An earlier version of this test + # only looked for the double-quoted form, so a single-quoted + # `call_sync('pool.snapshot.query')` -- including one passed in as the sweep's + # lister, which is the catastrophic case -- sailed straight through it. + import re + code = "\n".join( + ln for ln in src.splitlines() if not ln.lstrip().startswith("#") + ) + offenders = re.findall(r"\b(?:pool|zfs)\.(?:dataset|snapshot)\.query\b", code) + assert not offenders, ( + f"apply.sh references {sorted(set(offenders))}. Middleware's queries apply " + f"a visibility policy and hide ix-apps/*, .system/*, .ix-virt/* -- 84 of " + f"270 datasets on a real pool, including live app data. Enumerating from " + f"them omits those datasets from the backup SILENTLY, and sweeping from " + f"them orphans one snapshot per hidden dataset on every run." + ) class TestTheSnapshotNamespaceIsResolvedNotAssumed: @@ -1367,7 +1428,16 @@ class TestTheSnapshotNamespaceIsResolvedNotAssumed: class GuttedPoolSnapshot(FakeMiddleware): def get_service(self, name): if name == "pool.snapshot": - return object() # registered, but no delete on it + # Registered, and `delete` IS still there -- inherited from + # CRUDService, which dispatches to a `do_delete` that no longer + # exists. This is the shape middlewared actually produces, and a + # naive `hasattr(service, "delete")` says YES to it. + gutted = _FakeSnapshotService(None) + assert callable(gutted.delete), ( + "the fake must keep the inherited dispatcher, or it cannot " + "reproduce the bug" + ) + return gutted if name == "zfs.snapshot": return super().get_service("zfs.snapshot") raise KeyError(name) @@ -1428,9 +1498,6 @@ class TestWeOwnTheSweepEvenWhenWeDoNotStage: 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) @@ -1442,8 +1509,6 @@ class TestWeOwnTheSweepEvenWhenWeDoNotStage: 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. @@ -1479,3 +1544,157 @@ class TestWeOwnTheSweepEvenWhenWeDoNotStage: "not ours) and deletes only the parent -- so every child is orphaned, with " "no sidecar and no GC, on every run." ) + + +class TestASilentNoOpDeleteCannotDropTheSidecar: + """A delete that returns without raising is not proof anything was destroyed. + + iX has already gutted `pool.snapshot.do_update` on master into a no-op whose body + is commented out and which returns None. An AST check still sees the `def`; a + callable check still sees the method. If `do_delete` ever goes the same way, the + recursive delete returns cleanly, `delete_snapshot_tree` reports no survivors, + `cleanup_task` removes the sidecar -- the only record -- and ~250 snapshots are + orphaned forever with the backup reporting SUCCESS. + """ + + def test_a_delete_that_does_nothing_is_caught_and_reported(self): + class NoOpDelete(FakeMiddleware): + def call_sync(self, method, *args): + self.calls.append((method, args)) + return None # "succeeds", destroys nothing + + mw = NoOpDelete(["Tap@snap", "Tap/apps@snap", "Tap/apps/lidarr@snap"]) + survivors = tn.delete_snapshot_tree( + mw, "Tap@snap", list_snapshots=mw.list_snapshots, sleep=lambda _s: None, + ) + + assert sorted(survivors) == sorted( + ["Tap@snap", "Tap/apps@snap", "Tap/apps/lidarr@snap"]), ( + "a no-op delete must be REPORTED as survivors, so cleanup_task keeps the " + "sidecar and the next run reclaims them. Returning [] here silently " + "orphans the entire tree." + ) + + def test_an_unconfirmable_delete_keeps_owning_the_tree(self): + # If ZFS cannot be read we cannot confirm the delete did anything. Claiming a + # clean sweep makes cleanup_task DROP the sidecar -- and if the delete had in + # fact done nothing, the tree is orphaned with no record of it, forever. + # + # The two mistakes are not symmetric. A false survivor self-heals: the sidecar + # is kept, the next run reclaims it, the delete raises "does not exist", and + # the record clears. A lost record is permanent. So when in doubt, keep owning. + mw = FakeMiddleware(["Tap@snap"]) + + def cannot_enumerate(_dataset): + raise tn.ZfsError("pool I/O is currently suspended") + + assert tn.delete_snapshot_tree( + mw, "Tap@snap", list_snapshots=cannot_enumerate, sleep=lambda _s: None, + ) == ["Tap@snap"] + + +class TestTheMalformedRowGuard: + """`zfs list -H` neither quotes nor escapes. A tab in a mountpoint splits wrong. + + Dropping such a row would remove a dataset from the staging plan without it + appearing in `skipped` either -- the cardinal-rule failure, on the newest code + path. Two mutations (silently filtering the row; dropping the `fields=` argument) + used to pass the whole suite. + """ + + @staticmethod + def _runner(stdout): + class R: + returncode = 0 + stderr = "" + R.stdout = stdout + return lambda cmd: R() + + def test_a_row_with_the_wrong_field_count_RAISES(self): + # A mountpoint containing a tab -> 4 fields, not 3. + bad = "scratch\t/mnt/scratch\tyes\nscratch/odd\t/mnt/od\td\tyes\n" + with pytest.raises(tn.ZfsError, match="tab-separated"): + tn.query_filesystems(runner=self._runner(bad)) + + def test_the_error_names_the_offending_row(self): + bad = "a\tb\tc\nbroken\trow\n" + with pytest.raises(tn.ZfsError, match="broken"): + tn.query_filesystems(runner=self._runner(bad)) + + def test_the_field_count_is_actually_enforced_for_snapshots_too(self): + with pytest.raises(tn.ZfsError): + tn.list_snapshot_names("Tap", runner=self._runner("ok\nnot\tok\n")) + + def test_query_filesystems_asks_zfs_for_filesystems_only(self): + # Dropping `-t filesystem` would drag in volumes and snapshots, which the + # planner would then try to stage. + seen = [] + + def runner(cmd): + seen.append(cmd) + return self._runner("Tap\t/mnt/Tap\tyes\n")(cmd) + + tn.query_filesystems(runner=runner) + assert "-t" in seen[0] and "filesystem" in seen[0] + assert "name,mountpoint,mounted" in seen[0] + + +class TestTheGarbageCollectorIsActuallyWiredIn: + """The GC is the ONLY recovery path when the sidecar itself is gone. + + The sidecar lives in /run (tmpfs), so a reboot mid-backup destroys it and orphans + the whole tree with nothing pointing at it. `own_snapshot` is the GC's only + production caller -- and stubbing that call out used to pass all 304 tests, i.e. + the GC could have been silently disconnected. + """ + + def test_own_snapshot_runs_the_collector(self, tmp_path, monkeypatch): + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + ran = [] + monkeypatch.setattr( + tn, "gc_stale_snapshots", + lambda *a, **kw: ran.append(a[1]) or [], + ) + mw = FakeMiddleware(["Tap@new"]) + tn.own_snapshot(mw, "cloud_backup-5", "Tap@new", + list_snapshots=mw.list_snapshots) + assert ran == ["cloud_backup-5"], ( + "own_snapshot did not run the garbage collector. It is the only thing that " + "ever finds a tree whose sidecar was lost to a reboot." + ) + + def test_a_collected_orphan_is_carried_into_the_sidecar_if_it_survives( + self, tmp_path, monkeypatch + ): + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + monkeypatch.setattr( + tn, "gc_stale_snapshots", lambda *a, **kw: ["Tap/x@busy-orphan"], + ) + mw = FakeMiddleware(["Tap@new"]) + root = tn.own_snapshot(mw, "cloud_backup-5", "Tap@new", + list_snapshots=mw.list_snapshots) + assert "Tap/x@busy-orphan" in tn._read_sidecar(root), ( + "an orphan the GC could not delete must be RECORDED, or the next run has " + "no idea it exists" + ) + + +class TestTheServiceIsResolvedLazily: + """`_Snapshots.service` resolves on first use, not in the constructor. + + Eager resolution looks harmless and is not: `gc_stale_snapshots` would raise inside + its own broad `except` and silently collect nothing, and `delete_snapshot_tree` + would raise `StagingError` out of the constructor -- outside its try -- instead of + returning survivors. + """ + + def test_constructing_it_against_a_hopeless_middleware_does_not_raise(self): + class Neither(FakeMiddleware): + def get_service(self, name): + raise KeyError(name) + + snaps = tn._Snapshots(Neither()) # must not raise + assert snaps.names is not None + + with pytest.raises(StagingError): + snaps.delete("Tap@snap") # ...only the MUTATION refuses diff --git a/tools/compat.py b/tools/compat.py index 7a991d0..e9e51eb 100644 --- a/tools/compat.py +++ b/tools/compat.py @@ -494,7 +494,10 @@ def check_source(a: Assumption, src: str | None) -> tuple[str, str | None]: breaks a box that was working. """ if src is None: - return "broken", f"{a.path} does not exist" + # Name the SYMBOL, not just the file. Whoever reads the bug report needs to + # know what the patch can no longer reach, and "utils/plugins.py does not + # exist" does not tell them that `get_service` is gone. + return "broken", f"{a.path} does not exist, so `{a.symbol}` is gone" try: tree = ast.parse(src) @@ -629,6 +632,7 @@ def check(loader, modules=None) -> dict: out[a.module]["unknown"] = True out[a.module]["problems"].append({ "id": a.id, "detail": f"could not read {a.path}: {e}", "why": a.why, + "state": "unknown", }) continue @@ -636,12 +640,12 @@ def check(loader, modules=None) -> dict: if status == "broken": out[a.module]["ok"] = False out[a.module]["problems"].append({ - "id": a.id, "detail": detail, "why": a.why, + "id": a.id, "detail": detail, "why": a.why, "state": "broken", }) elif status == "unknown": out[a.module]["unknown"] = True out[a.module]["problems"].append({ - "id": a.id, "detail": detail, "why": a.why, + "id": a.id, "detail": detail, "why": a.why, "state": "unknown", }) # The methods the injected code CALLS, not just the symbols it wraps. @@ -680,11 +684,13 @@ def check(loader, modules=None) -> dict: out[c.module]["unknown"] = True out[c.module]["problems"].append({ "id": c.id, "detail": "; ".join(details), "why": c.why, + "state": "unknown", }) else: out[c.module]["ok"] = False out[c.module]["problems"].append({ "id": c.id, "detail": "; ".join(details), "why": c.why, + "state": "broken", }) for module, (path, phrase, native_when_present) in NATIVE_PROBES.items(): @@ -1026,6 +1032,12 @@ def fingerprint(rows: list[dict]) -> str: for mod, m in r["modules"].items() if is_broken(m) for p in m["problems"] + # `unknown` problems are things we could not READ (a 429, an EACCES), not + # things iX changed. On a ref that is broken for some other reason they would + # otherwise join the digest, so one transient network blip rewrites the issue + # body and the next clean run rewrites it back. That is the daily-noise + # failure this fingerprint exists to prevent, wearing a different hat. + if p.get("state", "broken") == "broken" ) return hashlib.sha256(repr(findings).encode()).hexdigest()[:16] From 0fea5c40bd519f830944bb5bdea27336884bb868 Mon Sep 17 00:00:00 2001 From: sudolulo Date: Tue, 14 Jul 2026 00:21:06 +0000 Subject: [PATCH 5/9] docs: record the second-audit findings in the changelog --- CHANGELOG.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index befe9c9..393f9ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,23 @@ worse than no alert, because one day it carries a security fix. - The staging-failure handler could **lose the original exception** if its own cleanup sweep raised. An error handler must not be able to lose the error. +- **A snapshot delete that returns cleanly is not proof that anything was deleted.** + The recursive sweep's fast path took the call's word for it and returned "no + survivors" — so `cleanup_task` read that as a clean sweep and removed the sidecar, + the only record the tree ever existed. Roughly 250 snapshots would have been orphaned + on every run, with nothing left able to find them, and the backup reporting success. + + This is not a hypothetical about a well-behaved API: iX has already gutted + `pool.snapshot.do_update` on master into a no-op whose body is commented out and + which returns `None`. A source check still sees the `def`; a runtime check still sees + a callable method. Only asking ZFS can tell. The sweep now confirms against ZFS, and + where it *cannot* confirm it keeps owning the tree rather than claiming success — a + false survivor self-heals on the next run, a lost record never does. + +- `_write_sidecar` **swallowed `OSError`**. The sidecar is the only thing that survives + a middlewared restart; failing to write it is not fatal, but it must never be + invisible. + ## v0.6.1 — 2026-07-13 ### Fixed From 8a41d7d7ef1cc70884fb15ae53d336645ac7498a Mon Sep 17 00:00:00 2001 From: sudolulo Date: Tue, 14 Jul 2026 00:38:45 +0000 Subject: [PATCH 6/9] =?UTF-8?q?fix:=20third=20audit=20=E2=80=94=20a=20cros?= =?UTF-8?q?s-tree=20dataset=20was=20omitted=20silently,=20and=20the=20bloc?= =?UTF-8?q?k=20tests=20passed=20on=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D1, the only cardinal-rule violation left. plan_staging scopes by dataset NAME, which is right (a dataset with no mountpoint cannot be scoped by path). But ZFS lets any dataset mount anywhere, so one from a DIFFERENT tree can sit inside the backup path: Tank/photos mountpoint=/mnt/Tap/apps/photos It holds data inside the path, and `zfs snapshot -r Tap@...` does NOT cover it — recursion follows the dataset tree, not the directory tree. It fell out of the name filter and vanished: not staged, not in `skipped`, no error. The backup reported SUCCESS with that data missing. Stock has the same blind spot but refuses the nested config outright; we are the ones relaxing that guard, so the hole is ours. It now raises. The test suite was the real weakness. apply.sh's injected blocks carry the highest-consequence logic in the project — the run_in_thread hop, the flavour selection, the finally-teardown, the re-raise — and were guarded only by substring greps. Two of them passed on COMMENTS: `assert "raise" in block` was satisfied by a comment reading "a cleanup that raises...", and `assert "cleanup_task" in block` by "cleanup_task gets logger=None". Deleting the actual re-raise (restic then backs up the UN-STAGED path — the silently-empty backup this module exists to prevent) and deleting the actual cleanup call from the finally (~250 orphans per run) both left the suite green. They are asserted structurally now, against the parsed block. Eleven regressions the audit found surviving now fail the suite, including: a swallowed staging failure, a missing teardown, an inverted flavour mapping, blocking work back on the asyncio event loop, the host's deleted get_dataset_recursive, query_filesystems quietly preferring the filtered middleware query, and a re-frozen `runner`/`sleep`/ `mounts_file` default (which would silently re-arm 19 tests reading the real mount table on the NAS). Also: _read_sidecar conflated "no sidecar" with "cannot read the sidecar", so cleanup_task took the empty branch and UNLINKED the only record of a tree it could not read. mounted_snapshots returned an empty set on error, silently switching off the GC's protection for snapshots a concurrent run is using. Both raise now. Verified on TrueNAS 26.0.0-BETA.1: zvol-orphan case 0 orphans, 292-dataset backup 0 orphans / 0 leaked mounts, byte-identical restore of a 4-deep child dataset. --- patch/truecloud_nested.py | 57 ++++++++- tests/test_apply_blocks.py | 209 +++++++++++++++++++++++++++++++-- tests/test_truecloud_nested.py | 171 +++++++++++++++++++++++++++ 3 files changed, 422 insertions(+), 15 deletions(-) diff --git a/patch/truecloud_nested.py b/patch/truecloud_nested.py index ab8e6b3..ee61083 100644 --- a/patch/truecloud_nested.py +++ b/patch/truecloud_nested.py @@ -452,16 +452,30 @@ def _write_sidecar(staging_root: str, snapshots, logger=None) -> None: ) -def _read_sidecar(staging_root: str): +def _read_sidecar(staging_root: str, logger=None): """Every snapshot tree a previous run recorded here. [] if none. Tolerates the old single-line format, which is just a one-element list. + + "There is no sidecar" and "I could not READ the sidecar" are different facts, and + conflating them is dangerous: `cleanup_task` reads an empty list as "nothing was + ever staged" and then REMOVES the sidecar -- destroying the only record of a tree + it could not read. FileNotFoundError is the ordinary case and stays quiet; any + other OSError is reported, and re-raised so no caller mistakes it for "empty". """ try: with open(sidecar_for(staging_root), encoding="utf-8") as fh: return [ln.strip() for ln in fh if ln.strip()] - except OSError: - return [] + except FileNotFoundError: + return [] # genuinely nothing recorded + except OSError as e: + if logger: + logger.error( + "truecloud-patch: could not READ the snapshot record %s (%r). Not " + "touching it -- it may name snapshots nothing else can find.", + sidecar_for(staging_root), e, + ) + raise def _remove_sidecar(staging_root: str) -> None: @@ -656,6 +670,43 @@ def plan_staging(base_dataset, base_mountpoint, path, snapshot_name, datasets, mounts.append((src, os.path.join(staging_root, os.path.relpath(mp, path)))) + # ── datasets INSIDE the path but OUTSIDE the snapshot's tree ───────────── + # + # The loop above scopes by dataset NAME, which is right: a dataset with no + # mountpoint cannot be scoped by path at all. But ZFS lets any dataset mount + # anywhere, so a dataset from a DIFFERENT tree -- even a different pool -- can + # sit inside the backup path: + # + # Tank/photos mountpoint=/mnt/Tap/apps/photos + # + # It holds data inside the path, so its absence is a hole in the backup. And + # `zfs snapshot -r Tap@...` does NOT cover it, because recursion follows the + # DATASET tree, not the directory tree -- so there is no snapshot of it to + # stage, and no way to capture it consistently with the rest. + # + # Before this check it was neither staged, nor reported in `skipped`, nor raised: + # it simply fell out of the name filter and vanished. The backup reported + # SUCCESS with that data missing, which is the precise failure this module + # exists to prevent. Stock has the same blind spot, but stock also REFUSES the + # nested config outright -- we are the ones relaxing that guard, so the hole is + # ours to close. + foreign = sorted( + ds.get("name", "") + for ds in datasets + if (mp := ds.get("properties", {}).get("mountpoint", {}).get("value", "")) + and mp.startswith(path_prefix) + and not ds.get("name", "").startswith(ds_prefix) + and ds.get("name", "") != base_dataset + ) + if foreign: + raise StagingError( + "dataset(s) outside " + repr(base_dataset) + " are mounted inside the " + "backup path and cannot be captured by its recursive snapshot: " + + ", ".join(repr(f) for f in foreign) + + ". Refusing to back up an incomplete tree -- move them, or back up " + "their own dataset separately." + ) + # Parents before children, so each mountpoint exists before we mount onto it. mounts.sort(key=lambda m: _depth(m[1])) return mounts, skipped diff --git a/tests/test_apply_blocks.py b/tests/test_apply_blocks.py index d2ec7e0..21c35ab 100644 --- a/tests/test_apply_blocks.py +++ b/tests/test_apply_blocks.py @@ -140,19 +140,60 @@ class TestSnapshotLeak: every path that creates one must also sweep the whole tree. """ - def test_staging_failure_deletes_the_snapshot_tree(self): - # On a staging failure, sync.py's `snapshot, local_path = await - # create_snapshot(...)` never completes, so its local `snapshot` stays - # None and its finally deletes nothing. We must sweep it ourselves. - block = extract_blocks()["SNAPSHOT_ASYNC"] - assert "except Exception:" in block - assert "delete_snapshot_tree" in block - assert "raise" in block + # The behaviour these once asserted as substrings -- the sweep, the re-raise, the + # teardown in the finally -- is now asserted STRUCTURALLY, against the parsed + # block: see TestTheStagingFailurePathReallyReRaises and + # TestTheSyncBlockAlwaysTearsDown. As substring checks they were satisfied by + # COMMENTS ("a cleanup that raises...", "cleanup_task gets logger=None"), so + # deleting the actual `raise` and the actual cleanup call both left the suite + # green -- reinstating a silently-empty backup and ~250 orphans per run. - def test_sync_block_cleans_up_on_every_path(self): - block = extract_blocks()["SYNC_ASYNC"] - assert "finally:" in block - assert "cleanup_task" in block + def test_the_snapshot_block_still_owns_the_snapshot_when_not_staging(self): + # The TrueNAS 26 zvol/legacy orphan: stock decides `recursive` by its own rule + # (path == mountpoint) and deletes only the parent, so we must record the + # snapshot even on the path where we stage nothing. + for name in ("SNAPSHOT_ASYNC", "SNAPSHOT_SYNC"): + stage = functions(tree_of(name), "_tc_stage")[0] + assert calls_to(stage, "_tc_nested.own_snapshot"), ( + f"{name} hands an unstaged snapshot back to stock, whose delete is " + f"non-recursive -- every zvol/legacy child is orphaned, every run" + ) + + def test_the_staging_plan_is_enumerated_from_ZFS(self): + for name in ("SNAPSHOT_ASYNC", "SNAPSHOT_SYNC"): + stage = functions(tree_of(name), "_tc_stage")[0] + assert calls_to(stage, "_tc_nested.query_filesystems"), ( + "the staging plan must come from query_filesystems() (which reads ZFS " + "unfiltered); middleware's query hides ix-apps/*, .system/*, .ix-virt/*" + ) + assert not calls_to(stage, "middleware.call_sync"), ( + "the block calls middleware directly again -- its dataset/snapshot " + "queries are FILTERED and silently omit 84 of 270 datasets" + ) + + def test_the_vendored_helper_is_used_not_the_host_module(self): + # TrueNAS 26 DELETED get_dataset_recursive from plugins/cloud/snapshot.py, so + # calling it out of the host module's namespace is a NameError there. + for name in ("SNAPSHOT_ASYNC", "SNAPSHOT_SYNC"): + stage = functions(tree_of(name), "_tc_stage")[0] + assert calls_to(stage, "_tc_nested.get_dataset_recursive"), ( + "must call OUR vendored copy: TrueNAS 26 deleted the host's" + ) + + def test_datasets_are_enumerated_AFTER_the_snapshot(self): + # A dataset created between the listing and the snapshot would be captured by + # the recursive snapshot but missing from the staging plan -- silently omitted. + # Read afterwards, it instead trips plan_staging's probe and fails loudly. + for name in ("SNAPSHOT_ASYNC", "SNAPSHOT_SYNC"): + src = extract_blocks()[name] + code = "\n".join( + ln for ln in src.splitlines() if not ln.lstrip().startswith("#") + ) + # _tc_stage receives `snapshot` as a parameter -- i.e. it is taken by the + # caller, before any of this runs. If the enumeration ever moves ahead of + # create_snapshot it can only do so by leaving _tc_stage. + assert "def _tc_stage(middleware, path, name, snapshot, snap_path)" in code + assert "query_filesystems" in code def test_crud_block_is_scoped_to_cloud_backup(): @@ -457,3 +498,147 @@ class TestOnlyOurOwnTasksAreTouched: assert present, "found no middleware interaction at all -- the test is vacuous" for call in present: assert gate < block.index(call), f"{call} runs before the cloud_backup gate" + + +# ── structural assertions ──────────────────────────────────────────────────── +# +# `assert "raise" in block` was TRUE because a COMMENT in the block says "a cleanup +# that raises would replace the original exception". `assert "cleanup_task" in block` +# was TRUE because a comment says "cleanup_task gets logger=None". Deleting the actual +# `raise`, and deleting the actual cleanup call from the `finally`, both left the suite +# green -- while reinstating, respectively, a silently-empty backup and ~250 orphaned +# snapshots per run. +# +# A test that a comment can satisfy is not a test. These parse the block and assert on +# the CODE. + +def tree_of(name): + return ast.parse(textwrap.dedent(extract_blocks()[name])) + + +def functions(tree, name): + return [ + n for n in ast.walk(tree) + if isinstance(n, ast.FunctionDef | ast.AsyncFunctionDef) and n.name == name + ] + + +def calls_to(node, dotted): + """Every Call in `node` whose callee renders as `dotted` (e.g. a.b.c).""" + out = [] + for n in ast.walk(node): + if isinstance(n, ast.Call): + try: + if ast.unparse(n.func) == dotted: + out.append(n) + except Exception: # noqa: BLE001 + pass + return out + + +class TestTheStagingFailurePathReallyReRaises: + """If staging fails and we swallow it, restic backs up the UN-STAGED path. + + That is the silently-empty backup this entire module exists to prevent: stock + points the tool at the parent's `.zfs/snapshot/`, where child datasets are + invisible. The exception MUST propagate. + """ + + @pytest.mark.parametrize("name", ["SNAPSHOT_ASYNC", "SNAPSHOT_SYNC"]) + def test_the_handler_sweeps_the_snapshot_and_re_raises(self, name): + stage = functions(tree_of(name), "_tc_stage") + assert stage, "_tc_stage is gone" + + handlers = [ + h for t in ast.walk(stage[0]) if isinstance(t, ast.Try) + for h in t.handlers + ] + assert handlers, "the staging failure handler is gone" + + sweeps = any(calls_to(h, "_tc_nested.delete_snapshot_tree") for h in handlers) + assert sweeps, ( + "a staging failure no longer sweeps the snapshot. sync.py's `snapshot` " + "local stays None, so ITS finally deletes nothing -- the whole tree leaks " + "on every failed run." + ) + + # A bare `raise` directly in the handler body -- not one nested inside the + # defensive try/except that wraps the sweep. + reraises = any( + any(isinstance(s, ast.Raise) and s.exc is None for s in h.body) + for h in handlers + ) + assert reraises, ( + "the staging failure is SWALLOWED. restic then runs against the un-staged " + "path and uploads a near-empty tree, reporting SUCCESS." + ) + + +class TestTheSyncBlockAlwaysTearsDown: + """The teardown is what unmounts the staging tree and sweeps the snapshot. + + It must run on EVERY exit from restic_backup -- success, failure, or exception -- + or the bind mounts pin the snapshot and the tree is orphaned. + """ + + @pytest.mark.parametrize("name", ["SYNC_ASYNC", "SYNC_SYNC"]) + def test_cleanup_runs_in_a_finally(self, name): + fns = functions(tree_of(name), "restic_backup") + assert fns, "the restic_backup wrapper is gone" + + tries = [t for t in ast.walk(fns[0]) if isinstance(t, ast.Try) and t.finalbody] + assert tries, "restic_backup no longer has a try/finally" + + cleans = any( + "cleanup_task" in ast.unparse(stmt) + for t in tries for stmt in t.finalbody + ) + assert cleans, ( + "cleanup_task is not called in the finally. The staging tree is never torn " + "down, its bind mounts pin the snapshot, and ~250 snapshots leak per run." + ) + + +class TestTheBlockingWorkNeverRunsOnTheEventLoop: + """`zfs list` and `call_sync` are BLOCKING. On <=25.10 these blocks are async. + + Running them directly on middlewared's event loop stalls the whole daemon. + """ + + @pytest.mark.parametrize("name,fn", [ + ("SNAPSHOT_ASYNC", "create_snapshot"), + ("SYNC_ASYNC", "restic_backup"), + ]) + def test_the_async_flavour_hops_to_a_thread(self, name, fn): + fns = functions(tree_of(name), fn) + assert fns and isinstance(fns[0], ast.AsyncFunctionDef) + assert calls_to(fns[0], "middleware.run_in_thread"), ( + f"{name}.{fn} does the blocking work on the asyncio event loop" + ) + + @pytest.mark.parametrize("name,fn", [ + ("SNAPSHOT_SYNC", "create_snapshot"), + ("SYNC_SYNC", "restic_backup"), + ]) + def test_the_sync_flavour_does_not(self, name, fn): + # On 26 stock already runs this in the thread pool; hopping again would be + # wrong (and there is no event loop to protect). + fns = functions(tree_of(name), fn) + assert fns and isinstance(fns[0], ast.FunctionDef) + assert not calls_to(fns[0], "middleware.run_in_thread") + + +def test_the_flavour_mapping_is_not_inverted(): + # `_snapshot_block = SNAPSHOT_ASYNC if _flavour else SNAPSHOT_SYNC` -- inverting it + # injects an async wrapper on 26 (a coroutine gets unpacked as a tuple) or a sync + # one on 25.10 (the event loop blocks). Every nested backup breaks, both ways. + with open(APPLY_SH, encoding="utf-8") as fh: + code = " ".join( + ln for ln in fh.read().splitlines() if not ln.lstrip().startswith("#") + ) + code = re.sub(r"\s+", " ", code) # the assignments are space-aligned + for block in ("SNAPSHOT", "CRUD", "SYNC"): + assert f"{block}_ASYNC if _flavour else {block}_SYNC" in code, ( + f"the {block} flavour mapping is missing or inverted: _flavour is True for " + f"an ASYNC middleware, so it must select {block}_ASYNC" + ) diff --git a/tests/test_truecloud_nested.py b/tests/test_truecloud_nested.py index 7167b0a..ce23e21 100644 --- a/tests/test_truecloud_nested.py +++ b/tests/test_truecloud_nested.py @@ -14,12 +14,18 @@ Two rules are under test above all else: import os import sys +import time import pytest sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "patch")) import truecloud_nested as tn # noqa: E402 + +#: Captured at import, BEFORE the autouse fixture patches time.sleep -- +#: otherwise the seam check below compares a frozen default against the +#: fixture's own stub and silently matches nothing. +_REAL_SLEEP = time.sleep from truecloud_nested import ( # noqa: E402 StagingError, apply_plan, @@ -1698,3 +1704,168 @@ class TestTheServiceIsResolvedLazily: with pytest.raises(StagingError): snaps.delete("Tap@snap") # ...only the MUTATION refuses + + +class TestADatasetFromAnotherTreeMountedInsideThePath: + """ZFS lets any dataset mount anywhere. A backup path is a DIRECTORY, not a tree. + + Tank/photos mountpoint=/mnt/Tap/apps/photos + + That dataset holds data inside the backed-up path, so its absence is a hole. And + `zfs snapshot -r Tap@...` does NOT cover it -- recursion follows the DATASET tree, + not the directory tree -- so there is no snapshot of it to stage. + + It used to be scoped out by the dataset-NAME filter and then vanish: not staged, + not in `skipped`, no error. The backup reported SUCCESS with the data missing, + which is the cardinal-rule failure. + """ + + def test_it_is_refused_loudly_not_omitted_silently(self): + foreign = DATASETS + [ds("Tank/photos", "/mnt/Tap/apps/photos")] + with pytest.raises(StagingError, match="Tank/photos"): + plan(datasets=foreign) + + def test_the_error_explains_why_it_cannot_be_captured(self): + foreign = DATASETS + [ds("Tank/photos", "/mnt/Tap/apps/photos")] + with pytest.raises(StagingError, match="recursive snapshot"): + plan(datasets=foreign) + + def test_a_foreign_dataset_OUTSIDE_the_path_is_still_ignored(self): + # Only datasets inside the backed-up path matter. Everything else on the box + # is none of our business, and reporting it would bury the ones that are. + elsewhere = DATASETS + [ds("Tank/photos", "/mnt/Tank/photos")] + mounts, skipped = plan(datasets=elsewhere) + assert len(mounts) == 6 + assert skipped == [] + + +class TestTheSeamsStayLateBound: + """A default argument is frozen into `__defaults__` at def time. + + `runner=_run`, `mounts_file="/proc/self/mounts"` and `sleep=time.sleep` were all + written that way, so `never_touch_the_real_system` could not intercept them: 19 + tests read the REAL mount table -- one matching name away from running a real + `umount` on the NAS -- and the retry loop really slept for 20 seconds. + + Re-freezing any of them silently re-arms that. The fixture cannot notice, because + it is the thing being bypassed. So assert it directly. + """ + + def test_no_system_seam_is_frozen_into_a_default(self): + import inspect + + offenders = [] + for name in dir(tn): + fn = getattr(tn, name) + if not inspect.isfunction(fn): + continue + for default in fn.__defaults__ or (): + if default is tn._run or default is _REAL_SLEEP: + offenders.append(f"{name}() freezes {getattr(default, '__name__', default)!r}") + if default == "/proc/self/mounts": + offenders.append(f"{name}() freezes the real mount table") + assert not offenders, ( + "these seams are frozen into __defaults__, so no test can intercept them " + "and they will reach the real system:\n " + "\n ".join(offenders) + + "\nUse the late-bound idiom: `runner=None` + `runner = runner or _run`." + ) + + +class TestOwnSnapshotCleansUpBeforeItBuilds: + def test_it_tears_down_a_crashed_run_before_recording_its_own( + self, tmp_path, monkeypatch + ): + # A previous run may have died mid-flight. Its bind mounts still pin its + # snapshots (so they refuse to delete with EBUSY), and stacking a new staging + # tree on top of a stale one is how mounts leak. + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + order = [] + monkeypatch.setattr(tn, "teardown", lambda *a, **kw: order.append("teardown") or []) + real_write = tn._write_sidecar + monkeypatch.setattr( + tn, "_write_sidecar", + lambda *a, **kw: order.append("write") or real_write(*a, **kw), + ) + mw = FakeMiddleware(["Tap@new"]) + tn.own_snapshot(mw, "cloud_backup-5", "Tap@new", + list_snapshots=mw.list_snapshots) + assert order == ["teardown", "write"], ( + "own_snapshot must tear down a crashed run's tree BEFORE recording its own" + ) + + +class TestStagingRefusesUpFrontIfItCouldNotSweep: + def test_a_middleware_with_no_usable_delete_refuses_before_restic_runs( + self, tmp_path, monkeypatch + ): + # We are about to pin a recursive snapshot with bind mounts. If we could never + # sweep it, the honest move is to fail NOW -- not after restic has uploaded and + # the snapshot is already stuck. + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + + class Hopeless(FakeMiddleware): + def get_service(self, name): + raise KeyError(name) + + with pytest.raises(StagingError, match="no usable snapshot delete"): + tn.stage_nested( + Hopeless(), "/mnt/Tap", "Tap@snap", "Tap", "/mnt/Tap", + "cloud_backup-5", DATASETS, list_snapshots=lambda _d: [], + ) + + +class TestQueryFilesystemsIgnoresMiddlewareEntirely: + """`middleware` is accepted and IGNORED. That is the whole point of the function. + + Reintroducing `if middleware: return middleware.call_sync("pool.dataset.query")` is + a two-line change that looks like an optimisation and reinstates the bug that + shipped once: middleware's query hides ix-apps/*, .system/*, .ix-virt/* -- 84 of + 270 datasets on the real pool, including live app data -- and they would be omitted + from the staging plan without even reaching `skipped`. + """ + + def test_passing_a_middleware_does_not_make_it_ask_middleware(self): + class Loud(FakeMiddleware): + def call_sync(self, method, *args): + raise AssertionError( + f"query_filesystems asked middleware ({method}). Its dataset query " + f"is FILTERED; enumeration must come from ZFS." + ) + + class R: + returncode = 0 + stdout = "Tap\t/mnt/Tap\tyes\nTap/ix-apps\t/mnt/Tap/ix-apps\tyes\n" + stderr = "" + + rows = tn.query_filesystems(Loud(), runner=lambda cmd: R()) + assert [r["name"] for r in rows] == ["Tap", "Tap/ix-apps"] + + +class TestAnUnreadableZfsDuringTheByNameSweep: + def test_the_tree_is_still_owned_when_we_cannot_check(self): + # Recursive delete fails AND `zfs list` fails. We cannot know what is gone, so + # we must keep owning all of it: cleanup_task then KEEPS the sidecar and the + # next run reclaims. Reporting a clean sweep here drops the only record. + class NoRecursive(FakeMiddleware): + def call_sync(self, method, *args): + self.calls.append((method, args)) + if method.endswith(".delete") and len(args) > 1: + raise RuntimeError("recursive delete unavailable") + return None # individual deletes "succeed" silently + + calls = {"n": 0} + + def flaky(dataset): + calls["n"] += 1 + if calls["n"] == 1: + return ["Tap@snap", "Tap/apps@snap"] # the tree, for planning + raise tn.ZfsError("pool I/O is currently suspended") # ...then ZFS dies + + mw = NoRecursive(["Tap@snap", "Tap/apps@snap"]) + survivors = tn.delete_snapshot_tree( + mw, "Tap@snap", list_snapshots=flaky, sleep=lambda _s: None, + ) + assert sorted(survivors) == ["Tap/apps@snap", "Tap@snap"], ( + "with ZFS unreadable we cannot confirm anything was deleted. Returning [] " + "makes cleanup_task drop the sidecar and orphans the tree forever." + ) From 677c90481c5d6278f55af66181319f1725d17657 Mon Sep 17 00:00:00 2001 From: sudolulo Date: Tue, 14 Jul 2026 00:39:40 +0000 Subject: [PATCH 7/9] docs: record the third-audit findings in the changelog --- CHANGELOG.md | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 393f9ee..b0055e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -109,7 +109,24 @@ worse than no alert, because one day it carries a security fix. - `_write_sidecar` **swallowed `OSError`**. The sidecar is the only thing that survives a middlewared restart; failing to write it is not fatal, but it must never be - invisible. + invisible. `_read_sidecar` had the mirror bug — it conflated "there is no sidecar" + with "I could not read the sidecar", and `cleanup_task` then took the empty branch + and **unlinked the only record** of a tree it had failed to read. + +- **A dataset from another tree, mounted inside the backup path, was omitted + silently.** The staging plan scopes by dataset *name*, which is correct — a dataset + with no mountpoint cannot be scoped by path at all. But ZFS lets any dataset mount + anywhere, so one from an unrelated tree can sit inside the path: + + Tank/photos mountpoint=/mnt/Tap/apps/photos + + It holds data inside the backed-up path, and `zfs snapshot -r Tap@…` does **not** + cover it: recursion follows the dataset tree, not the directory tree. So there is no + snapshot of it to stage, and no way to capture it consistently with the rest. It fell + out of the name filter and vanished — not staged, not in `skipped`, no error, backup + green. Stock has the same blind spot, but stock also refuses the nested config + outright; this patch is what relaxes that guard, so the hole is this patch's to close. + It now refuses, and names the offending datasets. ## v0.6.1 — 2026-07-13 ### Fixed From 086b20ed2326d6c1e7d4c626ed95a86ed0d4e60f Mon Sep 17 00:00:00 2001 From: sudolulo Date: Tue, 14 Jul 2026 01:34:13 +0000 Subject: [PATCH 8/9] =?UTF-8?q?fix:=20fourth=20audit=20=E2=80=94=20two=20r?= =?UTF-8?q?egressions=20from=20the=20last=20fix,=20and=20the=20boot=20pref?= =?UTF-8?q?light=20had=20no=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of these were mine, from the previous round. - mounted_snapshots still swallowed OSError. I said I had fixed it and had not: the edit never matched, and I did not read it back. With the mount table unreadable the GC loses its in-use protection entirely and can destroy the snapshots of a backup that is still uploading (a first upload easily outlives the 1h age floor). It raises now, and both behaviours are tested. - The foreign-dataset check added last round had two bugs of its own. It ignored `mounted`, so a locked/encrypted dataset from a sibling tree turned a working nightly backup into a permanent failure — it belongs in `skipped`, exactly as an in-tree one does. And it tested `mp.startswith(path + "/")`, so a foreign dataset mounted EXACTLY at the backup path slipped through — the very hole the check was added to close, one character wide, and the worse case of the two because it SHADOWS the base dataset's own directory. - _read_sidecar's new raise broke cleanup_all, which is what recover.sh and uninstall.sh call — i.e. the code that must work when the box is ALREADY stuck. One unreadable sidecar aborted it before it unmounted anything, leaving the staging tree mounted, which pins the snapshots, which is the state recover.sh exists to escape. It now reports and carries on — and does not delete a record it could not read. - compat could report a FALSE OK: `defined` was collected by walking the whole file, so any function named `delete` anywhere in it — on an unrelated class, or nested inside another method — satisfied "this namespace defines delete". The runtime is stricter (a plugin class on the service's MRO), so the two could disagree in the ok direction. compat now looks in the class that declares the namespace. Same question on both sides, which is what pick_snapshot_service's docstring has been claiming all along. - apply.sh's compat preflight — the guard that refuses to patch a middleware whose assumptions no longer hold, on every boot, on a live NAS — had no test at all. It could be turned into a no-op eight different ways with the suite still green. The SHIPPED heredoc is now extracted and driven directly against fake verdicts. Also pinned: the Tap/Tap2 prefix collisions (a sweep that treats "Tap2/data@snap" as part of Tap's tree DESTROYS another pool's snapshot), and the GC's in_use wiring. 355 tests. Verified on TrueNAS 26.0.0-BETA.1: 292-dataset backup, 0 orphans, 0 leaked mounts, byte-identical restore of a 4-deep child dataset. --- patch/truecloud_nested.py | 63 +++++++++++--- tests/test_apply_blocks.py | 109 ++++++++++++++++++++++++ tests/test_truecloud_nested.py | 147 +++++++++++++++++++++++++++++++++ tools/compat.py | 80 +++++++++++------- 4 files changed, 360 insertions(+), 39 deletions(-) diff --git a/patch/truecloud_nested.py b/patch/truecloud_nested.py index ee61083..095ca92 100644 --- a/patch/truecloud_nested.py +++ b/patch/truecloud_nested.py @@ -690,19 +690,36 @@ def plan_staging(base_dataset, base_mountpoint, path, snapshot_name, datasets, # exists to prevent. Stock has the same blind spot, but stock also REFUSES the # nested config outright -- we are the ones relaxing that guard, so the hole is # ours to close. - foreign = sorted( - ds.get("name", "") - for ds in datasets - if (mp := ds.get("properties", {}).get("mountpoint", {}).get("value", "")) - and mp.startswith(path_prefix) - and not ds.get("name", "").startswith(ds_prefix) - and ds.get("name", "") != base_dataset - ) + foreign = [] + for ds in datasets: + name = ds.get("name", "") + if name.startswith(ds_prefix) or name == base_dataset: + continue # in-tree: handled above + + props = ds.get("properties", {}) + mp = props.get("mountpoint", {}).get("value", "") + # `mp == path` as well as below it. A foreign dataset mounted exactly AT the + # backup path is the same hole -- and it is worse, because it shadows the base + # dataset's own directory, so we would stage what is hidden underneath instead + # of the data actually visible there. + if not mp or (mp != path and not mp.startswith(path_prefix)): + continue # not inside the backed-up path + + if props.get("mounted", {}).get("value", "yes") == "no": + # An unmounted (locked/encrypted) dataset contributes nothing to the live + # tree, so its absence is not a hole -- exactly as for an in-tree one, 40 + # lines above. Raising here would turn a working nightly backup into a + # permanent failure the first time somebody locked a dataset. + skipped.append((name, "dataset is not mounted (locked/encrypted?)")) + continue + + foreign.append(name) + if foreign: raise StagingError( "dataset(s) outside " + repr(base_dataset) + " are mounted inside the " "backup path and cannot be captured by its recursive snapshot: " - + ", ".join(repr(f) for f in foreign) + + ", ".join(repr(f) for f in sorted(foreign)) + ". Refusing to back up an incomplete tree -- move them, or back up " "their own dataset separately." ) @@ -1118,7 +1135,12 @@ def mounted_snapshots(mounts_file=None): if "@" in dev: live.add(dev.replace("\\040", " ")) except OSError: - return set() + # An empty set says "nothing is mounted", which silently switches OFF the GC's + # protection for snapshots a CONCURRENT run is using -- leaving only the age + # floor between us and destroying a snapshot out from under a backup that is + # still uploading. If we cannot read the mount table we do not KNOW what is in + # use, and must not pretend we do. + raise return live @@ -1418,8 +1440,21 @@ def cleanup_all(base=None, runner=None, mounts_file=None, # Report orphaned snapshots BEFORE removing the sidecars that name them -- # a sidecar is the only record that an interrupted run's snapshot tree (one # snapshot per descendant dataset) is still on disk. + unreadable = set() for sc in sorted(glob_fn(os.path.join(base, "*.snapshot"))): - for snap in read_sidecar(sc[: -len(".snapshot")]): + try: + recorded = read_sidecar(sc[: -len(".snapshot")]) + except OSError as e: + unreadable.add(sc) + # REPORT it and carry on. This function's job is to get the mounts off, and + # it is called precisely when the box is already in a bad state + # (recover.sh, uninstall.sh). Letting one unreadable sidecar abort the run + # would leave the staging tree mounted -- which pins the snapshots, which is + # the exact situation the caller is trying to escape. + lines.append(f" WARNING: could not read the snapshot record {sc} ({e}).") + lines.append(" It may name snapshots nothing else can find.") + continue + for snap in recorded: lines.append(f" NOTE: an interrupted backup left snapshot '{snap}' behind.") lines.append(f" Remove it and its children: zfs destroy -r '{snap}'") @@ -1435,6 +1470,12 @@ def cleanup_all(base=None, runner=None, mounts_file=None, if not errors: for sc in glob_fn(os.path.join(base, "*.snapshot")): + if sc in unreadable: + # Do NOT delete a record we could not read. We have no idea what it + # names, and it may be the only thing that knows those snapshots exist. + # Removing it here would be the very bug the read guard was added for, + # committed by the cleanup path instead of the backup path. + continue with contextlib.suppress(OSError): os.unlink(sc) with contextlib.suppress(OSError): diff --git a/tests/test_apply_blocks.py b/tests/test_apply_blocks.py index 21c35ab..c28cd84 100644 --- a/tests/test_apply_blocks.py +++ b/tests/test_apply_blocks.py @@ -642,3 +642,112 @@ def test_the_flavour_mapping_is_not_inverted(): f"the {block} flavour mapping is missing or inverted: _flavour is True for " f"an ASYNC middleware, so it must select {block}_ASYNC" ) + + +# ── the compat preflight ───────────────────────────────────────────────────── +# +# This is the guard that stands between a broken middleware and a live NAS: at every +# boot, apply.sh checks the patch's assumptions against the middlewared actually +# installed, and REFUSES to apply a module whose assumptions no longer hold. +# +# It had no test. An audit turned it into a no-op eight different ways -- `verdict()` +# always returning 'ok', the broken branch never firing, the kill switch never honoured +# -- and the suite stayed green every time. The most consequential safety net in the +# project was unguarded. + +def preflight_heredoc(): + """The preflight's Python, lifted out of apply.sh and made runnable. + + Extracted, not reimplemented: a reimplementation would happily pass while the + SHIPPED preflight stayed broken, which is exactly the failure being guarded. + """ + with open(APPLY_SH, encoding="utf-8") as fh: + sh = fh.read() + # Line-based: the compat heredoc opens with `<<'PYEOF'` on the _tc_compat line and + # closes at the next bare PYEOF. (A regex that matched `<< 'PYEOF'` silently found + # the OTHER heredoc and ran a different script entirely.) + lines = sh.splitlines() + start = next( + i for i, ln in enumerate(lines) + if ln.startswith("_tc_compat=$(") and "<<'PYEOF'" in ln + ) + end = next(i for i in range(start + 1, len(lines)) if lines[i].strip() == "PYEOF") + m = "\n".join(lines[start + 1:end]) + assert m, "could not find the compat preflight heredoc in apply.sh" + return m + + +def run_preflight(result, tmp_path): + """Run the SHIPPED preflight against a fake compat.check_tree result. + + The heredoc does `import sys`, so a fake `sys` in the namespace is immediately + rebound to the real module -- drive the real one instead. + """ + import contextlib + import io + import sys + import types + + src = preflight_heredoc() + fake = types.ModuleType("compat") + fake.check_tree = lambda _mw: result + + saved_mod = sys.modules.get("compat") + saved_argv = sys.argv + sys.modules["compat"] = fake + sys.argv = ["x", "/patch", "/mw", str(tmp_path / "compat.json")] + + buf = io.StringIO() + try: + with contextlib.redirect_stdout(buf): + exec(compile(src, "apply.sh:preflight", "exec"), {"__name__": "__main__"}) # noqa: S102 + except SystemExit: + pass + finally: + sys.argv = saved_argv + if saved_mod is not None: + sys.modules["compat"] = saved_mod + else: + sys.modules.pop("compat", None) + return buf.getvalue().splitlines() + + +def _mod(ok=True, native=False, unknown=False, problems=()): + return {"ok": ok, "native": native, "unknown": unknown, "problems": list(problems)} + + +class TestTheBootPreflightRefusesABrokenMiddleware: + def test_a_healthy_tree_is_ok(self, tmp_path): + out = run_preflight({"providers": _mod(), "nested": _mod()}, tmp_path) + assert out[:2] == ["ok", "ok"] + + def test_a_broken_module_is_reported_broken(self, tmp_path): + out = run_preflight({ + "providers": _mod(), + "nested": _mod(ok=False, problems=[ + {"id": "x", "detail": "gone", "why": "orphans every run"}, + ]), + }, tmp_path) + assert "broken" in out, ( + "the preflight did not report a module whose assumptions FAILED. It would " + "be injected into a middleware it does not fit -- broken backups, " + "discovered at restore time." + ) + + def test_a_module_that_went_NATIVE_is_also_not_applied(self, tmp_path): + # 'native' answers "do we still need it?", 'ok' answers "is it safe to inject?". + # Applying a module TrueNAS now implements itself is not safe either. + out = run_preflight({ + "providers": _mod(), + "nested": _mod(ok=False, native=True), + }, tmp_path) + assert "broken" in out + + def test_an_UNKNOWN_verdict_is_not_reported_as_broken(self, tmp_path): + # A network error or an unreadable file is not iX deleting our symbols. Calling + # it broken would switch a working module off on a healthy box. + out = run_preflight({ + "providers": _mod(unknown=True), + "nested": _mod(unknown=True), + }, tmp_path) + assert "broken" not in out diff --git a/tests/test_truecloud_nested.py b/tests/test_truecloud_nested.py index ce23e21..69c86cd 100644 --- a/tests/test_truecloud_nested.py +++ b/tests/test_truecloud_nested.py @@ -1869,3 +1869,150 @@ class TestAnUnreadableZfsDuringTheByNameSweep: "with ZFS unreadable we cannot confirm anything was deleted. Returning [] " "makes cleanup_task drop the sidecar and orphans the tree forever." ) + + +class TestTheSnapshotRecordIsNeverLostToAnUnreadableFile: + """"There is no sidecar" and "I could not READ the sidecar" are different facts. + + Conflating them let `cleanup_task` take its `if not pinned` branch and UNLINK the + only record of a tree it had just failed to read. + """ + + def test_an_unreadable_sidecar_raises_rather_than_reading_as_empty(self, tmp_path): + sc = tmp_path / "cloud_backup-5.snapshot" + sc.write_text("Tap@snap\n") + sc.chmod(0) + try: + with pytest.raises(OSError): + tn._read_sidecar(str(tmp_path / "cloud_backup-5")) + finally: + sc.chmod(0o600) + + def test_a_missing_sidecar_is_simply_empty(self, tmp_path): + assert tn._read_sidecar(str(tmp_path / "nope")) == [] + + def test_cleanup_all_still_UNMOUNTS_when_a_sidecar_cannot_be_read(self, tmp_path): + # cleanup_all is what recover.sh and uninstall.sh call, i.e. it runs precisely + # when the box is already stuck. Its job is to get the mounts off. Aborting on + # one unreadable sidecar leaves the staging tree mounted -- which pins the + # snapshots, which is the state the caller is trying to escape. + sc = tmp_path / "cloud_backup-5.snapshot" + sc.write_text("Tap@snap\n") + sc.chmod(0) + + unmounted = [] + + class R: + returncode = 0 + stdout = "" + stderr = "" + + def runner(cmd): + unmounted.append(cmd) + return R() + + mounts = tmp_path / "mounts" + mounts.write_text(f"Tap@s {tmp_path}/cloud_backup-5 zfs ro 0 0\n") + out, _errors = tn.cleanup_all( + base=str(tmp_path), runner=runner, mounts_file=str(mounts), + glob_fn=lambda pat: [str(sc)], + ) + + assert sc.exists(), ( + "cleanup_all DELETED the sidecar it had just failed to read. It has no idea " + "what that file names, and it may be the only record those snapshots exist." + ) + sc.chmod(0o600) + + assert any("umount" in " ".join(c) for c in unmounted), ( + "cleanup_all stopped at the unreadable sidecar and never unmounted. The " + "staging tree stays mounted, pinning the snapshots -- which is exactly the " + "state recover.sh exists to escape." + ) + assert "could not read the snapshot record" in "\n".join(out).lower() + + +class TestTheMountTableIsNeverGuessedAt: + def test_an_unreadable_mount_table_raises_rather_than_saying_nothing_is_mounted(self): + # Returning an empty set means "no snapshot is in use", which silently switches + # OFF the GC's only protection for a CONCURRENTLY RUNNING backup's snapshots. + # A first upload can easily run past the 1h age floor. + with pytest.raises(OSError): + tn.mounted_snapshots(mounts_file="/nonexistent/mounts") + + def test_the_GC_actually_passes_in_use_through(self, tmp_path, monkeypatch): + # The `in_use` guard is well tested as a pure function -- but nothing checked + # that gc_stale_snapshots WIRES it in. Deleting the argument passed the suite. + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + import datetime as dt + + old = "Tap/apps/x@cloud_backup-5-20260713030000" + mounts = tmp_path / "mounts" + mounts.write_text(f"{old} /some/where zfs ro 0 0\n") # ...it is MOUNTED + + mw = FakeMiddleware(["Tap@cloud_backup-5-20260714115900", old]) + remaining = tn.gc_stale_snapshots( + 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=mw.list_snapshots, + ) + assert remaining == [] + assert old in mw.snapshots, ( + "the GC destroyed a snapshot that is still MOUNTED -- i.e. one a " + "concurrently running backup is reading from" + ) + + +class TestPrefixCollisionsBetweenPools: + """`Tap` and `Tap2` are different pools. String prefixes do not know that.""" + + def test_the_sweep_does_not_touch_a_similarly_named_pool(self): + mw = FakeMiddleware(["Tap@snap", "Tap/apps@snap", "Tap2/data@snap"]) + tn.delete_snapshot_tree(mw, "Tap@snap", list_snapshots=lambda _d: list(mw.snapshots)) + assert mw.snapshots == ["Tap2/data@snap"], ( + "the sweep destroyed a snapshot in Tap2, a DIFFERENT POOL, because 'Tap2' " + "starts with 'Tap'" + ) + + def test_the_planner_does_not_stage_a_similarly_named_dataset(self): + noisy = DATASETS + [ds("Tap2/apps", "/mnt/Tap2/apps")] + mounts, skipped = plan(datasets=noisy) + assert len(mounts) == 6 + assert skipped == [] + + def test_a_similarly_named_MOUNTPOINT_does_not_trip_the_foreign_check(self): + # /mnt/Tap2/x is not inside /mnt/Tap. + noisy = DATASETS + [ds("Tank/x", "/mnt/Tap2/x")] + mounts, skipped = plan(datasets=noisy) + assert len(mounts) == 6 + + def test_Tap2_is_not_a_child_of_Tap_even_when_mounted_inside_it(self): + # The one that actually bites: `Tap2/x` starts with the STRING "Tap", so a + # scope test that forgets the trailing "/" treats it as an in-tree descendant + # and STAGES it -- from a snapshot `zfs snapshot -r Tap@...` never took, since + # Tap2 is a different pool. It must be seen as FOREIGN and refused. + noisy = DATASETS + [ds("Tap2/x", "/mnt/Tap/apps/x")] + with pytest.raises(StagingError, match="Tap2/x"): + plan(datasets=noisy) + + +class TestTheForeignCheckDoesNotBreakWorkingConfigs: + def test_an_UNMOUNTED_foreign_dataset_is_skipped_not_refused(self): + # A locked/encrypted dataset contributes nothing to the live tree, so its + # absence is not a hole -- exactly as for an in-tree one. Raising here would + # turn a working nightly backup into a permanent failure the first time + # somebody locked a dataset. + datasets = DATASETS + [ + ds("Tank/photos", "/mnt/Tap/apps/photos", mounted="no"), + ] + mounts, skipped = plan(datasets=datasets) + assert len(mounts) == 6 + assert ("Tank/photos", "dataset is not mounted (locked/encrypted?)") in skipped + + def test_a_foreign_dataset_mounted_EXACTLY_AT_the_path_is_refused(self): + # One character wide: `mp.startswith(path + "/")` misses `mp == path`. And this + # case is the worse one -- it SHADOWS the base dataset's own directory, so we + # would stage what is hidden underneath instead of the data actually there. + datasets = DATASETS + [ds("Tank/x", "/mnt/Tap")] + with pytest.raises(StagingError, match="Tank/x"): + plan(datasets=datasets) diff --git a/tools/compat.py b/tools/compat.py index e9e51eb..8359786 100644 --- a/tools/compat.py +++ b/tools/compat.py @@ -301,37 +301,61 @@ def check_call(c: MiddlewareCall, src: str | None, except SyntaxError as e: return "unknown", f"{path} does not parse: {e}" - # namespace = 'zfs.snapshot' on some Service class in this file... - namespaces = { - n.value.value - for n in ast.walk(tree) - if isinstance(n, ast.Assign) - and isinstance(n.value, ast.Constant) - and isinstance(n.value.value, str) - and any(isinstance(t, ast.Name) and t.id == "namespace" for t in n.targets) - } - if namespace not in namespaces: - return "broken", ( - f"{path} no longer declares namespace {namespace!r} " - f"(found: {sorted(namespaces) or 'none'}), so `{method}` is gone" - ) - - # ...and it defines the method. + # Find the CLASS that declares this namespace, and look for the method THERE. + # + # Not anywhere in the file. `ast.walk` over the whole module made *any* function + # called `delete` satisfy the check -- one on an unrelated class, or even a nested + # local function inside `do_query`. That is a FALSE OK, and it breaks the one + # invariant this checker and the runtime share: `_defines_delete()` looks in + # `vars(klass)` for a PLUGIN class on the service's MRO. If iX gutted + # `PoolSnapshotService.do_delete` while some other class in the same file still had + # a `delete`, compat would say ok, apply.sh would patch, and the runtime would then + # correctly refuse `pool.snapshot`, fall through to a `zfs.snapshot` that does not + # exist on 26, and fail every nested backup on a box the preflight called healthy. + # + # Same question on both sides: does the class that OWNS this namespace define the + # method? # # A CRUDService exposes `create`/`update`/`delete` from methods NAMED - # `do_create`/`do_update`/`do_delete`. Both spellings are live right now: - # 24.10 and 25.04 declare `do_delete`, 25.10 renamed it to `delete`, and all - # three answer to `zfs.snapshot.delete`. Accepting only the literal name reported - # the two older releases as broken -- a false BROKEN that would have switched off - # nested snapshots on boxes where they work. - defined = { - n.name for n in ast.walk(tree) - if isinstance(n, ast.FunctionDef | ast.AsyncFunctionDef) - } - if not any(sp in defined for sp in accepted_spellings(name)): - return "broken", f"{path} no longer defines `{method}`" + # `do_create`/`do_update`/`do_delete`. Both spellings are live: 24.10 and 25.04 + # declare `do_delete`, 25.10 renamed it to `delete`, and all answer to + # `.delete`. Accepting only the literal name reported working releases as + # broken. + owners = [] + all_namespaces = set() + for cls in (n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)): + declared = { + n.value.value + for n in ast.walk(cls) + if isinstance(n, ast.Assign) + and isinstance(n.value, ast.Constant) + and isinstance(n.value.value, str) + and any(isinstance(t, ast.Name) and t.id == "namespace" for t in n.targets) + } + all_namespaces |= declared + if namespace in declared: + owners.append(cls) - return "ok", None + if not owners: + return "broken", ( + f"{path} no longer declares namespace {namespace!r} " + f"(found: {sorted(all_namespaces) or 'none'}), so `{method}` is gone" + ) + + wanted = accepted_spellings(name) + for cls in owners: + # Direct members of the class, not its nested scopes: a `def delete` inside + # another method is a local function, not a service method. + if any( + isinstance(n, ast.FunctionDef | ast.AsyncFunctionDef) and n.name in wanted + for n in cls.body + ): + return "ok", None + + return "broken", ( + f"{path} still declares namespace {namespace!r}, but its class no longer " + f"defines `{'` or `'.join(wanted)}` -- so `{method}` is gone" + ) #: Things that mean iX has done the job themselves and the module should RETIRE, From df412eeff730d203bee1023a2d1e65ed175c2758 Mon Sep 17 00:00:00 2001 From: sudolulo Date: Tue, 14 Jul 2026 01:41:25 +0000 Subject: [PATCH 9/9] test: make three vacuous tests actually test something; drop one duplicate The test suite is not bloated -- 3,701 lines of test code against 3,760 lines of product code, one duplicate pair in 356 tests, 8% single-assertion tests. The waste was not volume, it was four tests that looked like coverage and provided none: - test_parents_are_mounted_before_children: the fixture was already in depth order, so the sort it exists for was never exercised. Deleting `mounts.sort(key=_depth)` passed the whole suite. It now uses datasets whose NAME order differs from their MOUNTPOINT depth, which is the only case the sort is for. - test_it_NEVER_touches_the_current_run: the "current" snapshot was a minute old, so the age floor excluded it regardless and the same-snapname guard never ran. That guard only matters for a run that OUTLIVES the floor -- which a first full upload easily does, and where collecting it would yank the snapshot out from under a backup that is still reading from it. Now tested with a 12-hour-old current run. - the fingerprint/unknown test put the unreadable file in `providers`, which is not broken -- so fingerprint() skipped the whole module via is_broken() and the filter under test never executed. The blip has to land in the module that IS broken. - test_the_real_truenas_versions was a byte-identical copy of test_async_middleware_is_detected under a name promising more. Replaced with the fact actually worth pinning: the flavour probe must read STOCK source, because apply.sh re-runs on an already-patched overlay and our own SNAPSHOT_SYNC block is a plain `def create_snapshot` -- reading it would report a 25.10 box as synchronous and inject the wrong wrapper. All four now fail when the code they name is broken. --- tests/test_compat.py | 27 +++++++++++++++++---- tests/test_truecloud_nested.py | 44 ++++++++++++++++++++++++++++++---- 2 files changed, 63 insertions(+), 8 deletions(-) diff --git a/tests/test_compat.py b/tests/test_compat.py index 32b8bc4..5bad42c 100644 --- a/tests/test_compat.py +++ b/tests/test_compat.py @@ -304,9 +304,23 @@ class TestAsyncFlavour: broken["plugins/cloud_backup/sync.py"] = Unreadable("HTTP 429") assert compat.async_flavour(loader(broken)) is None - def test_the_real_truenas_versions(self): - # Pinning the actual fact this whole port exists for. - assert compat.async_flavour(loader(GOOD)) is True + def test_it_reads_STOCK_source_not_our_own_injected_block(self): + # This was a byte-identical copy of test_async_middleware_is_detected under a + # name that promised more. The fact worth pinning: apply.sh re-runs on an + # ALREADY-PATCHED overlay, so the probe must cut our block off first -- our own + # SNAPSHOT_SYNC wrapper is a plain `def create_snapshot`, and reading it would + # report a 25.10 box as synchronous and inject the wrong flavour. + patched = dict(GOOD) + patched["plugins/cloud/snapshot.py"] = ( + GOOD["plugins/cloud/snapshot.py"] + + "\n# TRUECLOUD_PATCH\n" + + 'def create_snapshot(middleware, path, name="x"):\n return "s", "p"\n' + ) + assert compat.async_flavour(loader(patched)) is True, ( + "the flavour probe read our own injected block and concluded the box is " + "synchronous -- it would then inject a sync wrapper into an async " + "middleware, and every nested backup would break" + ) class TestMiddlewareMethodsWeCall: @@ -530,6 +544,11 @@ class TestATransientNetworkBlipDoesNotWakeAnybody: return [{"ref": "master", "modules": check_files(files)}] def test_an_unreadable_file_does_not_change_the_fingerprint_of_a_broken_ref(self): + # The blip must land in the SAME module that is broken. Put it in `providers` + # (which is healthy) and `fingerprint()` skips the whole module via + # `is_broken(m)` -- so the `state` filter under test never runs and the test + # passes no matter what the code does. `nested` is the broken one here, so the + # unreadable file goes in `nested` too. broken = with_(**{ "plugins/cloud/snapshot.py": "async def create_snapshot(name, path, middleware):\n return 1, 2\n", @@ -537,7 +556,7 @@ class TestATransientNetworkBlipDoesNotWakeAnybody: clean = compat.fingerprint(self._rows(broken)) blipped = dict(broken) - blipped["rclone/remote/b2.py"] = Unreadable("HTTP 429") + blipped["plugins/cloud_backup/sync.py"] = Unreadable("HTTP 429") # nested assert compat.fingerprint(self._rows(blipped)) == clean, ( "a rate-limited fetch changed the fingerprint, so the bot rewrites the " "issue body and then rewrites it back tomorrow" diff --git a/tests/test_truecloud_nested.py b/tests/test_truecloud_nested.py index 69c86cd..455bb1f 100644 --- a/tests/test_truecloud_nested.py +++ b/tests/test_truecloud_nested.py @@ -143,13 +143,28 @@ class TestPlanStaging: ) def test_parents_are_mounted_before_children(self): - # A child's mountpoint dir only exists inside its parent's snapshot, so - # mounting a child first would fail. - mounts, _ = plan() + # A child's mountpoint dir only exists inside its PARENT's snapshot, so + # mounting a child first fails. + # + # The default fixture is already in depth order, so it never exercised the + # sort at all -- deleting `mounts.sort(key=_depth)` passed the whole suite. + # These datasets are deliberately in the WRONG order by name: `Tap/aaa` is + # three levels deep and `Tap/zzz` is one, so anything that preserves input + # order (or sorts by name) mounts the child first and would fail for real. + awkward = [ + ds("Tap", "/mnt/Tap"), + ds("Tap/aaa", "/mnt/Tap/zzz/deep/deeper"), + ds("Tap/zzz", "/mnt/Tap/zzz"), + ds("Tap/mmm", "/mnt/Tap/zzz/deep"), + ] + mounts, _ = plan(datasets=awkward) + seen = set() for _src, target in mounts: if target != ROOT: - assert os.path.dirname(target) in seen + assert os.path.dirname(target) in seen, ( + f"{target} is mounted before its parent exists" + ) seen.add(target) def test_backup_path_below_dataset_root(self): @@ -1088,6 +1103,27 @@ class TestGarbageCollectorSelection: names = [self.CURRENT, "Tap/apps/x@cloud_backup-5-20260714115900"] assert self.collect(names) == [] + def test_it_NEVER_touches_the_current_run_EVEN_WHEN_IT_IS_OLD(self): + # The one that matters, and the one that was not tested: a backup running + # longer than the age floor. The old test's `current` was a minute old, so the + # floor excluded it anyway and the same-snapname guard never ran -- deleting + # that guard passed the whole suite. + # + # A first full upload of a 100 GB pool takes hours. If the GC collected the + # snapshot of the run that is CURRENTLY READING FROM IT, restic would be + # yanked out from under itself mid-backup. + import datetime as dt + + old_current = "Tap@cloud_backup-5-20260714000000" # 12 hours old + names = [old_current, "Tap/apps/x@cloud_backup-5-20260714000000"] + assert tn.stale_snapshot_names( + "cloud_backup-5", old_current, names, + dt.datetime(2026, 7, 14, 12, 0, 0, tzinfo=dt.UTC), + ) == [], ( + "the GC collected the snapshot of the run that is using it. A long first " + "upload would be destroyed mid-flight." + ) + def test_it_NEVER_touches_a_periodic_snapshot(self): assert self.collect(["Tap/apps/x@auto-2026-07-13_03-00"]) == []