Fix: --disable-nested-snapshots did not disable anything until reboot

apply.sh only ever ADDED patches; there was no revert path anywhere. Disabling
removed the opt-in marker and then merely skipped re-applying -- but the overlay
persists for the whole boot, so the previously patched cloud/{snapshot,crud}.py,
cloud_backup/sync.py and _truecloud_nested.py were all still on disk, and
middlewared re-imported them on the restart install.sh performs.

It printed "DISABLED (stock guard restored)" while the feature kept running until
the next reboot. Someone disabling it because they were worried about it would
have believed it was off.

apply.sh now reverts on every not-needed path (opt-out, or superseded by native
support): remove the module FIRST -- every injected block is guarded by
`if _tc_nested is not None`, so the stock guard comes back even if a later step
fails -- then strip the appended blocks from the three patched files.

restic.py also carries a TRUECLOUD_PATCH block but belongs to the providers
module; reverting it would silently break B2 backups, so it is explicitly
excluded. Verified: the three nested files restore byte-for-byte to stock, the
module is removed, and restic.py's block survives.

install.sh --disable also tears the staging tree down first, since those bind
mounts pin ZFS snapshots that could otherwise never be destroyed.

Updating WITHOUT the flag was always correct and is unchanged: the nested module
is never installed into middleware unless explicitly enabled.

109 tests, ruff and shellcheck clean.

CHANGELOG.md

@@ -1,5 +1,32 @@
 # Changelog
 
+## v0.3.2 — 2026-07-13
+
+### Fixed
+
+- **`install.sh --disable-nested-snapshots` did not actually disable anything
+  until the next reboot.** `apply.sh` only ever *added* patches — there was no
+  revert path. Disabling removed the opt-in marker and then merely *skipped*
+  re-applying, but the overlay persists for the whole boot, so the previously
+  patched `plugins/cloud/{snapshot,crud}.py`, `plugins/cloud_backup/sync.py` and
+  `_truecloud_nested.py` were all still sitting there — and middlewared
+  re-imported them on the restart `install.sh` performs.
+
+  It printed *"DISABLED (stock guard restored)"* while the feature kept running.
+  Someone turning it off *because they were worried about it* would have believed
+  it was off.
+
+  `apply.sh` now actively reverts: it removes the module first (every injected
+  block is guarded by `if _tc_nested is not None`, so the stock guard is restored
+  even if a later step fails), then strips its appended blocks from the three
+  patched files. `restic.py` also carries a `TRUECLOUD_PATCH` block but belongs to
+  the *providers* module and is deliberately left alone — reverting it would break
+  B2 backups. `install.sh --disable` also tears down any staging tree first, since
+  those bind mounts pin ZFS snapshots that could otherwise never be destroyed.
+
+  Updating **without** the flag was always correct and is unchanged: the nested
+  module is never installed into middleware unless it is explicitly enabled.
+
 ## v0.3.1 — 2026-07-13
 
 ### Added

install.sh

@@ -18,7 +18,7 @@
 
 set -euo pipefail
 
-VERSION="0.3.1"
+VERSION="0.3.2"
 
 # The directory containing install.sh is the permanent install location.
 PATCH_DIR="$(cd "$(dirname "$0")" && pwd)"
@@ -160,10 +160,17 @@ case "$_nested_choice" in
     off)
         if [ -f "$_NESTED_MARKER" ]; then
             rm -f "$_NESTED_MARKER"
-            echo "Nested-dataset snapshots: DISABLED (stock guard restored)."
+            # Tear down any staging tree first: those bind mounts PIN their ZFS
+            # snapshots, so leaving them would block those snapshots from ever
+            # being destroyed. apply.sh (below) then reverts the patched files.
+            python3 "$PATCH_DIR/patch/truecloud_nested.py" cleanup || \
+                echo "  WARNING: staging mounts remain; unmount them manually."
+            echo "Nested-dataset snapshots: DISABLED."
+            echo "  apply.sh will revert the patched middleware files and the stock"
+            echo "  guard is restored when middlewared restarts (this script does that)."
             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."
+            echo "  tasks first, or re-run with --enable-nested-snapshots."
         else
             echo "Nested-dataset snapshots: already disabled."
         fi

patch/apply.sh

@@ -32,7 +32,7 @@
 # Derive PATCH_DIR from this script's location (parent of the patch/ directory).
 PATCH_DIR="$(cd "$(dirname "$0")/.." && pwd)"
 LOG="$PATCH_DIR/apply.log"
-VERSION="0.3.1"
+VERSION="0.3.2"
 
 # Rotate log at 512 KB to avoid unbounded growth on a system volume.
 # Keep two prior generations (.1 and .2) so the last three boots are always available.
@@ -477,6 +477,57 @@ def patch_file(path, block):
     with open(path, "w", encoding="utf-8") as fh:
         fh.write(base.rstrip("\n") + "\n" + block)
 
