test: pin the deletion scope with regression tests

CHANGELOG.md

@@ -15,7 +15,52 @@ expose.
 
 ## [Unreleased]
 
-Nothing yet.
+### Added
+
+- **Regression tests pinning the deletion scope itself.** The suite already
+  proved that a candidate list is deleted, batched and checkpointed correctly,
+  but nothing proved the list contained only orphans: the ClickHouse fake
+  ignores the anti-join SQL and returns pre-canned blocks, so the one statement
+  that forms the entire safety boundary was never asserted on.
+
+  Mutation testing established the gap rather than assuming it. Against the
+  65-test suite, **nine of eleven** deliberate breakages of the delete scope
+  passed fully green — including turning `LEFT ANTI JOIN` into a plain
+  `LEFT JOIN` (every *referenced* object becomes a deletion candidate) and
+  making `--dry-run` delete for real. Only the two error-bookkeeping mutations
+  were caught.
+
+  The scope is now asserted directly: anti-join semantics, the
+  `remote_path = objpath` join key, the `disk_name` predicate, the
+  `clusterAllReplicas` fan-out when a cluster is configured, `active=true`,
+  the `--useage` window, candidates being drawn only from the auxiliary table,
+  that a dry run cannot reach S3 at all, and that the cluster/replica preflight
+  gates the delete path. All eleven mutations are now caught.
+
+  Why it is worth the words: the anti-join is correct today, so this changes no
+  behaviour. It exists so that a future edit which quietly widens the deletion
+  scope fails a test instead of reaching a customer bucket.
+
+### Known defects recorded
+
+- **`USEAGE_HOURS=0` silently removes the only protection against deleting a
+  part mid-write.** ClickHouse uploads a part's blobs to S3 and registers them
+  in `system.remote_data_paths` a moment later; in that window a live blob is
+  absent from the reference table and looks orphaned. There is no per-object
+  re-check before the S3 delete, so the `--useage` clause is the whole safety
+  margin — and `if args.useage else ""` emits no clause at all for 0, which
+  `render.py` accepts (only negatives are rejected). Documented by
+  `test_useage_zero_disables_the_age_guard`; the fix (refuse, or warn) is
+  deliberately deferred to an explicit decision and tracked in `TODO.md`.
+
+- **`--useafter` is interpolated unquoted**, so the value lands as a bare SQL
+  identifier rather than a string literal. It is the only unquoted value in the
+  anti-join `WHERE` clause. Fail-closed in practice (unknown identifier), and
+  recorded as a strict `xfail` so it flips to a failure the moment it is fixed.
+
+- **The cluster/replica preflight is a point-in-time check**, run once, while
+  the anti-join is re-issued per sample over what can be hours. A replica
+  dropping out mid-run is not re-detected.
 
 ## [0.6.0] - 2026-08-19
 

TODO.md

@@ -13,3 +13,11 @@ forward-looking backlog.
   fixtures, and remain excluded from CI.
 - [ ] Require the `Container / test` GitHub Actions check before pull-request
   merges in the repository branch-protection settings.
+- [ ] Decide what `USEAGE_HOURS=0` should do. It disables the age window that
+  is the only guard against deleting a part between its blob upload and its
+  registration in `system.remote_data_paths`. Options: reject it in
+  `render.py`, warn loudly in `s3gc.py`, or leave it and document it. Covered
+  today only by a test that documents the hazard.
+- [ ] Quote `--useafter` as a SQL string literal (strict `xfail` in the suite).
+- [ ] Consider re-checking cluster topology per sample, not once per run, so a
+  replica lost mid-run cannot widen the deletion scope.

tests/test_s3gc.py

@@ -932,3 +932,205 @@ def test_renderer_wires_auth_env_into_the_job(tmp_path):
     assert 'name: S3GC_S3AUTH' in result.stdout
     assert 'value: "aws"' in result.stdout
     assert 'value: "sso"' in result.stdout
