v0.3.5: log the recursive-delete failure instead of swallowing it

delete_snapshot_tree tries one recursive delete first, then falls back to sweeping
the tree by name. The exception from the fast path was discarded.

That failure is usually benign -- stock's finally already removed the parent once
our mounts were released, which is exactly what the sweep exists to handle. But if
the cause were anything else, this was the only place it was ever visible, and it
went straight to /dev/null. The sweep would then report some different, downstream
symptom. It is now logged before falling through.

Also annotated the two remaining static-analysis findings as considered rather than
leaving them to be re-litigated every audit: subprocess is always invoked in list
form (no shell, so ZFS dataset names cannot inject), and a partial `systemctl` path
is moot in a script that only ever runs as root.

Extended ruleset (E,F,W,B,S,SIM,UP,C4,RET,ARG,A,ISC) and shellcheck -S style both
report zero. 132 tests.
This commit is contained in:
flan
2026-07-13 16:06:12 +00:00
parent 126756498c
commit 45f957af23
7 changed files with 39 additions and 9 deletions
+15 -3
View File
@@ -279,7 +279,11 @@ def current_mounts_under(root, mounts_file="/proc/self/mounts"):
def _run(cmd):
return subprocess.run(cmd, capture_output=True, text=True, check=False)
# List form, never shell=True: `cmd` is built from our own mount plan, so ZFS
# dataset names cannot inject. Runs as root by definition (it mounts).
return subprocess.run( # noqa: S603
cmd, capture_output=True, text=True, check=False
)
def apply_plan(mounts, runner=_run, isdir=os.path.isdir):
@@ -379,8 +383,16 @@ async def delete_snapshot_tree(middleware, snapshot, logger=None):
try:
await middleware.call("zfs.snapshot.delete", snapshot, {"recursive": True})
return
except Exception: # noqa: BLE001 - fall through to the explicit sweep
pass
except Exception as e: # noqa: BLE001 - fall through to the explicit sweep
# Usually just "parent already gone" (stock's finally won the race once our
# mounts were released), which the sweep below handles. Log it rather than
# swallow it: if the real cause is something else, this is the only place
# it is visible -- the sweep would report a different, downstream failure.
if logger:
logger.debug(
"truecloud-patch: recursive delete of %s failed (%r); sweeping "
"the tree by name instead", snapshot, e,
)
# The parent may already be gone -- stock's `finally` can win the race once
# our mounts are released -- which fails the recursive delete while the