Commit Graph
9 Commits
Author SHA1 Message Date
flan 0d04c2cd1c Collect orphaned snapshots by name: the sidecar lives in tmpfs
CI / shell (shellcheck + syntax) (push) Successful in 10s
CI / python 3.11 (push) Successful in 13s
CI / python 3.12 (push) Successful in 16s
CI / python 3.13 (push) Successful in 16s
A reboot mid-backup orphaned the entire tree, permanently. The sidecar is the record
of which snapshots a run pinned -- and /run is tmpfs. A reboot or crash between the
recursive snapshot and its cleanup destroyed that record, leaving one snapshot per
descendant dataset (250+ on a real pool) with nothing pointing at them. Nothing would
ever have found them.

gc_stale_snapshots() identifies leftovers by NAME, so it works when the record is
gone. It runs after the sidecar reclaim -- the recorded path stays authoritative and
the collector only mops up what the record lost.

It deletes data on a name match, which is a weaker claim than a recorded fact, so the
selection is a pure function with the harshest tests here. A snapshot is collected
only if the name is exactly <dataset>@<task>-<YYYYMMDDHHMMSS>, it is not the current
run's, NOTHING IS MOUNTED FROM IT (this, not the age guard, is what protects a
concurrent backup), and it is over an hour old.

Checked against the real pool: of 4728 snapshots including 2341 periodic ones, it
selects exactly the orphans of the task being run and nothing else.
2026-07-13 19:56:32 +00:00
flan 2b8ef107f7 The sidecar must carry EVERY pending tree, not just the newest
CI / shell (shellcheck + syntax) (push) Successful in 9s
CI / python 3.12 (push) Successful in 13s
CI / python 3.13 (push) Successful in 17s
CI / python 3.11 (push) Successful in 15s
TrueNAS compatibility / compat (push) Successful in 11s
Release / release (push) Successful in 15s
Found live, in the code written to prevent exactly this.

The sidecar held ONE snapshot. So a run that reclaimed an older tree, failed to
finish reclaiming it, and then recorded its own snapshot OVERWROTE the only record of
the survivor -- orphaning it permanently.

Observed: job 24 left one snapshot busy and kept the sidecar (correct). Job 46
reclaimed it, hit ZFS's 300s automount window (the runs were minutes apart), left it
behind again, and then wrote its own snapshot over the record. Permanent orphan,
created by the safety net.

The sidecar is now a list. stage_nested carries forward whatever a reclaim could not
delete; cleanup_task sweeps every pending tree and writes back only the survivors.
cleanup_all reports them one per line instead of formatting a list into an f-string
at the user during uninstall.

Job 46 also confirms the automount fix itself: it swept all 256 of its own snapshots
with no straggler.
2026-07-13 19:35:44 +00:00
flan b50567e9a7 A few snapshots leaked on every nested run, forever
CI / shell (shellcheck + syntax) (push) Successful in 10s
CI / python 3.11 (push) Successful in 12s
CI / python 3.12 (push) Successful in 15s
CI / python 3.13 (push) Successful in 17s
TrueNAS compatibility / compat (push) Successful in 13s
Release / release (push) Successful in 14s
Found on real hardware: a 256-snapshot backup of /mnt/Tap swept 253 and left 3 with
'dataset is busy'.

ZFS AUTOMOUNTS <dataset>/.zfs/snapshot/<snap> when it is read, and keeps it mounted
for zfs_expire_snapshot seconds (300 default) after the last access. teardown()
unmounts OUR bind mounts but not the automount underneath, so zfs destroy refuses for
exactly the datasets restic read most recently. cleanup_task() then removed the
sidecar anyway -- destroying the only record those snapshots existed. Nothing would
ever have reclaimed them.

- release_snapdirs() unmounts ZFS's own automounts (deepest first) before deleting.
- delete_snapshot_tree() retries the transient busy and RETURNS what it could not
  delete, instead of swallowing it.
- The sidecar is removed only on a confirmed-clean sweep -- including on the
  staging-failure path, which used to remove it before the caller swept. A sidecar
  left behind when the tree is gone costs one no-op delete; a sidecar removed while
  the tree exists is unrecoverable.
