fix(reports): guard SUCCESS-state report execution against duplicate sends and stuck WORKING state#40657
Conversation
dosubot
Bot
added
the
alert-reports
Code Review Agent Run #5e0254Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40657 +/- ##
==========================================
- Coverage 64.03% 64.03% -0.01%
==========================================
Files 2662 2662
Lines 143243 143252 +9
Branches 32940 32941 +1
==========================================
+ Hits 91724 91728 +4
- Misses 49932 49937 +5
Partials 1587 1587
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The PR comments file contains only the header row and no actual comments. There is no content to analyze for this review suggestion. |
There was a problem hiding this comment.
Pull request overview
This PR hardens the report/alert execution state machine to better handle concurrency and failure scenarios in ReportSuccessState.next() by ensuring a WORKING-state guard is applied before sending notifications and by preventing schedules from getting stuck in WORKING when error-notification delivery fails.
Changes:
- Set
ReportState.WORKINGbeforesend()for non-ALERT (REPORT) schedules to reduce duplicate sends from concurrent scheduler ticks. - Wrap
send_error()in a try/except/finally so the schedule transitions toERROReven if sending the error notification raises. - Update/add unit tests to assert the new state transitions and the “ERROR is logged even if send_error raises” behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
superset/commands/report/execute.py |
Adds WORKING-state guard for REPORT success execution and ensures ERROR transition happens even when send_error() fails. |
tests/unit_tests/commands/report/execute_test.py |
Updates success-path expectations to include WORKING→SUCCESS and adds a regression test for send_error() raising. |
Code Review Agent Run #902385Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…ING) Two defects in ReportSuccessState.next(), both diverging from the robust pattern already used in ReportNotTriggeredErrorState.next(): - REPORT types never set the WORKING state before send(), so a concurrent scheduler tick (which routes to ReportSuccessState again while last_state is SUCCESS) is not blocked and can re-send — duplicate notifications. Set WORKING before send() for REPORT types (ALERT types already do). - In the ALERT error path, if send_error() itself raises, the ERROR-state transition was skipped, leaving the schedule stuck in WORKING until the working timeout. Wrap send_error() so the ERROR transition always runs. Update the REPORT success test for the new WORKING->SUCCESS sequence and add a test asserting ERROR is still logged when send_error() raises. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cceeds REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER was logged unconditionally in the finally block even when send_error() raised. find_last_error_notification() uses this marker to decide grace-period suppression, so recording it on notification failure incorrectly prevented subsequent error notifications. Now mirrors the ReportNotTriggeredErrorState pattern: start with the marker as the default error_message, capture send_ex if send_error() fails and override error_message with str(send_ex), so the marker only lands in the DB when the notification was actually delivered. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sadpandajoe
force-pushed
the
fix/report-success-state-guards
branch
from
2b3690b to
a4bb169
Compare
sha174n
added
the
merge-if-green
github-project-automation
Bot
moved this from Needs Review
to Approved and/or Merged
in Superset Review Help Wanted
|
Bito Automatic Review Skipped – PR Already Merged |
SUMMARY
Two correctness/security defects in
ReportSuccessState.next()(superset/commands/report/execute.py), both diverging from the robust pattern already used by the siblingReportNotTriggeredErrorState.next():Concurrent execution / duplicate notifications (CWE-362). The WORKING-state guard blocks a concurrent scheduler tick from re-running a report. It's set before
send()for ALERT types and on the not-triggered/error path, but not for REPORT types in the SUCCESS/GRACE state — they go straight tosend()with no WORKING marker. A second scheduler tick (which seeslast_state == SUCCESSand routes back intoReportSuccessState) is therefore not blocked, allowing duplicate sends. Now sets WORKING beforesend()for REPORT types too.Stuck WORKING state (CWE-755). In the ALERT error path, if
send_error()itself raises, theERROR-state transition was skipped, leaving the schedule stuck in WORKING until the working timeout.send_error()is now wrapped so theERRORtransition always runs (and a logging failure there is swallowed), mirroring the sibling'sfinallypattern.TESTING INSTRUCTIONS
Updated
test_success_state_report_sends_and_logs_successto expectWORKING→SUCCESS; addedtest_success_state_error_logged_when_send_error_raisesasserting ERROR is logged even whensend_error()raises.ADDITIONAL INFORMATION
🤖 Generated with Claude Code