villebro opened a new pull request, #43799:
URL: https://github.com/apache/superset/pull/43799
### SUMMARY
Follow-ups from a full re-review of the `gaq-to-gtf` epic (#43407). Three
findings; no in-scope security-boundary violation under `SECURITY.md`.
**1. High — queued-task aborts didn't wake websocket-mode waiters.**
`TaskManager.publish_completion()` is the only producer of the targeted
`task.status` message a chart waiter observes, but **no abort→terminal
transition called it**:
- `TaskDAO.abort_task()` moves a `PENDING` task straight to `ABORTED`.
- The pre-claim `InternalStatusTransitionCommand(ABORTED)` in `scheduler.py`
and the inline decorator (`_abort_if_preaborted`) emit only
`entity.changed`/required-by nudges.
With the websocket enabled the client does **not** run a recurring poll
(`asyncEvent.ts`), so cancelling a *queued* chart task, a last-tab cancel, or
an admin force-cancel left the waiting chart unaware until the ~10-minute
stale/give-up reconciliation.
Fix:
- `CancelTaskCommand` publishes completion when a cancel aborts straight to
a terminal state (`PENDING → ABORTED`). The `ABORTING` case is unchanged — the
worker finalizes `ABORTED` and publishes completion once its abort handlers run.
- The two pre-claim executor paths (`scheduler.execute_task`,
`decorators._abort_if_preaborted`) publish completion **when they performed the
transition** (guarded on the CAS result, so a cancel that already published for
the same task isn't echoed).
**2. High/conditional — embedded guest async could return a 202 the guest
can't resolve.**
`_should_run_async()` treated a guest subscriber key as sufficient async
eligibility, but completion is observed through `GET
/api/v1/task/status_changes` (gated by `can_read Task`) and, when enabled, the
websocket JWT (gated by `can_read Realtime`). The default guest role (`Public`,
`PUBLIC_ROLE_PERMISSIONS`) has neither, so an embedded guest got a `202` it
could never poll or observe. (An authenticated Gamma user has `can_read Task`
by default — it's neither admin-only nor Gamma-excluded — so this is
guest-specific.)
Fix: gate async eligibility on `security_manager.can_access("can_read",
"Task")` so a principal that can't observe completion transparently falls back
to the synchronous `200` flow. Documented the guest-role grants required to
enable embedded async in `UPDATING.md`.
**3. Low — stale docs after hardening.**
- `async_queries.py` docstring said engine-level query cancellation is
"outside this chart async path", though the worker registers
`ctx.on_abort(_cancel)` via `_capture_query_cancellation`.
- The cancel command's `action_taken` property and the cancel API docstring
listed only `aborted`/`unsubscribed`, omitting `detached`.
- The top-level `force_nonce` schema description still described the
pre-task-UUID one-nonce-per-refresh model (the per-query field already
documents the task-UUID semantics).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A.
### TESTING INSTRUCTIONS
- `pytest tests/unit_tests/charts/test_chart_data_api.py -k should_run_async`
- `pytest tests/integration_tests/tasks/commands/test_cancel.py -k
"pending_task_aborts or in_progress_abortable"` (requires the integration test
DB)
- Manual: with `GLOBAL_ASYNC_QUERIES` + `WEBSOCKET_ENABLE`, submit a slow
async chart, cancel it (or admin force-cancel) while still `PENDING`, and
confirm the chart resolves to a cancelled/error state immediately rather than
hanging.
- Manual: open an embedded dashboard as a guest on the default `Public` role
with async enabled and confirm chart data returns synchronously (no unresolved
202); grant the guest role `can_read Task` and confirm it goes async.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
---
Part of the GAQ → GTF migration epic (#43407).
--
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]
