test: pin the deletion scope with regression tests - #6
Conversation
The suite proved a candidate list was deleted, batched and checkpointed
correctly, but never proved the list contained only orphans. FakeCH's
query_row_block_stream 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
previous 65-test suite, nine of eleven deliberate breakages of the delete
scope passed fully green, including:
- LEFT ANTI JOIN -> LEFT JOIN, which makes every *referenced* object a
deletion candidate
- --dry-run performing real deletions
- dropping the clusterAllReplicas fan-out, so blobs referenced only by
another replica look orphaned
- pointing disk_name at a nonexistent disk, orphaning the whole bucket
- dropping the --useage window and the cluster/replica preflight
Only the two error-bookkeeping mutations were caught. All eleven are now
caught.
s3gc.py is deliberately untouched: the anti-join is correct today, so this
changes no behaviour. It exists so a future edit that quietly widens the
deletion scope fails a test instead of reaching a customer bucket.
Three defects found while auditing are recorded, not fixed, because each
needs an explicit decision (CHANGELOG.md, TODO.md):
- USEAGE_HOURS=0 silently removes the only guard against deleting a part
between its blob upload and its registration in remote_data_paths.
render.py accepts 0; `if args.useage else ""` then emits no clause.
- --useafter is interpolated unquoted, the only unquoted value in the
anti-join WHERE clause. Recorded as a strict xfail.
- The cluster/replica preflight is a point-in-time check, run once, while
the anti-join is re-issued per sample over hours.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review record — how this gap was foundPosting the method so the evidence is on the record and reproducible, not just the conclusion. The deletion scope was audited by mutation testing: the repo was copied to a scratch directory, the delete-scoping logic was deliberately broken eleven different ways, and the full suite was run against each. A mutation that "survives" is one the tests did not notice.
2 of 11 → 11 of 11. Root cause of the gap
def query_row_block_stream(self, query):
self.stream_query = query # stored, never evaluated
return FakeStream(self.blocks)The candidate list was hardcoded by the fake. The suite proved "given a list of orphans, deletion, batching and checkpointing behave correctly" — which it did well — but never "that list contains only orphans". The two tests that read What this PR does and does not claimThe anti-join is correct today, and there is no evidence s3gc has ever deleted referenced data. Findings surfaced during the auditThree defects were found and deliberately not fixed in this PR, so each got an explicit decision rather than a drive-by change:
Anyone can reproduce the table above; the mutation driver is a short script that patches |
1 participant
Summary
Adds regression coverage for the SQL safety boundary: only collected, unreferenced objects on the configured disk and replicas can be deleted.
Validation
First in the stack: #6 → #7 → #9 → #10.