fix(gtf): close async-chart-data waiter race + widen guest_key column - #43461
Conversation
Two correctness fixes from the PR #43407 review: - P1 (async chart-data can hang): waitForAsyncData awaited baselineReady before registering the task waiter. The 202 is returned when tasks are *scheduled*, not finished, so at that moment the shared poll cursor is <= now < any task's future terminal — but awaiting between the 202 and registration opened a gap in which a fast task could finish and a concurrent chart's poll advance the cursor past its terminal update (and the socket event is dropped with no waiter yet), hanging the chart/native filter forever. Register the waiter synchronously in the same tick as the 202 (no await first); the poll then always observes the completion. Removed the now-unused baselineReady handle. - P1 (guest subscriber key too long): superset.tasks.guest returns "guest-" + a 64-char SHA256 hex digest = 70 chars, but task_subscribers.guest_key was String(64), so a guest-created async task's subscription would error on Postgres / truncate on MySQL. Widen the column to String(128) in the model and the (feature-branch) migration. Tests: async-registration is exercised by delivering the completion in the same tick with no wait; a full-length (70-char) guest key round-trips through the DAO.
|
Bito Automatic Review Skipped - Branch Excluded |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| // in which a fast task could finish and a concurrent chart's poll advance the | ||
| // cursor past its terminal update, and the socket event (no waiter yet) would | ||
| // be dropped — hanging the request. | ||
| await new Promise<void>((resolve, reject) => { |
There was a problem hiding this comment.
Suggestion: Registering the waiter synchronously does not eliminate the baseline race because init() performs the baseline request asynchronously. A chart can receive its 202 and register here before that baseline resolves; if the task reaches a terminal state before the baseline response, the baseline cursor can be newer than the task transition and the first poll will permanently skip it when the websocket event is unavailable. Preserve a readiness mechanism that prevents cursor advancement past tasks registered before the baseline, while keeping waiter registration synchronous. [race condition]
Severity Level: Major ⚠️
- ❌ Chart data requests can remain pending indefinitely.
- ❌ Native-filter async requests can hang without websocket delivery.
- ⚠️ The shared status poll permanently skips affected task transitions.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/middleware/asyncEvent.ts
**Line:** 228:228
**Comment:**
*Race Condition: Registering the waiter synchronously does not eliminate the baseline race because `init()` performs the baseline request asynchronously. A chart can receive its 202 and register here before that baseline resolves; if the task reaches a terminal state before the baseline response, the baseline cursor can be newer than the task transition and the first poll will permanently skip it when the websocket event is unavailable. Preserve a readiness mechanism that prevents cursor advancement past tasks registered before the baseline, while keeping waiter registration synchronous.
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 — you're right that synchronous registration alone did not close the polling-only path, because the init baseline cursor is fetched asynchronously and stamped with the server's now() at resolve time, which can post-date an early task. Fixed in 31a22c9 with a cleaner approach than a per-task lookup: submit_chart_data_query_tasks now captures a status-poll cursor before any task is created and returns it in the 202 alongside task_ids. Since it predates every task's creation, the client polls its waiter from that cursor and can never skip a terminal — no dependence on the baseline timestamp. It's one small value per response (echoing every task id back would bloat requests for large dashboards). The client rewinds its shared poll cursor to it on registration, and the init baseline now only seeds the cursor when unset so it can't clobber that rewind. Regression tests: the 202 carries the cursor (unit + integration) and the poll issues a request using it.
|
The flagged issue is correct. The current implementation of I have reviewed the other changes in this PR, which include a database migration and model update to increase the superset-frontend/src/middleware/asyncEvent.ts |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## gaq-to-gtf #43461 +/- ##
==============================================
- Coverage 78.86% 78.86% -0.01%
==============================================
Files 2883 2883
Lines 164782 164770 -12
Branches 38083 38084 +1
==============================================
- Hits 129949 129938 -11
+ Misses 32388 32387 -1
Partials 2445 2445
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:
|
… poll race Registering the waiter synchronously fixed the socket path, but a polling-only client could still hang: the init baseline cursor is fetched asynchronously and stamped with the server's now() at resolve time, which can post-date a task that finished early — the first poll (changed_on >= cursor) then permanently skips it. Fix (per review): capture a status-poll cursor server-side in submit_chart_data_query_tasks BEFORE any task is created, and return it in the 202 alongside task_ids. Since it predates every task's creation, a client that polls from it is guaranteed to observe each terminal transition. This is one small value per response (not echoing every task id back, which would bloat requests for large dashboards). The client rewinds its shared poll cursor to this value on waiter registration; the init baseline now only seeds the cursor when unset, so it can't overwrite a waiter's older rewind. Tests: 202 body carries the cursor (unit + integration); the poll issues a request using the returned cursor.
1 participant
SUMMARY
Two P1 correctness fixes from the #43407 review (targets
gaq-to-gtf).1. Async chart-data can hang if a task finishes before its waiter registers.
waitForAsyncDataawaitedbaselineReadybefore registering the task waiter. The202is returned when the query tasks are scheduled, not finished, so at that instant the shared poll cursor is<= now < any task's future terminal transition— the poll would normally always catch the completion. But awaiting anything between receiving the202and registering the waiter opens a gap in which a fast task can reach a terminal state and a concurrent chart's poll can advance the shared cursor past that terminal update, while the per-principal socket event is dropped (no waiter yet). The chart / native filter then waits forever. Fix: register the waiter synchronously in the same tick as the202, before anyawait. Removed the now-unusedbaselineReadyhandle (the init poll-loop kickoff is unchanged).2. Embedded-guest subscriber key overflows its column.
superset.tasks.guest.get_current_guest_subscriber_key()returns"guest-"+ a 64-char SHA256 hex digest = 70 chars, buttask_subscribers.guest_keywasString(64)— so subscribing a guest-created async task would raise on Postgres / silently truncate on MySQL. Widened the column toString(128)in the model and the (feature-branch) migration7e2c9a4f1b83.Both were flagged as P1 in review. The third finding (ownership-less distributed-lock release) is pre-existing and touches a shared primitive used well beyond GTF, so it's handled in a separate PR.
TESTING INSTRUCTIONS
jest src/middleware/asyncEvent.test.ts— the acceleration test now delivers the completion in the same tick with no wait, proving the waiter registers synchronously (regression guard for the race).pytest tests/unit_tests/daos/test_tasks.py— a full-length (70-char) guest key round-trips throughadd_guest_subscriber.ADDITIONAL INFORMATION
task_subscribers.guest_keyin the existing7e2c9a4f1b83migration)