fix(gtf): async chart-data robustness + accurate DB errors; retire async_events - #43473
Conversation
…async_events Bundles the post-cutover review follow-ups: - Surface the true DB error on task create/update. on_error gains an opt-in preserve_message that carries the DBAPI cause (e.g. "database is locked") into TaskCreateFailedError / TaskUpdateFailedError instead of the generic message, so a transient contention failure is no longer reported as a flat "Task could not be created." - Don't run async when the result can't be read back. /chart/data refuses async under a NullCache DATA backend (falls back to sync 200), and the client falls back to a synchronous fetch if a post-completion re-request still returns 202 (oversized result / per-query disabled timeout), instead of looping/throwing. - Ownership-checked lock release is now atomic. A Lua compare-and-delete on the coordination backend replaces the get-then-delete in _release_redis, closing the window where an expired-then-reacquired lock could be dropped. - A dependent cancelled while waiting on prerequisites no longer publishes a FAILURE that never committed: execute_task only publishes FAILURE when the conditional transition actually landed, else reports the committed status. - Retire the vestigial async_events package: its only survivor, cache_backend (the coordination service's Redis client wrappers), moves to superset/coordination/cache_backend.py and the package is deleted.
dosubot
Bot
added
change:backend
|
Bito Automatic Review Skipped - Branch Excluded |
|
/review |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #8e9ef6Actionable Suggestions - 0Additional Suggestions - 2
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}
| cause = getattr(ex, "orig", None) or ex | ||
| raise reraise(str(cause), ex) from ex |
There was a problem hiding this comment.
Suggestion: The new preserve_message path copies the raw DBAPI exception text into the SupersetException message. Because task create/update exceptions serialize this message through their API error response, backend-specific errors can disclose schema, table, connection, or other database details to clients. Preserve only a sanitized/allowlisted operational message, or keep the detailed cause in server-side logs while returning the generic task error externally. [security]
Severity Level: Major ⚠️
- ⚠️ GTF task creation errors can reveal backend database details.
- ⚠️ Chart/task API clients may receive driver-specific exception text.
- ⚠️ Error messages can expose SQL, schema, host, or table metadata.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/decorators.py
**Line:** 249:250
**Comment:**
*Security: The new `preserve_message` path copies the raw DBAPI exception text into the `SupersetException` message. Because task create/update exceptions serialize this message through their API error response, backend-specific errors can disclose schema, table, connection, or other database details to clients. Preserve only a sanitized/allowlisted operational message, or keep the detailed cause in server-side logs while returning the generic task error externally.
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 fixThere was a problem hiding this comment.
Good catch — fixed in e63a05f by removing the preserve_message path entirely. Task create/update now reraise the generic TaskCreateFailedError / TaskUpdateFailedError to the client; the detailed DBAPI cause stays server-side (Celery task traceback / request log), which is exactly the sanitized-external / detailed-in-logs split you recommend.
Context: preserve_message was added to surface SQLite's "database is locked" during local dev. We've since moved local dev to Postgres and reverted the SQLite-lock workarounds, so its motivation is gone — dropping it is the right call on both counts.
|
The security concern regarding the exposure of backend-specific database details in In # Example of sanitizing the exception message
if preserve_message:
# Log the full exception for debugging
logger.exception("Task operation failed: %s", str(source))
# Return a sanitized message to the client
message = "An internal error occurred while processing the task."
# ... proceed with raising the exception using the sanitized messageWould you like me to fetch all other comments on this PR to validate them and implement a minimal fix for the rest as well? superset/utils/decorators.py |
| requestChartData(true).then(({ response: r, json: j }) => | ||
| handleChartDataResponse(r, j), | ||
| ) as Promise<QueryData[]>, | ||
| requestChartData(true).then(({ response: r, json: j }) => { |
There was a problem hiding this comment.
Suggestion: The cache-read retry still sends enableAsyncMode: true; when the completed result is not in the cache, the server submits another background task and returns 202 rather than merely probing the cache. The subsequent synchronous fallback therefore causes every uncacheable completion to execute the query twice and leaves the duplicate task running. Avoid scheduling another async task during the retry, or go directly to a synchronous request after task completion. [performance]
Severity Level: Major ⚠️
- ⚠️ Uncacheable chart results execute duplicate background tasks.
- ⚠️ Extra GTF work increases query and worker load.
- ⚠️ Duplicate tasks can compete for database resources.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/chartAction.ts
**Line:** 772:772
**Comment:**
*Performance: The cache-read retry still sends `enableAsyncMode: true`; when the completed result is not in the cache, the server submits another background task and returns 202 rather than merely probing the cache. The subsequent synchronous fallback therefore causes every uncacheable completion to execute the query twice and leaves the duplicate task running. Avoid scheduling another async task during the retry, or go directly to a synchronous request after task completion.
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 fixThere was a problem hiding this comment.
Fixed in e63a05f. The post-completion re-issue now runs synchronously (enableAsyncMode: false), so it never schedules a second background task — it reads the warm per-query cache, or computes inline once if the result wasn't cached. The repeat-202 branch (and its duplicate-task path) is gone entirely.
Added a regression test asserting the re-issue carries no async_mode in its body and that exactly two requests are made (async submit + one sync re-issue, no third call).
| // skips the write). Fall back to a synchronous fetch, which returns | ||
| // the payload inline, instead of looping on an uncacheable request. | ||
| if (r.status === 202) { | ||
| return requestChartData(true, true).then( |
There was a problem hiding this comment.
Suggestion: The synchronous fallback changes only the async mode and keeps force disabled through requestChartData(true, true). If the original request used force: true and an older result is already cached while the newly completed task failed to write its result, this fallback can return that older cached result instead of the result requested by the user. Preserve the original force semantics for the fallback, or explicitly bypass the cache. [logic error]
Severity Level: Major ⚠️
- ⚠️ Forced chart refreshes can display stale DATA-cache results.
- ⚠️ Dashboard refreshes may show outdated query results.
- ⚠️ User-requested cache bypass semantics are lost.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/chartAction.ts
**Line:** 779:779
**Comment:**
*Logic Error: The synchronous fallback changes only the async mode and keeps `force` disabled through `requestChartData(true, true)`. If the original request used `force: true` and an older result is already cached while the newly completed task failed to write its result, this fallback can return that older cached result instead of the result requested by the user. Preserve the original force semantics for the fallback, or explicitly bypass the cache.
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 fixThere was a problem hiding this comment.
Fixed in e63a05f. The re-issue now uses the caller's original force (no longer forced to false), so a forced refresh bypasses any stale cached entry instead of returning it. Combined with the sync change above, the post-completion request is a single synchronous call that preserves force — reading the warm cache on a normal request, or recomputing fresh when the user forced a refresh.
The regression test drives a force: true request and asserts the re-issue's URL carries force=true.
…rving re-issue Review follow-ups on #43473: - Security: remove the on_error `preserve_message` path. It copied the raw DBAPI exception text into the task create/update error, which serializes to API clients and could disclose schema/table/connection details. Revert to the generic task error; the detailed cause stays in server logs (Celery traceback / request log). Its original motivation (surfacing SQLite "database is locked") is moot now that local dev uses Postgres. - Performance + correctness: the post-completion chart-data re-issue now runs synchronously and preserves the caller's `force`. Previously the re-issue ran async again, so an uncacheable result scheduled a *second* background task before falling back; and it dropped `force`, so a forced refresh could return a stale cached entry. A single synchronous re-issue reads the warm per-query cache (or computes once), never schedules a duplicate task, and — with `force` preserved — never serves stale data on a forced refresh. Removes the repeat-202 branch entirely.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## gaq-to-gtf #43473 +/- ##
=============================================
Coverage ? 78.86%
=============================================
Files ? 2883
Lines ? 164809
Branches ? 38090
=============================================
Hits ? 129975
Misses ? 32388
Partials ? 2446
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:
|
1 participant
SUMMARY
Targets
gaq-to-gtf. Post-cutover robustness fixes for the GTF async chart-dataflow, addressing review findings on the merged child PRs, plus a small cleanup.
Async chart-data can no longer silently hang or throw when the result isn't cached.
Async delivery is cache-then-read-back, so a result that never lands in the DATA
cache (a
NullCachebackend, an oversized value, or a per-query disabled timeout)would previously leave the client re-requesting an uncacheable result — a repeat
202the refetch handler couldn't handle. Now:/chart/datarefuses async under aNullCacheDATA backend and runssynchronously (the
202→loop can't even start); and202, the client falls back to asynchronous fetch (which returns the payload inline) instead of throwing.
Distributed-lock release is now atomic. The ownership-checked release did a
separate
GETthenDEL; if the lock expired and was re-acquired between them, thestale holder could delete the new holder's lock. Replaced with a single Lua
compare-and-delete on the coordination backend.
A dependent task cancelled while waiting on prerequisites no longer publishes a
phantom
FAILURE.execute_tasknow only publishesFAILUREwhen the conditionaltransition actually committed; if the task was concurrently aborted, it reports the
status that actually landed.
Accurate DB error surfacing. A transient metadata-DB error during a task
create/update is surfaced with its true cause (e.g. "database is locked") instead of
the generic "Task could not be created/updated." (opt-in
preserve_messageon theshared
on_error).Cleanup — retire the
async_eventspackage. The GAQ cutover deleted everythingin it except
cache_backend.py, whose classes are now the coordination service'sRedis client wrappers (not async-events/GAQ-specific). Moved to
superset/coordination/cache_backend.pyand deleted the package.TESTING INSTRUCTIONS
Unit tests cover each fix:
pytest tests/unit_tests/charts/test_chart_data_api.py -k should_run_async— asyncrefused under
NullCache.pytest tests/unit_tests/distributed_lock/distributed_lock_tests.py— atomiccompare-and-delete release; a stale token doesn't drop a newer holder's lock.
pytest tests/unit_tests/coordination/test_cache_backend.py—compare_and_deletedelegates to the Lua script.
pytest tests/unit_tests/tasks/test_dependencies.py— a cancelled dependent reportsthe committed status, not a phantom
FAILURE.pytest tests/unit_tests/utils/test_decorators.py—preserve_messagesurfaces theDBAPI cause.
npm run test -- src/components/Chart/chartActions.test.ts— client sync fallback ona repeat
202.End-to-end: with
GLOBAL_ASYNC_QUERIES=on, a persistentDATA_CACHE_CONFIG, and aCelery worker, load a dashboard and confirm charts resolve; point
DATA_CACHE_CONFIGat
NullCacheand confirm requests run synchronously (no tasks scheduled).ADDITIONAL INFORMATION
GLOBAL_ASYNC_QUERIES(async path only)