compat: check the middlewared METHODS we call, not just the symbols we wrap

This gap was hiding a catastrophe. TrueNAS 26 deletes plugins/zfs_/dataset.py and
plugins/zfs_/snapshot.py outright, taking zfs.dataset.query, zfs.snapshot.query and
zfs.snapshot.delete with them (26 uses filesystem.statfs and zfs.resource.*).

Nothing about the five cloud_backup files reveals that, so every other check went
green -- including the one I had just added. The patch would have applied cleanly
and then failed on the first backup, or worse: snapshotted fine and failed to
DELETE, orphaning one snapshot per descendant dataset (250 on a real pool) on every
run, forever.

So 26 is BROKEN and the nested module will not apply there. The async/sync wrapper
work and the vendored get_dataset_recursive stay -- they are correct and necessary
-- but 26 is not supported until the ZFS calls are ported, and that needs a real 26
box to verify. Shipping a port nobody has run is the failure this project exists to
avoid.

Also: do_delete is recognised as delete (24.10/25.04 use the CRUDService
convention), which was reporting both as BROKEN -- a false verdict that would have
disabled nested snapshots on boxes where they work.

CHANGELOG.md

@@ -59,28 +59,53 @@ worse than no alert, because one day it carries a security fix.
 
 ### Added
 
-- **TrueNAS 26 support.** 26 rewrites the entire `cloud_backup` path from async to
-  **synchronous**, and separately **deletes `get_dataset_recursive()`** — which one
-  of the injected blocks called out of the host module's namespace. Either one is a
-  broken backup found at restore time: an `async def` wrapper hands `sync.py` a
-  coroutine where it unpacks a tuple, and the vanished helper is a straight
-  `NameError`.
+- **The compatibility check now covers the middlewared methods the patch _calls_,**
+  not only the symbols it wraps — and that gap was hiding a catastrophe.
 
-  The nested module is now **one synchronous implementation** (talking to middlewared
-  through `call_sync`) behind **two thin wrappers**. `apply.sh` reads which flavour
-  the installed middleware declares and injects the matching one: TrueNAS ≤ 25.10
-  reaches it via `await middleware.run_in_thread(...)`, and TrueNAS 26 — already in a
-  worker thread — calls it directly. The logic that owns the snapshots, the bind
-  mounts and the failure modes exists **once**; an async twin would mean every future
-  fix had to land twice, and the one that got missed would be the one that eats a
-  backup.
+  TrueNAS 26 **deletes `plugins/zfs_/dataset.py` and `plugins/zfs_/snapshot.py`
+  outright**, taking `zfs.dataset.query`, `zfs.snapshot.query` and
+  `zfs.snapshot.delete` with them (26 uses `filesystem.statfs` and `zfs.resource.*`).
+  Nothing about the five `cloud_backup` files reveals that, so every other check went
+  green. The patch would have applied perfectly and then **failed on the first
+  backup** — or, far worse, snapshotted successfully and failed to *delete*,
+  orphaning one snapshot per descendant dataset (**250 on a real pool**) on every
+  single run, forever.
 
-  A middleware whose three wrapped functions **disagree** about async-ness is refused
-  outright rather than guessed at. And `get_dataset_recursive` is now carried as our
-  own copy — removing the dependency on both versions instead of asserting it.
+  This is now an assumption class of its own, so a method disappearing is a BROKEN
+  verdict rather than a silent time bomb.
 
