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.

CHANGELOG.md

@@ -123,6 +123,40 @@
 - `patch/__pycache__/create_task.cpython-314.pyc` was committed to the
   repository; it is now untracked and `__pycache__/` is gitignored.
 
+### Fixed (post-merge 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 — which is the default — so `verify` printed `[FAIL]`
+  and exited 1 right after the README told users to run it. Status is now
+  reported per *module* with an `active` flag, and `verify` renders an inactive
+  module as `[SKIP]` rather than a failure.
+- **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 (`zfs destroy -r ...`) before clearing it.
+
+### Refactored
+
+- 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 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 the
+  middlewared-restart case (which empties it) that must not orphan a snapshot
+  tree. One record, on disk, or none.
+
 ### Known issues
 
 - Stock `restic_backup()` deletes the ZFS snapshot in its own `finally`, which

README.md

@@ -248,7 +248,7 @@ mount | grep truecloud-nested                            # expect no output
 | Backup fails: `dataset '…' has no snapshot '…'; refusing to back up an incomplete tree` | Working as designed — a descendant dataset was not covered by the snapshot. The backup is refused rather than silently omitting that data. |
 | Backup fails: `snapshot '…' cannot be read (Permission denied)` | The snapshot exists but is unreadable. Middleware runs as root, so this indicates a real permissions problem, not a missing snapshot. |
 | `cloud_backup-*` snapshots accumulating | The sweep is not running. Check `apply.log` for the nested patch applying, and confirm `sync.py` carries the `TRUECLOUD_PATCH` block. |
-| Stale mounts under `/run/truecloud-nested` | A crashed run. The next run tears them down; `uninstall.sh` also cleans them. |
+| Stale mounts under `/run/truecloud-nested` | A crashed run. The next backup tears them down. To clear them now: `python3 patch/truecloud_nested.py cleanup` (also run by `uninstall.sh` and `recover.sh`). It names any ZFS snapshot an interrupted run left pinned. |
 
 ## Supported providers after patching
 
@@ -539,19 +539,28 @@ patch re-runs at next reboot, or you run
 python3 /mnt/tank/truenas-truecloud-patch/patch/create_task.py verify
 ```
 
-If one or more entries show `[FAIL]`:
+`verify` reports one line per module:
+
+| Label | Meaning |
+|---|---|
+| `[OK  ]` | Module is active and applied. |
+| `[SKIP]` | Module is inactive — either TrueNAS now does it natively, or it is opt-in and switched off. **Not a failure.** `nested_snapshots` shows SKIP on a default install. |
+| `[FAIL]` | Module is needed but did not apply. |
+
+If a module shows `[FAIL]`:
 
 1. **Check the apply log** for errors during the last boot:
    ```bash
