flan/truenas-truecloud-patch
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases 12 Wiki Activity

fix: third audit — a cross-tree dataset was omitted silently, and the block tests passed on comments

Browse Source 
D1, the only cardinal-rule violation left. plan_staging scopes by dataset NAME, which
is right (a dataset with no mountpoint cannot be scoped by path). But ZFS lets any
dataset mount anywhere, so one from a DIFFERENT tree can sit inside the backup path:

    Tank/photos   mountpoint=/mnt/Tap/apps/photos

It holds data inside the path, and `zfs snapshot -r Tap@...` does NOT cover it —
recursion follows the dataset tree, not the directory tree. It fell out of the name
filter and vanished: not staged, not in `skipped`, no error. The backup reported
SUCCESS with that data missing. Stock has the same blind spot but refuses the nested
config outright; we are the ones relaxing that guard, so the hole is ours. It now
raises.

The test suite was the real weakness. apply.sh's injected blocks carry the
highest-consequence logic in the project — the run_in_thread hop, the flavour
selection, the finally-teardown, the re-raise — and were guarded only by substring
greps. Two of them passed on COMMENTS: `assert "raise" in block` was satisfied by a
comment reading "a cleanup that raises...", and `assert "cleanup_task" in block` by
"cleanup_task gets logger=None". Deleting the actual re-raise (restic then backs up the
UN-STAGED path — the silently-empty backup this module exists to prevent) and deleting
the actual cleanup call from the finally (~250 orphans per run) both left the suite
green. They are asserted structurally now, against the parsed block.

Eleven regressions the audit found surviving now fail the suite, including: a swallowed
staging failure, a missing teardown, an inverted flavour mapping, blocking work back on
the asyncio event loop, the host's deleted get_dataset_recursive, query_filesystems
quietly preferring the filtered middleware query, and a re-frozen `runner`/`sleep`/
`mounts_file` default (which would silently re-arm 19 tests reading the real mount
table on the NAS).

Also: _read_sidecar conflated "no sidecar" with "cannot read the sidecar", so
cleanup_task took the empty branch and UNLINKED the only record of a tree it could not
read. mounted_snapshots returned an empty set on error, silently switching off the GC's
protection for snapshots a concurrent run is using. Both raise now.

Verified on TrueNAS 26.0.0-BETA.1: zvol-orphan case 0 orphans, 292-dataset backup
0 orphans / 0 leaked mounts, byte-identical restore of a 4-deep child dataset.
This commit is contained in:
flan
2026-07-14 00:38:45 +00:00
parent 0fea5c40bd
commit 8a41d7d7ef
3 changed files with 422 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
patch/truecloud_nested.py
+54 -3
View File
@@ -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
tests/test_apply_blocks.py
+197 -12
View File
@@ -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"
)
tests/test_truecloud_nested.py
+171
View File
@@ -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."
)