villebro opened a new pull request, #43782:
URL: https://github.com/apache/superset/pull/43782
### 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**
- **Async chart-data 400 (`tab_id` unknown field).** The async chart-data
POST sends a top-level `tab_id` (it ref-counts the browser tab as a consumer of
the shared task; read by the API via `get_request_tab_id`).
`ChartDataQueryContextSchema` neither declared nor excluded it, so marshmallow
rejected **every** async request with `Request is incorrect: {'tab_id':
['Unknown field.']}`. `tab_id` is now declared and dropped 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 actually runs.
- **Stranded committed PENDING task (review finding).** `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 — notably `execute_task.delay()` in `submit_task` —
leaving a committed `PENDING` row 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**
- **Reframe the realtime transport as best-effort, not "lossy."** Redis
Pub/Sub to a connected, healthy socket doesn't routinely drop messages; the
realistic loss window is a *disconnect*, which the catch-up on
reconnect/registration reconciles, over an authoritative `status_changes`/REST
source. Reworded `UPDATING.md`, the websocket README, and the config/code
comments accordingly so the wording stops reading as "drops messages routinely."
- **Point at the real trajectory.** The realtime-channel comment now states
plainly 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. (Analysis-only for now; see below.)
- **Minor:** correct the `_merge_options` `timeout` docstring — `None`
inherits the decorator timeout (matching the implementation and its tests), it
does not disable it.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/charts/data/test_empty_query_context.py \
tests/unit_tests/tasks/test_timeout.py
tests/unit_tests/tasks/test_decorators.py \
tests/unit_tests/distributed_lock/
```
- The schema regression is covered by folding `async_mode`/`tab_id` into the
existing `test_query_context_schema_accepts_empty_queries` (a body carrying
both loads without a `ValidationError`).
- Manual: with `GLOBAL_ASYNC_QUERIES` on, open a dashboard and confirm
charts no longer 400 with the `tab_id` error 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 per
`SECURITY.md`, and the README already documents setting an allowlist; happy to
add a startup warning if maintainers prefer.
- [ ] Has associated issue:
- [x] Required feature flags: `GLOBAL_ASYNC_QUERIES` (the `tab_id` fix
affects the async path); optional `WEBSOCKET_ENABLE`
- [ ] Changes UI
- [x] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351)) — no
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
