mikebridge commented on PR #41549: URL: https://github.com/apache/superset/pull/41549#issuecomment-5131319263
Correction to an earlier comment of mine on this PR. I wrote that `require_archived` closed the window between authorization and purge. That was wrong, and I want it on the record before anyone reviews on the strength of it. `ForcePurgeCommand` runs the cascade with `enforce_window=False`, and in the code as it stands on this branch both the `SELECT ... FOR UPDATE` claim and the conditional `DELETE` add `deleted_at` predicates only under `enforce_window`. So `require_archived` filtered at resolution time and nowhere else: a restore committing after the resolve could still have its live row hard-deleted. It narrowed the race rather than closing it. The fix — carrying the predicate into both statements — is on the follow-up branch for #41550, along with a control run showing the regression test fails without it. Two of the tests I pointed at as covering this were also passing for the wrong reason (one exited at a `pragma: no cover` guard without ever constructing `ForcePurgeCommand`); those are repaired there too. Nothing here changes what this PR does — flagging it because my earlier description of it was inaccurate. -- 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]
