feat(gtf): per-tab websocket task-status fanout - #43689
Conversation
A browser tab now receives realtime task-status only for the tasks it is
watching, instead of every task any of the user's tabs watches.
- TaskSubscriptionPolicy gains routing_channels(task) (default None); chart-data
returns its per-tab consumer keys (else principal-grain fallback).
- publish_task_status computes routing keys and publishes {task_id,status,channels}
(was subscribers:[{principal_type,sub}]); the ws server delivers each to
realtime:<key>.
- ws server: parse tab_id at upgrade and dual-register the socket under its
principal channel and user:<id>:<tabId>; broadcastToAll now dedupes by socket.
- frontend advertises getTabId() on the ws connect URL.
Auth is unchanged: one user-specific cookie authorizes the principal; the tab id
rides the connect URL, and the per-tab channel is derived from the authorized
principal channel so it can never cross principals.
dosubot
Bot
added
change:backend
|
Bito Automatic Review Skipped - Branch Excluded |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## gaq-to-gtf #43689 +/- ##
==============================================
+ Coverage 75.02% 79.35% +4.32%
==============================================
Files 2892 2897 +5
Lines 166543 167175 +632
Branches 38473 38474 +1
==============================================
+ Hits 124953 132663 +7710
+ Misses 39044 32017 -7027
+ Partials 2546 2495 -51
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:
|
✅ 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}
| # task-status message reaches only the tabs watching this task. Empty -> | ||
| # None so a chart task with no recorded tab (all detached, or a no-tab | ||
| # caller) falls back to principal-grain fanout instead of dropping it. | ||
| return self._consumers(task) or None |
There was a problem hiding this comment.
Suggestion: The routing list is treated as authoritative without checking current subscriber rows. A no-tab unsubscribe leaves the existing per-tab entries intact while removing the principal subscriber, so a later status transition can still be routed to sockets for a principal that is no longer subscribed. Reconcile the recorded consumer entries with active subscribers, or clear all entries for that principal when the principal-grain unsubscribe path is used. [security]
Severity Level: Major ⚠️
- ⚠️ Legacy cancellation leaves stale tab routing state.
- ⚠️ Unsubscribed tabs can receive task-status events.
- ⚠️ Task private properties retain obsolete consumer entries.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/tasks/async_queries.py
**Line:** 134:134
**Comment:**
*Security: The routing list is treated as authoritative without checking current subscriber rows. A no-tab unsubscribe leaves the existing per-tab entries intact while removing the principal subscriber, so a later status transition can still be routed to sockets for a principal that is no longer subscribed. Reconcile the recorded consumer entries with active subscribers, or clear all entries for that principal when the principal-grain unsubscribe path is used.
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 78ec946. on_unsubscribe now clears all of the principal's tab entries on the principal-grain (no-tab_id) unsubscribe path, so a later status transition can no longer route to that principal's tab channels after it has left; the per-tab (client_ref) path is unchanged. Added a regression test (test_consumer_policy_no_client_ref_clears_that_principals_tab_entries).
Note on scope: even before the fix this was stale delivery to the same principal's own JWT-bound socket, not a cross-principal leak (a user:5:<tab> key only ever routes to user 5's socket) — but delivering to a principal that explicitly unsubscribed is still wrong, so the fix stands.
|
The flagged security issue concerns stale routing state in the task-status fanout mechanism. When a principal unsubscribes from a task, the system must ensure that any per-tab routing entries associated with that principal are also cleared to prevent unauthorized or stale status updates from being delivered to those tabs. To resolve this, you should ensure that the Would you like me to fetch all comments on this PR to validate the rest of the findings and implement a comprehensive fix? superset-core/src/superset_core/tasks/subscription.py |
| Array.isArray(candidate.channels) && | ||
| candidate.channels.every( | ||
| channel => typeof channel === 'string' && channel.length > 0, | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Suggestion: Requiring channels makes the websocket server reject the previous {task_id, status} task-status message format. During a rolling deployment, or when another publisher still emits the legacy shape, every status event is logged as invalid and dropped instead of being delivered at principal grain as before. Accept the legacy shape with principal-grain routing, or coordinate an atomic producer/consumer protocol migration. [api mismatch]
Severity Level: Major ⚠️
- ❌ Async chart task-status events drop during rolling upgrades.
- ⚠️ Browsers rely on polling backstop for completion updates.
- ⚠️ Mixed worker/websocket versions lose realtime delivery.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-websocket/src/index.ts
**Line:** 395:399
**Comment:**
*Api Mismatch: Requiring `channels` makes the websocket server reject the previous `{task_id, status}` task-status message format. During a rolling deployment, or when another publisher still emits the legacy shape, every status event is logged as invalid and dropped instead of being delivered at principal grain as before. Accept the legacy shape with principal-grain routing, or coordinate an atomic producer/consumer protocol migration.
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.
This is intentional and I don't think transitional dual-shape acceptance is warranted here. The subscribers→channels payload is an internal Redis wire contract between superset/tasks/manager.py (the sole producer) and this server (the sole consumer), and both shapes only ever exist within this GAQ→GTF epic — the task-status fanout feature is entirely new in the epic and the epic merges to master atomically (umbrella #43407). So there is no released/prior producer emitting the legacy {subscribers} shape in the wild: a real cluster upgrading from pre-epic has old app pods that don't emit task-status at all (feature absent) and new pods that emit {channels}, which this server reads.
The remaining window — an operator running a mid-epic intermediate build where the app pod and ws pod roll at slightly different times — is documented in the PR description: deploy the Flask app and ws server together; a mismatch degrades gracefully to the client's polling backstop (no crash, no leak). Adding legacy {subscribers} acceptance would be dead defensive code (plus the principalChannel-mapping + isTaskStatusSubscriber guard I just removed) guarding against a shape that never ships publicly, so I'd rather keep the wire contract single-shape. Happy to add it if you'd prefer belt-and-suspenders.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not require transitional dual-shape acceptance for internal Redis wire contracts when the producer and consumer are introduced and deployed together as part of the same atomic feature rollout; treat the new payload shape as the sole supported contract.
Applied to:
superset-websocket/**
💡 To manage or update this instruction, visit: CodeAnt AI Settings
…ain unsubscribe A no-tab (principal-grain) unsubscribe removed the principal's subscriber row but left its per-tab consumer entries in private.task.consumers, so a later status transition could still route task-status to that principal's tab channels after it had unsubscribed. Clear all of the principal's tab entries on the principal-grain unsubscribe path; per-tab (client_ref) unsubscribe is unchanged. Addresses review feedback on apache#43689.
|
CI note: the failing checks here ( |
| // `url` stays stable for connectRealtime's idempotency check. | ||
| let connectUrl = url; | ||
| try { | ||
| const parsed = new URL(url); |
There was a problem hiding this comment.
A root-relative websocket endpoint (for example /superset-ws behind a same-origin proxy) is valid for WebSocket, but new URL(url) rejects it and this catch connects without tab_id. Chart requests still subscribe under the tab-specific key, so their status events have no matching socket and only polling notices completion. Could this resolve against the page URL before adding the parameter?
There was a problem hiding this comment.
Good catch — fixed in a371d21. openSocket now builds the connect URL with new URL(url, window.location.href), so a root-relative endpoint (e.g. /superset-ws behind a same-origin proxy) resolves against the page and still carries tab_id; an http(s) result is normalized to ws(s). Absolute ws(s):// URLs are unaffected (an absolute URL ignores the base). Added a test (resolves a root-relative ws url against the page and adds tab_id).
| let connectUrl = url; | ||
| try { | ||
| const parsed = new URL(url); | ||
| parsed.searchParams.set('tab_id', getTabId()); |
There was a problem hiding this comment.
When a duplicated tab receives TAB_ID_DENIED, useTabId replaces its session-storage ID, but this socket remains registered under the old ID until it happens to reconnect. New chart subscriptions use the replacement ID, so their tab-targeted status events cannot reach this active socket. Should the tab-ID change trigger a realtime reconnect or otherwise keep the socket registration in sync?
There was a problem hiding this comment.
Fixed in a371d21 — the tab-id change now triggers a realtime reconnect so the socket re-registers under the new per-tab channel. useTabId exposes subscribeTabIdChange and calls it on the TAB_ID_DENIED reassignment; realtime.ts subscribes and, when a socket is active, tears down and reopens (which re-reads getTabId()), so new tab-targeted status events reach the current socket instead of only being noticed by polling. Test: reconnects with the new tab id when the tab id changes.
Review (sadpandajoe) on apache#43689: - realtime.ts: resolve the ws URL against the page URL so a root-relative endpoint (e.g. /superset-ws behind a same-origin proxy) still connects with tab_id (new URL(url) alone rejected it and dropped the tab id -> tab-targeted status never reached the socket). http(s) is normalized to ws(s). - realtime.ts: reconnect when this tab's id changes (a duplicated tab reassigned on a TAB_ID_DENIED collision) so the socket re-registers under the new per-tab channel; useTabId now exposes subscribeTabIdChange and notifies on reassignment. Failing test: - test_reap fixture wrote the engine cancel handle at top-level properties, but the apache#43678 task-state hierarchy moved cancel_query_id/cancel_database_id into private.task (where the reaper reads them); fixed the helper to use update_task_private so the orphan-query-cancel assertion passes. Tests: realtime root-relative-url + tab-id-change reconnect; shim mock gains subscribeTabIdChange. All frontend + touched python unit tests green.
SQL_LAB_GTF_MIGRATION_ANALYSIS.md is a local design-scratch doc (like GAQ_TO_GTF_EPIC.md); it does not belong in this PR. Untracked and added to .git/info/exclude; the file stays on disk locally.
|
On the two failing tests:
|
test_related_subscribers is the only task API test that switches users within a
single method (gamma then admin). The shared test client keeps one session and
login() just POSTs /login/, which redirects without re-authenticating when a
user is already logged in — so admin's request ran with gamma's scoping and the
subscriber dropdown returned only gamma (assert {1,2} <= {2} failed) even though
the fixture correctly auto-subscribes admin to its tasks. Log out before each
login so the admin switch takes effect.
|
Root-caused and fixed The captured log confirmed the fixture auto-subscribes admin to its tasks ( Fix: |
2 participants
SUMMARY
Delivers per-tab realtime
task-statusfanout for async chart-data, against theumbrella branch (#43407). Builds on the per-tab subscription refcount from #43685
(
private.task.consumers).Problem. Realtime
task-statusis delivered at principal grain:publish_task_statusnames the task's subscriber principals and thesuperset-websocketserver fans each out torealtime:user:<id>, so every taba user has open receives status for every task any of their tabs is watching,
and the browser discards the irrelevant ones by
task_id. Now that each tab'ssubscription is tracked per-tab, we can route
task-statusto only the tab thatis watching a given task.
This realizes the three channel tiers:
realtime:user:<id>:<tabId>(new: per-tabtask-status)realtime:user:<id>(all of a user's tabs — kept fornon-per-tab tasks and future per-user notices)
entity-changes:*broadcast tierDESIGN
cookie authorizes the principal (
channel: user:<id>, identity-bound — thetoken integrity check is kept); each tab opens its own
WebSocketandadvertises its
getTabId()on the connect URL (?tab_id=…), not in thetoken. The server derives the per-tab channel by prefixing the authorized
principal channel, so a client-supplied
tab_idcan never address anotherprincipal's sockets. No per-tab cookies, no bearer token.
user:<id>(principal — all-tabs/broadcast delivery + the per-principalconnection cap) and, when a
tab_idis present, alsouser:<id>:<tabId>. Onesocket id, two channel keys.
task-statuswire payloadchanges from
subscribers: [{principal_type, sub}]tochannels: [str]— theproducer computes the exact routing keys and the ws server just delivers to
realtime:<key>(this also simplifies the server: no re-derivation/validationof principal keys in the fanout path).
TaskSubscriptionPolicy.routing_channels(task) -> list[str] | None(concretedefault
None). Chart-data returns itsconsumerslist (already the per-tabkeys); any task type without a policy — or a chart task with no recorded tab —
falls back to principal-grain keys from
get_subscriber_principals(unchangedbehavior for non-chart tasks).
broadcastToAllnow iterates the unique socketregistry (via an extracted
sendToSockethelper) instead of per-channel, so adual-registered socket receives each entity-change nudge exactly once.
BEFORE/AFTER
chart's
task-status; the client filters bytask_id.task-statusonly for its own tasks, onrealtime:user:<id>:<tabId>. Entity-change list nudges still reach every tab.TESTING INSTRUCTIONS
pytest tests/unit_tests/tasks/test_manager.py tests/unit_tests/tasks/test_async_queries.pynpm --prefix superset-websocket run test && npm --prefix superset-websocket run lintnpm run test -- realtimetabs; confirm each tab's
task-statusarrives only on its ownrealtime:user:<id>:<tabId>channel (inspect ws frames) while entity-changelist nudges reach both tabs exactly once; stop the ws server and confirm
polling still resolves charts.
ADDITIONAL INFORMATION
Wire-contract change (
subscribers→channels) is a coordinated Python↔Nodechange with no version field, so deploy the Flask app and the
superset-websocketserver together. During a mismatched rolling upgrade each side drops the other's
task-statuspayload and degrades to client polling (no crash, no leak).GLOBAL_ASYNC_QUERIES/GLOBAL_TASK_FRAMEWORK; optionalWEBSOCKET_ENABLE