+
+
+# ---------------------------------------------------------------------------
+# Deletion-scope invariants.
+#
+# The tests above prove a candidate list is deleted, batched and checkpointed
+# correctly. They cannot prove the list contains only orphans, because
+# FakeCH.query_row_block_stream ignores the SQL and returns pre-canned blocks.
+#
+# Mutation testing confirmed the gap: turning LEFT ANTI JOIN into a plain LEFT
+# JOIN, pointing disk_name at a nonexistent disk, dropping the join key,
+# dropping the clusterAllReplicas fan-out, dropping the --useage window, and
+# even making --dry-run delete for real ALL left the suite fully green.
+#
+# These tests pin the scope itself: what may be considered for deletion, and
+# what must be excluded. They assert on the generated anti-join because that
+# single statement is the whole safety boundary.
+# ---------------------------------------------------------------------------
+
+
+def _antijoin_sql(s3gc_module, args_factory, monkeypatch, **overrides):
+    """Return the anti-join s3gc would stream, whitespace-normalised."""
+    namespace = s3gc_module["do_use"].__globals__
+    client = FakeCH()
+    overrides.setdefault("dryrun_flag", True)
+    monkeypatch.setitem(namespace, "args", args_factory(**overrides))
+    monkeypatch.setitem(namespace, "ch_client", client)
+
+    s3gc_module["do_use"]()
+
+    return " ".join(client.stream_query.split())
+
+
+def test_candidates_are_only_unreferenced_objects(s3gc_module, args_factory, monkeypatch):
+    """The join must be LEFT ANTI: keep rows with NO match in remote_data_paths.
+
+    A plain LEFT JOIN keeps matched rows too, so every referenced object becomes
+    a deletion candidate. That is the disaster case and it must not be reachable
+    by an edit that keeps the tests green.
+    """
+    sql = _antijoin_sql(s3gc_module, args_factory, monkeypatch)
+
+    assert "LEFT ANTI JOIN" in sql
+
+
+def test_candidates_come_only_from_the_collected_inventory(
+    s3gc_module, args_factory, monkeypatch
+):
+    """Deletion can only ever consider rows collected into the auxiliary table.
+
+    This is what makes objects created after the collect phase safe: they are
+    absent from the table, so the delete phase cannot see them at all.
+    """
+    sql = _antijoin_sql(s3gc_module, args_factory, monkeypatch)
+
+    assert "FROM `s3objects_for_s3` AS s3o" in sql
+
+
+def test_reference_check_matches_on_object_path(s3gc_module, args_factory, monkeypatch):
+    """Losing the join key makes nothing match, so every object looks orphaned."""
+    sql = _antijoin_sql(s3gc_module, args_factory, monkeypatch)
+
+    assert "rdp.remote_path = s3o.objpath" in sql
+
+
+@pytest.mark.parametrize("disk", ["s3", "gcs"])
+def test_reference_check_is_scoped_to_the_configured_disk(
+    s3gc_module, args_factory, monkeypatch, disk
+):
+    """A wrong disk name matches no reference at all, orphaning the whole bucket."""
+    sql = _antijoin_sql(s3gc_module, args_factory, monkeypatch, s3diskname=disk)
+
+    assert f"rdp.disk_name='{disk}'" in sql
+
+
+def test_cluster_cleanup_checks_every_replica(s3gc_module, args_factory, monkeypatch):
+    """With a cluster configured, references must be read from ALL replicas.
+
+    Reading only the local replica orphans blobs that another replica still
+    references — the zero-copy replication disaster case.
+    """
+    sql = _antijoin_sql(s3gc_module, args_factory, monkeypatch, clustername="prod")
+
+    assert "clusterAllReplicas('prod', system.remote_data_paths)" in sql
+
+
+def test_without_a_cluster_only_the_local_replica_is_consulted(
+    s3gc_module, args_factory, monkeypatch
+):
+    """Documents the narrowed scope of an unclustered run.
+
+    Deleting with no --cluster only sees one replica's references. The
+    Kubernetes entrypoint requires CLUSTERNAME for the delete phase; a direct
+    CLI run does not, so the narrowing is real and deliberate here.
+    """
+    sql = _antijoin_sql(s3gc_module, args_factory, monkeypatch, clustername="")
+
+    assert "clusterAllReplicas" not in sql
+    assert "system.remote_data_paths AS rdp" in sql
+
+
+def test_tombstoned_objects_are_not_reconsidered(s3gc_module, args_factory, monkeypatch):
+    """active=true excludes rows already tombstoned by an earlier delete attempt."""
+    sql = _antijoin_sql(s3gc_module, args_factory, monkeypatch)
+
+    assert "s3o.active=true" in sql
+
+
+def test_age_guard_excludes_recently_written_objects(
+    s3gc_module, args_factory, monkeypatch
+):
+    """--useage is the ONLY protection against deleting a part mid-write.
+
+    ClickHouse uploads a part's blobs to S3 and registers them in
+    remote_data_paths a moment later. In that window a live blob is absent from
+    remote_data_paths and looks orphaned. There is no per-object re-check before
+    the S3 delete, so this clause is the whole safety margin.
+    """
+    sql = _antijoin_sql(s3gc_module, args_factory, monkeypatch, useage=24)
+
+    assert "s3o.last_modified < now() - interval 24 hour" in sql
+
+
+def test_useage_zero_disables_the_age_guard(s3gc_module, args_factory, monkeypatch):
+    """Documents a HAZARD, not a desired behaviour.
+
+    `if args.useage else ""` means useage=0 emits no age clause, and
+    render.py accepts USEAGE_HOURS=0 (only negatives are rejected). A run
+    configured that way has no protection against the mid-write window above.
+    If s3gc is later changed to refuse or warn on 0, this test should fail and
+    be updated deliberately.
+    """
+    sql = _antijoin_sql(s3gc_module, args_factory, monkeypatch, useage=0)
+
+    assert "interval" not in sql
+
+
+def test_dry_run_performs_no_s3_operations(s3gc_module, args_factory, monkeypatch):
+    """A dry run must be incapable of touching S3, even with candidates present."""
+    namespace = s3gc_module["do_use"].__globals__
+
+    class ExplodingMinio:
+        def __getattr__(self, name):
+            raise AssertionError(f"dry run must not call S3: {name}")
+
+    client = FakeCH(blocks=[[("orphan-a", 10, "time"), ("orphan-b", 20, "time")]])
+    monkeypatch.setitem(namespace, "args", args_factory(dryrun_flag=True))
+    monkeypatch.setitem(namespace, "ch_client", client)
+    monkeypatch.setitem(namespace, "minio_client", ExplodingMinio())
+
+    s3gc_module["do_use"]()
+
+    assert client.inserts == []
+
+
+def test_delete_runs_preflight_before_touching_s3(
+    s3gc_module, args_factory, monkeypatch
+):
+    """The cluster/replica preflight must gate the delete path, not decorate it."""
+    namespace = s3gc_module["do_use"].__globals__
+
+    class ExplodingMinio:
+        def __getattr__(self, name):
+            raise AssertionError(f"preflight must run before S3: {name}")
+
+    def refuse():
+        raise RuntimeError("preflight refused")
+
+    monkeypatch.setitem(namespace, "args", args_factory(dryrun_flag=False))
+    monkeypatch.setitem(namespace, "ch_client", FakeCH(blocks=[[("orphan", 1, "time")]]))
+    monkeypatch.setitem(namespace, "minio_client", ExplodingMinio())
+    monkeypatch.setitem(namespace, "preflight_cluster", refuse)
+
+    with pytest.raises(RuntimeError, match="preflight refused"):
+        s3gc_module["do_use"]()
+
+
+def test_preflight_rejects_unexpected_replica_count(
+    s3gc_module, args_factory, monkeypatch
+):
+    """A topology that does not match must fail closed before any deletion."""
+    namespace = s3gc_module["preflight_cluster"].__globals__
+    monkeypatch.setitem(namespace, "args", args_factory(expected_replicas=2))
+    monkeypatch.setitem(namespace, "ch_client", FakeCH(cluster="cluster", replicas=3))
+
+    with pytest.raises(RuntimeError, match="replica preflight failed"):
+        s3gc_module["preflight_cluster"]()
+
+
+@pytest.mark.xfail(
+    strict=True,
+    reason="known defect: --useafter is interpolated unquoted, so the value "
+    "lands as a bare SQL identifier instead of a string literal "
+    "(s3gc.py, make_antijoin/after_condition). Fail-closed in practice, but "
+    "it is the only unquoted value in the WHERE clause.",
+)
+def test_useafter_is_quoted_as_a_string_literal(
+    s3gc_module, args_factory, monkeypatch
+):
+    sql = _antijoin_sql(s3gc_module, args_factory, monkeypatch, useafter="some/object")
+
+    assert "s3o.objpath > 'some/object'" in sql