eschutho commented on PR #42624: URL: https://github.com/apache/superset/pull/42624#issuecomment-5148905676
Thanks for the thorough pass — responses per item. Verification and fixes are on head `8feae1a1f0`. **1. Timeout-recovery branch lacks an ownership re-check (`execute.py:1602`)** Agreed the TOCTOU is real, with one important qualifier: this branch is structurally identical on current master (same `is_on_working_timeout()` check → same unguarded `update_report_schedule_and_log(ERROR)`; this PR added only the audit-row ownership comment and terminal logging there). A meaningful fix isn't just re-checking ownership — without a unique constraint on `ReportExecutionLog.uuid` or row locking, a re-check only narrows the window. That's schema/locking work (migration + backfill for existing duplicate uuids) that we've scoped as a follow-up rather than folding into an already-large behavioral PR. The in-process check matches the pre-existing guarantee; this PR doesn't claim to strengthen it on this path. **2. Refused-duplicate rows can share a uuid (`execute.py:1636`, `models.py:388`)** Also real, and a deliberate trade-off. The colliding case requires Celery redelivering the *same* task message (same `execution_id`) while the original still runs. Pre-PR behavior in that exact case was strictly worse: another *WORKING* row with the same uuid **plus** a schedule-state touch — the dangling-WORKING family this PR is fixing. Now the duplicate is a terminal audit row and the schedule is untouched. We can't assert uuid-uniqueness in `test_report_schedule_same_execution_replay_stays_working` yet because the invariant genuinely doesn't hold in this narrow case; the follow-up (same one as item 1) covers the unique index plus a same-`execution_id` guard in the refused branch (skip the audit insert when a WORKING row with our own uuid already exists — the active row already represents this execution). **3. Retry net doesn't cover the stale-recovery invocation's own write (`execute.py:1813`)** Intentional, and structural rather than an oversight: `persist_owned_report_execution_terminal_error` terminalizes the WORKING row matching *this invocation's* uuid — a stale-recovery invocation has no such row (it inserts a fresh ERROR row), so even with the flag set the helper would return `False` at the `working_log is None` check. Extending coverage would mean mutating *another execution's* audit row from a retry path while the original worker may still be alive — exactly what the ownership rules here forbid. The failure mode degrades safely: if the recovery invocation's own write is lost to a transient DB error, the next scheduled invocation re-enters the same timeout branch and re-attempts recovery. That's one extra schedule period at worst, versus the pre-PR staging bug where the terminal write never happened at all. Happy to add a code comment making this explicit if you'd like. **4. `SoftTimeLimitExceeded` handler fires for alerts too (`scheduler.py:114`)** Fixed via the "document as shared + test" option in `d06a3b55d2`: the PR description now states the handler is shared and why that's safe, and `tests/unit_tests/tasks/test_scheduler_soft_timeout.py` pins the alert path (metric, warning log, explicit FAILURE, re-raise). We chose documenting over scoping because type dispatch inside the handler would need a model read at the precise moment the DB may be implicated in the timeout, and for alerts the delta is observability-only — pre-PR the exception propagated uncaught to Celery's generic failure handling, which the re-raise preserves. **5. Delegation-only unit tests** Renamed in `8feae1a1f0` as suggested: `test_working_timeout_replay_delegates_single_terminal_update` and `test_stale_recovery_delegates_terminal_update_without_delivery`. Kept rather than dropped — they pin that `next()` delegates exactly once without directly mutating the stale row, which the integration tests don't isolate. **6. `logger.exception` on the no-fallback combine failure** Respectfully pushing back on this one: the exception reaching the generic handler in that path is a genuine combine fault (PIL-level failure while stitching tiles), not an intentional reject. The *intentional* rejects — budget exhaustion and per-tile readiness timeout — already route through dedicated `warning`-level branches above the generic handler precisely to keep customer-side slowness out of error-rate alerting. A combine failure is unexpected regardless of fallback policy, so error level is deliberate. The one fair sub-point is double-logging (inner `combine_screenshot_tiles` + outer handler log the same fault twice); can dedupe if you feel strongly. **7. "900-second" headline / 8. stale draft closing line** Both fixed in the description: the budget bullets now describe the 3600s default capped by `working_timeout`, the staged-validation section carries a note that its 900/930 figures were the staging configuration (that plan was executed against a 900s config and is kept as the historical record), and the do-not-merge closing line is replaced — the validation it was gating is complete, and the PR is ready for review. **CI status**: your suspicion in the earlier summary was correct — the run on the reviewed head failed exactly on `test_scheduler_report_timeout_uses_end_to_end_budget` (the stale 900/930 assertions; the previous green run predated the 900→3600 default change), plus an `auto-walrus` pre-commit failure and a persistence bug in my first fix of that same test (the `working_timeout` attribute write on the fixture object wasn't flushed on any backend; now a query-level UPDATE). All addressed in `8feae1a1f0`; CI is re-running on that head. -- 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]
