codeant-ai-for-open-source[bot] commented on code in PR #41549: URL: https://github.com/apache/superset/pull/41549#discussion_r3573721407
########## superset/migrations/versions/2026-06-23_13-00_e7d93a524ff6_add_purge_audit_log.py: ########## @@ -0,0 +1,66 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""add purge_audit_log + +Immutable, content-free audit record for deletion-retention purges. Survives +the entity it names; written write-ahead (pending -> confirmed). + +Revision ID: e7d93a524ff6 +Revises: 8f3a1b2c4d5e +Create Date: 2026-06-23 13:00:00.000000 + +""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "e7d93a524ff6" +down_revision = "8f3a1b2c4d5e" + + +def upgrade() -> None: + op.create_table( + "purge_audit_log", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("status", sa.String(length=16), nullable=False), + sa.Column("trigger", sa.String(length=16), nullable=False), + sa.Column("actor", sa.String(length=256), nullable=False), + sa.Column("entity_type", sa.String(length=64), nullable=False), + sa.Column("entity_uuid", sa.String(length=36), nullable=True), + sa.Column("affected_referrers", sa.Text(), nullable=True), + sa.Column( + "removed_dashboard_slices", + sa.Integer(), + server_default="0", + nullable=False, + ), + sa.Column("created_on", sa.DateTime(), nullable=False), + sa.Column("confirmed_on", sa.DateTime(), nullable=True), + sa.PrimaryKeyConstraint("id"), + ) + op.create_index( + "ix_purge_audit_log_entity_uuid", + "purge_audit_log", + ["entity_uuid"], + unique=False, Review Comment: **Suggestion:** The migration only indexes `entity_uuid`, but the retention workflow repeatedly reconciles pending rows by `status` and `created_on`; that query will degrade into full table scans as audit history grows. Add a composite index aligned to reconciliation access patterns (for example on `status, created_on`) to avoid unbounded daily scan cost. [performance] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ⚠️ Daily purge task scans entire purge_audit_log table. ⚠️ Operator force-purge incurs growing reconciliation query cost. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Apply the Alembic migration `e7d93a524ff6_add_purge_audit_log` via `superset db upgrade`, which creates the `purge_audit_log` table and an index on `entity_uuid` only, as defined in `superset/migrations/versions/2026-06-23_13-00_e7d93a524ff6_add_purge_audit_log.py:36-61`. 2. Run the Celery task `purge_soft_deleted` defined at `superset/tasks/deletion_retention.py:30-47`; the task calls `_purge_impl()` at line 44, which immediately invokes `audit.reconcile_pending()` at `superset/tasks/deletion_retention.py:38-40` on each purge pass. 3. The `reconcile_pending()` function in `superset/commands/deletion_retention/audit.py:188-223` builds a query `session.query(PurgeAuditLog).filter(PurgeAuditLog.status == STATUS_PENDING).filter(PurgeAuditLog.created_on < cutoff).all()`, using `status` and `created_on` as filter columns but with no supporting index on either column in the migration. 4. As `purge_audit_log` grows with confirmed, failed, and blocked rows from every retention or force purge (record insertion at `superset/commands/deletion_retention/audit.py:100-109`), each call to `reconcile_pending()` performs a full table scan on `purge_audit_log` to find stale pending rows, causing the daily purge task and operator `ForcePurgeCommand.run()` (which also calls `audit.reconcile_pending()` at `superset/commands/deletion_retention/force_purge.py:68`) to slow down linearly with table size. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f9a3cc87b32643418ce2377578772e18&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f9a3cc87b32643418ce2377578772e18&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/migrations/versions/2026-06-23_13-00_e7d93a524ff6_add_purge_audit_log.py **Line:** 56:60 **Comment:** *Performance: The migration only indexes `entity_uuid`, but the retention workflow repeatedly reconciles pending rows by `status` and `created_on`; that query will degrade into full table scans as audit history grows. Add a composite index aligned to reconciliation access patterns (for example on `status, created_on`) to avoid unbounded daily scan cost. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=63a300da1a23670fb777d2cd67835e6e1361e925022b9c95662320144d22f8c2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=63a300da1a23670fb777d2cd67835e6e1361e925022b9c95662320144d22f8c2&reaction=dislike'>👎</a> ########## superset/commands/deletion_retention/purge_cascade.py: ########## @@ -0,0 +1,526 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Shared hard-delete cascade for the purge task and force-purge command. + +A single code path keeps the two surfaces from drifting. Every dependent row +is removed by an explicit ``sa.delete``; +the DB ``ON DELETE CASCADE`` constraints are a backstop only — SQLite does +not enforce FKs unless ``PRAGMA foreign_keys=ON`` and Core bulk DML fires +only DB-level cascades, so relying on cascade silently leaks rows. + +Cascade tiers: + +* **M:N join rows** — hard-deleted for the purged entity, including join + rows owned by *surviving* entities (e.g. a live dashboard's + ``dashboard_slices`` row to a purged chart). The entity on the other side + is never touched except to lose that one relationship row. +* **Owned children** (``delete-orphan``, no independent existence) — a + dataset's columns and metrics, hard-deleted with it. +* **Independently-owned entities** (a dashboard's charts, a chart's dataset) + are **preserved**. A live chart's loose ``datasource_id`` to a purged + dataset is left dangling — legacy hard-delete semantics, no guard. +* **Version history** — the entity's own ``*_version`` shadows and + the ``version_changes`` scoped to them, plus an orphan-sweep of any + ``version_transaction`` left owning zero surviving shadows. Runs + behind a ``has_table`` check so it no-ops when versioning is absent. +""" + +from __future__ import annotations + +import logging +from collections.abc import Iterator +from contextlib import contextmanager +from dataclasses import dataclass, field +from datetime import datetime +from typing import Any + +import sqlalchemy as sa +from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm import Session + +logger = logging.getLogger(__name__) + + +@contextmanager +def suspend_version_capture() -> Iterator[None]: + """Turn Continuum's write tracking off for the duration of the block. + + A purge *removes* version history; it must not *generate* it. Flipping + Continuum's master ``versioning`` option off also stops the association + tracker (``track_association_operations``) from firing on the cascade's + raw ``sa.delete`` of tracked join tables (``dashboard_slices``), which + otherwise raises ``KeyError`` because there is no Continuum unit-of-work + for a Core delete. No-op when Continuum is absent or capture is already + off; the prior value is always restored. + """ + try: + from sqlalchemy_continuum import versioning_manager + except ImportError: + yield + return + previous = versioning_manager.options.get("versioning", True) + versioning_manager.options["versioning"] = False + try: + yield + finally: + versioning_manager.options["versioning"] = previous Review Comment: **Suggestion:** `suspend_version_capture` mutates a global `sqlalchemy_continuum` option for the whole process without synchronization. If another request/task is writing versioned entities at the same time, it can run while `versioning` is temporarily `False` and silently lose version history. Use a process/thread-safe guard (or a session-scoped mechanism) so purge-only suppression cannot affect concurrent transactions. [race condition] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx ❌ Background purge can suppress unrelated entity version history. ⚠️ Deletion-retention task weakens global audit/version tracking. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start a Superset Celery worker using `celery_app` from `superset/tasks/deletion_retention.py:30`, which will execute the `purge_soft_deleted` task alongside other Celery tasks in the same Python process when concurrency is configured with threads or greenlets. 2. When `purge_soft_deleted()` runs, it calls `_purge_impl()` at `superset/tasks/deletion_retention.py:31-79`; inside `_purge_impl` each eligible entity purge eventually enters `_purge_model` and `_purge_one`, which wrap the hard-delete cascade in `with suspend_version_capture():` as shown at `superset/tasks/deletion_retention.py:1-12`. 3. The `suspend_version_capture()` context manager in `superset/commands/deletion_retention/purge_cascade.py:59-80` imports `sqlalchemy_continuum.versioning_manager`, reads `previous = versioning_manager.options.get("versioning", True)` at line 75, then sets `versioning_manager.options["versioning"] = False` at line 76 for the duration of the purge, restoring the prior value in the `finally` block at line 80. 4. While the purge task is inside this context (global `versioning` option set to False), another Celery task or background thread in the same process performs ordinary writes to a versioned model (for example a dashboard or chart model configured with Continuum), causing Continuum to see `versioning=False` and skip recording version-history rows for that unrelated operation; the change commits without any version shadow, silently losing history due to the unsynchronized global flag mutation. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1a591f1da74f4d09a7786ee9eb346ef6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1a591f1da74f4d09a7786ee9eb346ef6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/commands/deletion_retention/purge_cascade.py **Line:** 75:80 **Comment:** *Race Condition: `suspend_version_capture` mutates a global `sqlalchemy_continuum` option for the whole process without synchronization. If another request/task is writing versioned entities at the same time, it can run while `versioning` is temporarily `False` and silently lose version history. Use a process/thread-safe guard (or a session-scoped mechanism) so purge-only suppression cannot affect concurrent transactions. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=c3ec0370207fbe6480a7ce7b4955375782c0b9b327122efe6460251ff4ec4e3f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=c3ec0370207fbe6480a7ce7b4955375782c0b9b327122efe6460251ff4ec4e3f&reaction=dislike'>👎</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