-   cat /mnt/tank/truenas-truecloud-patch/apply.log | tail -40
+   tail -40 /mnt/tank/truenas-truecloud-patch/apply.log
    ```
 2. **Check middlewared's own log** for Python tracebacks:
    ```bash
    grep -i "truecloud\|traceback\|error" /var/log/middlewared.log 2>/dev/null | tail -30
    journalctl -u middlewared -n 50
    ```
-3. **A FAIL is non-fatal.** middlewared runs normally; the affected provider
-   falls back to Storj-only. Your existing backups are not at risk.
+3. **A FAIL is non-fatal.** middlewared runs normally and the other module is
+   unaffected; the failed one is simply inactive. Existing backups are not at
+   risk.
 4. **If the detail says the module doesn't exist**, a TrueNAS update renamed
    or restructured the internal API.
    [Open an issue](https://github.com/sudolulo/truenas-truecloud-patch/issues)

patch/apply.sh

@@ -153,15 +153,22 @@ except Exception:
     pass
 
 # nested: stock gates nested datasets with a validation in plugins/cloud/crud.py.
-# We only ever append to that file, so the stock text survives our patch -- if the
-# guard is gone, iX removed it, which means they implemented the traversal.
+# If that guard is gone, iX removed it, which means they implemented the traversal.
+#
+# Only look at the STOCK part of the file. Our own CRUD_BLOCK quotes the guard
+# message (it filters on it), so scanning the whole file would find the string in
+# our own patch and conclude the guard is still there. That happens to be
+# harmless today because detection runs before patching, but it makes the probe
+# silently order-dependent -- so cut our block off explicitly.
+#
 # If the file cannot be read we assume 'no' and keep patching: worst case the
 # patch declines to apply and the option simply stays unavailable.
 try:
     crud = os.path.join(result['mw_dir'], 'plugins', 'cloud', 'crud.py')
     with open(crud, encoding='utf-8', errors='replace') as fh:
-        if 'no further nesting' not in fh.read():
-            result['native_nested'] = 'yes'
+        stock_src = fh.read().split('\n# TRUECLOUD_PATCH', 1)[0]
+    if 'no further nesting' not in stock_src:
+        result['native_nested'] = 'yes'
 except Exception:
     pass
 
@@ -532,29 +539,21 @@ else:
         print('WARNING: Stock nesting guard remains; snapshot option stays unavailable')
         print('WARNING: for nested datasets. Existing backups are unaffected.')
 
+# One entry per MODULE, not per file. `ok` means "nothing is wrong", so a module
+# that is inactive (superseded, or opt-in and off) is ok -- reporting a disabled
+# opt-in feature as FAIL would make `create_task.py verify` fail on a default
+# install. `active` says whether the module is doing anything.
 patches = {
-    'module.providers': {
-        'ok': bool(b2_ok and restic_ok) or not providers_needed,
+    'providers': {
+        'ok': (not providers_needed) or bool(b2_ok and restic_ok),
         'active': providers_needed,
         'detail': providers_detail,
     },
-    'module.nested_snapshots': {
-        'ok': nested_ok or not nested_needed,
+    'nested_snapshots': {
+        'ok': (not nested_needed) or nested_ok,
         'active': nested_needed,
         'detail': nested_detail,
     },
-    'middlewared.rclone.remote.b2': {
-        'ok': b2_ok,
-        'detail': 'patched on disk in overlay at boot' if b2_ok else providers_detail,
-    },
-    'middlewared.plugins.cloud_backup.restic': {
-        'ok': restic_ok,
-        'detail': 'patched on disk in overlay at boot' if restic_ok else providers_detail,
-    },
-    'middlewared.plugins.cloud.nested_snapshot': {
-        'ok': nested_ok,
-        'detail': nested_detail,
-    },
 }
 payload = {'patched_at': time.strftime('%Y-%m-%dT%H:%M:%SZ', time.gmtime()), 'patches': patches}
 tmp = status_path + '.tmp'
@@ -566,18 +565,34 @@ try:
 except OSError as e:
     print(f'WARNING: Could not write hook_status.json: {e}')
 
-# Exit 0 only if every module that is still NEEDED applied cleanly. A module that
-# was skipped (superseded or opt-out) is not a failure.
-_providers_done = (not providers_needed) or (b2_ok and restic_ok)
+# Exit code tells the caller whether a middlewared restart is still worth doing:
+#
+#   0  every module that was needed applied cleanly
+#   2  PARTIAL -- one module failed but another landed, so there IS something new
+#      on disk waiting to be loaded
+#   1  nothing landed; a restart would accomplish nothing
+#
+# Collapsing 2 into 1 would mean a failing providers patch suppresses the restart
+# that a freshly-applied nested patch needs, leaving it on disk and never loaded.
+_providers_done = (not providers_needed) or bool(b2_ok and restic_ok)
 _nested_done = (not nested_needed) or nested_ok
-sys.exit(0 if (_providers_done and _nested_done) else 1)
+_landed = (providers_needed and b2_ok and restic_ok) or (nested_needed and nested_ok)
+
+if _providers_done and _nested_done:
+    sys.exit(0)
+sys.exit(2 if _landed else 1)
 PYEOF
         then
             _backend_ok=1
         else
-            # Individual results already printed above; exit code 1 means at least
-            # one module that was still needed failed to apply.
-            _backend_ok=0
+            _rc=$?
+            if [ "$_rc" = "2" ]; then
+                # One module failed, but another was applied and still needs loading.
+                _backend_ok=1
+                echo "WARNING: a module failed to apply; the other landed and will be loaded."
+            else
+                _backend_ok=0
+            fi
         fi
 fi
 
@@ -625,15 +640,17 @@ fi
 
 echo "--- deferred restart ---"
 
-# Restart when ANY still-needed backend module landed. Keying this off the
-# providers module alone would skip the restart on a box where B2 has gone native
-# but the nested module was freshly patched — leaving it on disk and never loaded.
+# Restart when ANY still-needed backend module landed (_backend_ok, incl. the
+# partial case). Keying this off the providers module alone would skip the restart
+# on a box where B2 has gone native but the nested module was freshly patched —
+# leaving it on disk and never loaded.
+#
+# "No module active at all" cannot reach here: that is the kill-switch branch
+# above, which exits.
 if ! grep -aq middlewared "/proc/$PPID/cmdline" 2>/dev/null; then
     echo "Manual run (parent is not middlewared) — no restart scheduled."
-elif [ "$_providers_needed" = "0" ] && [ "$_nested_needed" = "0" ]; then
-    echo "No backend module active — no restart scheduled (nothing new to load)."
-elif [ "${_backend_ok:-0}" != "1" ]; then
-    echo "Backend patch incomplete — no restart scheduled (nothing new to load)."
+elif [ "$_backend_ok" != "1" ]; then
+    echo "Nothing landed on disk — no restart scheduled (nothing new to load)."
 else
     # A failed unit from an earlier attempt this boot would block systemd-run.
     systemctl reset-failed truecloud-mw-restart.service 2>/dev/null

patch/create_task.py

@@ -127,13 +127,21 @@ def cmd_verify():
     print(f"Hook status (recorded at {status.get('patched_at', 'unknown')})")
     print()
     all_ok = True
+    any_active = False
     for module, info in status.get("patches", {}).items():
         ok = info.get("ok", False)
+        # A module can be inactive because TrueNAS now does it natively, or
+        # because it is opt-in and switched off. Neither is a failure.
+        active = info.get("active", True)
         label = "OK  " if ok else "FAIL"
+        if ok and not active:
+            label = "SKIP"
         detail = f"  — {info['detail']}" if info.get("detail") else ""
         print(f"  [{label}] {module}{detail}")
         if not ok:
             all_ok = False
+        if active:
+            any_active = True
 
     # The disk status alone can false-positive: at boot the files are patched
     # while middlewared is already running with the stock modules imported.
@@ -146,7 +154,11 @@ def cmd_verify():
     mw_start = _middlewared_start_epoch()
 
     proc_stale = False
-    if patched_epoch is None or mw_start is None:
+    if not any_active:
+        # Nothing is patched into middlewared, so whether it restarted since is
+        # irrelevant -- there is nothing for it to have loaded.
+        print("  [--  ] running middlewared process — no active module; nothing to load")
+    elif patched_epoch is None or mw_start is None:
         print("  [??  ] running middlewared process — could not compare start time;")
         print("         the results above reflect the on-disk state only")
     elif mw_start + 2 < patched_epoch:

patch/truecloud_nested.py

@@ -60,10 +60,10 @@ import stat
 import subprocess
 
 __all__ = [
-    "ACTIVE",
     "STAGING_BASE",
     "StagingError",
     "apply_plan",
+    "cleanup_all",
     "cleanup_task",
     "current_mounts_under",
     "delete_snapshot_tree",
@@ -79,9 +79,10 @@ __all__ = [
 #: Where staging trees are assembled. tmpfs; bind mounts consume no space.
 STAGING_BASE = "/run/truecloud-nested"
 
-#: 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] = {}
+# Which snapshot a staging tree pins is recorded ONLY in the sidecar file, never
+# also in memory. An in-process dict would be a second source of truth that a
+# middlewared restart silently empties -- and it is exactly the restart case that
+# must not orphan a 250-snapshot tree. One record, on disk, or none.
 
 
 class StagingError(Exception):
@@ -459,8 +460,6 @@ async def stage_nested(middleware, path, snapshot, base_dataset, base_mountpoint
         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",
@@ -475,12 +474,7 @@ async def cleanup_task(middleware, task_name, logger=None):
     Safe to call unconditionally: a no-op when the task was never staged.
     """
     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)
+    snapshot = _read_sidecar(staging_root)
 
     if snapshot is None and not os.path.isdir(staging_root):
         return  # never staged; nothing to do
@@ -493,5 +487,65 @@ async def cleanup_task(middleware, task_name, logger=None):
     if snapshot is not None:
         await delete_snapshot_tree(middleware, snapshot, logger=logger)
 
-    with contextlib.suppress(OSError):
-        os.unlink(sidecar)
+    _remove_sidecar(staging_root)
+
+
+# ── offline cleanup (uninstall.sh / recover.sh) ───────────────────────────────
+
+
+def cleanup_all(base=None, runner=_run, mounts_file="/proc/self/mounts",
+                glob_fn=None, read_sidecar=_read_sidecar):
+    """Tear down every staging tree. Used by uninstall.sh and recover.sh.
+
+    Those scripts must work when middlewared is dead, so they cannot go through
+    the async path -- but they must not reimplement the teardown either: the
+    depth-ordering and lazy-umount fallback are fiddly, and a second copy in
+    shell would be the untested one. This is the same tested code.
+
+    Returns ``(lines, errors)``: report lines to print, and unmount errors.
+    """
+    import glob as _glob
+
+    base = base or STAGING_BASE
+    glob_fn = glob_fn or _glob.glob
+    lines = []
+
+    # Report orphaned snapshots BEFORE removing the sidecars that name them --
+    # a sidecar is the only record that an interrupted run's snapshot tree (one
+    # snapshot per descendant dataset) is still on disk.
+    for sc in sorted(glob_fn(os.path.join(base, "*.snapshot"))):
+        snap = read_sidecar(sc[: -len(".snapshot")])
+        if snap:
+            lines.append(f"  NOTE: an interrupted backup left snapshot '{snap}' behind.")
+            lines.append(f"        Remove it and its children:  zfs destroy -r '{snap}'")
+
+    mounts = current_mounts_under(base, mounts_file=mounts_file)
+    if not mounts:
+        lines.append("  None active.")
+    for mp in mounts:
+        lines.append(f"  Unmounting: {mp}")
+
+    errors = teardown(base, runner=runner, mounts_file=mounts_file)
+    for err in errors:
+        lines.append(f"  WARNING: could not unmount {err}")
+
+    if not errors:
+        for sc in glob_fn(os.path.join(base, "*.snapshot")):
+            with contextlib.suppress(OSError):
+                os.unlink(sc)
+        with contextlib.suppress(OSError):
+            os.rmdir(base)
+
+    return lines, errors
+
+
+if __name__ == "__main__":
+    import sys
+
+    if len(sys.argv) > 1 and sys.argv[1] == "cleanup":
+        _lines, _errors = cleanup_all()
+        for _line in _lines:
+            print(_line)
+        sys.exit(1 if _errors else 0)
+    print("usage: truecloud_nested.py cleanup", file=sys.stderr)
+    sys.exit(2)

recover.sh

@@ -21,6 +21,7 @@ VERSION="0.3.0"
 
 PATCH_DIR="$(cd "$(dirname "$0")" && pwd)"
 
+
 echo "=== TrueNAS TrueCloud Provider Patch v${VERSION} — Recover ==="
 echo ""
 
@@ -52,6 +53,14 @@ for _tag in mw ui; do
 done
 [ "$_any" -eq 0 ] && echo "  No overlays active."
 
+# Nested-snapshot staging trees are bind mounts that PIN their ZFS snapshots, so
+# leaving them mounted blocks those snapshots from ever being destroyed. The
+# overlays above are volatile, but these are not self-healing without a reboot,
+# and recover.sh is expected to work without one.
+echo "Unmounting nested-snapshot staging trees ..."
+# Best-effort: never block recovery. Same tested implementation as uninstall.sh.
+python3 "$PATCH_DIR/patch/truecloud_nested.py" cleanup || true
+
 # Cancel a deferred boot restart if one is still queued — we restart ourselves.
 systemctl stop truecloud-mw-restart.service 2>/dev/null
 systemctl reset-failed truecloud-mw-restart.service 2>/dev/null

tests/test_apply_blocks.py

@@ -144,6 +144,24 @@ class TestIndependentModules:
         i = sh.index("--- UI patch ---")
         assert '[ "$_providers_needed" = "0" ]' in sh[i:i + 400]
 
+    def test_status_reports_an_inactive_module_as_ok(self):
+        # `create_task.py verify` fails if any patches[*].ok is false. An opt-in
+        # module that is switched off (the DEFAULT) must not report FAIL, or a
+        # stock install fails verification out of the box.
+        src = heredoc_source()
+        assert "'ok': (not nested_needed) or nested_ok" in src
+        assert "'ok': (not providers_needed) or bool(b2_ok and restic_ok)" in src
+        assert "'active': nested_needed" in src
+
+    def test_nested_native_probe_ignores_our_own_block(self):
+        # CRUD_BLOCK quotes the guard message, so scanning the whole file would
+        # find the string in our own patch and never detect native support.
+        sh = self._sh()
+        assert "split('\\n# TRUECLOUD_PATCH', 1)[0]" in sh
+        assert "no further nesting" in extract_blocks()["CRUD_BLOCK"], (
+            "if this ever stops being true, the probe comment is stale"
+        )
+
     def test_restart_fires_when_any_needed_module_landed(self):
         # Keying the restart off providers alone would leave a freshly-patched
         # nested module on disk and never loaded on a native-B2 box.
@@ -153,6 +171,18 @@ class TestIndependentModules:
         assert '_backend_ok' in tail
         assert '"$_b2_ok"' not in tail
 
+    def test_partial_failure_still_schedules_the_restart(self):
+        # If providers fails but nested landed (or vice versa), something new IS
+        # on disk. Collapsing that into "nothing to do" would leave the module
+        # that succeeded permanently unloaded.
+        src = heredoc_source()
+        assert "sys.exit(2 if _landed else 1)" in src
+        assert "_landed = (providers_needed and b2_ok and restic_ok) or (nested_needed and nested_ok)" in src
+
+        sh = self._sh()
+        assert '_rc=$?' in sh
+        assert '[ "$_rc" = "2" ]' in sh
+
 
 class TestOptIn:
     """Nested-snapshot support must be opt-in and must never self-enable."""

tests/test_truecloud_nested.py

@@ -21,9 +21,9 @@ 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_all,
     cleanup_task,
     current_mounts_under,
     delete_snapshot_tree,
@@ -255,9 +255,6 @@ class TestDeleteSnapshotTree:
 
 
 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
@@ -344,13 +341,10 @@ class TestStageNestedOrdering:
 
 
 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.
+        # The sidecar is the ONLY record of the pinned snapshot, precisely so a
+        # middlewared restart cannot orphan the tree.
         import truecloud_nested as tn
 
         monkeypatch.setattr(tn, "STAGING_BASE", str(tmp_path))
@@ -359,7 +353,6 @@ class TestCleanupTask:
         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: [])
 
@@ -489,6 +482,86 @@ class TestTeardown:
         assert ["umount", "-l", ROOT] in runner.calls
 
 
+class TestCleanupAll:
+    """uninstall.sh and recover.sh call this instead of reimplementing teardown."""
+
+    def test_reports_orphan_snapshots_before_deleting_their_sidecars(self, tmp_path):
+        # The sidecar is the only record that an interrupted run's snapshot tree
+        # is still on disk. Deleting it without naming the snapshot orphans the
+        # whole tree silently.
+        base = tmp_path / "stage"
+        base.mkdir()
+        (base / "cloud_backup-5.snapshot").write_text("Tap@interrupted")
+
+        mounts_file = tmp_path / "mounts"
+        mounts_file.write_text("")
+
+        lines, errors = cleanup_all(
+            base=str(base), runner=FakeRunner(), mounts_file=str(mounts_file)
+        )
+        assert errors == []
+        assert any("Tap@interrupted" in ln for ln in lines)
+        assert any("zfs destroy -r" in ln for ln in lines)
+        # Sidecar cleared only after being reported.
+        assert not (base / "cloud_backup-5.snapshot").exists()
+
+    def test_unmounts_everything_under_the_base_deepest_first(self, tmp_path):
+        base = tmp_path / "stage"
+        base.mkdir()
+        mounts_file = tmp_path / "mounts"
+        mounts_file.write_text(
+            f"tmpfs {base} tmpfs rw 0 0\n"
+            f"tmpfs {base}/cloud_backup-5 tmpfs rw 0 0\n"
+            f"tmpfs {base}/cloud_backup-5/apps tmpfs rw 0 0\n"
+        )
+        runner = FakeRunner()
+        _lines, errors = cleanup_all(
+            base=str(base), runner=runner, mounts_file=str(mounts_file)
+        )
+        assert errors == []
+        order = [c[-1] for c in runner.calls if c[0] == "umount"]
+        assert order == [
+            f"{base}/cloud_backup-5/apps",
+            f"{base}/cloud_backup-5",
+            str(base),
+        ]
+
+    def test_keeps_sidecars_when_an_unmount_failed(self, tmp_path):
+        # If a mount is stuck, the snapshot is still pinned — so the record of it
+        # must survive for the next run (or the operator) to act on.
+        base = tmp_path / "stage"
+        base.mkdir()
+        (base / "cloud_backup-5.snapshot").write_text("Tap@stuck")
+        mounts_file = tmp_path / "mounts"
+        mounts_file.write_text(f"tmpfs {base}/cloud_backup-5 tmpfs rw 0 0\n")
+
+        class Stuck(FakeRunner):
+            def __call__(self, cmd):
+                self.calls.append(cmd)
+
+                class R:
+                    returncode = 32
+                    stderr = "target is busy"
+
+                return R
+
+        _lines, errors = cleanup_all(
+            base=str(base), runner=Stuck(), mounts_file=str(mounts_file)
+        )
+        assert errors, "a stuck unmount must be reported"
+        assert (base / "cloud_backup-5.snapshot").exists()
+
+    def test_is_a_noop_on_a_clean_system(self, tmp_path):
+        mounts_file = tmp_path / "mounts"
+        mounts_file.write_text("")
+        lines, errors = cleanup_all(
+            base=str(tmp_path / "absent"), runner=FakeRunner(),
+            mounts_file=str(mounts_file),
+        )
+        assert errors == []
+        assert lines == ["  None active."]
+
+
 class TestCurrentMountsUnder:
     def test_matches_only_the_staging_subtree(self, tmp_path):
         mounts_file = tmp_path / "mounts"

uninstall.sh

@@ -95,38 +95,13 @@ echo ""
 # These bind mounts pin their ZFS snapshots, so they must go before anything
 # tries to destroy those snapshots. Deepest first.
 
+# Delegated to the patch module rather than reimplemented here: the depth
+# ordering and lazy-umount fallback are fiddly, and a shell copy would be the
+# untested one.
 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."
+if ! python3 "$PATCH_DIR/patch/truecloud_nested.py" cleanup; then
+    echo "  WARNING: staging mounts remain. Unmount them manually; until you do,"
+    echo "           the ZFS snapshots they pin cannot be destroyed."
 fi
 
 # The opt-in marker lives in the repo dir; remove it so a later re-install