2026-07-13 19:18:22 +00:00
flan 498b2690e1 TrueNAS 26 support: one sync implementation, two wrappers
CI / python 3.13 (push) Successful in 15s
CI / shell (shellcheck + syntax) (push) Successful in 8s
CI / python 3.11 (push) Successful in 14s
CI / python 3.12 (push) Successful in 16s
TrueNAS compatibility / compat (push) Successful in 9s
26 rewrites cloud_backup from async to synchronous AND deletes
get_dataset_recursive(), which SNAPSHOT_BLOCK called out of the host module's
namespace. Either is a broken backup found at restore time.

The nested module is now one synchronous implementation talking to middlewared via
call_sync, behind two thin wrappers. apply.sh reads which flavour the installed
middleware declares and injects the matching one: <= 25.10 reaches it through
'await middleware.run_in_thread(...)', 26 is already in a worker thread and calls
it directly. The snapshot/bind-mount/failure logic exists once -- an async twin
would mean every future fix had to land twice.

A middleware whose three wrapped functions disagree about asyncness is refused,
not guessed at. get_dataset_recursive is vendored, removing the dependency on both
versions rather than asserting it.

master stays BROKEN on purpose: iX are still renaming middleware->context,
cloud_backup->entry and adding a required credentials param there. Chasing a
branch that moves daily is how you ship a patch nobody tested.
2026-07-13 18:18:28 +00:00
flan 8421a34d8d Delete the snapshot tree atomically instead of 252 calls
delete_snapshot_tree removed the parent and every child snapshot individually.
On a real pool `zfs snapshot -r` creates one snapshot per descendant dataset --
252 on Tap -- so cleanup was 252 sequential middleware calls.

Slow, but the real problem is that it is not atomic: a job killed part-way
through the sweep leaves behind exactly the orphaned snapshots this function
exists to prevent.

