fix(tasks): don't re-raise validation-class errors in async chart-data cache task (SC-118140) - #43464
fix(tasks): don't re-raise validation-class errors in async chart-data cache task (SC-118140)#43464eschutho wants to merge 3 commits into
Conversation
…a cache task (SC-118140) ChartDataQueryFailedError/ChartDataCacheLoadError map to 400/422 in the synchronous chart/data endpoint - expected, client-facing validation failures (e.g. a chart referencing columns a customer has since dropped from the dataset), not application bugs. load_chart_data_into_cache unconditionally re-raised every exception after reporting it via update_job, so these got double-reported: once cleanly to the client, and again as an unhandled Celery task exception (and Sentry ERROR). Fixes SUPERSET-PYTHON-13JV Co-Authored-By: Claude <noreply@anthropic.com>
dosubot
Bot
added
change:backend
Code Review Agent Run #109719Actionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| mock_query_context_schema_cls.return_value.load.side_effect = err | ||
|
|
||
| # Should not raise. | ||
| load_chart_data_into_cache(job_metadata, form_data) |
There was a problem hiding this comment.
Suggestion: The regression test raises ChartDataQueryFailedError from ChartDataQueryContextSchema.load, so it never executes ChartDataCommand.run() or the production logic that converts query payload errors into this exception. This can pass even if the real chart-data failure path is not caught correctly; mock ChartDataCommand.run to raise the exception after a valid query context is created, as the cache-load test does. [possible bug]
Severity Level: Minor 🧹
- ⚠️ Unit regression test does not cover `ChartDataCommand.run`.
- ⚠️ Incorrect exception handling could regress undetected.
- ⚠️ Async chart-data error behavior remains weakly protected.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/tasks/test_async_queries.py
**Line:** 94:97
**Comment:**
*Possible Bug: The regression test raises `ChartDataQueryFailedError` from `ChartDataQueryContextSchema.load`, so it never executes `ChartDataCommand.run()` or the production logic that converts query payload errors into this exception. This can pass even if the real chart-data failure path is not caught correctly; mock `ChartDataCommand.run` to raise the exception after a valid query context is created, as the cache-load test does.
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
|
The flagged issue is correct. In To resolve this, update the test to mock the command class and set the side effect on its tests/unit_tests/tasks/test_async_queries.py |
…r behavior (SC-118140) Self-review caught that the unit-test-only local verification missed tests/integration_tests/tasks/async_queries_tests.py::test_load_chart_data_into_cache_error, which still asserted the old re-raise behavior via pytest.raises(...) - would have failed CI. Updated it to match the new no-reraise contract (load_chart_data_into_cache no longer raises for this exception type, still reports it via update_job). Co-Authored-By: Claude <noreply@anthropic.com>
…reraise test (SC-118140) Automated PR review (bito-code-review) correctly flagged that this test mocked the schema loader's side effect instead of ChartDataCommand.run, so it never actually reached the run() call the exception is meant to simulate failing at. The except clause still catches the exception either way (same try block), so the assertion was never wrong, but mocking at the real trigger point matches the sibling ChartDataCacheLoadError test and the corrected integration test, and is more representative of the actual failure path. Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43464 +/- ##
==========================================
- Coverage 78.85% 78.85% -0.01%
==========================================
Files 2876 2876
Lines 164601 164605 +4
Branches 38015 38015
==========================================
+ Hits 129799 129801 +2
- Misses 32355 32357 +2
Partials 2447 2447
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #209ddfActionable 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 |
1 participant
Summary
Sentry: SUPERSET-PYTHON-13JV — 908 events / 0 users over the trailing 14 days, chronic since 2026-05-22, still actively firing.
Root cause
load_chart_data_into_cache(the Celery task backingGLOBAL_ASYNC_QUERIESchart loads) catches every exception fromChartDataCommand.run()in one genericexcept Exceptionblock that reports the failure to the client viaasync_query_manager.update_job(...)and then unconditionally re-raises.For
ChartDataQueryFailedError— raised when a chart still references columns that have since been dropped from its dataset (customer-side schema drift, not a Superset bug) — this is inconsistent with how the codebase already treats the same exception type in the synchronous path:ChartDataRestApi._get_data_responseexplicitly mapsChartDataQueryFailedErrorto a 400 (and its siblingChartDataCacheLoadErrorto 422), i.e. these are already classified elsewhere as expected, client-facing validation failures, not server bugs.Because the async task always re-raises, Celery's default task-exception handling (and the Sentry Celery integration) captures every occurrence as an unhandled ERROR, even though
update_jobhas already delivered a clean error to the client. This task is fire-and-forget (apply_async, no.get(), no retry policy, no result-backend consumer), so nothing depends on the task's own exception/FAILURE state for these two exception types.Fix
Add a dedicated
except (ChartDataCacheLoadError, ChartDataQueryFailedError)branch before the generic handler insuperset/tasks/async_queries.py: still callupdate_job(..., STATUS_ERROR, ...)so client-facing behavior is unchanged, log at INFO, and don't re-raise. All other exception types — including genuinely unexpected ones and the SIP-40SupersetErrorException/SupersetErrorsExceptionfamily — are untouched and still re-raise exactly as before.Tradeoffs
This changes failure-mode semantics for these two exception types specifically: the Celery task now completes without raising even though the underlying chart query failed (the client still sees the error via
update_job/the job-status poll, unchanged). I verified no code path inspects this task's own exception/result state — it's fire-and-forget with no retry policy — so this should be safe, but flagging it explicitly since it's a real behavior change, not a pure log-level tweak.Testing
ruff check/ruff format --checkclean on both touched files.pytest tests/unit_tests/tasks/test_async_queries.py— 9/9 passing: repointed the existing generic-error test at a plainRuntimeError(it was incidentally usingChartDataQueryFailedErroras a stand-in for "some exception"), and added two new tests assertingChartDataQueryFailedErrorandChartDataCacheLoadErrorare reported viaupdate_jobbut do not re-raise.superset_core, known local env issue) — ran the target file directly withsuperset-coreadded toPYTHONPATH.Follow-ups
None identified — the fix is self-contained to this one task.
Shortcut: sc-118140
🤖 Generated with Claude Code
Update: self-review (Claude Code, Codex unavailable — platform outage) caught that the unit-test-only local verification missed an integration test (
tests/integration_tests/tasks/async_queries_tests.py::test_load_chart_data_into_cache_error) still asserting the old re-raise behavior — would have failed CI. Fixed in a follow-up commit to match the new no-reraise contract; confirmed via manual trace + ruff (the integration suite itself isn't runnable in this environment, a localparameterizedpackage gap unrelated to this change).