codeant-ai-for-open-source[bot] commented on code in PR #41096:
URL: https://github.com/apache/superset/pull/41096#discussion_r3417355092
##########
superset/commands/report/execute.py:
##########
@@ -1170,6 +1169,21 @@ def run(self) -> None:
)
user = security_manager.find_user(username)
+ with override_user(user):
+ # Pre-commit any permalink rows before the state machine's
+ # @transaction() opens. When called inside a transaction,
+ # CreateDashboardPermalinkCommand only flushes (not commits),
+ # leaving the row invisible to Playwright's separate DB
+ # connection. Running get_dashboard_urls() here — outside any
+ # transaction — lets the command commit normally. The state
+ # machine's inner call to get_dashboard_urls() hits get_entry()
+ # for the same deterministic UUID and returns the
+ # already-committed row without a second INSERT.
+ if self._model.dashboard_id:
+ BaseReportState(
+ self._model, self._scheduled_dttm, self._execution_id
+ ).get_dashboard_urls()
Review Comment:
**Suggestion:** The new pre-warm permalink call runs before the state
machine, so any exception from `get_dashboard_urls()` now aborts execution
before the normal report-state error handling runs. That means failures that
were previously recorded as `ERROR` (with execution logs/notifications) can now
surface as task failures without updating the report schedule state, leaving
stale status and missing error logs. Make the pre-warm step best-effort (catch
and log, then continue into the state machine) or route pre-warm failures
through the same state transition/error logging path. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ `reports.execute` Celery task fails before state machine.
- ❌ ReportSchedule.last_state remains stale after permalink failure.
- ⚠️ No ReportExecutionLog row for failed execution.
- ⚠️ Owners may miss error notifications for these failures.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Schedule execution of a dashboard report via Celery task `execute` in
`superset/tasks/scheduler.py:120-199`, which calls
`AsyncExecuteReportScheduleCommand(task_id, report_schedule_id,
scheduled_dttm).run()` at
`scheduler.py:12-16` (from the snippet).
2. Inside `AsyncExecuteReportScheduleCommand.run` in
`superset/commands/report/execute.py:1160-1190` (snippet `lines 21-52` for
1140-1259),
after `self.validate()` and resolving `user`, execution enters the new
pre-warm block
`with override_user(user): ... BaseReportState(self._model,
self._scheduled_dttm,
self._execution_id).get_dashboard_urls()` at `execute.py:1172-1185`.
3. `BaseReportState.get_dashboard_urls` in
`superset/commands/report/execute.py:120-210`
(header `Showing lines 140 to 359`, snippet lines 120-210) eventually calls
`_get_tab_url`
at `execute.py:212-221`, which invokes
`CreateDashboardPermalinkCommand(...).run()` at
`execute.py:218-221`; that `run()` is decorated with
`@transaction(on_error=...,
reraise=DashboardPermalinkCreateFailedError)` in
`superset/commands/dashboard/permalink/create.py:60-71`. If a realistic
error occurs here
(for example, a `SQLAlchemyError` from `KeyValueDAO.create_entry` or
`DashboardDAO.get_by_id_or_slug`, as guarded in `create.py:60-67`),
`CreateDashboardPermalinkCommand.run` raises
`DashboardPermalinkCreateFailedError`, which
propagates back through `_get_tab_url` and `get_dashboard_urls` into the
pre-warm block.
4. This exception is raised before `ReportScheduleStateMachine.run()` is
called. It is
caught only by the outer `try/except` of
`AsyncExecuteReportScheduleCommand.run` at
`execute.py:51-64`, which wraps non-`CommandException` errors in
`ReportScheduleUnexpectedError` and re-raises. The Celery task `execute`
catches
`ReportScheduleUnexpectedError` at `scheduler.py:17-21` and marks the task
`FAILURE`, but
because `ReportScheduleStateMachine.run()` (decorated with `@transaction()`
at
`execute.py:22-27` in the 1110-1169 snippet) never executes, no state class
(`ReportNotTriggeredErrorState.next` at `execute.py:890-120` or
`ReportSuccessState.next`
at `execute.py:124-213`) runs, so
`BaseReportState.update_report_schedule_and_log` and
`create_log` at `execute.py:26-37` and `39-58` are never called. As a
result, the
`ReportSchedule` row's `last_state` and the `ReportExecutionLog` table are
not updated to
reflect the failure, whereas the same failure occurring later inside
`send()` →
`_get_notification_content()` → `_get_screenshots()` (see
`execute.py:658-79` and
`419-65`) would be caught by the state classes' broad `except
(SupersetErrorsException,
Exception)` blocks at `execute.py:72-76` and `196-212`, which explicitly
update state and
logs. Thus, the new pre-warm call introduces a reachable path where
permalink-related
failures are surfaced only as Celery task failures, leaving stale report
state and missing
execution logs.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=50a81e1ed8cd4e169c4e599c97966c67&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=50a81e1ed8cd4e169c4e599c97966c67&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/report/execute.py
**Line:** 1172:1185
**Comment:**
*Logic Error: The new pre-warm permalink call runs before the state
machine, so any exception from `get_dashboard_urls()` now aborts execution
before the normal report-state error handling runs. That means failures that
were previously recorded as `ERROR` (with execution logs/notifications) can now
surface as task failures without updating the report schedule state, leaving
stale status and missing error logs. Make the pre-warm step best-effort (catch
and log, then continue into the state machine) or route pre-warm failures
through the same state transition/error logging path.
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%2F41096&comment_hash=b71ae0fe2528a056a927623db648f2d635ebddd5a9884eb9c7b60e6a32d7e6c2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41096&comment_hash=b71ae0fe2528a056a927623db648f2d635ebddd5a9884eb9c7b60e6a32d7e6c2&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]
