diff --git a/CHANGELOG.md b/CHANGELOG.md index f0bf1c8..3b41301 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -101,6 +101,24 @@ worse than no alert, because one day it carries a security fix. `get_dataset_recursive` is carried as our own copy — removing the dependency on both versions instead of asserting it. +- **The patch no longer reaches into CloudSync tasks it has no business touching.** + `create_snapshot` is module-global in `plugins/cloud/snapshot.py` and is imported by + **`cloud_sync.py` as well as `cloud_backup/sync.py`** — so the wrapper sat in the + path of every rclone/Storj **CloudSync** task with `snapshot=true`, and issued a + `zfs.dataset.query` before deciding it had nothing to do. That added a brand-new + failure mode to jobs that worked fine before this patch was installed, and worse: a + CloudSync task that ever *did* get staged would **never be torn down**, because the + teardown is wired into `cloud_backup`'s `restic_backup` and `CRUD_BLOCK` + deliberately leaves CloudSync's nesting guard intact — the bind mounts would pin the + ZFS snapshot forever. The staging path now bails out immediately unless the snapshot + is named `cloud_backup-*`, before any middleware call. + +- **Teardown warnings are no longer silently swallowed on TrueNAS ≤ 25.10.** The async + wrapper's `finally` dropped the `logger=` kwarg that the sync one passes, so a + cleanup that failed to unmount a bind mount *or* to delete a snapshot tree logged + **nothing at all** — on the only platform anyone actually runs. `run_in_thread` + forwards `**kwargs` via `functools.partial`; it was a regression, not a limitation. + - **`do_delete` is recognised as `delete`.** TrueNAS 24.10 and 25.04 declare `do_delete` (the `CRUDService` convention); 25.10 renamed it to `delete`. Both answer to `zfs.snapshot.delete`. Accepting only the literal name reported both older diff --git a/patch/apply.sh b/patch/apply.sh index 087ebeb..728febc 100755 --- a/patch/apply.sh +++ b/patch/apply.sh @@ -526,6 +526,28 @@ if _tc_nested is not None: def _tc_stage(middleware, path, name, snapshot, snap_path): # Synchronous, and always called from a worker thread (see above). + # + # ONLY cloud_backup. Bail out before touching anything otherwise. + # + # create_snapshot is module-global in plugins/cloud/snapshot.py and is + # imported by cloud_sync.py as well as cloud_backup/sync.py -- so this + # wrapper sits in the path of every rclone/Storj CloudSync task with + # snapshot=true, not just ours. Two consequences, and the second is worse: + # + # * everything below is a NEW failure mode for tasks that worked before we + # were installed. A `zfs.dataset.query` that errors would break a + # CloudSync job we have no business touching. + # * if a CloudSync task ever were staged, nothing would ever tear it down: + # the teardown is wired into cloud_backup's restic_backup finally, and + # CRUD_BLOCK deliberately leaves CloudSync's nesting guard intact. The + # bind mounts would pin the snapshot forever. + # + # cloud_backup names its snapshot "cloud_backup-"; cloud_sync names it + # "cloud_sync-"; the stock default is "cloud_task-onetime". Anything that + # is not ours gets stock behaviour, untouched, with no extra middleware call. + if not name.startswith("cloud_backup"): + return snapshot, snap_path + _logger = getattr(middleware, "logger", None) try: # Enumerate datasets AFTER the snapshot, never before. The snapshot is diff --git a/tests/test_apply_blocks.py b/tests/test_apply_blocks.py index 3c0f362..44f716b 100644 --- a/tests/test_apply_blocks.py +++ b/tests/test_apply_blocks.py @@ -407,3 +407,39 @@ class TestTheTwoNativeProbesCannotDrift: assert static_native == expect_native, ( f"compat.py says native={static_native} for {src!r}" ) + + +class TestOnlyOurOwnTasksAreTouched: + """create_snapshot is module-global, and cloud_sync.py imports it too. + + plugins/cloud/snapshot.py::create_snapshot is imported by BOTH + cloud_backup/sync.py and cloud_sync.py, so our wrapper sits in the path of every + rclone/Storj CloudSync task with snapshot=true -- tasks this patch has no business + touching. Two consequences, the second much worse than the first: + + * every middleware call we add is a NEW failure mode for a job that worked + before we were installed; + * a staged CloudSync task would NEVER be torn down. The teardown is wired into + cloud_backup's restic_backup finally, and CRUD_BLOCK deliberately leaves + CloudSync's nesting guard intact -- so the bind mounts would pin the ZFS + snapshot forever. + + cloud_backup names its snapshot "cloud_backup-", cloud_sync "cloud_sync-", + and stock's default is "cloud_task-onetime". + """ + + @pytest.mark.parametrize("name", ["SNAPSHOT_ASYNC", "SNAPSHOT_SYNC"]) + def test_the_staging_path_is_gated_on_cloud_backup(self, name): + block = extract_blocks()[name] + assert 'if not name.startswith("cloud_backup"):' in block + + @pytest.mark.parametrize("name", ["SNAPSHOT_ASYNC", "SNAPSHOT_SYNC"]) + def test_the_bail_out_precedes_every_middleware_call(self, name): + # The point is to add NO new failure mode to a CloudSync task. If any + # middleware call happened before the bail-out, we would already have broken + # the thing we are trying not to touch. + block = extract_blocks()[name] + gate = block.index('if not name.startswith("cloud_backup"):') + for call in ("middleware.call_sync(", "_tc_nested.stage_nested(", + "_tc_nested.delete_snapshot_tree("): + assert gate < block.index(call), f"{call} runs before the cloud_backup gate"