villebro opened a new pull request, #43701: URL: https://github.com/apache/superset/pull/43701
### SUMMARY A full design + implementation review of the `gaq-to-gtf` epic (7 parallel subsystem reviewers across coordination, the GTF core, task commands/DAO/models, the chart-data cutover, the websocket transport, the frontend, and security) turned up one HIGH regression and a set of MEDIUM edge-case correctness items. The security review came back clean (no in-scope boundary violation). This PR fixes the confirmed, contained findings; each is covered by a test. **Why this matters.** The epic is architecturally sound, but the review found a real regression that would break a whole class of dashboards and several "something fired late/again after the work finished" edge cases that undermine the correctness properties the framework exists to guarantee. Better to close them before the epic merges to master. #### Fixed - **HIGH — async chart-data dropped `g.form_data`.** `execute_chart_query` never re-established form data in the worker (the old GAQ task called `set_form_data`). `get_form_data()` falls back to `g.form_data` "for cache warmup and async queries", and Jinja `filter_values`/`get_filters`/`get_time_filter` read through it — so a Jinja-templated dataset rendered **empty filter values** in the worker, both executing/caching the **wrong SQL** and computing a `query_cache_key` that **diverges from the submit-time `task_key`**, so the client's re-request never reads back the cache the task wrote (cache-miss reschedule loop). Fix: re-establish form data from the serialized payload. Every existing unit test mocked `load_serialized_query`, so nothing exercised this path — added regression tests asserting `set_form_data` is called with the payload's form data. (A templated-dataset end-to-end integration test is the durable guard and is called out as a follow-up; the existing async integ ration test is `@unittest.skip`-ped as MySQL-flaky.) - **MEDIUM — coordination signal listener died on a transient `ConnectionError`.** `_read_stream` caught only `(RedisTimeoutError, OSError)`; a `redis.ConnectionError` escaped to `_run_listen_loop`'s broad except, which logged "crashed" and let the abort/signal listener thread exit permanently, so a live task might not honor a user-cancel until its GTF timeout. Fix: degrade any `RedisError` in the blocking read to a predicate re-check (with a bounded backoff so it can't busy-spin) — this fixes both the background listener and `wait_for_signal`, matching the "reconnect/failover-resilient" contract. - **MEDIUM — keepalive turned "reconcile on drop" into a periodic full reconcile.** The sliding-window keepalive tears down + reopens a *healthy* socket every ~0.6×lifetime, firing the open-listeners — so every list view did a full displayed-row re-fetch each cycle, re-adding the load the no-polling design set out to avoid. Fix: tag each socket open with a `reason` (`initial` / `reconnect` / `keepalive`); list views reconcile only on a real `reconnect`. Chart-data catch-up still runs on all opens (it's cheap and coalesced, and covers the sub-second keepalive handover). - **MEDIUM — poll mode never gave up on a persistent fetch error.** The stale-timeout give-up lived only in the success branch; the `catch` just logged and rescheduled at the eager 500ms interval, and `giveUpId` is WS-mode-only — so a websocket-off chart could spin forever (hammering a degraded endpoint) if `status_changes` failed persistently. Fix: the error path now ages toward the same give-up and backs off. - **MEDIUM — sync inline path could flip a finished task to ABORTED.** `_execute_inline` never called `ctx.mark_execution_completed()` (the async executor does), so a cancel signal detected after the body completed still ran the abort branch. Fix: mark completion after the body in both the success and exception paths, mirroring the async executor. #### Deferred (follow-ups, with rationale) - **Multi-host status-poll clock skew** (`get_statuses_changed_since` uses host-local `changed_on >= cursor`). Real for multi-host, but the fix (move the cursor to the DB clock) is invasive and touches cursor semantics; matters mainly in websocket-off deployments. Tracked as a follow-up. - **Timeout-path completion gating + dead `_refresh_task` fallback** (GTF core): narrow windows, and chart-data async uses the guarded path. Deferred to keep this PR focused on the confirmed, contained fixes. - **Async failure traceback not persisted** (properties replaced wholesale on the FAILURE transition): logged today, minor. - **Insecure-by-default websocket cookie flags** (`SameSite=None` without `Secure`; origin allowlist off): operator configuration; worth a docs/default hardening pass separately. - **Authenticated co-subscriber name disclosure**: out-of-scope per `SECURITY.md` (optional privacy hardening). ### TESTING INSTRUCTIONS - Python: `pytest tests/unit_tests/tasks tests/unit_tests/coordination` (492 pass; new coverage for `set_form_data` re-establishment, the coordination connection-error degrade, and the sync `mark_execution_completed`). - Frontend: `npm run test -- asyncEvent realtime hooks` (new coverage for poll-mode give-up on persistent error, list-view no-reconcile on keepalive, and the keepalive-vs-reconnect reason). - ruff/mypy/oxfmt/oxlint green on changed files; changed-file tsc clean (the frontend type-check hook has pre-existing unrelated `gaq-to-gtf` failures). ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration - [ ] 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]
