test: pin the deletion scope with regression tests - #6
Open
lesandie wants to merge 1 commit into
Open
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The suite proved that a candidate list is deleted, batched and checkpointed correctly. It never proved the list contains only orphans.
FakeCH.query_row_block_streamignores the anti-join SQL and returns pre-canned blocks, so the one statement that forms the entire safety boundary was never asserted on. The two tests that did readstream_queryonly checkedORDER BY.Evidence
Mutation testing established the gap rather than assuming it — the repo was copied to a scratch dir, the delete scope was broken eleven ways, and the full suite was run against each.
LEFT ANTI JOIN→LEFT JOIN— candidates include referenced objects--dry-runperforms real deletionsclusterAllReplicasfan-out — blobs referenced only by another replica look orphaneddisk_nameat a nonexistent disk — orphans the whole bucketremote_path = objpathjoin key--useagegrace windows3o.active=truepreflight_cluster()call2 of 11 → 11 of 11.
What this does not change
s3gc.pyis deliberately untouched. The anti-join is correct today and no behaviour changes. These tests exist so that a future edit which quietly widens the deletion scope fails a test instead of reaching a customer bucket.What is now pinned
13 tests asserting the scope directly: anti-join semantics, the
remote_path = objpathjoin key, thedisk_namepredicate, theclusterAllReplicasfan-out when a cluster is configured,active=true, the--useagewindow, 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.Defects recorded, not fixed
Each needs an explicit decision, so they are documented in
CHANGELOG.mdand tracked inTODO.mdrather than changed here:USEAGE_HOURS=0silently removes the only guard against deleting a part mid-write. ClickHouse uploads a part's blobs to S3 and registers them insystem.remote_data_pathsa moment later; in that window a live blob looks orphaned. There is no per-object re-check before the S3 delete, so the--useageclause is the whole safety margin — andif args.useage else ""emits no clause for 0, whichrender.pyaccepts (only negatives are rejected). Covered bytest_useage_zero_disables_the_age_guard, which documents the hazard.--useafteris interpolated unquoted, the only unquoted value in the anti-joinWHEREclause. Recorded as a strictxfailso it flips to a failure the moment it is fixed.Checks
🤖 Generated with Claude Code