mikebridge commented on PR #41549: URL: https://github.com/apache/superset/pull/41549#issuecomment-5130937766
@rusackas assessed all three — two are real and fixed, and the third is real but not at the severity it was given. `2cc0d92abe`. **Ambiguous UUID resolution in force-purge (Critical — valid, fixed).** `_resolve()` scanned every soft-delete model and returned the first match. UUID uniqueness is only enforced per table and the import APIs accept caller-supplied UUIDs, so the search can genuinely be ambiguous — and an operator running a compliance deletion had no way to express which entity they meant. It now collects every match and refuses when there is more than one, naming the types so the caller can disambiguate. A `model_cls` argument goes alongside it, so a caller that already knows the type can pin resolution to it rather than relying on the search being unambiguous. That matters for #41550, which adds the first end-user-facing caller: its authorization is checked against one specific entity, so resolution must not be free to wander to another. Worth noting the flaw only becomes *reachable by an end user* in that PR — here the callers are the CLI and the retention task, both operator-grade. **Dry runs finalizing audit rows (Major — valid, fixed).** `audit.reconcile_pending()` ran unconditionally, and it finalizes stale pending records, which is a durable write. An operator sizing up a rollout was mutating the audit record they were inspecting. Now skipped when `dry_run` is set, with the real path unchanged and both pinned by test. **Identity re-check in the retention task (Critical — valid, but the severity overstates it).** The finding is correct that `_purge_one` snapshots a uuid, writes the audit row, re-resolves by integer id, and never checks it got the same row back. I've added the check. But the destructive half of the claim doesn't hold. `cascade_hard_delete` runs with `enforce_window=True` here, and its conditional claim re-checks `deleted_at IS NOT NULL AND deleted_at < cutoff` under `SELECT ... FOR UPDATE`, then repeats those predicates on the `DELETE` itself. A live entity that inherited a recycled id could never have been purged by this path — it would fail the claim and raise `PurgeRaceLostError`. What could actually drift is **attribution**: if an id were reused between the two resolutions and the new occupant were *itself* soft-deleted and past the cutoff, the purge would proceed while the audit row named the previous object. That needs id reuse, which SQLite does with rowids but Postgres and MySQL sequences do not. So: a real hole in the audit trail, not "concurrent purge can delete the wrong entity". Fixed regardless — an audit row that identifies the wrong entity is worse than a skipped purge, and the check is one comparison. Each new test fails against the previous code: the dry-run assertion sees `reconcile` called once, and the ambiguity tests can't import a symbol that didn't exist. 50 tests green across the retention suites; pre-commit clean. -- 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]