-  Both breaks were found by the daily compatibility check **while 26 was still in
-  beta**, which is the entire point of it.
+- **Groundwork for TrueNAS 26** (async→sync and the deleted helper — see below).
+  **26 is still reported BROKEN and the nested module will not apply there**, because
+  the ZFS API rewrite above is not yet ported. Porting it needs a real 26 box to
+  verify against, and shipping a port nobody has run is exactly the failure this
+  project exists to avoid. On 26, TrueNAS is left stock: B2/S3 keeps working, nested
+  datasets are simply not covered.
+
+### Fixed
+
+- **The nested module is now one synchronous implementation behind two thin
+  wrappers.** TrueNAS 26 rewrites `cloud_backup` from async to **synchronous** and
+  separately **deletes `get_dataset_recursive()`**, which an injected block called out
+  of the host module's namespace. Either alone is a broken backup found at restore
+  time: an `async def` wrapper hands `sync.py` a coroutine where it unpacks a tuple,
+  and the vanished helper is a straight `NameError`.
+
+  The module now talks to middlewared through `call_sync`, and `apply.sh` reads which
+  flavour the installed middleware declares and injects the matching wrapper —
+  TrueNAS ≤ 25.10 reaches it via `await middleware.run_in_thread(...)`; a synchronous
+  TrueNAS, already in a worker thread, calls it directly. The logic that owns the
+  snapshots, the bind mounts and the failure modes exists **once**; an async twin
+  would mean every future fix had to land twice, and the one that got missed would be
+  the one that eats a backup. A middleware whose three wrapped functions **disagree**
+  about async-ness is refused outright rather than guessed at, and
+  `get_dataset_recursive` is carried as our own copy — removing the dependency on both
+  versions instead of asserting it.
+
+- **`do_delete` is recognised as `delete`.** TrueNAS 24.10 and 25.04 declare
+  `do_delete` (the `CRUDService` convention); 25.10 renamed it to `delete`. Both
+  answer to `zfs.snapshot.delete`. Accepting only the literal name reported both older
+  releases as BROKEN — a false verdict that would have switched nested snapshots off
+  on boxes where they work perfectly.
 
 - **An incompatible TrueNAS no longer sets the permanent kill switch.** `apply.sh`
   reused a "nothing left to do" exit that touches `disabled`, which suppresses

README.md

@@ -6,8 +6,10 @@
 > 24.10**. There is nothing here to install on an older release, and `install.sh`
 > will refuse.
 >
