fix(deletion-retention): dedupe repeated blocked audits - #42863
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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 #8e2f54
Actionable Suggestions - 2
-
superset/tasks/deletion_retention.py - 1
- Missing retained disposition metric · Line 283-293
-
superset/migrations/versions/2026-08-06_18-00_b8d2f4a6c901_index_purge_audit_predecessor.py - 1
- Migration lineage break · Line 29-29
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/integration_tests/deletion_retention/audit_tests.py - 1
- Deprecated datetime.utcnow() in tests · Line 213-213
Review Details
-
Files reviewed - 7 · Commit Range:
9ef7504..9ef7504- superset/commands/deletion_retention/audit.py
- superset/migrations/versions/2026-08-06_18-00_b8d2f4a6c901_index_purge_audit_predecessor.py
- superset/models/purge_audit_log.py
- superset/tasks/deletion_retention.py
- tests/integration_tests/deletion_retention/audit_tests.py
- tests/integration_tests/deletion_retention/force_purge_tests.py
- tests/integration_tests/deletion_retention/purge_tests.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 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
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here:
Blocking: _retention_predecessor() selects one row ordered only by created_on, which is not a unique sequence. Multiple prior scheduled outcomes can share the newest timestamp; if a tied blocked row is selected while another tied row is failed or confirmed, the current audit is suppressed even though the preceding outcome is ambiguous.
Could we retain the current row whenever the maximum predecessor timestamp is tied—or introduce a durable ordering key—and cover tied blocked plus non-blocked predecessors with a regression test?
Reviewed at 6f6c61e. The current provider status exposes only documentation-preview checks, not the advertised backend test and migration validation.
Code Review Agent Run #b3e811Actionable Suggestions - 0Review 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 |
|
Post-approval note for the record: head (Prepared with AI assistance (Claude), on behalf of and reviewed by @mikebridge.) |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42863 +/- ##
==========================================
+ Coverage 66.37% 66.38% +0.01%
==========================================
Files 2857 2857
Lines 161048 161125 +77
Branches 37046 37060 +14
==========================================
+ Hits 106892 106960 +68
- Misses 52141 52146 +5
- Partials 2015 2019 +4
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:
|
Code Review Agent Run #c17b0aActionable Suggestions - 0Review 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 |
…hed during the 08-06 GitHub Actions incident)
…merge b8d2f4a6c901 chained off c4a1b8e2d739, which was one of the two divergent heads that apache#42878 merged into d7cecc48bd55. Chaining off the pre-merge revision would have re-forked the graph on merge; the index migration now sits on the repaired single head. Content unchanged.
87aeea0 to
7bb8046
Compare
|
Addressed the launch-review migration findings in c2e8c38:\n\n- downgrade now removes only the predecessor index and retains backward-compatible DATETIME(6), avoiding irreversible audit-timestamp truncation\n- runtime model emits DATETIME(6) for both MySQL and MariaDB\n- added focused model/migration tests for MySQL, MariaDB, SQLite, upgrade, and data-preserving downgrade\n- verified upgrade/downgrade/re-upgrade on MySQL 8 with 10,000 pre-migration rows; downgrade retained all 10,002 rows and two timestamps one microsecond apart\n- full affected suite: 65 passed; full MySQL audit module: 22 passed; staged-file pre-commit passed\n\nThe PR description now documents the non-atomic MySQL DDL and intentional data-preserving schema asymmetry. |
|
Superseding my earlier note that the approval covered unchanged content: the Aug 7 commits added substantive migration behavior and the final predecessor-ambiguity fix, so a current-head re-ack is appropriate.\n\nRichard's tied-predecessor concern is addressed in d6a4211. The lookup now examines the complete newest timestamp group semantically: mixed statuses are ambiguous and retain the current audit, while an all-blocked tied group remains eligible for suppression. A discriminating mixed blocked/failed regression test was added.\n\nFresh verification at d6a4211:\n- complete affected suite: 66 passed\n- audit module: 23/23 on SQLite and 23/23 on MySQL 8\n- changed-file pre-commit: passed, including MyPy, Ruff, and Pylint\n\n@rusackas, when convenient, could you please re-ack the current head, including the DATETIME(6)/data-preserving downgrade commits and this ambiguity fix? |
|
Launch-review follow-up pushed in a37acd3:\n\n- corrected the model documentation to distinguish discardable provisional records from immutable retained outcomes\n- parameterized SQLAlchemy TypeEngine annotations\n- explicitly typed migration-test context-manager mocks\n- retained the private-helper assertion in the mixed-tie regression because it makes the pre-fix control deterministic; public behavior alone can pass depending on which tied row the database selects\n\nFresh verification remains 66/66 affected tests with changed-file pre-commit fully green. This commit is documentation/test typing only; the current-head re-ack request above still applies. |
Code Review Agent Run #88c9afActionable 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 |
…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>
…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>
…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>
4 participants
SUMMARY
Deduplicate consecutive scheduled retention audits that remain blocked for the same entity while preserving the write-ahead audit invariant. The scheduled purge still writes a provisional
pendingrecord before evaluating the cascade, but removes only that current provisional record when its unambiguous, strictly older scheduled predecessor is alreadyblocked.The change preserves completed audit history, retains evidence on ambiguous ordering or persistence uncertainty, and leaves force-purge auditing independent. It also adds suppression/fallback metrics and a composite predecessor-lookup index. On MySQL/MariaDB, the migration changes
purge_audit_log.created_onfromDATETIME(0)toDATETIME(6)so sequential audit attempts retain the sub-second ordering the deduplication decision requires.Shortcut: SC-115343.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; backend audit behavior only.
TESTING INSTRUCTIONS
Expected: 66 tests pass.
The migration was verified with upgrade/downgrade/re-upgrade cycles on isolated SQLite and MySQL 8 metadata databases. A populated MySQL 8 table with 10,000 pre-migration audit rows upgraded in under one second of Alembic-reported migration time (3.97 seconds for the full Superset CLI process, including startup). After inserting two audit rows one microsecond apart, downgrade retained all 10,002 rows and both exact fractional timestamps while removing the predecessor index; re-upgrade then succeeded. MySQL reports
created_onasdatetime(6), and the full 23-test audit module passes against both MySQL and SQLite. The audit tests include mixed-status rows tied at the newest predecessor timestamp; this ambiguity retains the current evidence while an all-blocked tied group remains eligible for bounded suppression. Focused unit tests verify MySQL/MariaDB model DDL, the dialect-specific upgrade, the non-MySQL path, and data-preserving downgrade behavior.EXPLAIN QUERY PLANselectedix_purge_audit_log_retention_predecessorfor the predecessor lookup on SQLite. Changed-file pre-commit passes, including MyPy, Ruff, and Pylint.ADDITIONAL INFORMATION
The migration changes one MySQL/MariaDB timestamp column from
DATETIME(0)toDATETIME(6)and adds one non-unique index topurge_audit_log. The table and scheduled purge were introduced recently, and each normal run processes at most 500 eligible entities per batch, so existing deployments are expected to have audit histories on the order of thousands—not millions—of rows unless the task has been invoked unusually often. At that expected size, both alterations should complete quickly. Runtime remains proportional to the actual audit-table row count. PostgreSQL may block writes during index creation; MySQL/MariaDB may rebuild or copy the table for the precision alteration and may hold a metadata lock during both operations. Operators with unexpectedly large audit tables should measure the table and schedule the migration in a maintenance window. No application downtime is otherwise expected.MySQL DDL is non-transactional, so the upgrade is not atomic. Downgrade is application-backwards-compatible and data-preserving: it removes the new index but deliberately retains
DATETIME(6), which the previous application can read and write. RestoringDATETIME(0)would irreversibly truncate audit-ordering evidence written after upgrade, so exact schema symmetry is intentionally not attempted.