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." + )