-> Verified on **24.10**, **25.04**, **25.10**, and the unreleased **26.0 beta**
-> (see [TrueNAS compatibility](#truenas-compatibility)).
+> Verified on **24.10**, **25.04** and **25.10**. The unreleased **26.0** is not yet
+> supported for nested snapshots — see [TrueNAS
+> compatibility](#truenas-compatibility). Nothing breaks if you upgrade: the patch
+> checks, and declines.
 
 Extends TrueNAS SCALE's **TrueCloud Backup** feature to:
 
@@ -44,7 +46,7 @@ costs a fraction of the new Storj price.
 | 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 | ok | — |
+| 26.0.0-BETA.3 _(unreleased)_ | ok | **BROKEN** | — |
 | master _(unreleased)_ | **BROKEN** | **BROKEN** | — |
 
 | verdict | meaning |
@@ -62,31 +64,40 @@ The table above is **regenerated daily by CI** — it is not a claim somebody ty
 once and forgot. **TrueCloud Backup does not exist before 24.10**, so earlier
 versions are absent rather than "unsupported".
 
-### TrueNAS 26 — supported, and this is how we knew in advance
+### TrueNAS 26 — nested snapshots are not supported yet
 
-TrueNAS 26 rewrites the whole `cloud_backup` path **from async to synchronous**.
-Every block the nested module injected was an `async def` wrapping an `await`ed
-original, so on 26 it would have handed `sync.py` a coroutine where it unpacks a
-tuple, and 26 also **deleted `get_dataset_recursive()`**, which one of those blocks
-called. Both are backup-breaking, and neither would have surfaced until a restore
-failed.
+**You do not need to do anything, and upgrading will not break your backups.** On
+26, `apply.sh` finds that the nested module's assumptions no longer hold and **does
+not apply it**. TrueNAS is left stock: B2/S3 backups keep running (the providers
+module is unaffected), datasets nested under the target are simply not included, and
+the reason is named in `apply.log`. A broken backup is worse than a missing feature.
 
-The daily compatibility check found both **while 26 was still in beta**, and filed
-the bug report itself. The patch now reads which flavour of `cloud_backup` your box
-has and injects the wrapper that matches — one implementation of the actual logic,
-two thin wrappers — and carries its own copy of the deleted helper.
+TrueNAS 26 changes three things underneath this module, and each one alone is
+backup-breaking:
 
-**If a future TrueNAS breaks it anyway, nothing bad happens quietly.** `apply.sh`
-re-checks these assumptions against the middleware *actually installed on your box*
-at every boot and will not apply a module that no longer fits: TrueNAS is left
-stock, backups keep running without that module's feature, and the reason is named
-in `apply.log`. A broken backup is worse than a missing feature.
+| what changed | what it would have done |
+| --- | --- |
+| `cloud_backup` rewritten **async → synchronous** | an `async def` wrapper hands `sync.py` a coroutine where it unpacks a tuple |
+| `get_dataset_recursive()` **deleted** from `plugins/cloud/snapshot.py` | `NameError` — the injected block called it out of the host module's namespace |
+| `plugins/zfs_/dataset.py` and `zfs_/snapshot.py` **deleted** | `zfs.dataset.query`, `zfs.snapshot.query` and `zfs.snapshot.delete` all vanish. 26 uses `filesystem.statfs` and `zfs.resource.*` instead |
 
-`master` (the development branch after 26) currently reports **BROKEN**: iXsystems
-are still reshaping these functions there — renaming `middleware` to `context`,
-`cloud_backup` to `entry`, adding a required `credentials` parameter. That is a
-moving target and is deliberately not chased; the check will keep reporting it until
-it settles into a beta, which is exactly when it becomes worth fixing.
+The first two are fixed: the patch now reads which flavour of `cloud_backup` your box
+declares and injects the wrapper that matches (one implementation of the real logic,
+two thin wrappers), and it carries its own copy of the deleted helper.
+
+The third is **not** fixed, and is why 26 still reports BROKEN. Porting it means
+rewriting the module's ZFS calls onto 26's new API, and there is no single API that
+spans 24.10 through 26 — so it needs a real 26 box to verify against, not a
+plausible-looking diff. **Shipping a port nobody has run is exactly the failure this
+project exists to avoid.** The third row is also the one that would have hurt most:
+`zfs.snapshot.delete` is what sweeps the recursive snapshot, and without it every run
+would orphan one snapshot per descendant dataset — 250 on a real pool — forever.
+
+`master` (development after 26) reports BROKEN too: iXsystems are still reshaping
+these functions there, renaming `middleware` → `context` and `cloud_backup` →
+`entry`, and adding a required `credentials` parameter. That is a moving target and
+is deliberately not chased; the check keeps reporting it until it settles into a
+beta, which is when it becomes worth fixing.
 
 ### How this is kept honest
 

patch/apply.sh

@@ -640,10 +640,16 @@ if _tc_nested is not None:
             return await _tc_orig_restic_backup(middleware, job, cloud_backup, *args, **kwargs)
         finally:
             try:
+                # logger= must be passed here too, exactly as the sync variant does.
+                # run_in_thread forwards **kwargs (functools.partial), and without it
+                # cleanup_task gets logger=None -- so every teardown warning ("could
+                # not unmount X") is silently swallowed on <= 25.10, which is most
+                # boxes. The two wrappers must differ ONLY in how they reach the core.
                 await middleware.run_in_thread(
                     _tc_nested.cleanup_task,
                     middleware,
                     f"cloud_backup-{cloud_backup.get('id', 'onetime')}",
+                    logger=getattr(middleware, "logger", None),
                 )
             except Exception as e:
                 middleware.logger.warning("truecloud-patch: staging cleanup failed: %r", e)

tests/test_compat.py

@@ -51,6 +51,22 @@ 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"
+        "    class Config:\n"
+        "        namespace = 'zfs.dataset'\n"
+        "    def query(self, filters, options):\n        pass\n"
+    ),
+    "plugins/zfs_/snapshot.py": (
+        "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"
+    ),
 }
 
 
@@ -271,3 +287,75 @@ class TestAsyncFlavour:
     def test_the_real_truenas_versions(self):
         # Pinning the actual fact this whole port exists for.
         assert compat.async_flavour(loader(GOOD)) is True
+
+
+class TestMiddlewareMethodsWeCall:
+    """The assumption class that was MISSING, and that hid a catastrophic break.
+
+    The manifest recorded the symbols the patch WRAPS. It said nothing about the
+    middlewared methods the patch CALLS -- and TrueNAS 26 deleted
+    plugins/zfs_/dataset.py and plugins/zfs_/snapshot.py outright, taking
+    `zfs.dataset.query`, `zfs.snapshot.query` and `zfs.snapshot.delete` with them.
+
+    Nothing about the five cloud_backup files reveals that. The patch applied
+    perfectly and every other check went green. The first backup would have failed --
+    or, far worse, snapshotted fine and then failed to DELETE, orphaning one snapshot
+    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"
+    )
+
+    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
+
+    def test_present_methods_are_ok(self):
+        r = check_files(self._tree())
+        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}))
+        assert is_broken(r[NESTED])
+        details = " ".join(p["detail"] for p in r[NESTED]["problems"])
+        assert "zfs.snapshot.delete" in details
+
+    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'"),
+        }))
+        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(
+                "def delete(", "def do_delete("),
+        }))
+        assert r[NESTED]["ok"], r[NESTED]["problems"]
+
+    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}))
+        whys = " ".join(p["why"] for p in r[NESTED]["problems"])
+        assert "orphan" in whys

tools/compat.py

@@ -135,6 +135,114 @@ ASSUMPTIONS = [
 ]
 
 
+class MiddlewareCall:
+    """A middlewared METHOD the injected code calls at runtime.
+
+    THIS CLASS OF ASSUMPTION IS WHY THE CHECKER EXISTS, AND IT WAS THE ONE MISSING.
+
+    The manifest above records the symbols the patch *wraps*. It said nothing about
+    the methods the patch *calls* -- and that gap hid two separate TrueNAS 26 breaks
+    that both pass every other check:
+
+      * `get_dataset_recursive()` was deleted from plugins/cloud/snapshot.py, and the
+        injected block called it out of the host module's namespace (now vendored).
+      * plugins/zfs_/dataset.py and plugins/zfs_/snapshot.py were DELETED outright,
+        taking `zfs.dataset.query`, `zfs.snapshot.query` and `zfs.snapshot.delete`
+        with them. 26 uses filesystem.statfs and zfs.resource.* instead.
+
+    Nothing about the five cloud_backup files reveals that. The patch would apply
+    perfectly, and then the FIRST BACKUP would fail -- or, far worse, succeed at
+    snapshotting and fail at `zfs.snapshot.delete`, orphaning one snapshot per
+    descendant dataset (250 on a real pool) on every single run, forever.
+
+    A method is present when some plugin file declares its namespace AND defines it.
+    If iX merely MOVES a method to a different file we report BROKEN wrongly, and the
+    module declines to apply -- costing a feature, not a backup. That asymmetry is
+    the whole design: declining is always the cheaper mistake.
+    """
+
+    def __init__(self, ident, module, method, path, why=""):
+        self.id = ident
+        self.module = module
+        self.method = method              # "zfs.snapshot.delete"
+        self.path = path                  # plugin file that declares it
+        self.why = why
+
+    @property
+    def namespace(self):
+        return self.method.rsplit(".", 1)[0]
+
+    @property
+    def name(self):
+        return self.method.rsplit(".", 1)[1]
+
+
+#: Every middlewared method the nested module calls at runtime.
+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",
+        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",
+    ),
+]
+
+
+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"
+        )
+
+    try:
+        tree = ast.parse(_stock(src))
+    except SyntaxError as e:
+        return "unknown", f"{c.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 c.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"
+        )
+
+    # ...and it defines 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 c.name not in defined and f"do_{c.name}" not in defined:
+        return "broken", f"{c.path} no longer defines `{c.method}`"
+
+    return "ok", None
+
+
 #: Things that mean iX has done the job themselves and the module should RETIRE,
 #: not break. Absence of the nesting guard = nested snapshots went native.
 #: `restic = True` already on B2RcloneRemote = B2 restic support went native.
@@ -445,6 +553,31 @@ def check(loader, modules=None) -> dict:
                 "id": a.id, "detail": detail, "why": a.why,
             })
 
+    # The methods the injected code CALLS, not just the symbols it wraps.
+    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,
+            })
+            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":
+            out[c.module]["unknown"] = True
+            out[c.module]["problems"].append({
+                "id": c.id, "detail": detail, "why": c.why,
+            })
+
     for module, (path, phrase, native_when_present) in NATIVE_PROBES.items():
         if module not in out:
             continue