+
+def unpatch_file(path):
+    """Strip our appended block, restoring the stock file. True if it was patched."""
+    try:
+        with open(path, encoding="utf-8") as fh:
+            content = fh.read()
+    except OSError:
+        return False
+    idx = content.find("\n# TRUECLOUD_PATCH")
+    if idx == -1:
+        return False
+    try:
+        with open(path, "w", encoding="utf-8") as fh:
+            fh.write(content[:idx].rstrip("\n") + "\n")
+    except OSError:
+        return False
+    return True
+
+
+def revert_nested(cloud_dir, sync_path):
+    """Undo the nested patch. Returns the names of what was actually reverted.
+
+    Skipping the patch is NOT enough to disable the feature. The overlay persists
+    for the whole boot, so an earlier run this boot may already have written the
+    patched files -- and middlewared re-imports them on the restart that
+    install.sh performs. Without this, `install.sh --disable-nested-snapshots`
+    would report "disabled" while the feature kept running until the next reboot.
+    """
+    reverted = []
+
+    # Remove the module FIRST. Every injected block is guarded by
+    # `if _tc_nested is not None`, so once it is gone they all no-op even if a
+    # later step here fails -- the guard is restored no matter what.
+    try:
+        os.unlink(os.path.join(cloud_dir, '_truecloud_nested.py'))
+        reverted.append('_truecloud_nested.py')
+    except OSError:
+        pass
+
+    # NB: restic.py also carries a TRUECLOUD_PATCH block, but that belongs to the
+    # providers module. Only these three are ours to revert.
+    for name, path in (
+        ('crud.py', os.path.join(cloud_dir, 'crud.py')),
+        ('sync.py', sync_path),
+        ('snapshot.py', os.path.join(cloud_dir, 'snapshot.py')),
+    ):
+        if unpatch_file(path):
+            reverted.append(name)
+
+    return reverted
+
 b2_ok = restic_ok = False
 nested_ok = False
 
@@ -517,16 +568,28 @@ 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 = ''
+if not nested_needed:
+    # Not just "skip": actively revert. The overlay lives for the whole boot, so a
+    # previously-applied patch is still sitting there and middlewared would
+    # re-import it on restart. See revert_nested().
     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')
     elif nested_native:
         nested_detail = 'superseded: TrueNAS handles nested-dataset snapshots natively'
         print('INFO: Nested module skipped — TrueNAS now handles nesting natively.')
-elif not nested_needed:
+    else:
         nested_detail = 'not needed'
         print('INFO: Nested module skipped.')
+
+    reverted = revert_nested(cloud_dir, sync_path)
+    if reverted:
+        print('OK: Reverted a previously-applied nested patch (' + ', '.join(reverted) + ').')
+        print('    The stock nesting guard is restored once middlewared restarts.')
+        nested_detail += ' — previous patch reverted'
+
+    if not nested_enabled:
+        print('INFO: Enable with: bash install.sh --enable-nested-snapshots')
 else:
     try:
         snapshot_py = os.path.join(cloud_dir, 'snapshot.py')

recover.sh

@@ -17,7 +17,7 @@
 #   bash /mnt/tank/truenas-truecloud-patch/patch/apply.sh
 #   systemctl restart middlewared
 
-VERSION="0.3.1"
+VERSION="0.3.2"
 
 PATCH_DIR="$(cd "$(dirname "$0")" && pwd)"
 

tests/test_apply_blocks.py

@@ -263,10 +263,57 @@ class TestOptIn:
     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:")
+        gate = src.index("if not nested_needed:")
         crud = src.index("patch_file(crud_py, CRUD_BLOCK)")
         assert gate < crud, "crud.py patch must sit inside the opt-in branch"
 
+    def test_disabling_REVERTS_the_patch_rather_than_merely_skipping_it(self):
+        """Skipping is not disabling.
+
+        The overlay persists for the whole boot, so a patch applied by an earlier
+        run this boot is still on disk — and middlewared re-imports it on the
+        restart install.sh performs. Without an active revert,
+        `--disable-nested-snapshots` reports "disabled" while the feature keeps
+        running until the next reboot.
+        """
+        src = heredoc_source()
+        assert "def unpatch_file(" in src
+        assert "def revert_nested(" in src
+        # The revert must run on every not-needed path (opt-out, superseded).
+        gate = src.index("if not nested_needed:")
+        revert = src.index("reverted = revert_nested(")
+        patch = src.index("patch_file(crud_py, CRUD_BLOCK)")
+        assert gate < revert < patch, "revert belongs in the not-needed branch"
+
+    def test_revert_removes_the_module_before_unpatching_files(self):
+        # Every injected block is guarded by `if _tc_nested is not None`, so
+        # deleting the module first means the guard is restored even if a later
+        # unpatch step fails.
+        src = heredoc_source()
+        body = src[src.index("def revert_nested("):src.index("def patch_file(") if
+                   src.index("def patch_file(") > src.index("def revert_nested(") else len(src)]
+        body = src[src.index("def revert_nested("):]
+        body = body[:body.index("\n\n\n")] if "\n\n\n" in body else body
+        assert body.index("_truecloud_nested.py") < body.index("crud.py")
+
+    def test_revert_never_touches_the_providers_patch(self):
+        # restic.py also carries a TRUECLOUD_PATCH block, but it belongs to the
+        # providers module. Reverting it would silently break B2 backups.
+        src = heredoc_source()
+        body = src[src.index("def revert_nested("):]
+        body = body[:body.index("return reverted")]
+        # Comments legitimately *mention* restic.py to explain why it is excluded;
+        # what matters is that no code line touches it.
+        code = "\n".join(
+            ln for ln in body.splitlines() if not ln.lstrip().startswith("#")
+        )
+        assert "restic" not in code
+        assert "b2.py" not in code
+        # It must only ever revert these three, plus the module itself.
+        assert "crud.py" in code
+        assert "sync_path" in code
+        assert "snapshot.py" in code
+
 
 def test_guard_is_relaxed_only_after_traversal_is_installed():
     # Ordering in apply.sh is a safety property: copy module -> patch snapshot.py

uninstall.sh

@@ -3,7 +3,7 @@
 
 set -euo pipefail
 
-VERSION="0.3.1"
+VERSION="0.3.2"
 
 PATCH_DIR="$(cd "$(dirname "$0")" && pwd)"
 _HOOK_COMMENT='TrueCloud provider patch (S3/B2)'