feat(deletion-retention): persist purge block reason codes on the audit log - #43485
Conversation
…urge audit log Blocked purge audit records gain a stable machine-readable reason code (report_schedule, user_attribute, unhandled_reference) so the audit table alone answers why an entity was not purged. The suppression predicate now keys on status AND reason: same-reason nights stay suppressed, a reason change writes exactly one new blocked row, and the predecessor is the latest same-entity row overall so clock skew can never suppress against a stale row. NULL-safe both ways: pre-feature reason-less predecessors never match, and a missing current code never suppresses (warned, fail-open). Failed, confirmed, and reconciled rows never carry a reason. One additive nullable migration; no backfill. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code Review Agent Run #6ac23bActionable Suggestions - 0Filtered 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 |
✅ 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 #43485 +/- ##
==========================================
+ Coverage 78.85% 78.88% +0.02%
==========================================
Files 2876 2876
Lines 164601 165059 +458
Branches 38015 38095 +80
==========================================
+ Hits 129799 130203 +404
- Misses 32355 32394 +39
- Partials 2447 2462 +15
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:
|
…itly An eight-lens review panel converged on the reason code being derived from the blocker's physical table name while the module comment promised a table rename must not change a persisted code. The blocker map is now keyed by related table with a BlockerReason(code, phrase) value, so the code is declared and a rename touches only the lookup key. A new test pins that decoupling. Also from the panel: finalize() takes reason as an explicit keyword and persists it only for blocked outcomes (the documented invariant is now structural, with a test); a missing reason code is counted as blocked_audit_missing_reason instead of logging a second warning for one event; the migration and UPDATING.md document the migrate-before-deploy ordering, its fail-closed symptom, the rolling-deploy window, and the downgrade's data loss; reconcile_pending documents the blocked-attempt demotion; PEP 257 and deferred-import justification comments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mikebridge
changed the title
Code Review Agent Run #8f8a14Actionable Suggestions - 0Filtered 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 |
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Approve — well-scoped, correctly fail-safe, and the risky parts hold up. I traced the concurrency-sensitive suppression predicate, the cross-dialect NULL-safety, the BlockerReason refactor threading, and the migration; an un-migrated table was confirmed to make the scheduled purge fail closed. No blocking bug. Two non-blocking Mediums worth addressing:
🟡 Medium — blocked_audit_missing_reason metric is dead code that defeats its own safety net
In _finalize_blocked (superset/tasks/deletion_retention.py:209-219) the if result.blocker is None: branch that increments blocked_audit_missing_reason is unreachable: its only caller gates on elif result.blocked_reason is not None: (:305), and CascadeResult.blocked_reason is None iff blocker is None (property in purge_cascade.py). So the metric can never fire, and the inline comment ("one gap produces one alertable signal") describes an impossible state — anyone told to watch this signal will always see zero. A genuinely blocker-less "blocked" result would instead fall to else: audit.fail(record_id) and be recorded FAILED, not BLOCKED. No test references the metric. Fix: remove the dead branch (and its comment), or relocate the emptiness check to where a blocked-but-blocker-less result is actually representable.
🟡 Medium — Oscillating block reason grows the audit table without bound (undisclosed)
_suppress_redundant_block (superset/commands/deletion_retention/audit.py:285-322) suppresses only when the single latest same-entity row carries the same reason. If an entity's reason alternates each run (e.g. external automation that adds/removes a report_schedule reference on alternating nights, flipping between report_schedule and cascade_integrity_failure), no row ever matches its predecessor and one permanent row accumulates per run indefinitely. Pre-PR (status-only dedupe) that entity stayed at one row, so this is an undisclosed growth regression, and purge_audit_log has no TTL of its own. Rows are tiny and content-free and the churn pattern is narrow, but the growth is genuinely unbounded. Fix: disclose this edge case in UPDATING.md and consider an audit-table retention job.
|
Bito Automatic Review Skipped – PR Already Merged |
…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>
…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>
…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>
3 participants
SUMMARY
Blocked purge audit records currently record only a status: an auditor reading
purge_audit_logcannot distinguish "blocked because an alert/report references the entity" from "blocked because a user has the dashboard as their welcome page" from "hit an unexpected database integrity constraint during the cascade" — outcomes with entirely different remediations. The reason is already known at decision time (logs, CLI output) but is discarded before it reaches the durable record.This PR threads a stable machine-readable reason code from the purge decision points into the audit record, and amends the #42863 growth-suppression predicate to key on status and reason:
REASON_REPORT_SCHEDULE("report_schedule"),REASON_USER_ATTRIBUTE("user_attribute"), andREASON_CASCADE_INTEGRITY_FAILURE("cascade_integrity_failure") declared inpurge_policy.py(the vocabulary's owner; deliberately model-free). The values are frozen identifiers pinned by a golden-set test — a future physical table rename must not re-mint persisted history.BlockerReasonkeeps each code and operator phrase together throughDependencyPolicy,PurgeBlockedError, andCascadeResult; derived read-only compatibility properties preserve existing call sites; the first declared blocker that matches wins (declaration order is part of the contract, unit- and integration-tested).CascadeResult.blockeris set by both cascade handlers (policy block from the exception;IntegrityError→cascade_integrity_failure), withblocked_reasonandblocked_reason_codederived from that value;audit.block()andaudit.finalize_retention_blocked()take the reason as a required parameter, captured in the recovery snapshot from the finalization argument and carried through both crash-recovery branches (never read from the reason-less pending row).blockedstatus (so growth suppression continues to apply) —status = blocked AND reason = cascade_integrity_failuredenotes a cascade-coverage defect, not an intended policy outcome. Run-level metrics conflate this class with reference blocks, making the persisted code the sole durable classifier.created_ontimestamp causes fail-safe retention instead of suppression against an older row;created_onorders this predicate but does not establish causal order across workers. NULL-safe both ways: a reason-less (pre-upgrade) predecessor never matches, and a missing current code never suppresses (WARNING, fail-open). Timestamp-tie ambiguity widens to "differs in status or reason" viais_distinct_from(NULL-safe on PostgreSQL/MySQL/SQLite).reconcile_pendinglater finalizes failed/NULL) is documented and deliberately not "fixed" by fabricating a code — the next night re-anchors with the real one.String(64)column via the shared migration utils. No backfill, no index, no reader changes; historical rows keep NULL and every reader tolerates absence. The fix(deletion-retention): dedupe repeated blocked audits #42863 suppression paragraph inUPDATING.mdis amended to state the new predicate.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Backend-only. Before/after of the audit table for the same three blocked outcomes:
TESTING INSTRUCTIONS
Manual: soft-delete a chart referenced by an active report, run the retention purge (or
superset deletion-retention force-purge), thenSELECT status, reason FROM purge_audit_log— the blocked row carriesreport_schedule.The changed-reason suppression test was control-run: it fails against the pre-amendment status-only predicate and passes with this PR.
ADDITIONAL INFORMATION
Runtime: metadata-only DDL on PostgreSQL/SQLite; instant DDL on MySQL 8.0.12+ (trailing nullable ADD COLUMN); table rebuild only on older MySQL —
purge_audit_logis small and growth-suppressed by design. No downtime expected. Single alembic head verified (1072de5ed955) before authoring.Internal tracking: sc-115342. Follows #42863 (audit dedupe) and #42888 (declarative purge policies).
This PR was generated by Claude (AI) on behalf of @mikebridge.
🤖 Generated with Claude Code