feat(deletion-retention): scheduled pruning policy for the purge audit log - #43490
feat(deletion-retention): scheduled pruning policy for the purge audit log#43490mikebridge wants to merge 17 commits into
Conversation
Code Review Agent Run #9ce86eActionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Coordination note for whoever merges this: #43485 (SC-115342, block-reason column) touches the same subsystem and four of the same files — The two changes are semantically independent: that one adds a nullable Whichever lands second rebases; no ordering preference from this side. One thing worth knowing if the reason column ever grows the status vocabulary: this PR adds an exhaustiveness test asserting every status in Comment by Claude (AI) on behalf of @mikebridge. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43490 +/- ##
==========================================
+ Coverage 80.13% 80.16% +0.02%
==========================================
Files 2924 2925 +1
Lines 172288 172535 +247
Branches 39995 40019 +24
==========================================
+ Hits 138060 138309 +249
+ Misses 31642 31639 -3
- Partials 2586 2587 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #398665
Actionable Suggestions - 4
-
superset/migrations/versions/2026-08-24_15-50_a6c21e5b4d93_index_purge_audit_pruning.py - 1
- Missing unit test coverage for new migration · Line 35-46
-
tests/unit_tests/commands/deletion_retention/test_prune_audit.py - 1
- Duplicate test setup code · Line 238-238
-
tests/integration_tests/deletion_retention/prune_audit_tests.py - 2
- Replace private API with public interface · Line 473-473
- Replace private API with public interface · Line 477-477
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/unit_tests/commands/deletion_retention/test_prune_audit.py - 1
- Wrong metric assertion in test · Line 273-273
-
superset/initialization/__init__.py - 1
- Duplicated imports-warning logic · Line 1056-1069
-
superset/commands/deletion_retention/prune_audit.py - 1
- Removed public interface member · Line 148-152
Review Details
-
Files reviewed - 14 · Commit Range:
e2e624f..831b571- superset/commands/deletion_retention/audit.py
- superset/commands/deletion_retention/prune_audit.py
- superset/config.py
- superset/initialization/__init__.py
- superset/migrations/versions/2026-08-24_15-50_a6c21e5b4d93_index_purge_audit_pruning.py
- superset/migrations/versions/2026-08-24_16-20_c7f53d184ea2_coordinate_purge_audit_pruning.py
- superset/models/purge_audit_log.py
- superset/tasks/deletion_retention.py
- tests/integration_tests/deletion_retention/_base.py
- tests/integration_tests/deletion_retention/audit_tests.py
- tests/integration_tests/deletion_retention/prune_audit_tests.py
- tests/unit_tests/commands/deletion_retention/test_prune_audit.py
- tests/unit_tests/initialization_test.py
- tests/unit_tests/migrations/test_purge_audit_coordination.py
-
Files skipped - 1
- UPDATING.md - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Thanks @mikebridge, LGTM. Spent real time on the survivor/boundary logic and the coordination lock since this is deletion logic on an audit table: the lock is a plain UPDATE on a singleton row rather than SELECT FOR UPDATE (SQLite doesn't support that), held through commit, and since now gets captured before any lock is taken, a concurrent writer's row always lands at or after that cutoff, so it's excluded from the run rather than racing into it. The clock-skew gap is the one the docstring already calls out. Bot threads all look properly resolved rather than waved off.
831b571 to
4592c42
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Code Review Agent Run #79f175Actionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…p ties when pruning Address the review threads on apache#43490: * A blockage streak's survivors are now the first row of each run of consecutive same-reason blocked rows — the streak's earliest row and the first row after every change of block reason — instead of the earliest row only. This is the audit writer's own suppression rule (finalize_retention_blocked) applied retroactively; previously a `report_schedule` block followed by a `cascade_integrity_failure` block lost the only durable evidence of the latter on the next prune. Run heads are exempt from age-out like the earliest row was; resolved-streak run heads still age out. The reason comparison is NULL-safe on every supported dialect so pre-feature rows form a run of their own that the first coded block ends, exactly as at write time. * Timestamp ties (legacy second-precision rows, two writers in one clock tick) are resolved on the preserving side, consistently: a pending row tied with a blocked row counts as preceding it (the block is deferred until the attempt resolves); a blocked row tied with a boundary sits on the boundary's resolved side (it ages out rather than seeding a new current streak) and the evidence guard now keeps the boundary until that tied row is gone; tied same-reason blocked rows are all retained. * Startup diagnostics warn when PURGE_AUDIT_PRUNING_ENABLED is not a boolean. The task already failed closed on such values, but the initializer collapsed them into "disabled" and emitted nothing, so a typo like "true" left the audit log growing without any warning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
|
Pushed 86e86b4 addressing the three open threads (reason transitions now survive pruning, timestamp ties resolve on the preserving side, non-boolean |
Code Review Agent Run #ec1a4dActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…p ties when pruning Address the review threads on apache#43490: * A blockage streak's survivors are now the first row of each run of consecutive same-reason blocked rows — the streak's earliest row and the first row after every change of block reason — instead of the earliest row only. This is the audit writer's own suppression rule (finalize_retention_blocked) applied retroactively; previously a `report_schedule` block followed by a `cascade_integrity_failure` block lost the only durable evidence of the latter on the next prune. Run heads are exempt from age-out like the earliest row was; resolved-streak run heads still age out. The reason comparison is NULL-safe on every supported dialect so pre-feature rows form a run of their own that the first coded block ends, exactly as at write time. * Timestamp ties (legacy second-precision rows, two writers in one clock tick) are resolved on the preserving side, consistently: a pending row tied with a blocked row counts as preceding it (the block is deferred until the attempt resolves); a blocked row tied with a boundary sits on the boundary's resolved side (it ages out rather than seeding a new current streak) and the evidence guard now keeps the boundary until that tied row is gone; tied same-reason blocked rows are all retained. * Startup diagnostics warn when PURGE_AUDIT_PRUNING_ENABLED is not a boolean. The task already failed closed on such values, but the initializer collapsed them into "disabled" and emitted nothing, so a typo like "true" left the audit log growing without any warning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
86e86b4 to
a24a131
Compare
|
Heads-up for re-review: I ran a focused data-systems/python/sqlalchemy review over the last delta (the reason-transition dedup + tie policy,
Also in the same amend: corrected a docstring that overclaimed writer-parity for reason-less (pre-feature) runs (the pruner is stricter there — it collapses legacy duplicates; the writer never suppresses a reason-less block), and added an A,A,B,A,A reason-return test. Integration suite 28/28 on Postgres + SQLite, unit 83/83, pre-commit clean. Sorry to bounce the approval — flagging so it gets a fresh look at the new head. |
Code Review Agent Run #f0cda0Actionable Suggestions - 0Additional Suggestions - 5
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
There was a problem hiding this comment.
Reviewed at a24a1313fedcd97c958877fc1c5fcbb811e5a43f. External-contributor review, so this is a COMMENT only — no approval implied either way.
🔴 First, before anything else: the APPROVED badge on this PR is stale
GitHub reports reviewDecision: APPROVED, which makes this PR look one-click mergeable. It is not. There is exactly one approving review on the PR, and it is not at head:
rusackas APPROVED @ 831b571c8529ec5ca22a0aec9d35bffbaddcb296 2026-08-25T19:50:40Z
LIVE HEAD = a24a1313fedcd97c958877fc1c5fcbb811e5a43f 2026-09-01T17:14:23Z
GET /repos/apache/superset/pulls/43490/reviews returns no APPROVED review whose commit_id equals headRefOid. Worse — 831b571c85 is not among the seven commits returned by GET /pulls/43490/commits at all. The approved tree is unreachable from head: the branch was rebased after the approval.
(Set membership is the reliable test here, not timestamps. Five of the seven commits carry author dates predating the approval, which looks reassuring and is not — their committer dates are 2026-08-25T20:14–20:19Z, roughly 25 minutes after the approval. That gap is the rebase.)
The seven commits now on the branch:
5b0922bc8d feat(deletion-retention): scheduled pruning policy for the purge audit log
961a87bec1 fix: close survivor-invariant holes found in review
8dd78c9d39 fix: close the promotion path an adversarial re-review found
99389d208e fix: apply the unstable-block guard to age-based expiry
4592c4217b fix: coordinate purge audit pruning
79ebceba18 fix: chain the pruning migrations after the merged reason column
a24a1313fe fix: keep reason transitions and resolve timestamp ties when pruning
Three of those are explicitly review-driven fixes to the deletion predicate.
831b571c85 shares both its subject (fix(deletion-retention): coordinate purge audit pruning) and its author date with on-branch 4592c4217b, so it is that commit's pre-rebase counterpart. The approval therefore covered a tree equivalent to the branch through 4592c4217b, and the two commits provably never seen are 79ebceba18 and a24a1313fe — the latter, the timestamp-tie fix, landing a full week later on 2026-09-01.
This repo does not appear to dismiss stale reviews on push. So three things are true at once: the badge says APPROVED, CI is green, and the tree that was approved no longer exists on the branch. Every surface a reviewer glances at says "approved and clean", while the thing that was approved is gone. A merge on that approval would ship code rusackas never saw, on a feature that irreversibly deletes rows from an audit log.
The concrete ask: a maintainer should dismiss the stale approval, so the badge stops asserting coverage that does not exist — and 79ebceba18 + a24a1313fe should get a fresh look at head. For scope, this PR's own delta is 15 files, +2349/−27; that is what needs re-reviewing.
Right now the only thing standing between that stale badge and a merge is that the branch is mergeable: false / mergeable_state: "dirty" — it is conflicting and cannot land until rebased. Please don't rely on that. Whoever merges this should get a fresh approval at head. The re-review request in the 2026-09-01 comment is the right instinct; the badge just doesn't reflect it.
Everything below is a genuine review of the code at head, not a rubber stamp of that badge.
The deletion predicate: I worked it hard and did not find an over-deletion path
This is the part that matters. Scheduled, automated, irreversible deletion from the record you consult precisely when something has gone wrong. I traced every path in prune_audit.py end to end and tried to construct a row that should survive but doesn't. I could not. Specifics, so you can check my work rather than take my word:
Candidate selection is inside the DELETE, not paged from a snapshot. _delete_statement (superset/commands/deletion_retention/prune_audit.py:513-520) embeds the candidate SELECT as a derived table, so every survivor/boundary/status/age predicate is evaluated against the same committed state as the mutation. No stale id list crosses a transaction boundary. This is the right shape and it structurally kills the whole class of "selected as duplicate, promoted to survivor before the delete lands" races.
Ties resolve on the preserving side, and the ordering question turns out to be a non-issue. .order_by(table.c.created_on) (:410, :466, :508) is indeed not a total order — but it isn't choosing a survivor. Survivorship is decided entirely by the set predicate _repeats_an_earlier_block (:288-347), which uses strict earlier.created_on < table.created_on, so tied same-reason blocks are not "earlier" than each other and all of them are retained. ORDER BY only picks which of several equally-deletable rows go in this batch. Adding id as a tiebreaker would make batching deterministic, but it would not change which rows survive.
Reason transitions survive, including the tied case. The inclusive >= / <= bounds on reason_change (:325-326) are correct and, as the comment says, can only add boundaries — i.e. only ever preserve more. I hand-walked A,A,B,B,A: the second A and second B prune; the run heads A@t1, B@t3, and the returning A@t5 all survive. I also checked the case that worried me most — whether deleting a duplicate can destroy the only distinct-reason witness that protects a later run head. It cannot: a duplicate's own run head is necessarily inside the same witness interval (if it weren't, an intervening distinct-reason row would have disqualified the duplicate in the first place), so the witness survives the delete. The predicate is stable under its own deletions.
All three previously-broken invariants are genuinely closed at head — verified in code, not from commit subjects:
| Invariant | Where it is enforced | Verified |
|---|---|---|
| Survivor invariant (boundary never recedes) | _evidence_candidates bounds_surviving_blocks, :490-501 — refuses to delete evidence while an older blocked/pending row exists for the entity |
✅ |
| Promotion path | _in_current_streak :274-285 is strict (>), so a row tied with a boundary sits on the boundary's resolved side and cannot mint a new exempt survivor |
✅ |
| Unstable-block guard on age-based expiry | _operational_candidates :445-454 — unstable_block applies _preceded_by_unresolved_attempt to the age category, not just the duplicate category |
✅ |
The clock/lock ordering is right, and this is the subtle part I want to call out as correct. write_ahead acquires the coordination lock at superset/commands/deletion_retention/audit.py:148 and only then calls utc_now() at :156. That ordering is what actually makes the invariant hold: a writer that loses the race to a prune batch gets a timestamp after that batch committed, so it cannot materialize in pruning's already-processed logical past. _recover_retention_blocked does the same (:369 then :394). If those two lines were ever reordered the whole guarantee collapses — worth a comment saying so, because it reads like incidental statement order.
finalize() / finalize_retention_blocked() deliberately do not take the lock, and that's fine — they mutate in place and never assign a new timestamp, and any blocked row after a pending row is already protected by _preceded_by_unresolved_attempt (:350-378, with <= so ties defer). Because the DELETE is a single statement, the boundary subquery and the pending guard always see the same snapshot, so there is no window where one sees "pending" and the other sees "confirmed".
Other gates I re-derived at head: alembic is single-headed (c7f53d184ea2 → a6c21e5b4d93 → 39097d124752, 383 revisions, exactly one head — the re-chain in 79ebceba18 is correct); pruning is disabled by default (superset/config.py:1034, and the task at superset/tasks/deletion_retention.py requires enabled is not True to bail, so 1/"true" fail closed); each batch is a single statement plus commit, so no partially-applied DELETE; CI is 61 success / 9 skipped / 3 neutral with zero failures; 7 review threads, 0 unresolved.
Test coverage pins the invariants, not just the happy path. tests/integration_tests/deletion_retention/prune_audit_tests.py has named tests for each: test_delete_rechecks_survivor_after_a_pending_attempt_appears, test_evidence_expiry_spares_a_boundary_that_still_bounds_blocked_rows, test_age_does_not_make_an_unstable_block_expirable, plus four dedicated tie tests (test_a_tied_reason_change_still_breaks_the_run, test_a_block_tied_with_an_unresolved_attempt_is_deferred, test_evidence_expiry_spares_a_boundary_tied_with_a_blocked_row, test_tied_same_reason_blocks_are_all_retained). That is the right shape for a change like this.
Findings
1. A prune batch holds the global audit lock across the DELETE, and a slow batch makes concurrent scheduled purges fail closed
_delete_batch (prune_audit.py:531-534) takes acquire_coordination_lock(db.session) and holds it through a DELETE whose predicate contains three correlated EXISTS subqueries plus a GROUP BY boundary subquery over purge_audit_log. Meanwhile write_ahead (audit.py:148) needs the same singleton row before every write-ahead audit insert.
If a batch runs long on a large table — exactly the deployment this feature targets — a concurrent scheduled purge blocks on that row and can hit the metadata DB's lock-wait timeout. write_ahead's except Exception swallows it and returns None, and superset/tasks/deletion_retention.py:244-250 then raises to skip the purge:
if record_id is None:
# Fail closed: the scheduled purge must not delete unauditably.So the failure mode is correct (fail closed, entity retried next run) rather than silent audit loss — good. But the outcome is that a heavy prune can cause purge skips, and the operator sees only a warning from write_ahead with no attribution to pruning. Two things would help: (a) 03:30 vs 00:00 gives 3.5h of separation but nothing enforces it, so please say in UPDATING.md that the two schedules should not be collapsed; (b) consider distinguishing PurgeAuditCoordinationError from a generic audit-write failure in that log line, so "your purge was skipped because pruning held the lock" is diagnosable. Is holding the lock across the DELETE actually required, given that candidate re-evaluation inside the statement already handles the concurrency case?
2. _evidence_candidates fails open on a NULL entity_uuid, where the other two categories fail closed
entity_uuid is Column(String(36), nullable=True) (superset/models/purge_audit_log.py), and entity_uuid() in purge_cascade.py:99-102 returns None for any model without a uuid attribute — so NULL-uuid confirmed / target_absent rows are reachable.
The other two categories handle that explicitly: _duplicate_candidates filters entity_uuid.is_not(None) (:405), and _operational_candidates marks NULL-uuid blocked rows unstable_block with the comment "No identity, so no streak to be proven redundant against" (:449-450). _evidence_candidates has neither. Its guard correlates on older.c.entity_uuid == table.c.entity_uuid (:497), which is NULL-comparison-false, so the EXISTS is always false and the row is unconditionally deletable once past the window.
This is not a survivor-invariant hole — a NULL-uuid row is excluded from _streak_boundary_subquery (:256) and so was never a boundary, meaning nothing gets promoted. But it is an inconsistency in the module's own stated fail-closed posture, applied to the most protected status class. Un-attributed destruction evidence expires while attributed evidence for a comparable entity is held back by its guard. Either add the same entity_uuid.is_not(None) fail-closed treatment, or document explicitly why evidence is the one category where a missing identity licenses deletion.
3. No dry-run mode on an irreversible deleter
SOFT_DELETE_PURGE_DRY_RUN exists at superset/config.py:1026 for the purge task, and the config comment for PURGE_AUDIT_PRUNING_ENABLED (:1029-1034) says the switch is opt-in "so operators can validate retention policy and workload characteristics before the first irreversible run". But the only two states are off and deleting for real — there is no way to actually validate that policy against production data first.
Given the category counts already flow through PruneRunResult and out to metrics, a PURGE_AUDIT_PRUNE_DRY_RUN that counts candidates per category and skips the DELETE would be a small addition, and it is the difference between an operator being able to answer "what would this remove from my audit log?" and having to find out by removing it. Strongly recommended for a feature in this blast radius.
4. _has_candidates leaves an open read transaction that run_prune never closes
_has_candidates (prune_audit.py:537-539) executes a SELECT on db.session with no commit or rollback, and run_prune (:642-654) returns straight out of the category loop. On the drained-early path the last _delete_batch commits and the session is clean, but on the budget-exhausted path the run ends with an open transaction that survives until the task tears the session down — an idle-in-transaction connection on the metadata DB after every carried-over run. A db.session.rollback() after the read, or once at the end of run_prune, closes it.
5. Recovery now rewrites the block's timestamp — intentional, but it should be documented
_recover_retention_blocked changed created_on=snapshot.created_on to created_on=utc_now() (audit.py:391-394). The comment explains why (it must not land in pruning's logical past), and I agree that's necessary. But the consequence is that on the crash-recovery path the audit row no longer records when the block actually occurred — it records when recovery noticed. For a table whose whole purpose is "blocked since", that is a real fidelity change and it is not mentioned in UPDATING.md. Worth a line there, or a recovered_from marker so the discontinuity is visible in the data rather than only in the source.
6. Batch constants are not operator-tunable
BATCH_SIZE = 500 and MAX_BATCHES_PER_RUN = 10 (:143, :146) cap a run at 5,000 rows. The retention windows are config; the throughput that has to keep up with them is not. A deployment generating more than 5,000 prunable rows/day never converges, and the only signal is the carried_over gauge staying at 1 forever with no lever to pull. The starvation-avoidance reservation logic in run_prune (:642-652) is careful and correct — I checked it can't overrun the budget — which makes it more of a shame that the budget itself is fixed. (Bito raised this too; I think it's right.)
7. create_index is non-concurrent — expected, but call it out for operators
a6c21e5b4d93 builds a four-column index on purge_audit_log via create_index (2026-08-24_15-50_a6c21e5b4d93_index_purge_audit_pruning.py:37-41), which wraps plain op.create_index — no CONCURRENTLY. On PostgreSQL that takes a lock blocking writes to purge_audit_log for the build. This matches the repo's shared-utils convention so I'm not asking you to change it, but this table is large by hypothesis (that's the premise of the PR), and UPDATING.md currently says nothing about migration duration. A one-line heads-up would be kind.
The index itself is well chosen — (status, entity_type, entity_uuid, created_on) matches the boundary GROUP BY, _repeats_an_earlier_block, _preceded_by_unresolved_attempt, and bounds_surviving_blocks. Both downgrade()s are clean and non-data-destructive with respect to purge_audit_log; c7f53d184ea2's drops only the coordination table, which is derived state.
Also good
UPDATING.md is unusually honest for a change like this — it names the beat entry, the default-off switch, the "never touched unless you opt in" evidence rule, and the CELERY_CONFIG-override footgun. The startup warnings in initialization/__init__.py are correctly gated on PURGE_AUDIT_PRUNING_ENABLED and are decoupled from SOFT_DELETE, which is right — audit rows outlive the flag. The non-boolean warning added there is a good catch.
Method / caveats
Everything above is INSPECTED, not RAN. This environment cannot import superset_core, so pytest fails at tests/conftest.py before collection; that is an environment gap on my side, not a defect in this PR. I confirmed statically that the new tests import prune_audit directly, so reverting the production module would fail them at import — but I did not execute them, and I am not claiming otherwise. All file:line citations were verified against a24a1313fe.
Nothing here is a blocker on the deletion logic — the predicate held up to everything I threw at it, which for +669 lines of audit-deleting SQL is genuinely impressive work. The one thing I would not merge without is a fresh approval at head.
github-actions
Bot
added
the
requires:rebase
…p ties when pruning Address the review threads on apache#43490: * A blockage streak's survivors are now the first row of each run of consecutive same-reason blocked rows — the streak's earliest row and the first row after every change of block reason — instead of the earliest row only. This is the audit writer's own suppression rule (finalize_retention_blocked) applied retroactively; previously a `report_schedule` block followed by a `cascade_integrity_failure` block lost the only durable evidence of the latter on the next prune. Run heads are exempt from age-out like the earliest row was; resolved-streak run heads still age out. The reason comparison is NULL-safe on every supported dialect so pre-feature rows form a run of their own that the first coded block ends, exactly as at write time. * Timestamp ties (legacy second-precision rows, two writers in one clock tick) are resolved on the preserving side, consistently: a pending row tied with a blocked row counts as preceding it (the block is deferred until the attempt resolves); a blocked row tied with a boundary sits on the boundary's resolved side (it ages out rather than seeding a new current streak) and the evidence guard now keeps the boundary until that tied row is gone; tied same-reason blocked rows are all retained. * Startup diagnostics warn when PURGE_AUDIT_PRUNING_ENABLED is not a boolean. The task already failed closed on such values, but the initializer collapsed them into "disabled" and emitted nothing, so a typo like "true" left the audit log growing without any warning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
a24a131 to
5acd469
Compare
|
Rebased onto latest What changed in the rebase:
@rusackas — heads-up that the existing approval predates both the earlier delta-review fix (the differing-reason timestamp-tie that could delete a reason-transition run-head, fixed with inclusive tie bounds + a control-run test) and this rebase, so GitHub is still showing the old green badge. Could you take a fresh look before merge rather than merging on the stale approval? Thanks. |
…eoff Addresses Amin Finding 1 on apache#43490: the pruning batch holds the coordination lock across its bulk DELETE, so a concurrent scheduled purge's write_ahead can block on it up to the metadata DB lock-wait timeout (then fails closed and retries next cycle). The lock scope is deliberate and load-bearing: it is the same singleton lock write_ahead takes before stamping created_on, so holding it across the DELETE guarantees any row committed after a batch is timestamped after that batch's cutoff and cannot materialize inside a streak it already pruned. Narrowing the lock to a timestamp-only prelude would reintroduce that race. The window is bounded instead (BATCH_SIZE=500 + the ix_purge_audit_log_pruning index), so no logic changes here -- only the missing documentation: - write_ahead: comment the lock-then-clock ordering Amin flagged as reading like incidental statement order. - _delete_batch: comment why the lock spans the DELETE plus the bounded liveness tradeoff. - UPDATING.md: spell out the operator-visible tradeoff (skipped-and-retried purge cycle, run pruning off-peak). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
sadpandajoe's over-deletion finding #3 on apache#43490 is real: a `force`-trigger blocked row following a same-reason `scheduled` block in the same streak was pruned as a duplicate, erasing the force-purge audit record. Confirmed by integration test against head e2dddd9 (the force row was deleted). `_repeats_an_earlier_block` now excludes force-trigger rows: the pruner mirrors the writer, whose `_suppress_redundant_block` only collapses consecutive scheduled same-reason blocks. Both consumers benefit — the duplicate category skips force rows, and the operational category marks them survivors while their streak is current. A force row may still be the *earlier* anchor a later scheduled repeat collapses into, so legitimate pruning is unaffected. Scope, stated precisely (this protects, it is not blanket immortality): a force block is exempt from duplicate collapse and from current-streak age-out. Once its streak resolves, a force block ages on the operational window like any resolved-streak blocked row — the confirmed/target_absent boundary is the durable evidence. A characterization test pins that boundary for the committer to sign off on. Findings #1 (pending tied to the later block at MySQL second granularity) and #2 (reason-transition block amid repeats) were verified COVERED by the existing `<=` unresolved-attempt tie and the reason-change discrimination; reverted-fix controls confirmed both guards are load-bearing. Added integration tests for all three scenarios, a control proving the force exemption still collapses a later scheduled repeat, and the resolved-streak scope-boundary test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
c9173e8 to
5706b96
Compare
github-actions
Bot
removed
the
requires:rebase
Code Review Agent Run #37eaeaActionable Suggestions - 0Additional Suggestions - 7
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
Bot
added
the
requires:rebase
…t log The purge audit log grew without bound: every scheduled run against the same blocked entity wrote another identical record (until apache#42863 deduped the write path), and nothing ever aged out operational noise. This adds the deletion-only pruning task defined by SC-116701: - Blocked duplicates within an entity's current blockage streak reduce to the streak's earliest record (blocked-since), regardless of age; the survivor is never deleted while the streak is current. Streak ordering rides apache#42863's microsecond precision and predecessor index — no schema change. - Operational records (failed, resolved-streak blocked) age out past PURGE_AUDIT_RETENTION_DAYS (default 90, validated fail-closed: invalid values skip the category, never widen removal). - Completed-destruction evidence (confirmed, target_absent) is never touched unless the separate PURGE_AUDIT_EVIDENCE_RETENTION_DAYS opt-in is explicitly set — the operator's compliance assertion. Pending rows and future-dated rows are never candidates. One shared budget per run (10 batches x 500 rows, priority order: duplicates, operational, evidence) bounds every run and converges backlogs across runs; conditional per-batch deletes counted from rowcounts make overlapping runs safe without a lock. The Celery task (deletion_retention.prune_purge_audit, default beat entry daily 03:30) reports per-category counts through deletion_retention.prune_audit.* metrics and one structured log line; a disabled run reports itself. The retention startup warning now also covers the new beat entry. Tests: 25 unit (classification, fail-closed validation, task reporting/isolation, clock parity with the audit writer) + 11 integration (age-independent dedup, bounded convergence with carryover, cross-entity isolation, resolved streaks, evidence protection by count-and-identity, opt-in expiry, pending/future immunity, rerun idempotence). Spec chain in specs/sc-116701-purge-audit-pruning/. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A nine-lens review panel found one HIGH and several MEDIUM defects in the pruning policy. All of them centred on the same thing: the "blocked since" survivor was protected at candidate-selection time but not at deletion time, so anything that moved a streak boundary could destroy the one row the feature exists to keep. - A streak boundary is no longer removed while it still bounds something. Evidence expiry refuses to delete a confirmed/target_absent row while a blocked row older than it survives for the same entity. Without this a boundary could recede (deleted by an overlapping run, or by an evidence window shorter than the operational one) and promote already-selected duplicates into a current streak, whose new survivor the in-flight run would then delete. Because boundaries can now only move forward, a selected duplicate can never become a survivor before it is deleted -- which is what makes the invariant hold across overlapping runs and concurrent reconcile_pending without a distributed lock. - A failed attempt no longer ends a blockage streak. A failed purge is an infrastructure outcome, not evidence the blockage cleared; treating it as a boundary let one transient error demote the blocked-since survivor and restate the blockage as beginning after the failure. - Blocked rows with no entity_uuid belong to no streak and so can never be proven redundant; they are kept rather than aged out (fail closed). - Future-dated rows are excluded from streak classification, not just from candidacy, so a skewed writer clock cannot resolve a live streak. - Config resolution returns a ResolvedWindow that distinguishes "off" from "misconfigured", instead of a bare None the caller had to re-read config to interpret. PURGE_AUDIT_RETENTION_DAYS is renamed PURGE_AUDIT_OPERATIONAL_RETENTION_DAYS -- it never governed evidence, and the name is free to fix before release. - The metric leaf matches the task name, carried_over is a metric (backlog non-convergence was previously log-only), and the failure path rolls the session back. - Tidying: statuses partition an ALL_STATUSES set exported by the model so an uncategorised new status fails a test; one shared utc_now with the audit writer; table-driven category drain; precise types in place of Any. Tests: +8 (boundary guard including the inverted-window case, failed streak-transparency, uuid-less block retention, skew-resistant classification, status-change-between-select-and-delete, exhaustiveness contract). Unit 102, integration 89, pre-commit and mypy clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eview found The previous commit claimed the survivor invariant held by construction because streak boundaries could only move forward. That proof was wrong: moving a boundary forward demotes the current survivor, which *promotes* the next blocked row into the survivor slot — and that row may already be in the delete batch. A re-review reached the original harm through it. The reachable interleaving needs no overlapping prune runs. An entity is blocked, a later purge attempt writes a pending row and stalls, a third evaluation records another block. Pruning classifies the newest block as a duplicate (pending is not a boundary), and before the delete lands the stalled attempt is finalized in place — by the worker or by reconcile_pending — keeping its original timestamp. The boundary appears mid-history, the newest block becomes the new streak's survivor, and the conditional delete still matches it because its status never changed. The asymmetry is that a pending row is the only thing that can insert a boundary into history: every other write lands at `now`, newer than everything. So blocked rows preceded by an unresolved attempt are no longer eligible for deduplication until it resolves — their classification is not stable. Pending rows are transient, so the deferred rows are collected by a later run. The evidence boundary guard counts older pending rows too, since the purge path can finalize one to blocked. Also from the same pass: - Each category is guaranteed at least one batch of the shared budget. Strict priority let a permanent duplicate backlog starve the age-based categories forever, reintroducing the unbounded growth this feature exists to stop. - carried_over no longer reports a backlog when the budget ran out but nothing remained; it is the only alarm for non-convergence, so a false positive blunts it. - The module docstring's safety argument is corrected — it was the written justification for shipping without a lock, so leaving it wrong would have kept the hole open. - The ascending-order comment no longer claims a guarantee that the boundary guard actually provides. Tests: +3, including a red-first control (removing the guard fails test_a_block_after_an_unresolved_attempt_is_not_treated_as_a_duplicate and nothing else). Unit 102, integration 92, pre-commit and mypy clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…expiry A third adversarial pass found the previous fix incomplete. The guard that defers blocked rows preceded by an unresolved attempt was applied only to the duplicate category, so the operational category deleted the very rows the duplicate category had just deferred -- in the same run, with no concurrency involved at all. Reproduced deterministically: an entity blocked at 100d, a purge attempt that stalls at 98d, another block recorded at 95d. Deduplication defers the 95d row (correct), age-based expiry then removes it because it is past the 90-day window and is not the current survivor. When the stalled attempt finalizes in place, the boundary appears between the two blocks, and the row that should have become the new streak's survivor is already gone. The remaining history then asserts the object was destroyed at 98d with no trace it was still blocked at 95d -- an affirmatively false record, not merely a lost one. Age never made that classification stable: an old blocked row inside a live streak is exactly the "blocked for years" case FR-009 exists to protect. Both categories now share the guard. Also caps the per-category allowance at the remaining budget, so the anti-starvation floor cannot overspend a budget set below the category count. The module docstring no longer claims every write lands at `now`. It does not: `created_on` is stamped by the writing process, not the database, and _recover_retention_blocked re-inserts a row at its original timestamp. The guards are select-time, so a row becoming visible with a backdated timestamp between this module's SELECT and DELETE would evade them. That residual race is now documented precisely, with the three ways to close it properly (candidacy inside the DELETE, serializing against audit writers, or a database-assigned created_on), rather than papered over -- the previous docstring's false safety proof is what let this hole persist through two remediations. Tests: +1 with a red-first control (removing the operational guard fails test_age_does_not_make_an_unstable_block_expirable and nothing else). Unit 102, integration 93, pre-commit and mypy clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d reason column apache#43485 (purge block reason codes) merged to master as 88d2c29, adding 1072de5ed955 -> 39097d124752. This branch's migrations still pointed at 1072de5ed955, so both chains forked from the same parent and alembic saw two heads -- failing enforce-single-migration-head and, because `db upgrade` then refuses to run, every database-backed CI suite with it. Re-points a6c21e5b4d93 (the pruning index) at 39097d124752 so the chain is linear: 1072de5ed955 -> 39097d124752 -> a6c21e5b4d93 -> c7f53d184ea2 No migration content changes -- only the parent pointer and its matching docstring header. The two PRs were always independent (this one adds an index and a coordination table; that one adds a column), so ordering is the only thing that had to be resolved. Verified: `superset db heads` reports a single head, a full `db upgrade` from an empty database applies all three in order (exit 0), and downgrade back to 39097d124752 followed by a re-upgrade round-trips cleanly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…p ties when pruning Address the review threads on apache#43490: * A blockage streak's survivors are now the first row of each run of consecutive same-reason blocked rows — the streak's earliest row and the first row after every change of block reason — instead of the earliest row only. This is the audit writer's own suppression rule (finalize_retention_blocked) applied retroactively; previously a `report_schedule` block followed by a `cascade_integrity_failure` block lost the only durable evidence of the latter on the next prune. Run heads are exempt from age-out like the earliest row was; resolved-streak run heads still age out. The reason comparison is NULL-safe on every supported dialect so pre-feature rows form a run of their own that the first coded block ends, exactly as at write time. * Timestamp ties (legacy second-precision rows, two writers in one clock tick) are resolved on the preserving side, consistently: a pending row tied with a blocked row counts as preceding it (the block is deferred until the attempt resolves); a blocked row tied with a boundary sits on the boundary's resolved side (it ages out rather than seeding a new current streak) and the evidence guard now keeps the boundary until that tied row is gone; tied same-reason blocked rows are all retained. * Startup diagnostics warn when PURGE_AUDIT_PRUNING_ENABLED is not a boolean. The task already failed closed on such values, but the initializer collapsed them into "disabled" and emitted nothing, so a typo like "true" left the audit log growing without any warning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
Master advanced 39097d124752 with 8f31c5d726ab (index_dataset_dependency_lookups). Re-point a6c21e5b4d93 onto 8f31c5d726ab so the pruning migrations form a single alembic head (c7f53d184ea2) instead of branching a second head off 39097d124752. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
Replace 'is now a resolved-streak row' with 'has become a resolved-streak row' to satisfy the timeless-comment rule (CLAUDE.md) and the pre-push tripwire. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
…eoff Addresses Amin Finding 1 on apache#43490: the pruning batch holds the coordination lock across its bulk DELETE, so a concurrent scheduled purge's write_ahead can block on it up to the metadata DB lock-wait timeout (then fails closed and retries next cycle). The lock scope is deliberate and load-bearing: it is the same singleton lock write_ahead takes before stamping created_on, so holding it across the DELETE guarantees any row committed after a batch is timestamped after that batch's cutoff and cannot materialize inside a streak it already pruned. Narrowing the lock to a timestamp-only prelude would reintroduce that race. The window is bounded instead (BATCH_SIZE=500 + the ix_purge_audit_log_pruning index), so no logic changes here -- only the missing documentation: - write_ahead: comment the lock-then-clock ordering Amin flagged as reading like incidental statement order. - _delete_batch: comment why the lock spans the DELETE plus the bounded liveness tradeoff. - UPDATING.md: spell out the operator-visible tradeoff (skipped-and-retried purge cycle, run pruning off-peak). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
sadpandajoe's over-deletion finding #3 on apache#43490 is real: a `force`-trigger blocked row following a same-reason `scheduled` block in the same streak was pruned as a duplicate, erasing the force-purge audit record. Confirmed by integration test against head e2dddd9 (the force row was deleted). `_repeats_an_earlier_block` now excludes force-trigger rows: the pruner mirrors the writer, whose `_suppress_redundant_block` only collapses consecutive scheduled same-reason blocks. Both consumers benefit — the duplicate category skips force rows, and the operational category marks them survivors while their streak is current. A force row may still be the *earlier* anchor a later scheduled repeat collapses into, so legitimate pruning is unaffected. Scope, stated precisely (this protects, it is not blanket immortality): a force block is exempt from duplicate collapse and from current-streak age-out. Once its streak resolves, a force block ages on the operational window like any resolved-streak blocked row — the confirmed/target_absent boundary is the durable evidence. A characterization test pins that boundary for the committer to sign off on. Findings #1 (pending tied to the later block at MySQL second granularity) and #2 (reason-transition block amid repeats) were verified COVERED by the existing `<=` unresolved-attempt tie and the reason-change discrimination; reverted-fix controls confirmed both guards are load-bearing. Added integration tests for all three scenarios, a control proving the force exemption still collapses a later scheduled repeat, and the resolved-streak scope-boundary test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
A sibling migration (7e2c9a4f1b83 create_task_dependencies_table) merged onto the shared parent b3e9c1a75d24, so pointing the pruning chain at b3e9c1a75d24 produced two alembic heads. Re-point the first pruning migration (a6c21e5b4d93) onto the current master tip 7e2c9a4f1b83 so the chain is single-headed again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
The operator guide still stated the purge_audit_log is "never pruned by design" and must be pruned manually, directly contradicting the automatic pruning this PR introduces (sadpandajoe review, 2026-09-05). Replace that sentence with an accurate description of the new policy: automatic pruning via the deletion_retention.prune_purge_audit beat task, the "blocked since" record and completed-destruction evidence preserved, only operational noise aged out past PURGE_AUDIT_RETENTION_DAYS, and PURGE_AUDIT_PRUNING_ENABLED = False to restore the previous never-pruned behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
…l window Per Evan's review call on the pruning survivor exemption: go with full immortality for operator force-purge blocks. The force exemption already kept a force block out of duplicate collapse and marked it a current-streak survivor, but a resolved-streak force block still aged out on the operational window. Extend the exemption so a force block is never pruned by either category — it is retained permanently, not just while its streak is current. `_operational_candidates` now excludes `status == BLOCKED AND trigger == FORCE` (failed rows, force-triggered or not, age normally). Updated the `_repeats_an_earlier_block` / `_operational_candidates` docstrings that previously said a resolved-streak force block ages out. Inverted the characterization test `test_resolved_streak_force_block_ages_on_operational_window` into `test_force_block_survives_operational_window_permanently`, pinning that a 100-day-old resolved-streak force block survives a 90-day operational prune. A longer-but-not-forever cleanup pass for these is a possible follow-up, deliberately out of scope (Evan's note). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
5706b96 to
bcfa505
Compare
github-actions
Bot
removed
the
requires:rebase
|
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
| .where(_repeats_an_earlier_block(table, boundary)) | ||
| .where(sa.not_(_preceded_by_unresolved_attempt(table))) | ||
| .order_by(table.c.created_on) | ||
| .limit(limit) |
There was a problem hiding this comment.
This limits deletions to 500, but it does not bound the work done while holding the singleton audit lock: once no duplicates remain, the age-unbounded candidate query and its boundary/EXISTS checks must examine all matching audit rows before returning zero. On the large tables this feature targets, that can hold the lock long enough for concurrent write_ahead() calls to time out and skip purge cycles; could we bound candidate discovery or provide representative query-plan/runtime evidence instead of treating BATCH_SIZE as a lock-time bound?
There was a problem hiding this comment.
Agreed — the 500 cap bounds deletions, not discovery, so the terminal (and, when duplicates are sparse, the first) batch scans O(blocked rows) under the coordination lock. Proposing the fix before I push it, since it changes the lock model you flagged:
Split discovery from the locked mutation. Run the candidate SELECT (LIMIT 500) UNLOCKED to get ≤500 candidate ids — a hint, no lock held for the scan. Then take the coordination lock and issue a DELETE scoped to those ids that re-applies the candidacy predicates as correlated WHERE clauses on the id-filtered target (not a re-embedded candidacy sub-SELECT): DELETE … WHERE id IN (:ids) AND status='blocked' AND <in_current_streak> AND <repeats_an_earlier_block> AND NOT <preceded_by_unresolved_attempt>. The planner evaluates those correlated, index-backed predicates for only the ≤500 rows, so the locked work is bounded to ~500 PK probes plus their per-entity index lookups.
This keeps the backdated-write invariant intact: the LOCKED delete re-verifies the full predicate over current committed state, so a row a concurrent write_ahead turned into a survivor between the unlocked discovery and the lock fails the re-check and is not deleted (a stale hint only ever deletes fewer rows, never wrong ones); a write_ahead arriving after the lock waits and stamps post-cutoff. The rowcount-keyed drain loop converges the remainder next run, as today.
I'll attach an EXPLAIN of the locked delete showing it's ≤500-bounded (index probes, no seq scan / full per-entity aggregate) — that doubles as the runtime evidence you asked for. Does this shape address your concern? I'll push the diff + EXPLAIN for review rather than merging first.
There was a problem hiding this comment.
Good catch — you're right that BATCH_SIZE bounded deletions but not the discovery scan, which ran inside the DELETE under the coordination lock. Fixed in ad34aee by splitting discovery from the locked mutation:
- Discovery runs unlocked. The ≤
BATCH_SIZEcandidate ids are selected before the lock is taken, so the age-unbounded scan and its boundary/EXISTSchecks — including the sparse/no-duplicate steady state you describe — no longer happen while holding the singleton lock. - Only a bounded re-check + delete run under the lock. Under the lock the batch is re-checked with a SELECT that re-applies the identical candidacy predicates over just those ≤
BATCH_SIZEids, then the survivors are deleted by literal id. Both locked statements are ≤BATCH_SIZE-scoped → per-PK and per-entity index probes, so lock-hold no longer scales with table size. - The boundary query is now bounded too. The streak boundary was rewritten from a global
GROUP BY entityaggregate to a correlated per-entityMAX(created_on)scalar, shared by discovery and the re-check through one predicate list so the two cannot drift. - The re-check is a SELECT rather than a self-referencing DELETE subquery because MySQL rejects the latter (ERROR 1093); the discovery transaction is ended before the lock is taken, so the re-check reads current committed state regardless of isolation level (Superset pins MySQL to READ COMMITTED, so shipped config was already safe — this is isolation-independence hardening).
Verification: the existing behavioral run_prune integration tests pass unchanged (34/34 on Postgres), plus a new regression test for the discovery→lock re-check that forces REPEATABLE READ and was controlled red-first (reverting the pre-lock transaction reset makes it fail). Would appreciate your ack on the approach.
| This also resolves the limitation noted under *Soft delete and restore for datasets*: a database blocked by soft-deleted datasets can now be freed by purging those datasets (per-entity endpoint, retention task, or `force-purge` CLI) instead of hard-deleting `tables` rows out-of-band. | ||
|
|
||
| The `purge_audit_log` table is **never pruned by design** — the audit must survive the entities it names; operators who need to age it out should prune manually. | ||
| The `purge_audit_log` table is pruned automatically by the `deletion_retention.prune_purge_audit` Celery beat task (daily, 03:30), so it no longer grows unbounded and does not need manual pruning. The policy is written to preserve the audit's meaning rather than trade it away: within an entity's current blockage streak the earliest — "blocked since" — record always survives (only redundant duplicate `blocked` records are collapsed), and completed-destruction evidence (`confirmed`, `target_absent`) is **never** removed unless the separate `PURGE_AUDIT_EVIDENCE_RETENTION_DAYS` opt-in is explicitly set. What ages out is operational noise — `blocked` records from already-resolved streaks and `failed` records — once older than `PURGE_AUDIT_RETENTION_DAYS` (default 90). Set `PURGE_AUDIT_PRUNING_ENABLED = False` to restore the previous never-pruned, unbounded-growth behavior. See the release-note entry above for the beat-schedule and `CELERY_CONFIG` details. |
There was a problem hiding this comment.
This can leave operators with pruning disabled while believing the table is bounded: the shipped default is False, and PURGE_AUDIT_RETENTION_DAYS is not read anywhere (the key is PURGE_AUDIT_OPERATIONAL_RETENTION_DAYS). Could this state that pruning must be enabled explicitly and use the actual key?
There was a problem hiding this comment.
Fixed in f933a93. You are right on both counts: PURGE_AUDIT_RETENTION_DAYS is read nowhere (the key the code reads is PURGE_AUDIT_OPERATIONAL_RETENTION_DAYS, prune_audit.py:149 / config.py:1049), and pruning ships off (PURGE_AUDIT_PRUNING_ENABLED = False, config.py:1040). Rewrote this UPDATING.md entry to say pruning is off by default and must be enabled explicitly with PURGE_AUDIT_PRUNING_ENABLED = True, and corrected the retention key — so it no longer reads as bounded-by-default. (The release-note entry higher in the file was already correct; this detailed paragraph was the one that drifted.)
…t in UPDATING.md sadpandajoe review on apache#43490: the detailed UPDATING.md entry cited a config key that is never read (`PURGE_AUDIT_RETENTION_DAYS`) and framed pruning as on by default ("set `PURGE_AUDIT_PRUNING_ENABLED = False` to restore ... unbounded growth"). Both are wrong and could leave an operator believing the table is bounded while it grows forever. Correct it to the key the code actually reads (`PURGE_AUDIT_OPERATIONAL_RETENTION_DAYS`, prune_audit.py:149 / config.py:1049) and state that pruning is off by default (`PURGE_AUDIT_PRUNING_ENABLED = False`, config.py:1040) and must be enabled explicitly. Matches the release-note entry higher in the file, which was already correct. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
…ked discovery + locked re-check sadpandajoe review on apache#43490: the BATCH_SIZE cap bounds deletions but not candidate discovery. Each `_delete_batch` embedded the full candidacy query (boundary/repeat/pending correlated subqueries over a global GROUP-BY streak boundary) inside the DELETE under the singleton audit coordination lock. When duplicates are sparse/absent — the steady state — that scan is O(blocked rows) and holds the lock long enough that concurrent `write_ahead()` calls time out and skip purge cycles, on the large tables this targets. Split discovery from the locked mutation: - Discover <=BATCH_SIZE candidate ids with an UNLOCKED select — a hint. - Under the lock, RE-CHECK the ids with a SELECT re-applying the same candidacy predicates, then DELETE the survivors by literal id. The re-check is a SELECT, not a correlated WHERE on the DELETE, because MySQL rejects a DELETE whose subquery reads the target table (ERROR 1093). - End the discovery transaction (rollback; discovery is read-only) BEFORE taking the lock, so the re-check opens a FRESH snapshot. This makes the locked re-check read current committed state regardless of isolation level: under REPEATABLE READ, discovery's SELECT would otherwise fix the transaction's consistent-read snapshot and the lock's UPDATE would not refresh it, so a re-check on that snapshot could miss a row a concurrent `write_ahead` committed in the discovery->lock window. This is isolation-independence hardening, not a shipped-config data-loss fix: Superset pins MySQL to READ COMMITTED at runtime (`set_db_default_isolation`) and in CI, where every statement re-reads latest-committed and the stale read cannot occur. Keeping the re-check correct under any isolation is cheap and right; the cross-model pass flagged the raw-SQL-under-RR property, and the reset closes it. To make the locked re-check bounded, the streak boundary is rewritten from a global `GROUP BY entity` aggregate to a correlated per-entity `MAX(created_on)` scalar (same key, `entity_uuid IS NOT NULL` and `created_on <= now` filters, and strict-`>` tie handling), used by BOTH discovery and re-check via one shared predicate list per category so the two cannot drift. Both locked statements are <=BATCH_SIZE-scoped -> per-PK + per-entity index probes, so lock-hold stays short. `_delete_batch` now returns `(discovered, removed)` and `_drain` keys "drained" on the discovery count, not the delete rowcount, so an overlapping run that thinned the hints cannot be misread as a fully-drained backlog. Tests: behavioral `run_prune` integration tests unchanged (parity) — all 34 in the file pass on Postgres. Added `test_recheck_reads_current_state_under_repeatable_read`: it FORCES REPEATABLE READ on the session (so the guard has teeth even though the app runs RC), injects a pending row from a separate connection in the discovery->lock window, and asserts the later block is spared. Verified red-first on local Postgres RR — reverting the pre-lock rollback makes it fail (later block wrongly deleted); it runs in the Postgres lane too, so it does not depend on CI MySQL isolation. Skipped on SQLite (no snapshot isolation). Unit tests adapted to the new internals (the MySQL-safety test asserts the DELETE has no target-reading subquery). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
Code Review Agent Run #0ad479Actionable Suggestions - 0Additional Suggestions - 5
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
4 participants
SUMMARY
The
purge_audit_logtable grew without bound. Every scheduled purge run that hit the same blocked entity wrote another identical record (until #42863 deduped the write path), and nothing ever aged out operational noise. This adds the retention policy defined by SC-116701 and the scheduled task that applies it. The pruning logic is deletion-only, but the PR carries two supporting, reversible migrations: a covering index onpurge_audit_logfor the prune queries (a6c21e5b4d93) and a small coordination table seeded on upgrade (c7f53d184ea2). Both have upgrade+downgrade unit tests.Three categories, drained in priority order under one shared per-run budget:
failedrows andblockedrows from resolved streaks age out pastPURGE_AUDIT_OPERATIONAL_RETENTION_DAYS(default 90).confirmed/target_absent— the only surviving trace of a destroyed object — are never touched unlessPURGE_AUDIT_EVIDENCE_RETENTION_DAYSis explicitly set. Setting it is the operator's assertion that an approved compliance policy permits expiring destruction evidence.pendingrows belong toreconcile_pending()and are structurally unreachable. Invalid retention values fail closed: the run logs a warning and skips that category rather than widening removal.PURGE_AUDIT_PRUNING_ENABLED = Falserestores the previous behaviour exactly, and a disabled run says so rather than silently doing nothing.Design notes worth a reviewer's attention:
failedattempt does not end a blockage streak. A failed purge is an infrastructure outcome — the cascade raised — not evidence the block cleared; the blocking policy is untouched. Treating it as a boundary would let one transient error demote the blocked-since survivor and restate the blockage as beginning after the failure.pendingrow resolves it in place, keeping its original timestamp, so an unresolved attempt is a boundary that can appear mid-history and turn the row after it into a survivor. Evidence expiry likewise refuses to delete a row that still bounds surviving blocked rows, which is what stops a boundary receding — and makes an evidence window shorter than the operational one safe rather than corrupting.BATCH_SIZEcandidate ids with an unlocked SELECT, then — under the same singleton coordination lock the audit writer takes before stampingcreated_on— re-checks those ids with a SELECT that re-applies the identical candidacy predicates over current committed state, and deletes only the survivors by literal id. Re-verifying candidacy under the lock is what closes the race: a row that becomes visible with a pastcreated_onbetween discovery and the lock fails the re-check and is spared, and the lock serializes pruning against concurrent writers. The re-check is a SELECT rather than a self-referencing DELETE subquery because MySQL rejects the latter (ERROR 1093). The discovery transaction is ended before the lock is taken, so the re-check reads a fresh snapshot and is correct regardless of isolation level — Superset pins MySQL toREAD COMMITTED, so shipped config was already safe; this is isolation-independence hardening. (created_onremains writer-stamped, not database-assigned; the lock plus the locked re-check are what close the race, so nocreated_onmigration is needed.)CELERY_CONFIGis told pruning is not running.This PR was reviewed by a nine-lens panel plus three adversarial data-systems passes before opening; the survivor-invariant findings from those passes are fixed here, each with a red-first control run.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI. Operator-visible surface is configuration, metrics (
deletion_retention.prune_purge_audit.*— per-category removal counts pluscarried_over), and one structured completion log line per run.TESTING INSTRUCTIONS
26 unit tests (classification and the exhaustiveness contract against the model's
ALL_STATUSES, fail-closed config validation, task reporting/isolation, clock parity with the audit writer) and 20 integration tests against the real table: age-independent dedup, bounded convergence with carryover, cross-entity isolation, resolved streaks,failedstreak-transparency, evidence protection by count-and-identity, opt-in expiry and its disable-again edge, pending/future-row immunity, skew-resistant classification, unstable-block deferral in both categories, boundary-guard deferral including the inverted-window case, uuid-less block retention, starvation resistance, rerun idempotence, and the status re-check.Manual: enable the beat entry, seed duplicate
blockedrows for one entity, rundeletion_retention.prune_purge_audit, and confirm the earliest row survives and the counts appear in logs/metrics. Full recipe inspecs/sc-116701-purge-audit-pruning/quickstart.md.ADDITIONAL INFORMATION
SOFT_DELETEbecause audit rows outlive the flag. The beat-entry startup warning is gated on it.a6c21e5b4d93) plus a coordination table (c7f53d184ea2)tests/unit_tests/migrations/test_purge_audit_coordination.pyexercises upgrade + downgrade for bothBehaviour change on upgrade (recorded in
UPDATING.md): operational audit records older than 90 days begin pruning automatically. Completed-destruction evidence is untouched. Opt out withPURGE_AUDIT_PRUNING_ENABLED = False; deployments overridingCELERY_CONFIGmust carry the new beat entry forward.This PR was developed with AI assistance (Claude Code), including the implementation, tests, and review remediation; a human (@mikebridge) reviews before merge.