zfs.snapshot.delete accepts {"recursive": True}, which destroys the parent and
all children in one call. Use that as the fast path and keep the name-by-name
sweep as the fallback -- it is still needed when the parent is already gone
(stock's finally can win the race once our mounts are released), which makes a
recursive delete fail while the children survive.

The test fake now emulates real `zfs destroy -r` semantics, so a test cannot pass
while the shipped code deletes only the parent.

76 tests, ruff and shellcheck clean.
2026-07-13 14:35:04 +00:00
flan 24f1f2c648 Fix post-merge audit findings; unify staging teardown
Audit
-----
- create_task.py verify failed on a DEFAULT install. hook_status.json emitted a
  per-file entry for the nested module with ok:false whenever the feature was
  switched off -- the default -- so verify printed [FAIL] and exited 1, right
  after the README tells users to run it. Status is now per MODULE with an
  `active` flag, and verify renders an inactive module as [SKIP].

- A partial apply suppressed the middlewared restart. The exit code conflated
  "nothing applied" with "one module applied, one failed", so a failing providers
  patch would prevent the restart that a freshly-applied nested patch needs,
  leaving it on disk and never loaded. Exit 2 now means partial and the restart
  still fires.

- The native-nested probe could never fire. It scanned crud.py for the guard
  message, but our own injected block quotes that message, so once applied the
  probe would always conclude the guard was still present. It now reads only the
  stock portion of the file.

- recover.sh did not unmount staging trees, so an emergency recovery left bind
  mounts pinning ZFS snapshots that could then never be destroyed.

- uninstall.sh deleted sidecar files without reading them. A sidecar is the only
  record that an interrupted run's snapshot tree is still on disk; both scripts
  now name the snapshot before clearing it.

- Removed a dead branch in the restart gate (unreachable: the kill switch exits).

Refactor
--------
- Staging teardown had been copy-pasted into uninstall.sh and recover.sh -- two
  untested shell copies of the fiddly depth-ordering and lazy-umount logic. Both
  now call `python3 patch/truecloud_nested.py cleanup`, so there is exactly one
  implementation and it is the one under test.

- Dropped the in-memory ACTIVE dict. The sidecar file was already the source of
  truth; a second in-process record could only desync -- and it is precisely the
  middlewared-restart case (which empties it) that must not orphan a snapshot
  tree. One record, on disk, or none.

Not done: the overlay-unmount loop is duplicated across apply.sh/uninstall.sh/
recover.sh. It is pre-existing, and apply.sh runs at PREINIT under a tight
timeout -- giving it a source dependency would trade 10 lines of duplication for
a boot-time failure mode.

74 tests, ruff and shellcheck clean.
2026-07-12 22:29:31 +00:00
flan a80de88078 Distinguish a missing snapshot from an unreadable one
os.path.isdir() returns False both when a snapshot directory does not exist and
when it cannot be stat'd. Staging aborted either way -- correct -- but reported
every case as "has no snapshot", which sends you hunting for a snapshot that is
sitting right there.

Found while dry-running the planner against a real recursive snapshot of Tap:
running as a non-root user, /mnt/Tap/apps/paperless/data is mode 0700 and the
probe reported "has no snapshot" when `zfs list` showed the snapshot present.
Middleware runs as root so this would not fire in production, but a backup
system must not misreport why it failed.

plan_staging now takes a probe() that classifies the path as ok / missing /
unreadable, and the error names which.

Dry-run results against Tap (250 datasets, real `zfs snapshot -r`):
- 170 mounted filesystems under /mnt/Tap/, and the plan produces exactly 170
  descendant mounts -- no omissions
- 18 legacy-mountpoint datasets reported as skipped, never dropped silently
- Tap/ix-apps mounts at /mnt/.ix-apps, correctly outside the backup path
- parent snapshot exposes 0 entries under /apps; the staged sources expose 71,
  and lidarr/config resolves with lidarr.db present
- snapshot_tree_names() identifies all 250; deleting only the parent (what stock
  does) leaves 249 orphans, and the sweep clears them
2026-07-12 22:04:56 +00:00
flan bb26edf351 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.
2026-07-12 21:52:09 +00:00
flan a572eb2164 Support ZFS snapshots on datasets with child datasets
TrueCloud Backup's "Take Snapshot" option is rejected on any path containing
child datasets:

  This option is only available for datasets that have no further nesting

That excludes every pool running Apps, where each app is its own dataset and
often has config/pgdata children. Without the option the backup reads live
files, so databases are captured mid-write and an app that continuously
rewrites its files can stall a run as restic chases a moving target.

The stock guard is correct and must not simply be removed. create_snapshot()
already takes a recursive ZFS snapshot, but points the backup tool at the
parent dataset's .zfs/snapshot/, and ZFS does not expose child datasets there:

  /mnt/Tap/.zfs/snapshot/<snap>/apps/                -> 0 entries
  /mnt/Tap/apps/lidarr/config/.zfs/snapshot/<snap>/  -> the real data

Deleting the check would make restic walk a near-empty tree, report success,
and upload almost nothing.

Implement the missing traversal instead. After the recursive snapshot is taken,
each descendant dataset's own .zfs/snapshot/<snap> is bind-mounted into a
staging tree mirroring the original layout, and the backup tool is pointed at
the staging root. The guard is relaxed only after that machinery is in place.

Safety properties:
- staging failure aborts the backup; a partial tree is never handed to restic
- a post-mount pass asserts every target is a mountpoint and the root is
  non-empty, so this cannot regress into the empty backup it exists to prevent
- apply.sh patches crud.py last, so a partial failure leaves the guard intact
  rather than exposing "guard removed, traversal missing"
- every injected block no-ops when _truecloud_nested is absent
- unmountable/locked datasets are skipped and reported, never dropped silently
- scoped to cloud_backup; cloudsync has no teardown wired in, so its guard stays

The staging root is stable per task, so restic can find its parent snapshot
between runs; stock's timestamped .zfs path changes every run and forces a
full re-scan.

Add CI (shellcheck, bash -n, ruff, pytest on 3.11-3.13), including tests that
compile the *_BLOCK strings, which are Python source appended to live
middlewared modules and were previously unchecked.

Also: sync stale version strings, untrack a committed .pyc, gitignore
__pycache__.
2026-07-12 21:20:22 +00:00