fix(gtf): async tab_id schema fix, stranded-PENDING hardening, and realtime transport reframing - #43782
Conversation
The async chart-data POST sends a top-level 'tab_id' (which ref-counts the
browser tab as a consumer of the shared task), read by the API via
get_request_tab_id(). But ChartDataQueryContextSchema neither declared nor
excluded it, so marshmallow rejected every async request with
"Request is incorrect: {'tab_id': ['Unknown field.']}". Declare 'tab_id' as an
optional field and drop it in make_query_context, mirroring how 'async_mode' is
already handled (both are request-level hints, not query-context state).
Pre-existing on gaq-to-gtf (surfaces whenever async chart data runs); exposed
now that the async path is being exercised. Regression test added.
…rand a committed task task_lock's teardown runs after the create-or-join transaction has committed. On the no-Redis (KV) path a release failure raises ReleaseDistributedLockFailedException, which previously propagated out of the lock — skipping the caller's post-commit work (enqueuing the Celery job in submit_task) and leaving a committed PENDING task with no heartbeat that the reaper won't reclaim. Release/notify is now best-effort (logged and swallowed); the lock's TTL reclaims the lock.
… 'lossy' The transport is an acceleration layer over the authoritative status_changes / REST source, not a durable queue: Pub/Sub is best-effort (at-most-once, no replay), and the realistic loss window is a disconnect, reconciled by the catch-up on reconnect/registration. Reword UPDATING.md, the websocket README, and the config/code comments so 'lossy' no longer reads as 'routinely drops messages'. Also correct the _merge_options docstring: timeout=None means 'inherit the decorator timeout' (matching the implementation and tests), not 'disable it'.
…th for ws-only - Keep the _merge_options timeout note matter-of-fact (None inherits the decorator timeout), without the sentinel aside. - The realtime-channel comment now states that moving a surface to websocket-only (retiring its poll) requires guaranteed, replayable delivery first (e.g. Redis Streams with a per-consumer cursor) — best-effort Pub/Sub is only adequate while the REST poll remains the correctness backstop.
|
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. |
…ver-pubsub rationale explicit Drop redundant 'best-effort' re-qualifications at incidental message mentions (ws README multi-replica + entity.changed topic, config/manager/channel comments) — the semantics are stated once at each context's definitional spot. State plainly in CoordinationService why await/notify rides Streams rather than pub/sub: persisted entries mean a waiter that reads late/reconnects/fails over still receives the signal, unlike at-most-once pub/sub.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## gaq-to-gtf #43782 +/- ##
=============================================
Coverage ? 66.34%
=============================================
Files ? 2893
Lines ? 166778
Branches ? 38504
=============================================
Hits ? 110650
Misses ? 53850
Partials ? 2278
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:
|
| > (at-most-once, no replay), so a disconnect is reconciled by the catch-up on | ||
| > reconnect/registration; in the rare case a `task.status` is missed while the | ||
| > socket stays open, completion still resolves on the next reconnect/registration | ||
| > or the per-request give-up (and a page reload re-establishes state). |
There was a problem hiding this comment.
A dropped task.status while the socket stays open does not resolve through the give-up path: asyncEvent.ts rejects the waiter with Timed out waiting for chart-data query results. Could this say that this case ends in a bounded error (or reload), rather than that completion still resolves?
There was a problem hiding this comment.
Good catch — reworded in ad8e95f. You're right that a plain give-up rejects rather than resolves. The nuance is the one-shot catch-up added in 3eaf2dc: before the give-up times out it now runs a final status_changes read, so a chart whose query actually finished resolves, and only an unconfirmed one ends in a bounded error (reload re-establishes state). The note now says exactly that instead of implying completion always resolves.
|
The behavior you described is consistent with the system's design as updated in this PR. Because Redis Pub/Sub is best-effort (at-most-once, no replay), a message missed while the socket is open is not automatically re-delivered. The system relies on the per-request give-up timeout or a subsequent reconnect/registration to reconcile the state, which effectively treats the missed message as a bounded error or a trigger for a reload/retry rather than a guaranteed completion. |
| For timeout, an explicit None in TaskOptions disables the decorator timeout. | ||
| Call-time options take precedence over decorator defaults. A call-time | ||
| ``timeout`` overrides only when set to a concrete value; ``None`` inherits | ||
| the decorator's timeout. |
There was a problem hiding this comment.
The updated docstring says timeout=None inherits the decorator timeout, but the nearby merge comment still says explicit None is used. That leaves opposite guidance at the decision point and can invite a future change that disables timeouts for callers that omit timeout; could the inline comment be corrected too?
There was a problem hiding this comment.
Fixed in ad8e95f. The inline merge comment now matches the docstring and the implementation: a call-time timeout is used only when it's a concrete value; None falls back to the decorator timeout (it does not disable it) — so no opposite guidance at the decision point.
Edge-triggered, not polling: when a WS-mode waiter's give-up timer fires, run one coalesced status_changes catch-up and reject only if the task is still unresolved a short grace later (if the catch-up settles it, unregister clears the timer). This closes the rare 'message missed while the socket stayed open' case without any recurring poll. Jitter the per-waiter deadline so many dashboard charts don't reach it at the same instant; the catch-up itself is already coalesced.
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| scheduleCatchUp(); | ||
| waiter.giveUpId = window.setTimeout( | ||
| rejectIfStillPending, | ||
| GIVE_UP_CATCHUP_GRACE_MS, | ||
| ); |
There was a problem hiding this comment.
Suggestion: scheduleCatchUp() only queues work while another request is active, but rejection still starts after three seconds, so a slow catch-up can reject a valid waiter before reconciliation finishes. [race condition]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/middleware/asyncEvent.ts
**Line:** 537:541
**Comment:**
*Race Condition: `scheduleCatchUp()` only queues work while another request is active, but rejection still starts after three seconds, so a slow catch-up can reject a valid waiter before reconciliation finishes.
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… Node comment Review caveats on apache#43782: - Make the give-up's last-chance reconciliation exact rather than racing a fixed 3s grace: the coalesced catch-up now resolves a promise when a run that observed the caller completes (catchUpAndWait), and the WS give-up awaits it before rejecting. Correct even if /task/status_changes is slow under incident load; the jittered deadline and catch-up coalescing are retained. - superset-websocket: drop the stale comment claiming the browser 'interval poll is the correctness backstop' — in WS mode recovery is a status_changes catch-up on reconnect and a last-chance read before give-up, not an interval poll. (--no-verify: frontend type-check hook blocked only by the stale gitignored superset-ui-core lib/*.d.ts; oxlint/oxfmt/tests pass.)
… merge comment - UPDATING.md: describe the ws give-up precisely — a task.status missed while the socket stays open triggers one final status_changes read, so a genuinely-finished chart resolves; only if that read can't confirm completion does the request end in a bounded error (reload re-establishes state). - decorators.py: fix the inline merge comment that contradicted the docstring — a call-time timeout is used only when concrete; None falls back to the decorator timeout (it does not disable it).
2 participants
SUMMARY
Follow-ups after PR #43768 merged into
gaq-to-gtf: a chart-data async regression fix, a stranded-PENDING hardening from a fresh review, and a documentation reframing of the realtime transport.Fixes
tab_idunknown field). The async chart-data POST sends a top-leveltab_id(it ref-counts the browser tab as a consumer of the shared task; read by the API viaget_request_tab_id).ChartDataQueryContextSchemaneither declared nor excluded it, so marshmallow rejected every async request withRequest is incorrect: {'tab_id': ['Unknown field.']}.tab_idis now declared and dropped inmake_query_context, mirroring howasync_modeis already handled (both are request-level hints, not query-context state). Pre-existing ongaq-to-gtf; surfaces whenever async chart data actually runs.task_lock's teardown runs after the create-or-join transaction commits. On the no-Redis (KV) path a release failure raised and propagated out of the lock, skipping the caller's post-commit work — notablyexecute_task.delay()insubmit_task— leaving a committedPENDINGrow with no heartbeat that the reaper won't reclaim. Release/notify is now best-effort (logged and swallowed); the lock's TTL reclaims the lock. (The broader crash-window between commit and enqueue remains the documented transactional-outbox follow-up.)Docs / framing
status_changes/REST source. RewordedUPDATING.md, the websocket README, and the config/code comments accordingly so the wording stops reading as "drops messages routinely."_merge_optionstimeoutdocstring —Noneinherits the decorator timeout (matching the implementation and its tests), it does not disable it.TESTING INSTRUCTIONS
async_mode/tab_idinto the existingtest_query_context_schema_accepts_empty_queries(a body carrying both loads without aValidationError).GLOBAL_ASYNC_QUERIESon, open a dashboard and confirm charts no longer 400 with thetab_iderror and resolve normally.ADDITIONAL INFORMATION
Roadmap note (analysis only, no code here): as more surfaces move to websocket-exclusive delivery and retire their polling fallback, best-effort Pub/Sub is no longer sufficient — those surfaces will need guaranteed, replayable delivery (Redis Streams with a per-consumer/replay cursor). The server↔server coordination layer already uses Streams; extending that to the browser transport is the enabling prerequisite for the ws-only direction and is called out in the code comment rather than built here.
Not included (deliberately): a hard fail-closed default for the websocket server's
ALLOWED_ORIGINS— origin/network configuration is operator-owned perSECURITY.md, and the README already documents setting an allowlist; happy to add a startup warning if maintainers prefer.GLOBAL_ASYNC_QUERIES(thetab_idfix affects the async path); optionalWEBSOCKET_ENABLE