From bb26edf3514a6a04f5f9d91c9f163b4ad9524e53 Mon Sep 17 00:00:00 2001 From: flan Date: Sun, 12 Jul 2026 21:52:09 +0000 Subject: [PATCH] Make nested snapshots opt-in; fix snapshot leaks found in audit Opt-in ------ Nested-dataset snapshot support changes how backups read their source data, so it is now off by default and gated behind a marker file: install.sh --enable-nested-snapshots install.sh --disable-nested-snapshots With neither flag install.sh preserves the current setting, so a routine `git pull && bash install.sh` can never silently flip it. When disabled, apply.sh skips the patch entirely and the stock guard remains. uninstall.sh tears down staging mounts and removes the marker. Snapshot lifecycle ------------------ zfs.snapshot.delete defaults to recursive=False and stock restic_backup() calls it with no options. Stock is safe only because its validation means recursive is never True in the field. Enabling nested datasets makes recursive snapshots real: the parent then has one child snapshot per descendant dataset (160+ on an Apps pool), so stock's delete would orphan every child on EVERY successful run. The patch now owns the lifecycle end to end: - delete_snapshot_tree() sweeps the parent and all children, and is idempotent against stock's finally winning the race once our mounts are released - on a staging failure the tree is deleted here, because sync.py never completes `snapshot, local_path = await create_snapshot(...)` and so its finally deletes nothing at all - the snapshot is recorded in a sidecar file before anything is mounted, so a middlewared restart mid-backup cannot orphan it - a crashed run's snapshot tree is reclaimed on the next run instead of being overwritten and leaked Silent-omission fix ------------------- The dataset list is now enumerated AFTER the snapshot. Read beforehand it could miss a dataset created in the gap, which the recursive snapshot would capture but the staging plan would not -- silently omitting its data. Read afterwards, an unsnapshotted dataset trips the staging check and fails the run loudly. Also from the audit ------------------- - plan_staging scopes by dataset name, so skipped-dataset warnings no longer include every mountpoint-less dataset on the box, which buried the ones that matter - staging_root_for rejects "." / ".." components that would escape the staging base, and resolves STAGING_BASE at call time rather than freezing it into a default argument - uninstall.sh no longer `rm -rf`s a tree that may still contain live bind mounts, and unmounts by path depth rather than string length - apply_plan takes an injectable isdir; verify_staged drops an unused parameter - pin the shellcheck action instead of tracking @master 61 tests, ruff and shellcheck clean. --- .github/workflows/ci.yml | 4 +- .gitignore | 1 + CHANGELOG.md | 26 ++- README.md | 12 ++ install.sh | 75 +++++++ patch/apply.sh | 105 ++++++---- patch/truecloud_nested.py | 274 ++++++++++++++++++------ tests/test_apply_blocks.py | 44 ++++ tests/test_truecloud_nested.py | 371 ++++++++++++++++++++++++++------- uninstall.sh | 46 ++++ 10 files changed, 785 insertions(+), 173 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a748d05..c71a4c2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,8 +24,10 @@ jobs: done < <(find . -name '*.sh' -not -path './.git/*') exit $fail + # Pinned to a release tag, not @master: a third-party action on a moving + # branch runs whatever that branch contains at the time CI fires. - name: shellcheck - uses: ludeeus/action-shellcheck@master + uses: ludeeus/action-shellcheck@2.0.0 env: SHELLCHECK_OPTS: -S warning -e SC1091 diff --git a/.gitignore b/.gitignore index 2d4f5cb..daf6914 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,7 @@ /apply.log.2 /hook_status.json /disabled +/nested_snapshots_enabled # Python __pycache__/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bbb1ee..cda9e3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,13 @@ ### Added -- **`snapshot = true` now works on datasets that have child datasets.** +- **`snapshot = true` now works on datasets that have child datasets** — + **opt-in, off by default** (`install.sh --enable-nested-snapshots` / + `--disable-nested-snapshots`). It changes how backups read their source data, + so it is never enabled implicitly; with neither flag `install.sh` preserves + the existing setting, so a `git pull && bash install.sh` cannot silently flip + it. When disabled, `apply.sh` skips the patch entirely and the stock guard + remains. `uninstall.sh` tears down any staging mounts and removes the marker. Stock TrueNAS refuses this with *"This option is only available for datasets that have no further nesting"*, which makes the snapshot option unusable for the single most common case on any box running Apps — every app is its own @@ -46,6 +52,24 @@ `snapshot.py`, then `sync.py`, and only then `crud.py`. A partial failure leaves the guard intact and the option merely unavailable — never "guard removed, traversal missing". + - **The patch owns the whole snapshot lifecycle.** `zfs.snapshot.delete` + defaults to `recursive=False` and stock `restic_backup()` calls it with no + options. Stock gets away with that only because its validation means + `recursive` is never True in the field — but enabling nested datasets makes + recursive snapshots real, so the parent now has one child snapshot per + descendant dataset (160+ on a typical Apps pool). Relying on stock's delete + would therefore orphan every child snapshot **on every successful run**. + This patch sweeps the parent *and* all children, is idempotent against + stock's `finally` winning the race, records the snapshot in a sidecar file + (so a middlewared restart mid-backup cannot orphan it), reclaims the tree + left by a crashed run, and deletes the tree when staging fails — where + sync.py's own `finally` would otherwise delete nothing at all, because its + `snapshot` local never gets assigned. + - **The dataset list is enumerated *after* the snapshot, never before.** A + list read beforehand can miss a dataset created in the gap: the recursive + snapshot would capture it but the staging plan would not, silently omitting + its data. Read afterwards, an unsnapshotted dataset trips the staging check + and fails the run loudly instead. - **Every injected block no-ops** if `_truecloud_nested` is absent. - Datasets that cannot contribute to a file tree (`mountpoint=none|legacy`, unmounted/locked, encrypted-and-locked) are skipped and **reported** — diff --git a/README.md b/README.md index 3b4aaed..5c69057 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,18 @@ support and the reason is logged to `apply.log` in your repo root. ## Nested-dataset snapshots +> **Opt-in, and off by default.** This feature changes how backups read their +> source data, so it is never enabled implicitly: +> +> ```bash +> bash install.sh --enable-nested-snapshots # turn it on +> bash install.sh --disable-nested-snapshots # turn it back off +> ``` +> +> With neither flag, `install.sh` leaves the current setting alone — so a +> `git pull && bash install.sh` can never silently flip it. The B2/S3 provider +> patch is unaffected either way. + TrueCloud Backup's **Take Snapshot** option makes restic read from a frozen ZFS snapshot instead of live files. Without it the backup reads data *while apps are writing to it* — databases get captured mid-write, and an app that rewrites its diff --git a/install.sh b/install.sh index b906268..56facbe 100755 --- a/install.sh +++ b/install.sh @@ -23,6 +23,45 @@ VERSION="0.3.0" # The directory containing install.sh is the permanent install location. PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" _HOOK_COMMENT='TrueCloud provider patch (S3/B2)' +_NESTED_MARKER="$PATCH_DIR/nested_snapshots_enabled" + +# ── Options ─────────────────────────────────────────────────────────────────── +# Nested-dataset snapshot support is OPT-IN and off by default. It changes how +# backups read their source data, so an unattended re-run (e.g. after a +# `git pull`) must never flip it on or off by itself: with neither flag given, +# whatever was chosen previously is preserved. +_nested_choice="" + +usage() { + cat <&2 + echo "" >&2 + usage >&2 + exit 1 + ;; + esac + shift +done if [ ! -f "$PATCH_DIR/patch/apply.sh" ]; then echo "ERROR: patch files not found at $PATCH_DIR/patch/" >&2 @@ -104,6 +143,42 @@ if [ -f "$PATCH_DIR/disabled" ]; then echo "" fi +# ── Nested-dataset snapshot support (opt-in) ────────────────────────────────── + +case "$_nested_choice" in + on) + touch "$_NESTED_MARKER" + echo "Nested-dataset snapshots: ENABLED" + echo " The \"Take Snapshot\" option will be allowed on datasets that have" + echo " child datasets. Backups then read from a frozen, complete staging" + echo " tree instead of live files." + echo "" + echo " This changes how your backups read their source data. Verify that a" + echo " backup completes AND that its restic snapshot actually contains" + echo " child-dataset data before you rely on it." + ;; + off) + if [ -f "$_NESTED_MARKER" ]; then + rm -f "$_NESTED_MARKER" + echo "Nested-dataset snapshots: DISABLED (stock guard restored)." + echo " Any task that already has snapshot=true on a nested dataset will" + echo " fail validation on its next edit. Turn the option off on those" + echo " tasks, or re-run with --enable-nested-snapshots." + else + echo "Nested-dataset snapshots: already disabled." + fi + ;; + *) + if [ -f "$_NESTED_MARKER" ]; then + echo "Nested-dataset snapshots: enabled (unchanged)." + else + echo "Nested-dataset snapshots: disabled (default)." + echo " Enable with: bash install.sh --enable-nested-snapshots" + fi + ;; +esac +echo "" + # ── Apply now ───────────────────────────────────────────────────────────────── echo "Applying patches ..." diff --git a/patch/apply.sh b/patch/apply.sh index ab35268..96b9907 100755 --- a/patch/apply.sh +++ b/patch/apply.sh @@ -187,14 +187,25 @@ else _SYNC_PY="$_MW_DIR/plugins/cloud_backup/sync.py" _NESTED_SRC="$PATCH_DIR/patch/truecloud_nested.py" + # Nested-dataset snapshot support is OPT-IN. It changes how backups read + # their source data, so it is never enabled implicitly by a `git pull`. + # Enable: bash install.sh --enable-nested-snapshots + # Disable: bash install.sh --disable-nested-snapshots + if [ -f "$PATCH_DIR/nested_snapshots_enabled" ]; then + _NESTED_ENABLED=1 + else + _NESTED_ENABLED=0 + fi + # ── patch b2.py + restic.py + nested-snapshot + hook_status.json ──────── # (single subprocess: PREINIT has a tight timeout budget) if "$PYTHON" - "$_B2_PY" "$_RESTIC_PY" "$PATCH_DIR/hook_status.json" \ - "$_CLOUD_DIR" "$_SYNC_PY" "$_NESTED_SRC" << 'PYEOF' + "$_CLOUD_DIR" "$_SYNC_PY" "$_NESTED_SRC" "$_NESTED_ENABLED" << 'PYEOF' import json, os, shutil, sys, time b2_path, restic_path, status_path = sys.argv[1], sys.argv[2], sys.argv[3] cloud_dir, sync_path, nested_src = sys.argv[4], sys.argv[5], sys.argv[6] +nested_enabled = sys.argv[7] == "1" B2_BLOCK = """ # TRUECLOUD_PATCH — added by truenas-truecloud-patch/patch/apply.sh @@ -277,25 +288,42 @@ if _tc_nested is not None: _tc_orig_create_snapshot = create_snapshot async def create_snapshot(middleware, path, name="cloud_task-onetime"): - # Determine nesting BEFORE delegating. We must never silently fall back - # to stock behaviour on a nested path: stock returns the parent's - # .zfs/snapshot/ path, where children are invisible, which is exactly - # the near-empty backup this feature exists to prevent. If we cannot - # tell, we raise -- loud failure beats a backup that lies. - datasets = await middleware.call("zfs.dataset.query", [["type", "=", "FILESYSTEM"]]) - dataset, nested = get_dataset_recursive(datasets, path) - # Stock takes the (already recursive) snapshot; we only replace the PATH. snapshot, snap_path = await _tc_orig_create_snapshot(middleware, path, name) - if not nested: - return snapshot, snap_path # no children: stock behaviour, untouched + _logger = getattr(middleware, "logger", None) + try: + # Enumerate datasets AFTER the snapshot, never before. The snapshot is + # the point-in-time truth; a list read beforehand could miss a dataset + # created in the gap, which the recursive snapshot WOULD capture but + # 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( + "zfs.dataset.query", [["type", "=", "FILESYSTEM"]] + ) + dataset, 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). + return snapshot, snap_path + + staging_root = await _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) + raise - staging_root = await _tc_nested.stage_nested( - middleware, path, snapshot, - dataset["properties"]["mountpoint"]["value"], name, - logger=getattr(middleware, "logger", None), - ) return snapshot, staging_root create_snapshot._truecloud_patched = True @@ -401,29 +429,34 @@ else: # guard LAST. If anything fails partway, the guard is still in place and the # option stays unavailable -- we never expose "guard removed, traversal missing". nested_detail = '' -try: - snapshot_py = os.path.join(cloud_dir, 'snapshot.py') - crud_py = os.path.join(cloud_dir, 'crud.py') - nested_dst = os.path.join(cloud_dir, '_truecloud_nested.py') +if not nested_enabled: + nested_detail = 'disabled (opt-in; enable with: install.sh --enable-nested-snapshots)' + print('INFO: Nested-dataset snapshot support is disabled (opt-in feature).') + print('INFO: Enable with: bash install.sh --enable-nested-snapshots') +else: + try: + snapshot_py = os.path.join(cloud_dir, 'snapshot.py') + crud_py = os.path.join(cloud_dir, 'crud.py') + nested_dst = os.path.join(cloud_dir, '_truecloud_nested.py') - missing = [p for p in (snapshot_py, crud_py, sync_path, nested_src) if not os.path.exists(p)] - if missing: - raise FileNotFoundError('missing: ' + ', '.join(missing)) + missing = [p for p in (snapshot_py, crud_py, sync_path, nested_src) if not os.path.exists(p)] + if missing: + raise FileNotFoundError('missing: ' + ', '.join(missing)) - 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 + 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 - nested_ok = True - nested_detail = 'nested-dataset snapshots enabled (staging tree)' - print(f'OK: Installed nested-snapshot support → {nested_dst}') - print(f'OK: Patched snapshot.py, sync.py, crud.py → {cloud_dir}') -except Exception as e: - nested_detail = f'not applied: {e}' - print(f'WARNING: Failed to apply nested-snapshot patch: {e}') - print('WARNING: Stock nesting guard remains; snapshot option stays unavailable') - print('WARNING: for nested datasets. Existing backups are unaffected.') + nested_ok = True + nested_detail = 'nested-dataset snapshots enabled (staging tree)' + print(f'OK: Installed nested-snapshot support → {nested_dst}') + print(f'OK: Patched snapshot.py, sync.py, crud.py → {cloud_dir}') + except Exception as e: + nested_detail = f'not applied: {e}' + print(f'WARNING: Failed to apply nested-snapshot patch: {e}') + print('WARNING: Stock nesting guard remains; snapshot option stays unavailable') + print('WARNING: for nested datasets. Existing backups are unaffected.') patches = { 'middlewared.rclone.remote.b2': { diff --git a/patch/truecloud_nested.py b/patch/truecloud_nested.py index cfe631a..0a432f2 100644 --- a/patch/truecloud_nested.py +++ b/patch/truecloud_nested.py @@ -1,4 +1,4 @@ -"""Nested-dataset snapshot support for TrueCloud Backup / Cloud Sync. +"""Nested-dataset snapshot support for TrueCloud Backup. Why this exists --------------- @@ -12,8 +12,8 @@ already takes a **recursive** ZFS snapshot, but it then points the backup tool at the *parent* dataset's ``.zfs/snapshot//`` directory -- and ZFS does not expose child datasets through a parent's snapshot directory:: - /mnt/Tap/.zfs/snapshot//apps/ -> 0 entries (children invisible) - /mnt/Tap/apps/lidarr/config/.zfs/snapshot// -> the real data + /mnt/Tap/.zfs/snapshot//apps/ -> 0 entries + /mnt/Tap/apps/lidarr/config/.zfs/snapshot// -> the real data So without the guard, the backup tool would walk a near-empty tree, report SUCCESS, and upload almost nothing. A backup that lies about succeeding is the @@ -32,19 +32,24 @@ Cardinal safety rule tree. Silently backing up an incomplete tree is precisely the failure this feature exists to prevent, and it would be worse than not having the feature. -Notes ------ -* ZFS snapshots are immutable, so a plain ``mount --bind`` is inherently - read-only; no remount dance is needed. -* Bind-mounting ``.zfs/snapshot/`` pins the snapshot, so ``zfs destroy`` - of that snapshot returns EBUSY until we unmount. Stock ``restic_backup()`` - deletes the snapshot in its ``finally``, which therefore logs one benign - "Error deleting snapshot ... busy" warning; :func:`cleanup_task` then unmounts - and deletes the snapshot for real. See ``patch/apply.sh``. -* Staging roots live under a stable, per-task path so that the backup tool sees - the *same* path every run. Stock's ``.zfs/snapshot/-/`` path - changes every run, which defeats restic's parent-snapshot detection; the - staging tree is an improvement on that. +Snapshot lifecycle -- read this before changing anything +-------------------------------------------------------- +``zfs.snapshot.delete`` 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 +(``Tap@snap``) has one child snapshot per descendant dataset (160+ here). +Deleting only the parent would orphan every child on **every successful run**. + +Therefore this module owns the whole lifecycle: + +* :func:`delete_snapshot_tree` sweeps the parent *and* every child snapshot, and + is idempotent -- it copes with stock's ``finally`` having already removed the + parent. +* The snapshot name is recorded in a sidecar file next to the staging root, not + only in memory, so a middlewared restart mid-backup cannot orphan it. +* Bind-mounting ``.zfs/snapshot/`` pins the snapshot, so stock's delete + fails with EBUSY and logs one benign warning; we unmount and then sweep. """ from __future__ import annotations @@ -54,21 +59,27 @@ import os import subprocess __all__ = [ - "StagingError", - "STAGING_BASE", "ACTIVE", - "staging_root_for", - "plan_staging", - "current_mounts_under", + "STAGING_BASE", + "StagingError", "apply_plan", - "verify_staged", + "cleanup_task", + "current_mounts_under", + "delete_snapshot_tree", + "plan_staging", + "sidecar_for", + "snapshot_tree_names", + "stage_nested", + "staging_root_for", "teardown", + "verify_staged", ] #: Where staging trees are assembled. tmpfs; bind mounts consume no space. STAGING_BASE = "/run/truecloud-nested" -#: staging_root -> zfs snapshot name ("pool/ds@snap"), for cleanup. +#: staging_root -> zfs snapshot name. A cache; the sidecar file is the source of +#: truth, so that a middlewared restart cannot orphan a snapshot. ACTIVE: dict[str, str] = {} @@ -76,18 +87,76 @@ class StagingError(Exception): """Staging could not produce a complete tree. The backup must not proceed.""" -def staging_root_for(name: str, base: str = STAGING_BASE) -> str: - """Stable staging root for a task name (e.g. ``cloud_backup-5``).""" - safe = "".join(c if (c.isalnum() or c in "-_.") else "_" for c in name) or "task" +# ── pure helpers ────────────────────────────────────────────────────────────── + + +def staging_root_for(name: str, base: str | None = None) -> str: + """Stable staging root for a task name (e.g. ``cloud_backup-5``). + + ``base`` defaults to :data:`STAGING_BASE` at CALL time, not at import time -- + a ``base=STAGING_BASE`` default would freeze the value into the function + object and silently ignore any later override. + """ + if base is None: + base = STAGING_BASE + safe = "".join(c if (c.isalnum() or c in "-_.") else "_" for c in name) + # A component of "." or ".." would escape STAGING_BASE once joined. + if not safe or safe.strip(".") == "": + safe = "task" return os.path.join(base, safe) +def sidecar_for(staging_root: str) -> str: + """Path of the file recording which ZFS snapshot a staging tree pins.""" + return staging_root + ".snapshot" + + +def _write_sidecar(staging_root: str, snapshot: str) -> None: + """Record the pinned snapshot on disk. Blocking; call via run_in_thread.""" + with contextlib.suppress(OSError): + os.makedirs(os.path.dirname(staging_root), exist_ok=True) + with open(sidecar_for(staging_root), "w", encoding="utf-8") as fh: + fh.write(snapshot) + + +def _read_sidecar(staging_root: str) -> str | None: + """The snapshot a previous run recorded here, if any.""" + try: + with open(sidecar_for(staging_root), encoding="utf-8") as fh: + return fh.read().strip() or None + except OSError: + return None + + +def _remove_sidecar(staging_root: str) -> None: + with contextlib.suppress(OSError): + os.unlink(sidecar_for(staging_root)) + + def _depth(path: str) -> int: return len([p for p in path.split("/") if p]) -def plan_staging(base_mountpoint, path, snapshot_name, datasets, staging_root, - isdir=os.path.isdir): +def snapshot_tree_names(snapshot: str, all_names) -> list[str]: + """Every snapshot produced by ``zfs snapshot -r @``. + + That is the parent plus one per descendant dataset, all sharing the same + name after the ``@``. Pure, so the sweep logic is testable without ZFS. + """ + dataset, _, snapname = snapshot.partition("@") + if not snapname: + return [] + parent = f"{dataset}@{snapname}" + prefix = dataset + "/" + suffix = "@" + snapname + return [ + n for n in all_names + if n == parent or (n.startswith(prefix) and n.endswith(suffix)) + ] + + +def plan_staging(base_dataset, base_mountpoint, path, snapshot_name, datasets, + staging_root, isdir=os.path.isdir): """Compute the bind-mount plan for staging a nested tree. Pure function. ``datasets`` is a list of dicts shaped like ``zfs.dataset.query`` results: @@ -96,9 +165,12 @@ def plan_staging(base_mountpoint, path, snapshot_name, datasets, staging_root, Returns ``(mounts, skipped)`` where ``mounts`` is an ordered list of ``(source, target)`` pairs (parents before children) and ``skipped`` is a - list of ``(dataset_name, reason)``. + list of ``(dataset_name, reason)`` covering only datasets that are *in + scope* -- i.e. descendants of ``base_dataset``. Datasets elsewhere on the + system are ignored silently; reporting them would bury the ones that matter. - Raises StagingError if a descendant holds data we would silently omit. + Raises StagingError if an in-scope descendant holds data we would otherwise + silently omit. """ def snapdir(mountpoint): return os.path.join(mountpoint, ".zfs", "snapshot", snapshot_name) @@ -113,20 +185,30 @@ def plan_staging(base_mountpoint, path, snapshot_name, datasets, staging_root, mounts = [(root_src, staging_root)] skipped = [] - prefix = path.rstrip("/") + "/" + ds_prefix = base_dataset.rstrip("/") + "/" + path_prefix = path.rstrip("/") + "/" + for ds in datasets: + name = ds.get("name", "") + # Scope by DATASET NAME, not mountpoint: a dataset with no mountpoint + # cannot be scoped by path, and scoping by path first would drag in + # every mountpoint-less dataset on the box (all of Tank/.system/*, ...). + if not name.startswith(ds_prefix): + continue + props = ds.get("properties", {}) mp = props.get("mountpoint", {}).get("value", "") - name = ds.get("name", "?") if not mp or mp in ("none", "legacy", "-"): skipped.append((name, f"mountpoint is {mp or 'unset'}")) continue - if not mp.startswith(prefix): - continue # not a descendant of the backup path - mounted = props.get("mounted", {}).get("value", "yes") - if mounted == "no": + if not mp.startswith(path_prefix): + # A descendant dataset mounted outside the backed-up path is + # genuinely not part of this tree. Not an omission. + continue + + if props.get("mounted", {}).get("value", "yes") == "no": # An unmounted (e.g. locked/encrypted) dataset contributes nothing to # the live tree either, so skipping matches stock semantics -- but it # is a real gap and must be visible, never silent. @@ -142,8 +224,7 @@ def plan_staging(base_mountpoint, path, snapshot_name, datasets, staging_root, f"refusing to back up an incomplete tree" ) - target = os.path.join(staging_root, os.path.relpath(mp, path)) - mounts.append((src, target)) + mounts.append((src, os.path.join(staging_root, os.path.relpath(mp, path)))) # Parents before children, so each mountpoint exists before we mount onto it. mounts.sort(key=lambda m: _depth(m[1])) @@ -168,11 +249,14 @@ def current_mounts_under(root, mounts_file="/proc/self/mounts"): return found +# ── mount / unmount ─────────────────────────────────────────────────────────── + + def _run(cmd): return subprocess.run(cmd, capture_output=True, text=True, check=False) -def apply_plan(mounts, runner=_run): +def apply_plan(mounts, runner=_run, 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 @@ -186,7 +270,7 @@ def apply_plan(mounts, runner=_run): try: os.makedirs(staging_root, exist_ok=True) for src, target in mounts: - if not os.path.isdir(target): + if not isdir(target): # Child mountpoint dirs come from the parent snapshot, which is # read-only -- we cannot mkdir them. Only the root is ours. raise StagingError(f"staging target {target!r} does not exist") @@ -206,7 +290,7 @@ def apply_plan(mounts, runner=_run): return staging_root -def verify_staged(mounts, runner=_run, ismount=os.path.ismount, listdir=os.listdir): +def verify_staged(mounts, ismount=os.path.ismount, listdir=os.listdir): """Assert the staged tree is real and complete. Raises StagingError if not. This is the anti-regression guard: it is what stops this feature from ever @@ -251,15 +335,61 @@ def teardown(staging_root, runner=_run, mounts_file="/proc/self/mounts"): # ── async orchestration (middleware is duck-typed; no middlewared import) ───── -async def stage_nested(middleware, path, snapshot, base_mountpoint, task_name, logger=None): +async 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 + no options, so relying on stock would orphan one snapshot per descendant + dataset on every run. Idempotent: tolerates the parent already being gone + (stock's ``finally`` may have won the race once our mounts were released). + """ + dataset = snapshot.partition("@")[0] + + try: + snaps = await middleware.call( + "zfs.snapshot.query", [["name", "^", dataset]], {"select": ["name"]} + ) + # 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]) + except Exception as e: # noqa: BLE001 - fall back to at least the parent + if logger: + logger.warning( + "truecloud-patch: could not enumerate snapshot tree for %s: %r", + snapshot, e, + ) + names = [snapshot] + + for name in names: + try: + await middleware.call("zfs.snapshot.delete", name) + except Exception as e: # noqa: BLE001 - already gone is fine + if logger: + logger.warning( + "truecloud-patch: could not delete snapshot %s: %r", name, e + ) + + +async 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..."). + + `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. + 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 = staging_root_for(task_name) @@ -267,23 +397,45 @@ async def stage_nested(middleware, path, snapshot, base_mountpoint, task_name, l # A previous run may have crashed mid-flight; never build on top of that. await middleware.run_in_thread(teardown, staging_root) - datasets = await middleware.call("zfs.dataset.query", [["type", "=", "FILESYSTEM"]]) + # ...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) + 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) - mounts, skipped = await middleware.run_in_thread( - plan_staging, base_mountpoint, path, snapshot_name, datasets, staging_root - ) - if logger: - for name, reason in skipped: - logger.warning("truecloud-patch: not staging dataset %r: %s", name, reason) + # 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) - await middleware.run_in_thread(apply_plan, mounts) try: + mounts, skipped = await middleware.run_in_thread( + plan_staging, base_dataset, base_mountpoint, path, snapshot_name, + datasets, staging_root, + ) + if logger: + for name, reason in skipped: + logger.warning( + "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) except Exception: await middleware.run_in_thread(teardown, staging_root) + await middleware.run_in_thread(_remove_sidecar, staging_root) raise ACTIVE[staging_root] = snapshot + if logger: logger.info( "truecloud-patch: staged %d dataset(s) from %s at %s", @@ -296,13 +448,14 @@ async 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. - - Stock `restic_backup()` deletes the snapshot in its own `finally`, which - fails with EBUSY while our bind mounts pin it (it logs a warning and moves - on). We unmount here and then delete the snapshot for real. """ staging_root = staging_root_for(task_name) + sidecar = sidecar_for(staging_root) + snapshot = ACTIVE.pop(staging_root, None) + if snapshot is None: + # Sidecar survives a middlewared restart; ACTIVE does not. + snapshot = _read_sidecar(staging_root) if snapshot is None and not os.path.isdir(staging_root): return # never staged; nothing to do @@ -313,10 +466,7 @@ async def cleanup_task(middleware, task_name, logger=None): logger.warning("truecloud-patch: staging teardown: %s", err) if snapshot is not None: - try: - await middleware.call("zfs.snapshot.delete", snapshot) - except Exception as e: # noqa: BLE001 - cleanup must never mask the real error - if logger: - logger.warning( - "truecloud-patch: could not delete snapshot %s: %r", snapshot, e - ) + await delete_snapshot_tree(middleware, snapshot, logger=logger) + + with contextlib.suppress(OSError): + os.unlink(sidecar) diff --git a/tests/test_apply_blocks.py b/tests/test_apply_blocks.py index 64e4139..caaa17a 100644 --- a/tests/test_apply_blocks.py +++ b/tests/test_apply_blocks.py @@ -78,11 +78,55 @@ def test_nested_blocks_degrade_safely_without_the_module(name): assert "if _tc_nested is not None:" in block +class TestSnapshotLeak: + """zfs.snapshot.delete is non-recursive and stock calls it with no options. + + A recursive snapshot has one child per descendant dataset (160+ here), so + 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_BLOCK"] + 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"] + 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"] +class TestOptIn: + """Nested-snapshot support must be opt-in and must never self-enable.""" + + def test_heredoc_gates_on_the_opt_in_flag(self): + src = heredoc_source() + assert "nested_enabled = sys.argv[7]" in src + assert "if not nested_enabled:" in src + + def test_apply_sh_reads_the_marker_file(self): + with open(APPLY_SH, encoding="utf-8") as fh: + sh = fh.read() + assert 'if [ -f "$PATCH_DIR/nested_snapshots_enabled" ]' in sh + assert '"$_NESTED_ENABLED"' in sh + + def test_patching_is_skipped_entirely_when_disabled(self): + # The guard-relaxing crud.py patch must be inside the enabled branch. + src = heredoc_source() + gate = src.index("if not nested_enabled:") + crud = src.index("patch_file(crud_py, CRUD_BLOCK)") + assert gate < crud, "crud.py patch must sit inside the opt-in branch" + + def test_guard_is_relaxed_only_after_traversal_is_installed(): # Ordering in apply.sh is a safety property: copy module -> patch snapshot.py # -> patch sync.py -> patch crud.py. crud.py (which unlocks the feature) must diff --git a/tests/test_truecloud_nested.py b/tests/test_truecloud_nested.py index b3ff060..aba764f 100644 --- a/tests/test_truecloud_nested.py +++ b/tests/test_truecloud_nested.py @@ -1,11 +1,18 @@ """Tests for nested-dataset snapshot staging. -The cardinal rule under test: a tree that cannot be staged completely must fail -LOUDLY. A silently-incomplete backup is the exact failure that stock TrueNAS's -"no further nesting" guard exists to prevent, and it is the one regression this -feature must never introduce. +Two rules are under test above all else: + +1. A tree that cannot be staged completely must fail LOUDLY. A silently + incomplete backup is the exact failure that stock TrueNAS's "no further + nesting" guard exists to prevent. + +2. Every snapshot we cause to exist must be cleaned up. ``zfs.snapshot.delete`` + is non-recursive by default and stock calls it with no options, so a + recursive snapshot would otherwise orphan one snapshot per descendant dataset + on EVERY run. """ +import asyncio import os import sys @@ -14,10 +21,15 @@ import pytest sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "patch")) from truecloud_nested import ( # noqa: E402 + ACTIVE, StagingError, apply_plan, + cleanup_task, current_mounts_under, + delete_snapshot_tree, plan_staging, + sidecar_for, + snapshot_tree_names, staging_root_for, teardown, verify_staged, @@ -47,22 +59,24 @@ DATASETS = [ ds("Tap/apps/immich/pgdata", "/mnt/Tap/apps/immich/pgdata"), ] -ALL_DIRS_EXIST = lambda _p: True # noqa: E731 + +def yes(_path): + return True + + +def plan(datasets=DATASETS, base_dataset="Tap", base_mp="/mnt/Tap", + path="/mnt/Tap", isdir=yes): + return plan_staging(base_dataset, base_mp, path, SNAP, datasets, ROOT, isdir=isdir) class TestPlanStaging: def test_stages_every_descendant_dataset(self): - mounts, skipped = plan_staging( - "/mnt/Tap", "/mnt/Tap", SNAP, DATASETS, ROOT, isdir=ALL_DIRS_EXIST - ) + mounts, skipped = plan() assert skipped == [] - - # Root + all 5 descendants. The base dataset itself is the root, not a - # descendant, so it must not be double-mounted. - assert len(mounts) == 6 + assert len(mounts) == 6 # root + 5 descendants assert mounts[0] == (f"/mnt/Tap/.zfs/snapshot/{SNAP}", ROOT) - by_target = dict((t, s) for s, t in mounts) + by_target = {t: s for s, t in mounts} assert by_target[f"{ROOT}/apps"] == f"/mnt/Tap/apps/.zfs/snapshot/{SNAP}" assert by_target[f"{ROOT}/apps/immich/pgdata"] == ( f"/mnt/Tap/apps/immich/pgdata/.zfs/snapshot/{SNAP}" @@ -71,72 +85,282 @@ 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_staging( - "/mnt/Tap", "/mnt/Tap", SNAP, DATASETS, ROOT, isdir=ALL_DIRS_EXIST - ) + mounts, _ = plan() seen = set() for _src, target in mounts: - parent = os.path.dirname(target) if target != ROOT: - assert parent in seen or parent == ROOT, f"{target} mounted before {parent}" + assert os.path.dirname(target) in seen seen.add(target) def test_backup_path_below_dataset_root(self): - mounts, _ = plan_staging( - "/mnt/Tap", "/mnt/Tap/apps", SNAP, DATASETS, ROOT, isdir=ALL_DIRS_EXIST - ) - # Root source is the *subdirectory* inside the base dataset's snapshot. - assert mounts[0] == (f"/mnt/Tap/.zfs/snapshot/{SNAP}/apps", ROOT) + mounts, _ = plan(base_dataset="Tap/apps", base_mp="/mnt/Tap/apps", + path="/mnt/Tap/apps") + assert mounts[0] == (f"/mnt/Tap/apps/.zfs/snapshot/{SNAP}", ROOT) targets = [t for _s, t in mounts] - assert f"{ROOT}/lidarr" in targets # relative to /mnt/Tap/apps + assert f"{ROOT}/lidarr" in targets assert f"{ROOT}/apps/lidarr" not in targets def test_base_dataset_is_not_a_descendant_of_itself(self): - mounts, _ = plan_staging( - "/mnt/Tap", "/mnt/Tap", SNAP, [ds("Tap", "/mnt/Tap")], ROOT, isdir=ALL_DIRS_EXIST - ) - assert len(mounts) == 1 # just the root + mounts, _ = plan(datasets=[ds("Tap", "/mnt/Tap")]) + assert len(mounts) == 1 -class TestSkipping: - @pytest.mark.parametrize("mp", ["none", "legacy", "-", ""]) - def test_unmountable_mountpoints_are_skipped_and_reported(self, mp): - datasets = DATASETS + [ds("Tap/weird", mp)] - mounts, skipped = plan_staging( - "/mnt/Tap", "/mnt/Tap", SNAP, datasets, ROOT, isdir=ALL_DIRS_EXIST - ) +class TestScoping: + def test_unrelated_datasets_are_ignored_silently(self): + # Regression: scoping by mountpoint first dragged in every + # mountpoint-less dataset on the box (all of Tank/.system/*), burying the + # warnings that actually matter. + noisy = DATASETS + [ + ds("Tank/.system", "none"), + ds("Tank/.system/cores", "legacy"), + ds("Tank/backups", "/mnt/Tank/backups"), + ] + mounts, skipped = plan(datasets=noisy) assert len(mounts) == 6 - assert any(name == "Tap/weird" for name, _reason in skipped) + assert skipped == [], "datasets outside the base dataset must not be reported" + + def test_in_scope_dataset_without_mountpoint_is_reported(self): + datasets = DATASETS + [ds("Tap/apps/weird", "none")] + _mounts, skipped = plan(datasets=datasets) + assert ("Tap/apps/weird", "mountpoint is none") in skipped def test_unmounted_dataset_is_skipped_but_never_silently(self): - # A locked/encrypted dataset contributes nothing to the live tree either, - # so skipping matches stock semantics -- but it MUST be reported. datasets = DATASETS + [ds("Tap/apps/vault", "/mnt/Tap/apps/vault", mounted="no")] - mounts, skipped = plan_staging( - "/mnt/Tap", "/mnt/Tap", SNAP, datasets, ROOT, isdir=ALL_DIRS_EXIST - ) + mounts, skipped = plan(datasets=datasets) assert f"{ROOT}/apps/vault" not in [t for _s, t in mounts] assert ("Tap/apps/vault", "dataset is not mounted (locked/encrypted?)") in skipped + def test_descendant_mounted_outside_the_path_is_not_an_omission(self): + datasets = DATASETS + [ds("Tap/elsewhere", "/mnt/other")] + mounts, skipped = plan(datasets=datasets) + assert len(mounts) == 6 + assert skipped == [] + class TestSilentOmissionGuard: """The whole point of the feature. These are the tests that matter.""" - def test_missing_snapshot_on_descendant_raises(self): - # If the recursive snapshot somehow missed a dataset, staging it would - # silently omit its data. Refuse rather than upload an incomplete tree. - def isdir(path): - return "/mnt/Tap/apps/immich/pgdata/" not in path + @staticmethod + def _missing_pgdata(path): + return "/mnt/Tap/apps/immich/pgdata/" not in path + def test_missing_snapshot_on_descendant_raises(self): with pytest.raises(StagingError, match="incomplete tree"): - plan_staging("/mnt/Tap", "/mnt/Tap", SNAP, DATASETS, ROOT, isdir=isdir) + plan(isdir=self._missing_pgdata) def test_error_names_the_offending_dataset(self): - def isdir(path): - return "/mnt/Tap/apps/immich/pgdata/" not in path - with pytest.raises(StagingError, match="Tap/apps/immich/pgdata"): - plan_staging("/mnt/Tap", "/mnt/Tap", SNAP, DATASETS, ROOT, isdir=isdir) + plan(isdir=self._missing_pgdata) + + +class TestSnapshotTreeNames: + """zfs.snapshot.delete is non-recursive; we must sweep children ourselves.""" + + ALL = [ + "Tap@cloud_backup-5-20260712030000", + "Tap/apps@cloud_backup-5-20260712030000", + "Tap/apps/lidarr/config@cloud_backup-5-20260712030000", + "Tap@auto-2026-07-12_03-00", # unrelated periodic snapshot + "Tap/apps@cloud_backup-9-20260712030000", # another task + "Tank/backups@cloud_backup-5-20260712030000", # different pool + ] + + def test_returns_parent_and_all_children(self): + got = snapshot_tree_names("Tap@cloud_backup-5-20260712030000", self.ALL) + assert set(got) == { + "Tap@cloud_backup-5-20260712030000", + "Tap/apps@cloud_backup-5-20260712030000", + "Tap/apps/lidarr/config@cloud_backup-5-20260712030000", + } + + def test_never_touches_periodic_or_other_tasks_or_other_pools(self): + got = snapshot_tree_names("Tap@cloud_backup-5-20260712030000", self.ALL) + assert "Tap@auto-2026-07-12_03-00" not in got + assert "Tap/apps@cloud_backup-9-20260712030000" not in got + assert "Tank/backups@cloud_backup-5-20260712030000" not in got + + def test_malformed_snapshot_name_yields_nothing(self): + assert snapshot_tree_names("Tap", self.ALL) == [] + + +class FakeMiddleware: + def __init__(self, snapshots=None): + self.snapshots = list(snapshots or []) + self.calls = [] + self.logger = None + + async def call(self, method, *args): + self.calls.append((method, args)) + if method == "zfs.snapshot.query": + return [{"name": n} for n in self.snapshots] + if method == "zfs.snapshot.delete": + if args[0] not in self.snapshots: + raise RuntimeError("does not exist") + self.snapshots.remove(args[0]) + return True + raise AssertionError(f"unexpected call {method}") + + async def run_in_thread(self, fn, *args): + return fn(*args) + + +class TestDeleteSnapshotTree: + def test_deletes_parent_and_every_child(self): + mw = FakeMiddleware([ + "Tap@snap", "Tap/apps@snap", "Tap/apps/lidarr@snap", "Tap@keepme", + ]) + asyncio.run(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")) + assert mw.snapshots == [] + + def test_survives_query_failure_by_deleting_at_least_the_parent(self): + class Broken(FakeMiddleware): + async def call(self, method, *args): + if method == "zfs.snapshot.query": + raise RuntimeError("boom") + return await super().call(method, *args) + + mw = Broken(["Tap@snap"]) + asyncio.run(delete_snapshot_tree(mw, "Tap@snap")) + assert mw.snapshots == [] + + def test_attempts_no_delete_when_the_tree_is_already_gone(self): + # A successful query returning nothing means there is nothing to do. + # Falling back to the parent here would log a spurious "does not exist" + # warning on every clean run. + mw = FakeMiddleware(["Tap@unrelated"]) + asyncio.run(delete_snapshot_tree(mw, "Tap@snap")) + assert [m for m, _a in mw.calls if m == "zfs.snapshot.delete"] == [] + assert mw.snapshots == ["Tap@unrelated"] + + +class TestStageNestedOrdering: + def setup_method(self): + ACTIVE.clear() + + def test_sidecar_is_written_before_anything_is_mounted(self, tmp_path, monkeypatch): + # middlewared can die at any moment. If the snapshot were recorded only + # after apply_plan, a crash in that window would orphan a 160-snapshot + # tree -- the precise failure the sidecar exists to prevent. + import truecloud_nested as tn + + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + order = [] + + 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", + "cloud_backup-5", DATASETS, + )) + + assert order.index("_write_sidecar") < order.index("apply_plan") + + def test_reclaims_the_snapshot_tree_left_by_a_crashed_run(self, tmp_path, + monkeypatch): + # teardown() reclaims the crashed run's MOUNTS, but nothing else would + # ever reclaim its SNAPSHOTS -- and we are about to overwrite the only + # record of them. One crash would orphan 160+ snapshots permanently. + import truecloud_nested as tn + + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + root = tn.staging_root_for("cloud_backup-5") + os.makedirs(os.path.dirname(root), exist_ok=True) + with open(sidecar_for(root), "w", encoding="utf-8") as fh: + fh.write("Tap@old-crashed-run") + + mw = FakeMiddleware(["Tap@old-crashed-run", "Tap/apps@old-crashed-run"]) + + 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", + "cloud_backup-5", DATASETS, + )) + + assert mw.snapshots == [], "the crashed run's snapshot tree must be reclaimed" + + def test_sidecar_is_removed_when_staging_fails(self, tmp_path, monkeypatch): + import truecloud_nested as tn + + 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) + + with pytest.raises(StagingError): + asyncio.run(tn.stage_nested( + Failing(), "/mnt/Tap", "Tap@snap", "Tap", "/mnt/Tap", + "cloud_backup-5", DATASETS, + )) + + assert not os.path.exists(sidecar_for(root)) + + +class TestCleanupTask: + def setup_method(self): + ACTIVE.clear() + + def test_recovers_snapshot_from_sidecar_after_middlewared_restart(self, tmp_path, + monkeypatch): + # ACTIVE is in-process; a restart wipes it. The sidecar is the source of + # truth, otherwise the snapshot tree is orphaned forever. + import truecloud_nested as tn + + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path)) + root = tn.staging_root_for("cloud_backup-5", base=str(tmp_path)) + os.makedirs(root, exist_ok=True) + with open(sidecar_for(root), "w", encoding="utf-8") as fh: + fh.write("Tap@snap") + + ACTIVE.clear() # simulate the restart + mw = FakeMiddleware(["Tap@snap", "Tap/apps@snap"]) + monkeypatch.setattr(tn, "teardown", lambda *_a, **_k: []) + + asyncio.run(cleanup_task(mw, "cloud_backup-5")) + + assert mw.snapshots == [] + assert not os.path.exists(sidecar_for(root)) + + def test_is_a_noop_when_never_staged(self, tmp_path, monkeypatch): + import truecloud_nested as tn + + monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path / "nope")) + mw = FakeMiddleware(["Tap@snap"]) + asyncio.run(cleanup_task(mw, "cloud_backup-5")) + assert mw.calls == [] + assert mw.snapshots == ["Tap@snap"] class TestVerifyStaged: @@ -144,22 +368,17 @@ class TestVerifyStaged: def test_passes_when_every_target_is_mounted_and_root_non_empty(self): mounts = [("/src", ROOT), ("/src/a", f"{ROOT}/a")] - assert verify_staged( - mounts, ismount=lambda p: True, listdir=lambda p: ["apps"] - ) + assert verify_staged(mounts, ismount=lambda p: True, listdir=lambda p: ["apps"]) def test_raises_when_a_target_is_not_actually_mounted(self): # This is the case that would produce a silently-empty backup. mounts = [("/src", ROOT), ("/src/a", f"{ROOT}/a")] with pytest.raises(StagingError, match="not a mountpoint"): - verify_staged( - mounts, ismount=lambda p: p == ROOT, listdir=lambda p: ["apps"] - ) + verify_staged(mounts, ismount=lambda p: p == ROOT, listdir=lambda p: ["apps"]) def test_raises_when_staging_root_is_empty(self): - mounts = [("/src", ROOT)] with pytest.raises(StagingError, match="empty"): - verify_staged(mounts, ismount=lambda p: True, listdir=lambda p: []) + verify_staged([("/src", ROOT)], ismount=lambda p: True, listdir=lambda p: []) def test_raises_on_empty_plan(self): with pytest.raises(StagingError): @@ -185,25 +404,26 @@ class FakeRunner: class TestApplyPlanRollback: - def test_rolls_back_mounts_when_one_fails(self, tmp_path, monkeypatch): + def test_rolls_back_mounts_when_one_fails(self, tmp_path): # A half-built tree must never reach the backup tool. root = str(tmp_path / "root") mounts = [("/src", root), ("/src/a", root + "/a"), ("/src/b", root + "/b")] - monkeypatch.setattr(os.path, "isdir", lambda p: True) runner = FakeRunner(fail_on="/src/b") with pytest.raises(StagingError, match="bind-mount"): - apply_plan(mounts, runner=runner) + apply_plan(mounts, runner=runner, isdir=yes) - umounts = [c for c in runner.calls if c[0] == "umount"] - # Everything successfully mounted before the failure is unmounted again. - assert [c[-1] for c in umounts] == [root + "/a", root] + umounts = [c[-1] for c in runner.calls if c[0] == "umount"] + assert umounts == [root + "/a", root] - def test_raises_when_target_missing(self, tmp_path, monkeypatch): + def test_raises_when_target_missing(self, tmp_path): root = str(tmp_path / "root") - monkeypatch.setattr(os.path, "isdir", lambda p: p == root) with pytest.raises(StagingError, match="does not exist"): - apply_plan([("/src", root), ("/src/a", root + "/a")], runner=FakeRunner()) + apply_plan( + [("/src", root), ("/src/a", root + "/a")], + runner=FakeRunner(), + isdir=lambda p: p == root, + ) class TestTeardown: @@ -226,7 +446,7 @@ class TestTeardown: f"{ROOT}/apps", ROOT, ] - assert "/somewhere/else" not in order # never touch unrelated mounts + assert "/somewhere/else" not in order def test_is_idempotent_when_nothing_mounted(self, tmp_path): mounts_file = tmp_path / "mounts" @@ -257,13 +477,12 @@ class TestTeardown: class TestCurrentMountsUnder: def test_matches_only_the_staging_subtree(self, tmp_path): mounts_file = tmp_path / "mounts" - # "/run/truecloud-nested/cloud_backup-50" must NOT match "cloud_backup-5". + # "cloud_backup-50" must NOT match "cloud_backup-5". mounts_file.write_text( f"tmpfs {ROOT} tmpfs rw 0 0\n" "tmpfs /run/truecloud-nested/cloud_backup-50 tmpfs rw 0 0\n" ) - found = current_mounts_under(ROOT, mounts_file=str(mounts_file)) - assert found == [ROOT] + assert current_mounts_under(ROOT, mounts_file=str(mounts_file)) == [ROOT] class TestStagingRootFor: @@ -271,4 +490,10 @@ class TestStagingRootFor: assert staging_root_for("cloud_backup-5") == "/run/truecloud-nested/cloud_backup-5" def test_sanitises_path_separators(self): - assert "/" not in staging_root_for("evil/../../etc").rsplit("/", 1)[-1] + assert "/" not in staging_root_for("evil/name").rsplit("/", 1)[-1] + + @pytest.mark.parametrize("name", ["..", ".", "...", "/", ""]) + def test_dot_components_cannot_escape_the_staging_base(self, name): + # os.path.join(BASE, "..") normalises to /run — teardown would rmdir it. + root = staging_root_for(name) + assert os.path.normpath(root).startswith("/run/truecloud-nested/") diff --git a/uninstall.sh b/uninstall.sh index 80d105a..9c1e866 100755 --- a/uninstall.sh +++ b/uninstall.sh @@ -91,6 +91,52 @@ if [ "$_ov_found" -eq 0 ]; then fi echo "" +# ── Unmount nested-snapshot staging trees ───────────────────────────────────── +# These bind mounts pin their ZFS snapshots, so they must go before anything +# tries to destroy those snapshots. Deepest first. + +echo "Unmounting nested-snapshot staging trees (if any) ..." +_stage_found=0 +_stage_failed=0 +# Deepest FIRST, by path depth (slash count) — not string length, which would +# let a long shallow path jump ahead of a short deep one and leave a child +# mounted (and its ZFS snapshot pinned). +while IFS= read -r _mp; do + [ -n "$_mp" ] || continue + if umount "$_mp" 2>/dev/null || umount -l "$_mp" 2>/dev/null; then + echo " Unmounted: $_mp" + else + echo " WARNING: Could not unmount $_mp" + _stage_failed=1 + fi + _stage_found=1 +done < <(awk '$2 == "/run/truecloud-nested" || index($2, "/run/truecloud-nested/") == 1 { + n = gsub(/\//, "/", $2); print n, $2 + }' /proc/self/mounts 2>/dev/null | sort -rn | cut -d' ' -f2-) + +if [ "$_stage_found" -eq 0 ]; then + echo " None active." +fi + +# NEVER `rm -rf` here: if an unmount failed, that would recurse *through* a live +# bind mount into the ZFS snapshot behind it. Remove empty directories only. +if [ "$_stage_failed" -eq 0 ]; then + find /run/truecloud-nested -depth -type d -exec rmdir {} + 2>/dev/null || true + rm -f /run/truecloud-nested/*.snapshot 2>/dev/null || true + rmdir /run/truecloud-nested 2>/dev/null || true +else + echo " WARNING: staging mounts remain; leaving /run/truecloud-nested in place." + echo " Unmount them manually, then remove the directory." +fi + +# The opt-in marker lives in the repo dir; remove it so a later re-install +# starts from the safe default (feature off). +if [ -f "$PATCH_DIR/nested_snapshots_enabled" ]; then + rm -f "$PATCH_DIR/nested_snapshots_enabled" + echo " Removed nested-snapshot opt-in marker." +fi +echo "" + if [ "$_restore_failed" -eq 1 ]; then echo "" echo "ERROR: One or more UI bundle backups could not be restored." >&2