From 498b2690e1c1eedd9d0929a8922a96b8f11414a1 Mon Sep 17 00:00:00 2001 From: sudolulo Date: Mon, 13 Jul 2026 18:18:28 +0000 Subject: [PATCH] TrueNAS 26 support: one sync implementation, two wrappers 26 rewrites cloud_backup from async to synchronous AND deletes get_dataset_recursive(), which SNAPSHOT_BLOCK called out of the host module's namespace. Either is a broken backup found at restore time. The nested module is now one synchronous implementation talking to middlewared via call_sync, behind two thin wrappers. apply.sh reads which flavour the installed middleware declares and injects the matching one: <= 25.10 reaches it through 'await middleware.run_in_thread(...)', 26 is already in a worker thread and calls it directly. The snapshot/bind-mount/failure logic exists once -- an async twin would mean every future fix had to land twice. A middleware whose three wrapped functions disagree about asyncness is refused, not guessed at. get_dataset_recursive is vendored, removing the dependency on both versions rather than asserting it. master stays BROKEN on purpose: iX are still renaming middleware->context, cloud_backup->entry and adding a required credentials param there. Chasing a branch that moves daily is how you ship a patch nobody tested. --- CHANGELOG.md | 29 +++++- README.md | 36 +++++-- patch/apply.sh | 178 ++++++++++++++++++++++++--------- patch/truecloud_nested.py | 99 ++++++++++++++---- tests/test_apply_blocks.py | 76 +++++++++----- tests/test_compat.py | 56 +++++++++-- tests/test_truecloud_nested.py | 114 +++++++++++---------- tools/compat.py | 66 ++++++++++-- 8 files changed, 475 insertions(+), 179 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f084282..03e146a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,11 +57,30 @@ worse than no alert, because one day it carries a security fix. appeared somewhere in the signature, so it happily passed a call that could never work. -- **TrueNAS 26 rewrites the entire `cloud_backup` path from async to synchronous.** - Every block the nested module injects is an `async def` wrapping an `await`ed - original, so on 26 it would hand `sync.py` a coroutine where it unpacks a tuple — - a broken backup, discovered at restore time. On TrueNAS 26 the nested module now - stays off rather than applying and breaking. +### 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 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. + + 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. + + Both breaks were found by the daily compatibility check **while 26 was still in + beta**, which is the entire point of it. - **An incompatible TrueNAS no longer sets the permanent kill switch.** `apply.sh` reused a "nothing left to do" exit that touches `disabled`, which suppresses diff --git a/README.md b/README.md index a52f661..9f5e848 100644 --- a/README.md +++ b/README.md @@ -6,8 +6,8 @@ > 24.10**. There is nothing here to install on an older release, and `install.sh` > will refuse. > -> Verified on **24.10**, **25.04** and **25.10**. Not yet compatible with the -> unreleased **26.0** (see [TrueNAS compatibility](#truenas-compatibility)). +> Verified on **24.10**, **25.04**, **25.10**, and the unreleased **26.0 beta** +> (see [TrueNAS compatibility](#truenas-compatibility)). Extends TrueNAS SCALE's **TrueCloud Backup** feature to: @@ -44,7 +44,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 | **BROKEN** | — | +| 26.0.0-BETA.3 _(unreleased)_ | ok | ok | — | | master _(unreleased)_ | **BROKEN** | **BROKEN** | — | | verdict | meaning | @@ -62,17 +62,31 @@ 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 breaks nested snapshots (not yet released) +### TrueNAS 26 — supported, and this is how we knew in advance TrueNAS 26 rewrites the whole `cloud_backup` path **from async to synchronous**. -Every block the nested module injects is an `async def` wrapping an `await`ed -original, so on 26 it would hand `sync.py` a coroutine where it unpacks a tuple. +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. `apply.sh` checks these assumptions against the -middleware **actually installed on your box** at every boot, and will not apply a -module that no longer fits. On TrueNAS 26 the nested module simply stays off: -backups keep running, without nested-dataset coverage. 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. + +**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. + +`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. ### How this is kept honest diff --git a/patch/apply.sh b/patch/apply.sh index 2f8445c..af79b51 100755 --- a/patch/apply.sh +++ b/patch/apply.sh @@ -491,20 +491,41 @@ else: # the guard removed but the traversal missing -- that would be a silently empty # backup, the worst possible outcome. -SNAPSHOT_BLOCK = """ +# ── nested blocks: one core, two wrappers ───────────────────────────────────── +# +# TrueNAS <= 25.10 has an ASYNC cloud_backup path; TrueNAS 26 rewrote it SYNCHRONOUS +# (`middleware.call_sync` throughout, no awaits). An `async def` wrapper on 26 hands +# sync.py a coroutine where it unpacks a tuple, and a `def` wrapper on 25.10 blocks +# the event loop. So each block is assembled from: +# +# * a CORE, written once, synchronous, using middleware.call_sync -- which is safe +# from a worker thread and deadlocks on the event loop; and +# * a WRAPPER matching the stock function's own flavour, chosen at apply time by +# reading whether the installed middlewared declares it `async def`. +# +# On <= 25.10 the async wrapper hops to a thread via `await middleware.run_in_thread` +# -- exactly the thread call_sync needs. On 26 the stock function is already running +# in middlewared's thread pool (its own code calls call_sync), so the sync wrapper +# calls the core directly. +# +# The logic that matters -- snapshots, bind mounts, 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. + +_NESTED_IMPORT = """ # TRUECLOUD_PATCH — added by truenas-truecloud-patch/patch/apply.sh try: from middlewared.plugins.cloud import _truecloud_nested as _tc_nested except ImportError: _tc_nested = None +""" +SNAPSHOT_CORE = _NESTED_IMPORT + """ if _tc_nested is not None: _tc_orig_create_snapshot = create_snapshot - async def create_snapshot(middleware, path, name="cloud_task-onetime"): - # Stock takes the (already recursive) snapshot; we only replace the PATH. - snapshot, snap_path = await _tc_orig_create_snapshot(middleware, path, name) - + def _tc_stage(middleware, path, name, snapshot, snap_path): + # Synchronous, and always called from a worker thread (see above). _logger = getattr(middleware, "logger", None) try: # Enumerate datasets AFTER the snapshot, never before. The snapshot is @@ -513,10 +534,13 @@ 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 = await middleware.call( + datasets = middleware.call_sync( "zfs.dataset.query", [["type", "=", "FILESYSTEM"]] ) - dataset, nested = get_dataset_recursive(datasets, path) + # 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. + dataset, nested = _tc_nested.get_dataset_recursive(datasets, path) if not nested: # No children: stock behaviour, untouched. Stock's `finally` owns @@ -524,38 +548,42 @@ if _tc_nested is not None: # because a non-nested snapshot has no children). return snapshot, snap_path - staging_root = await _tc_nested.stage_nested( + staging_root = _tc_nested.stage_nested( middleware, path, snapshot, dataset["name"], dataset["properties"]["mountpoint"]["value"], name, datasets, logger=_logger, ) except Exception: # The snapshot exists, but this exception means sync.py never completes - # `snapshot, local_path = await create_snapshot(...)`, so its local - # `snapshot` 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. - await _tc_nested.delete_snapshot_tree(middleware, snapshot, logger=_logger) + # `snapshot, local_path = create_snapshot(...)`, so its local `snapshot` + # 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) raise return snapshot, staging_root +""" + +SNAPSHOT_ASYNC = SNAPSHOT_CORE + """ + async def create_snapshot(middleware, path, name="cloud_task-onetime"): + snapshot, snap_path = await _tc_orig_create_snapshot(middleware, path, name) + return await middleware.run_in_thread( + _tc_stage, middleware, path, name, snapshot, snap_path + ) create_snapshot._truecloud_patched = True """ -CRUD_BLOCK = """ -# TRUECLOUD_PATCH — added by truenas-truecloud-patch/patch/apply.sh -try: - from middlewared.plugins.cloud import _truecloud_nested as _tc_nested -except ImportError: - _tc_nested = None +SNAPSHOT_SYNC = SNAPSHOT_CORE + """ + def create_snapshot(middleware, path, name="cloud_task-onetime"): + snapshot, snap_path = _tc_orig_create_snapshot(middleware, path, name) + return _tc_stage(middleware, path, name, snapshot, snap_path) -if _tc_nested is not None: - _tc_orig_validate = CloudTaskServiceMixin._validate - - async def _tc_validate(self, app, verrors, name, data): - await _tc_orig_validate(self, app, verrors, name, data) + create_snapshot._truecloud_patched = True +""" +_CRUD_FILTER = """ # Only cloud_backup: staging teardown is wired into cloud_backup.sync's # finally. cloudsync would leak bind mounts, so leave its guard intact. if getattr(getattr(self, "_config", None), "namespace", "") != "cloud_backup": @@ -576,34 +604,63 @@ if _tc_nested is not None: CloudTaskServiceMixin._validate._truecloud_patched = True """ -SYNC_BLOCK = """ -# TRUECLOUD_PATCH — added by truenas-truecloud-patch/patch/apply.sh -try: - from middlewared.plugins.cloud import _truecloud_nested as _tc_nested -except ImportError: - _tc_nested = None +CRUD_ASYNC = _NESTED_IMPORT + """ +if _tc_nested is not None: + _tc_orig_validate = CloudTaskServiceMixin._validate + async def _tc_validate(self, app, verrors, name, data): + await _tc_orig_validate(self, app, verrors, name, data) +""" + _CRUD_FILTER + +CRUD_SYNC = _NESTED_IMPORT + """ +if _tc_nested is not None: + _tc_orig_validate = CloudTaskServiceMixin._validate + + def _tc_validate(self, app, verrors, name, data): + _tc_orig_validate(self, app, verrors, name, data) +""" + _CRUD_FILTER + +# *args/**kwargs, not the stock signature spelled out. +# +# 24.10 and 25.04 have `restic_backup(middleware, job, cloud_backup, dry_run)`; +# 25.10 added `rate_limit`. Naming them and forwarding all five raised +# `TypeError: takes 4 positional arguments but 5 were given` on every nested backup +# on the two older releases. Forwarding whatever we were handed makes this wrapper +# indifferent to iX adding or dropping a trailing parameter -- which they have now +# done twice. +# +# Our bind mounts pin the ZFS snapshot, so stock's `finally` cannot destroy it +# (EBUSY) and logs one benign warning. We unmount here and then delete it for real. +SYNC_ASYNC = _NESTED_IMPORT + """ if _tc_nested is not None: _tc_orig_restic_backup = restic_backup async def restic_backup(middleware, job, cloud_backup, *args, **kwargs): - # *args/**kwargs, not the stock signature spelled out. - # - # 24.10 and 25.04 have `restic_backup(middleware, job, cloud_backup, dry_run)`; - # 25.10 added `rate_limit`. Naming them here and forwarding all five raised - # `TypeError: takes 4 positional arguments but 5 were given` on every nested - # backup on the two older releases. Forwarding whatever we were handed makes - # this wrapper indifferent to iX adding or dropping a trailing parameter -- - # which they have now done twice. - # - # Our bind mounts pin the ZFS snapshot, so stock's `finally` cannot - # destroy it (EBUSY) and logs one benign warning. We unmount here and - # then delete the snapshot for real. try: return await _tc_orig_restic_backup(middleware, job, cloud_backup, *args, **kwargs) finally: try: - await _tc_nested.cleanup_task( + await middleware.run_in_thread( + _tc_nested.cleanup_task, + middleware, + f"cloud_backup-{cloud_backup.get('id', 'onetime')}", + ) + except Exception as e: + middleware.logger.warning("truecloud-patch: staging cleanup failed: %r", e) + + restic_backup._truecloud_patched = True +""" + +SYNC_SYNC = _NESTED_IMPORT + """ +if _tc_nested is not None: + _tc_orig_restic_backup = restic_backup + + def restic_backup(middleware, job, cloud_backup, *args, **kwargs): + try: + return _tc_orig_restic_backup(middleware, job, cloud_backup, *args, **kwargs) + finally: + try: + _tc_nested.cleanup_task( middleware, f"cloud_backup-{cloud_backup.get('id', 'onetime')}", logger=getattr(middleware, "logger", None), @@ -706,10 +763,39 @@ else: if missing: raise FileNotFoundError('missing: ' + ', '.join(missing)) + # Which flavour of cloud_backup is installed? <= 25.10 is async; TrueNAS 26 + # rewrote it synchronous. Inject the wrapper that matches: an `async def` on + # 26 hands sync.py a coroutine where it unpacks a tuple, and a plain `def` on + # 25.10 blocks the event loop. + # + # None means the three stock functions disagree, or one could not be read. + # Refuse rather than guess -- a half-converted middleware is one this patch + # has never seen, and guessing wrong there costs a backup, not a feature. + # nested_src is /patch/truecloud_nested.py, so tools/ is its sibling. + # APPEND, never insert(0) -- shadowing the stdlib for this interpreter is a + # far worse failure than not finding compat. + sys.path.append( + os.path.join(os.path.dirname(os.path.dirname(nested_src)), 'tools') + ) + import compat + _flavour = compat.async_flavour_tree(mw_dir) + if _flavour is None: + raise RuntimeError( + 'cannot tell whether this TrueNAS cloud_backup path is async or ' + 'sync (the wrapped functions disagree, or could not be read)' + ) + + _snapshot_block = SNAPSHOT_ASYNC if _flavour else SNAPSHOT_SYNC + _sync_block = SYNC_ASYNC if _flavour else SYNC_SYNC + _crud_block = CRUD_ASYNC if _flavour else CRUD_SYNC + shutil.copyfile(nested_src, nested_dst) # 1. traversal implementation - patch_file(snapshot_py, SNAPSHOT_BLOCK) # 2. build the staging tree - patch_file(sync_path, SYNC_BLOCK) # 3. tear it down afterwards - patch_file(crud_py, CRUD_BLOCK) # 4. ONLY NOW allow nested tasks + patch_file(snapshot_py, _snapshot_block) # 2. build the staging tree + patch_file(sync_path, _sync_block) # 3. tear it down afterwards + patch_file(crud_py, _crud_block) # 4. ONLY NOW allow nested tasks + + print('OK: cloud_backup is %s; injected the matching wrappers.' + % ('async (TrueNAS <= 25.10)' if _flavour else 'synchronous (TrueNAS 26+)')) nested_ok = True nested_detail = 'nested-dataset snapshots enabled (staging tree)' diff --git a/patch/truecloud_nested.py b/patch/truecloud_nested.py index 18bea3a..f58a1c8 100644 --- a/patch/truecloud_nested.py +++ b/patch/truecloud_nested.py @@ -362,10 +362,69 @@ def teardown(staging_root, runner=_run, mounts_file="/proc/self/mounts"): return errors -# ── async orchestration (middleware is duck-typed; no middlewared import) ───── +# ── orchestration (middleware is duck-typed; no middlewared import) ─────────── +# +# These are SYNCHRONOUS and talk to middlewared via `middleware.call_sync`, which +# is safe from a worker thread and deadlocks on the event loop. That is the whole +# reason this file has one implementation instead of two: +# +# TrueNAS <= 25.10 cloud_backup is async. The injected wrapper is `async def` and +# hands these to `await middleware.run_in_thread(...)`, which is +# exactly the thread `call_sync` needs. +# TrueNAS >= 26 cloud_backup is synchronous and already runs in middlewared's +# thread pool (its own code calls `call_sync`). The injected +# wrapper calls these directly. +# +# So the async/sync difference lives entirely in the three injected blocks, and the +# logic below -- the part with the snapshots, the bind mounts and the failure modes +# -- is written once. Duplicating it as an async twin would mean every future fix +# had to be made twice, and the one that got missed would be the one that eats a +# backup. -async def delete_snapshot_tree(middleware, snapshot, logger=None): +def get_dataset_recursive(datasets, directory): + """The dataset containing `directory`, and whether anything is nested under it. + + Vendored from middlewared's own plugins/cloud/snapshot.py (TrueNAS <= 25.10), + because TrueNAS 26 DELETED it -- create_snapshot there uses filesystem.statfs + instead. The injected block used to call it out of the host module's namespace, + which on 26 is a straight NameError. + + Carrying our own copy removes the dependency on both versions rather than adding + an assumption about it. It is ~10 lines of pure list arithmetic over data we + already have in hand, and it has no reason to change. + + Returns (dataset, has_children): + dataset -- the DEEPEST dataset whose mountpoint is a prefix of `directory` + has_children -- whether any OTHER dataset is mounted beneath `directory` + """ + datasets = [ + dict(dataset, prefixlen=len( + os.path.dirname(os.path.commonprefix( + [dataset["properties"]["mountpoint"]["value"] + "/", directory + "/"])) + )) + for dataset in datasets + if dataset["properties"]["mountpoint"]["value"] != "none" + ] + + dataset = sorted( + [ + dataset + for dataset in datasets + if (directory + "/").startswith(dataset["properties"]["mountpoint"]["value"] + "/") + ], + key=lambda dataset: dataset["prefixlen"], + reverse=True, + )[0] + + return dataset, any( + (ds["properties"]["mountpoint"]["value"] + "/").startswith(directory + "/") + for ds in datasets + if ds != dataset + ) + + +def delete_snapshot_tree(middleware, snapshot, logger=None): """Delete the parent snapshot AND every child created by ``zfs snapshot -r``. ``zfs.snapshot.delete`` is non-recursive by default and stock calls it with @@ -381,7 +440,7 @@ async def delete_snapshot_tree(middleware, snapshot, logger=None): # through 252 sequential deletes leaves exactly the orphans this function # exists to prevent. try: - await middleware.call("zfs.snapshot.delete", snapshot, {"recursive": True}) + middleware.call_sync("zfs.snapshot.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 @@ -398,7 +457,7 @@ async def delete_snapshot_tree(middleware, snapshot, logger=None): # our mounts are released -- which fails the recursive delete while the # children survive. Sweep them by name. try: - snaps = await middleware.call( + snaps = middleware.call_sync( "zfs.snapshot.query", [["name", "^", dataset]], {"select": ["name"]} ) # An empty result means the tree is already gone -- delete nothing, and @@ -415,7 +474,7 @@ async def delete_snapshot_tree(middleware, snapshot, logger=None): for name in names: try: - await middleware.call("zfs.snapshot.delete", name) + middleware.call_sync("zfs.snapshot.delete", name) except Exception as e: # noqa: BLE001 - already gone is fine if logger: logger.warning( @@ -423,8 +482,8 @@ async def delete_snapshot_tree(middleware, snapshot, logger=None): ) -async def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint, - task_name, datasets, logger=None): +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`. `snapshot` is a full ZFS snapshot name ("Tap@cloud_backup-5-2026..."). @@ -447,30 +506,30 @@ async def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint staging_root = staging_root_for(task_name) # A previous run may have crashed mid-flight; never build on top of that. - await middleware.run_in_thread(teardown, staging_root) + teardown(staging_root) # ...and if it left a sidecar behind, that snapshot tree is still on disk and # nothing else will ever reclaim it. Sweep it before we overwrite the record, # or a single crashed run orphans 160+ snapshots permanently. - stale = await middleware.run_in_thread(_read_sidecar, staging_root) + stale = _read_sidecar(staging_root) if stale and stale != snapshot: if logger: logger.warning( "truecloud-patch: reclaiming snapshot tree from an earlier " "interrupted run: %s", stale, ) - await delete_snapshot_tree(middleware, stale, logger=logger) + delete_snapshot_tree(middleware, stale, logger=logger) # Record the snapshot BEFORE mounting anything, not after. middlewared can # die at any point (this patch even schedules a restart at boot), and the # sidecar is the only thing that survives it -- an in-process dict would take # the sole record of a 160-snapshot tree with it. Writing it after apply_plan # would leave exactly the crash window the sidecar exists to close. - await middleware.run_in_thread(_write_sidecar, staging_root, snapshot) + _write_sidecar(staging_root, snapshot) try: - mounts, skipped = await middleware.run_in_thread( - plan_staging, base_dataset, base_mountpoint, path, snapshot_name, + mounts, skipped = plan_staging( + base_dataset, base_mountpoint, path, snapshot_name, datasets, staging_root, ) if logger: @@ -479,11 +538,11 @@ async def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint "truecloud-patch: not staging dataset %r: %s", name, reason ) - await middleware.run_in_thread(apply_plan, mounts) - await middleware.run_in_thread(verify_staged, mounts) + apply_plan(mounts) + verify_staged(mounts) except Exception: - await middleware.run_in_thread(teardown, staging_root) - await middleware.run_in_thread(_remove_sidecar, staging_root) + teardown(staging_root) + _remove_sidecar(staging_root) raise if logger: @@ -494,7 +553,7 @@ async def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint return staging_root -async def cleanup_task(middleware, task_name, logger=None): +def cleanup_task(middleware, task_name, logger=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. @@ -505,13 +564,13 @@ async def cleanup_task(middleware, task_name, logger=None): if snapshot is None and not os.path.isdir(staging_root): return # never staged; nothing to do - errors = await middleware.run_in_thread(teardown, staging_root) + errors = teardown(staging_root) if errors and logger: for err in errors: logger.warning("truecloud-patch: staging teardown: %s", err) if snapshot is not None: - await delete_snapshot_tree(middleware, snapshot, logger=logger) + delete_snapshot_tree(middleware, snapshot, logger=logger) _remove_sidecar(staging_root) diff --git a/tests/test_apply_blocks.py b/tests/test_apply_blocks.py index 537fca3..3c0f362 100644 --- a/tests/test_apply_blocks.py +++ b/tests/test_apply_blocks.py @@ -14,14 +14,27 @@ import pytest APPLY_SH = os.path.join(os.path.dirname(__file__), "..", "patch", "apply.sh") +#: Every block that is actually injected into a middlewared module. +#: +#: The three nested blocks come in two flavours. TrueNAS <= 25.10 has an ASYNC +#: cloud_backup path; TrueNAS 26 rewrote it synchronous. apply.sh reads which one is +#: installed and injects the matching wrapper -- an `async def` on 26 would hand +#: sync.py a coroutine where it unpacks a tuple, and a plain `def` on 25.10 would +#: block the event loop. Both flavours must therefore be valid Python, always. EXPECTED_BLOCKS = { "B2_BLOCK", "RESTIC_BLOCK", - "SNAPSHOT_BLOCK", - "CRUD_BLOCK", - "SYNC_BLOCK", + "SNAPSHOT_ASYNC", + "SNAPSHOT_SYNC", + "CRUD_ASYNC", + "CRUD_SYNC", + "SYNC_ASYNC", + "SYNC_SYNC", } +NESTED_BLOCKS = ["SNAPSHOT_ASYNC", "SNAPSHOT_SYNC", "CRUD_ASYNC", "CRUD_SYNC", + "SYNC_ASYNC", "SYNC_SYNC"] + def heredoc_source(): with open(APPLY_SH, encoding="utf-8") as fh: @@ -32,18 +45,30 @@ def heredoc_source(): def extract_blocks(): + """The blocks as apply.sh actually builds them. + + EVALUATED, not read off as string literals: each nested block is a CORE + concatenated with a flavour-specific wrapper, so reading only `ast.Constant` + would silently return nothing for them -- a green suite over blocks nobody + checked. Assignments that need the runtime (argv, imports) simply fail to + evaluate and are skipped. + """ tree = ast.parse(heredoc_source()) - blocks = {} - for node in ast.walk(tree): - if isinstance(node, ast.Assign): - for tgt in node.targets: - if ( - isinstance(tgt, ast.Name) - and tgt.id.endswith("_BLOCK") - and isinstance(node.value, ast.Constant) - and isinstance(node.value.value, str) - ): - blocks[tgt.id] = node.value.value + ns, blocks = {}, {} + for node in tree.body: + if not isinstance(node, ast.Assign): + continue + try: + value = eval( # noqa: S307 - our own shipped source, on purpose + compile(ast.Expression(node.value), "", "eval"), {}, ns + ) + except Exception: + continue + for tgt in node.targets: + if isinstance(tgt, ast.Name) and isinstance(value, str): + ns[tgt.id] = value + if tgt.id in EXPECTED_BLOCKS: + blocks[tgt.id] = value return blocks @@ -98,7 +123,7 @@ def test_injected_block_carries_the_idempotency_marker(name): assert extract_blocks()[name].lstrip("\n").startswith("# TRUECLOUD_PATCH") -@pytest.mark.parametrize("name", ["SNAPSHOT_BLOCK", "CRUD_BLOCK", "SYNC_BLOCK"]) +@pytest.mark.parametrize("name", NESTED_BLOCKS) def test_nested_blocks_degrade_safely_without_the_module(name): # If _truecloud_nested failed to install, every nested block must no-op. # Critically this includes CRUD_BLOCK: relaxing the guard without the @@ -119,20 +144,21 @@ class TestSnapshotLeak: # 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_BLOCK"] + block = extract_blocks()["SNAPSHOT_ASYNC"] assert "except Exception:" in block assert "delete_snapshot_tree" in block assert "raise" in block def test_sync_block_cleans_up_on_every_path(self): - block = extract_blocks()["SYNC_BLOCK"] + block = extract_blocks()["SYNC_ASYNC"] assert "finally:" in block assert "cleanup_task" in block def test_crud_block_is_scoped_to_cloud_backup(): # cloudsync has no staging teardown wired in, so its guard must stay. - assert '!= "cloud_backup"' in extract_blocks()["CRUD_BLOCK"] + for name in ("CRUD_ASYNC", "CRUD_SYNC"): + assert '!= "cloud_backup"' in extract_blocks()[name] class TestIndependentModules: @@ -220,7 +246,7 @@ class TestIndependentModules: # find the string in our own patch and never detect native support. sh = self._sh() assert "split('\\n# TRUECLOUD_PATCH', 1)[0]" in sh - assert "no further nesting" in extract_blocks()["CRUD_BLOCK"], ( + assert "no further nesting" in extract_blocks()["CRUD_ASYNC"], ( "if this ever stops being true, the probe comment is stale" ) @@ -264,7 +290,7 @@ class TestOptIn: # The guard-relaxing crud.py patch must be inside the enabled branch. src = heredoc_source() gate = src.index("if not nested_needed:") - crud = src.index("patch_file(crud_py, CRUD_BLOCK)") + crud = src.index("patch_file(crud_py, _crud_block)") assert gate < crud, "crud.py patch must sit inside the opt-in branch" def test_disabling_REVERTS_the_patch_rather_than_merely_skipping_it(self): @@ -282,7 +308,7 @@ class TestOptIn: assert "from mw_patch import patch_file, revert_nested" in src gate = src.index("if not nested_needed:") revert = src.index("reverted = revert_nested(") - patch = src.index("patch_file(crud_py, CRUD_BLOCK)") + patch = src.index("patch_file(crud_py, _crud_block)") assert gate < revert < patch, "revert belongs in the not-needed branch" def test_import_failure_skips_the_patch_rather_than_crashing(self): @@ -302,9 +328,9 @@ def test_guard_is_relaxed_only_after_traversal_is_installed(): src = heredoc_source() order = [ src.index("shutil.copyfile(nested_src, nested_dst)"), - src.index("patch_file(snapshot_py, SNAPSHOT_BLOCK)"), - src.index("patch_file(sync_path, SYNC_BLOCK)"), - src.index("patch_file(crud_py, CRUD_BLOCK)"), + src.index("patch_file(snapshot_py, _snapshot_block)"), + src.index("patch_file(sync_path, _sync_block)"), + src.index("patch_file(crud_py, _crud_block)"), ] assert order == sorted(order), "crud.py must be patched last" @@ -324,7 +350,7 @@ class TestWrappersDoNotHardcodeStockArity: """ def test_restic_backup_forwards_rather_than_naming_stock_params(self): - block = extract_blocks()["SYNC_BLOCK"] + block = extract_blocks()["SYNC_ASYNC"] assert "async def restic_backup(middleware, job, cloud_backup, *args, **kwargs)" in block assert "_tc_orig_restic_backup(middleware, job, cloud_backup, *args, **kwargs)" in block diff --git a/tests/test_compat.py b/tests/test_compat.py index 5ca6d9c..7de58b7 100644 --- a/tests/test_compat.py +++ b/tests/test_compat.py @@ -129,14 +129,11 @@ class TestFalseOkWouldBreakBackups: })) assert is_broken(r[PROVIDERS]) - def test_async_to_sync_is_caught(self): - # THE TrueNAS 26 change. + def test_a_vanished_symbol_is_broken(self): r = check_files(with_(**{ - "plugins/cloud/snapshot.py": - 'def create_snapshot(middleware, path, name="x"):\n return 1, 2\n', + "plugins/cloud/snapshot.py": "def something_else():\n pass\n", })) assert is_broken(r[NESTED]) - assert "async def" in r[NESTED]["problems"][0]["detail"] class TestFalseBrokenWouldDisableWorkingBoxes: @@ -172,8 +169,7 @@ class TestFalseBrokenWouldDisableWorkingBoxes: r = check_files(with_(**{ "plugins/cloud/snapshot.py": Unreadable("HTTP 429"), "plugins/cloud_backup/sync.py": - "def restic_backup(middleware, job, cloud_backup, dry_run=False, " - "rate_limit=None):\n pass\n", + "async def restic_backup(job, middleware, cloud_backup):\n pass\n", })) assert is_broken(r[NESTED]), "unknown must not launder away a proven break" @@ -189,7 +185,7 @@ class TestTheNativeVerdict: r = check_files(with_(**{ "plugins/cloud/crud.py": "class CloudTaskServiceMixin:\n" - " def _validate(self, app, verrors, name, data):\n" + " async def _validate(self, verrors, name):\n" " verrors.add('x', 'no children allowed')\n", })) assert r[NESTED]["native"] @@ -231,3 +227,47 @@ class TestUpdateReadmeCannotPublishAGuess: with pytest.raises(Unreadable): compat.update_readme(rows, path=str(readme)) assert "old" in readme.read_text(), "a blip must not repaint the matrix" + + +class TestAsyncFlavour: + """TrueNAS <= 25.10 is async; 26 is synchronous. Both are supported -- apply.sh + injects the wrapper that matches. So asyncness is DETECTED, never assumed.""" + + def test_async_middleware_is_detected(self): + assert compat.async_flavour(loader(GOOD)) is True + + def test_sync_middleware_is_detected(self): + sync = dict(GOOD) + sync["plugins/cloud/snapshot.py"] = ( + 'def create_snapshot(middleware, path, name="x"):\n return "s", "p"\n' + ) + sync["plugins/cloud/crud.py"] = ( + "class CloudTaskServiceMixin:\n" + " def _validate(self, app, verrors, name, data):\n" + " verrors.add('x', 'no further nesting')\n" + ) + sync["plugins/cloud_backup/sync.py"] = ( + "def restic_backup(middleware, job, cloud_backup, dry_run=False, " + "rate_limit=None):\n pass\n" + ) + assert compat.async_flavour(loader(sync)) is False + + def test_a_HALF_converted_middleware_is_refused(self): + # The dangerous middle. If iX converts create_snapshot but not restic_backup, + # there is no single wrapper flavour that works -- and guessing means either + # a coroutine unpacked as a tuple, or the event loop blocked. None means + # "do not patch"; apply.sh turns that into a skip, not a guess. + half = dict(GOOD) + half["plugins/cloud/snapshot.py"] = ( + 'def create_snapshot(middleware, path, name="x"):\n return "s", "p"\n' + ) + assert compat.async_flavour(loader(half)) is None + + def test_an_unreadable_source_refuses_rather_than_guesses(self): + broken = dict(GOOD) + 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 diff --git a/tests/test_truecloud_nested.py b/tests/test_truecloud_nested.py index d85386c..ea7e418 100644 --- a/tests/test_truecloud_nested.py +++ b/tests/test_truecloud_nested.py @@ -12,7 +12,6 @@ Two rules are under test above all else: on EVERY run. """ -import asyncio import os import sys @@ -199,12 +198,20 @@ class TestSnapshotTreeNames: class FakeMiddleware: + """middlewared as this module actually uses it: `call_sync`, from a thread. + + The module is synchronous on purpose -- see the orchestration note in + 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. + """ + def __init__(self, snapshots=None): self.snapshots = list(snapshots or []) self.calls = [] self.logger = None - async def call(self, method, *args): + def call_sync(self, method, *args): self.calls.append((method, args)) if method == "zfs.snapshot.query": return [{"name": n} for n in self.snapshots] @@ -222,8 +229,35 @@ class FakeMiddleware: return True raise AssertionError(f"unexpected call {method}") - async def run_in_thread(self, fn, *args): - return fn(*args) + +def stub_core(monkeypatch, tn, *, plan=None, order=None, plan_raises=None): + """Replace the blocking core (plan/apply/verify/teardown) with recorders. + + stage_nested calls these directly now, so they are patched by NAME rather than + intercepted at a `run_in_thread` boundary that no longer exists. + """ + def record(name, result): + def fn(*args, **kwargs): + if order is not None: + order.append(name) + if name == "plan_staging" and plan_raises is not None: + raise plan_raises + return result() if callable(result) else result + fn.__name__ = name + return fn + + real_write = tn._write_sidecar + + def write_sidecar(*args, **kwargs): + if order is not None: + order.append("_write_sidecar") + return real_write(*args, **kwargs) + + monkeypatch.setattr(tn, "_write_sidecar", write_sidecar) + monkeypatch.setattr(tn, "plan_staging", record("plan_staging", plan or ([], []))) + monkeypatch.setattr(tn, "apply_plan", record("apply_plan", True)) + monkeypatch.setattr(tn, "verify_staged", record("verify_staged", True)) + monkeypatch.setattr(tn, "teardown", record("teardown", [])) class TestDeleteSnapshotTree: @@ -231,20 +265,20 @@ class TestDeleteSnapshotTree: mw = FakeMiddleware([ "Tap@snap", "Tap/apps@snap", "Tap/apps/lidarr@snap", "Tap@keepme", ]) - asyncio.run(delete_snapshot_tree(mw, "Tap@snap")) + delete_snapshot_tree(mw, "Tap@snap") 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"]) - asyncio.run(delete_snapshot_tree(mw, "Tap@snap")) + delete_snapshot_tree(mw, "Tap@snap") 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"]) - asyncio.run(delete_snapshot_tree(mw, "Tap@snap")) + delete_snapshot_tree(mw, "Tap@snap") assert mw.snapshots == [] deletes = [a for m, a in mw.calls if m == "zfs.snapshot.delete"] assert len(deletes) == 1, "should be ONE recursive call, not one per snapshot" @@ -255,20 +289,20 @@ class TestDeleteSnapshotTree: def test_survives_recursive_and_query_failure_by_deleting_the_parent(self): class Broken(FakeMiddleware): - async def call(self, method, *args): + def call_sync(self, method, *args): if method == "zfs.snapshot.query": raise RuntimeError("boom") if method == "zfs.snapshot.delete" and len(args) > 1: raise RuntimeError("recursive delete unavailable") - return await super().call(method, *args) + return super().call_sync(method, *args) mw = Broken(["Tap@snap"]) - asyncio.run(delete_snapshot_tree(mw, "Tap@snap")) + delete_snapshot_tree(mw, "Tap@snap") assert mw.snapshots == [] def test_leaves_unrelated_snapshots_alone_when_the_tree_is_gone(self): mw = FakeMiddleware(["Tap@unrelated"]) - asyncio.run(delete_snapshot_tree(mw, "Tap@snap")) + delete_snapshot_tree(mw, "Tap@snap") assert mw.snapshots == ["Tap@unrelated"] @@ -281,20 +315,13 @@ class TestStageNestedOrdering: monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) order = [] + stub_core(monkeypatch, tn, order=order, + plan=([("/src", str(tmp_path / "cloud_backup-5"))], [])) - class Recorder(FakeMiddleware): - async def run_in_thread(self, fn, *args): - order.append(fn.__name__) - if fn.__name__ == "plan_staging": - return ([("/src", str(tmp_path / "cloud_backup-5"))], []) - if fn.__name__ in ("apply_plan", "verify_staged", "teardown"): - return [] if fn.__name__ == "teardown" else True - return fn(*args) - - asyncio.run(tn.stage_nested( - Recorder(), "/mnt/Tap", "Tap@snap", "Tap", "/mnt/Tap", + tn.stage_nested( + FakeMiddleware(), "/mnt/Tap", "Tap@snap", "Tap", "/mnt/Tap", "cloud_backup-5", DATASETS, - )) + ) assert order.index("_write_sidecar") < order.index("apply_plan") @@ -312,28 +339,12 @@ class TestStageNestedOrdering: fh.write("Tap@old-crashed-run") mw = FakeMiddleware(["Tap@old-crashed-run", "Tap/apps@old-crashed-run"]) + stub_core(monkeypatch, tn, plan=([("/src", root)], [])) - class Stub(FakeMiddleware): - def __init__(self, inner): - super().__init__() - self.inner = inner - - async def call(self, method, *args): - return await self.inner.call(method, *args) - - async def run_in_thread(self, fn, *args): - if fn.__name__ == "plan_staging": - return ([("/src", root)], []) - if fn.__name__ == "teardown": - return [] - if fn.__name__ in ("apply_plan", "verify_staged"): - return True - return fn(*args) - - asyncio.run(tn.stage_nested( - Stub(mw), "/mnt/Tap", "Tap@new", "Tap", "/mnt/Tap", + tn.stage_nested( + mw, "/mnt/Tap", "Tap@new", "Tap", "/mnt/Tap", "cloud_backup-5", DATASETS, - )) + ) assert mw.snapshots == [], "the crashed run's snapshot tree must be reclaimed" @@ -342,18 +353,13 @@ class TestStageNestedOrdering: monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) root = tn.staging_root_for("cloud_backup-5") - - class Failing(FakeMiddleware): - async def run_in_thread(self, fn, *args): - if fn.__name__ == "plan_staging": - raise StagingError("boom") - return fn(*args) + stub_core(monkeypatch, tn, plan_raises=StagingError("boom")) with pytest.raises(StagingError): - asyncio.run(tn.stage_nested( - Failing(), "/mnt/Tap", "Tap@snap", "Tap", "/mnt/Tap", + tn.stage_nested( + FakeMiddleware(), "/mnt/Tap", "Tap@snap", "Tap", "/mnt/Tap", "cloud_backup-5", DATASETS, - )) + ) assert not os.path.exists(sidecar_for(root)) @@ -374,7 +380,7 @@ class TestCleanupTask: mw = FakeMiddleware(["Tap@snap", "Tap/apps@snap"]) monkeypatch.setattr(tn, "teardown", lambda *_a, **_k: []) - asyncio.run(cleanup_task(mw, "cloud_backup-5")) + cleanup_task(mw, "cloud_backup-5") assert mw.snapshots == [] assert not os.path.exists(sidecar_for(root)) @@ -384,7 +390,7 @@ class TestCleanupTask: monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path / "nope")) mw = FakeMiddleware(["Tap@snap"]) - asyncio.run(cleanup_task(mw, "cloud_backup-5")) + cleanup_task(mw, "cloud_backup-5") assert mw.calls == [] assert mw.snapshots == ["Tap@snap"] diff --git a/tools/compat.py b/tools/compat.py index 6822df2..8defeb5 100644 --- a/tools/compat.py +++ b/tools/compat.py @@ -105,19 +105,22 @@ ASSUMPTIONS = [ ), # ── nested snapshots. Every block here is an async wrapper. ─────────────── + # is_async is deliberately NOT asserted on these three. The patch now injects an + # async OR a sync wrapper to match whichever the installed middleware declares + # (TrueNAS <= 25.10 is async; 26 rewrote them synchronous), so asyncness is a + # thing to DETECT, not a thing to require -- see async_flavour(). What must still + # hold is the shape: same name, same leading positional parameters. Assumption( "create-snapshot", NESTED, "plugins/cloud/snapshot.py", "create_snapshot", - is_async=True, params=["middleware", "path", "name"], - why="SNAPSHOT_BLOCK replaces it with `async def` that AWAITS the original " - "and returns (snapshot, staging_root). TrueNAS 26 made it synchronous: " - "the wrapper would return a coroutine that sync.py unpacks as a tuple", + params=["middleware", "path", "name"], + why="SNAPSHOT_BLOCK wraps it and returns (snapshot, staging_root) instead of " + "(snapshot, snap_path)", ), Assumption( "crud-mixin-validate", NESTED, "plugins/cloud/crud.py", "CloudTaskServiceMixin._validate", - kind="method", is_async=True, params=["self", "app", "verrors", "name", "data"], - why="CRUD_BLOCK replaces it with `async def` that AWAITS the original, to " - "drop the no-further-nesting error", + kind="method", params=["self", "app", "verrors", "name", "data"], + why="CRUD_BLOCK wraps it to drop the no-further-nesting error", ), Assumption( # SYNC_BLOCK's wrapper is (middleware, job, cloud_backup, *args, **kwargs) and @@ -125,10 +128,9 @@ ASSUMPTIONS = [ # 25.04 have `(…, dry_run)`, 25.10 added `rate_limit`. Only the leading three # are named by the patch, so only they have to hold. "restic-backup", NESTED, "plugins/cloud_backup/sync.py", "restic_backup", - is_async=True, forwards=True, + forwards=True, params=["middleware", "job", "cloud_backup"], - why="SYNC_BLOCK replaces it with `async def` that AWAITS the original, to " - "tear down bind mounts in a finally", + why="SYNC_BLOCK wraps it to tear down bind mounts in a finally", ), ] @@ -462,6 +464,50 @@ def check(loader, modules=None) -> dict: return out +#: The three stock symbols the nested module wraps. TrueNAS <= 25.10 declares them +#: `async def`; TrueNAS 26 rewrote them synchronous. apply.sh injects the wrapper +#: that matches, so this is the question it has to answer at every boot. +NESTED_WRAPPED = [ + ("plugins/cloud/snapshot.py", "create_snapshot"), + ("plugins/cloud/crud.py", "CloudTaskServiceMixin._validate"), + ("plugins/cloud_backup/sync.py", "restic_backup"), +] + + +def async_flavour(loader) -> bool | None: + """Is the installed cloud_backup path async? True, False, or None if unclear. + + None means "do not patch": either a symbol is missing, or -- the case worth + naming -- the three DISAGREE. A middleware caught half-converted is one this + patch has never seen, and guessing a flavour there means injecting an `async def` + that a synchronous caller unpacks as a tuple. Declining costs a feature; guessing + costs a backup. + """ + flavours = set() + for path, symbol in NESTED_WRAPPED: + try: + src = loader(path) + except Unreadable: + return None + if src is None: + return None + try: + tree = ast.parse(_stock(src)) + except SyntaxError: + return None + + node = _find(tree, symbol) + if not isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef): + return None + flavours.add(isinstance(node, ast.AsyncFunctionDef)) + + return flavours.pop() if len(flavours) == 1 else None + + +def async_flavour_tree(root: str) -> bool | None: + return async_flavour(lambda p: _read(root, p)) + + def check_ref(ref: str, modules=None) -> dict: return check(lambda p: _fetch(ref, p), modules)