villebro opened a new pull request, #43768:
URL: https://github.com/apache/superset/pull/43768
### SUMMARY
Review-driven hardening of the GAQ→GTF epic, folded back onto `gaq-to-gtf`.
This
branch implements fixes for **12 findings** surfaced by a fresh end-to-end
review
(five parallel subsystem passes: coordination/locks, GTF task lifecycle,
async
chart-data, realtime/websocket, frontend) plus an independent external
review. Each
finding was verified against the code before fixing; every fix ships with
tests.
Both reviews independently concluded there is **no in-scope security boundary
violation** (per `SECURITY.md`): the model remains "an authorized route
creates a
task, the worker runs as the initiating principal, and task/list/result
state is
fetched through protected APIs; websocket messages are nudges or targeted
terminal
status, not authority."
#### Fixes (grouped by severity)
**HIGH**
- **Async worker rebuilt `g.form_data` with the wrong shape.** The worker set
`g.form_data` to the top-level slice form-data (no `queries`), but
`get_form_data()`'s no-request-context fallback and the Jinja macros
(`filter_values`/`get_filters`/`url_param`) read query-level filters from
`form_data["queries"][0]`. For a templated dataset this rendered empty
filters in
the worker, so it cached the wrong SQL under a `query_cache_key` that
**diverged
from the submit-time `task_key`** → a reschedule loop. Now the worker
reconstructs
a body-shaped `g.form_data` via the canonical
`set_query_context_form_data(query_context, …)`. (Regressed the fix
claimed in
step #43701; the previous test masked it with a body-shaped dict
`serialize_query`
never emits — replaced with a real round-trip assertion.)
- **Concurrent forced refresh re-ran synchronously on a web worker.** The
force
idempotency marker was keyed by a **client-minted** random nonce, so a
second
concurrent forced refresh (new nonce) joined the shared task, then forced
its own
synchronous read-back and recomputed the query in-process. The nonce is
now the
**task's own UUID** — server-assigned, already returned in the 202
`task_ids`. The
worker stamps each query with its task UUID and records the marker; the
client
threads each query's task id (index-aligned) as its per-query
`force_nonce` on the
synchronous read-back. Because the token is the task's identity, a
concurrent
refresh joining the same SHARED task reads back under the same id and does
not
re-execute. Drops client-side `nanoid` and the submit-path nonce threading.
**Important**
- **Cleanup-handler failure could flip a committed `SUCCESS` → `FAILURE`.**
Cleanup
runs in the executor's `finally`, *after* the `SUCCESS` commit; the write
used
`UpdateTaskCommand.set_status` with no compare-and-swap. It now routes
through a
conditional `InternalStatusTransitionCommand` (no-op on a terminal task)
and, when
the task already succeeded, records the cleanup-failure detail via a
properties-only update — preserving `SUCCESS`.
- **Timeout/self-fence gated on a stale session-bound entity.**
`_abort_locally`
read `self._task.properties_dict` (a construction-time snapshot) from a
background
timer/fence thread; `is_abortable` is written to the in-memory
`_properties_cache`
during execution. It could skip the abort (stranding the task
`IN_PROGRESS`) or
trigger a cross-thread session reload. Now gates on `_properties_cache`.
- **Terminal `FAILURE` wiped the whole properties column.**
`conditional_status_update`
replaces the JSON column, and the failure callers passed only
`{error_message}`,
dropping runtime fields and the structured `error_update(ex)` debug detail
(exception class + traceback). Failure writes now merge the executor's
property
cache with `error_update(ex)` via a new pure `merge_properties` helper
(also used
by `Task.update_properties`, DRY).
- **GAQ could be enabled without GTF via dynamic flag callbacks.** The "GAQ
implies
GTF" auto-enable only mutated the static flag map at `init_app`; an
`IS_FEATURE_ENABLED_FUNC`/`GET_FEATURE_FLAGS_FUNC` could resolve
GAQ=on/GTF=off, so
`.schedule()` raised instead of degrading. The rule is now enforced at
flag-resolution time, covering the callback paths.
**Medium**
- **DAG dependencies were ignored on the sync/inline path.**
`_execute_inline` ran
immediately regardless of `depends_on`; only the Celery path enforced the
gate. The
pure DAG *decision* (`unmet_prerequisite`) and *fail action* are extracted
into a
shared `superset/tasks/dependencies.py` that both paths call — fully DRY;
only the
*wait* action differs (async defers via `self.retry`; sync blocks on the
existing
`TaskManager.wait_for_completion`).
- **Websocket-enabled-but-unreachable hung charts for the full give-up
window.** With
`WEBSOCKET_ENABLE` on, the socket is the sole completion transport and a
deployed-but-down server left charts spinning ~10 min. The socket is now
self-healing and observable: exponential backoff + jitter with a cap (was
a fixed
5s loop), a reconnect-attempt counter, and a new connection-state signal
(`connecting`/`open`/`reconnecting`/`unhealthy`). On each `reconnecting`
transition
the client runs the socket-independent `status_changes` catch-up (so a
completion
during an outage is still observed); on `unhealthy` it settles pending
waiters with
a prompt, bounded error. No interval poll is reintroduced. A synchronous
`WebSocket` constructor failure now schedules a reconnect instead of
dead-ending.
- **Coordination listener/waiter resilience + KV lock-release atomicity.**
(a) The
baseline `stream_last_id` capture is guarded so a transient backend error
at
startup degrades to reading from `0-0` instead of killing the daemon /
aborting a
lock acquisition. (b) A transient `check()`/`on_signal()` error inside the
listen
loop retries with backoff instead of permanently terminating the one-shot
listener
(which would drop the awaited cancel/abort signal). (c) The KV
distributed-lock
release row-locks the entry (`SELECT … FOR UPDATE`) so the ownership check
and
delete are atomic against a concurrent expire+re-acquire — the KV
equivalent of the
Redis compare-and-delete.
**Low (hygiene)**
- Typed the task payload / `TaskPayloadPopover` (`Record<string, unknown>`)
and the
`TaskList` subscriber predicate (`TaskSubscriber`) — removing the last
`any`s in
the touched task-UI files.
- Added a `CheckConstraint` enforcing the "exactly one of `user_id` /
`guest_key`"
invariant on `task_subscribers` (in the branch's own migration + the model
`__table_args__`).
#### Deliberately deferred (both LOW; riskier/heavier than their value)
- Switching the `entity.changed` nudge id from the integer PK to the task
UUID — it
is cross-cutting to the entity-agnostic `useListViewResource` hook and the
Task API
row key, and the security review classed it a benign metadata
side-channel, not an
authorization bypass.
- Making `notify`'s `xadd`+`expire` atomic — requires extending the
coordination
backend abstraction (Lua/pipeline); the residual leak is a single
MAXLEN-trimmed
stream entry.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend correctness/resilience and typing changes, plus a
websocket-client
robustness change with no visual surface (a genuinely-down socket now
surfaces a
prompt chart error instead of a ~10-minute spinner).
### TESTING INSTRUCTIONS
Automated (all green except one pre-existing, unrelated failure — see below):
```bash
# Backend
pytest tests/unit_tests/tasks/ tests/unit_tests/coordination/ \
tests/unit_tests/distributed_lock/ \
tests/unit_tests/common/test_query_context_processor.py \
tests/unit_tests/common/test_query_serialization.py \
tests/unit_tests/charts/test_chart_data_api.py \
tests/unit_tests/feature_flag_test.py tests/unit_tests/daos/test_tasks.py
# Frontend
npm run test -- src/middleware/realtime.test.ts \
src/middleware/asyncEvent.test.ts src/components/Chart/chartActions.test.ts
```
Manual (end-to-end, with `GLOBAL_ASYNC_QUERIES` on and
`DISTRIBUTED_COORDINATION_CONFIG` set):
1. Load a dashboard with a **Jinja-templated dataset** (`{{
filter_values(...) }}` /
`{{ get_filters(...) }}`) under `async_mode`; confirm charts resolve with
the
correct filtered SQL and there is **no reschedule loop**.
2. **Double-force** a chart (two quick refreshes): confirm a single warehouse
execution and both refreshes read back from cache.
3. With `WEBSOCKET_ENABLE` on, **stop the websocket server** while a chart
is loading:
confirm it surfaces a prompt error rather than a ~10-minute hang; restart
the
server and confirm reconnect + reconcile.
4. Cancel/timeout a running async chart on a cancellable engine (e.g.
Postgres) and
confirm the terminal task still carries the structured error detail.
> **Pre-existing failure (not from this branch):**
>
`tests/unit_tests/tasks/test_deletion_retention.py::test_default_celery_config_registers_daily_purge`
> fails on this branch **with these changes stashed** — it depends on a local
> `superset_config.CeleryConfig` override, unrelated to this work.
> **CI note:** the frontend type-check may fail locally on a **stale,
gitignored**
> `packages/superset-ui-core/lib/**/TableCollection/index.d.ts` (predates
the branch's
> `TableCollection` prop additions — same class as the Butterfly `TS6305`
errors). The
> source type-checks clean; a `npm run plugins:build` refreshes the
artifact. oxlint /
> oxfmt / backend hooks pass.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [x] Required feature flags: `GLOBAL_ASYNC_QUERIES` (auto-enables
`GLOBAL_TASK_FRAMEWORK`); optional `WEBSOCKET_ENABLE` for realtime transport
- [x] Changes UI (task payload/subscriber typing; websocket-client behavior
— a down socket surfaces an error instead of spinning)
- [x] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [x] Migration is atomic, supports rollback & is backwards-compatible —
adds a `CheckConstraint` to the branch's existing (unreleased)
`task_subscribers` migration; downgrade drops it
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] 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]
