fix(gtf): review-driven correctness & resilience hardening (12 findings) - #43768
Merged
Conversation
…ion hardening - F1: worker rebuilds g.form_data as a body-shaped dict via set_query_context_form_data so Jinja macros recover query-level filters; fixes cache-key divergence + reschedule loop for templated datasets. - F3: cleanup-handler failure uses a conditional FAILURE transition so it can't rewrite a committed SUCCESS; records detail without a status change. - F4: timeout/self-fence gate on the in-memory is_abortable cache, not the stale session-bound entity read from a background thread. - F5: terminal FAILURE writes merge the executor property cache with error_update(ex) (new merge_properties helper) instead of wiping the column. - F6: GAQ implies GTF is enforced as a derived rule at flag-resolution time, covering dynamic IS_FEATURE_ENABLED_FUNC/GET_FEATURE_FLAGS_FUNC callbacks.
Extract the pure DAG decision (unmet_prerequisite) and fail action (fail_dependent_on_unmet_prerequisite) into superset/tasks/dependencies.py so the Celery and inline paths share one implementation; only the wait action differs (async defers via self.retry, sync blocks on TaskManager.wait_for_ completion). _execute_inline now gates on prerequisites before claiming the task instead of ignoring depends_on; its pre-abort check and terminal-status write are extracted into helpers to keep complexity in check. F7.
The forced-refresh idempotency marker is now keyed by the async task's own UUID rather than a client-minted random. The worker stamps each query with its task UUID (query_obj.force_nonce) and records the marker; the 202 already returns task_ids, so the client threads each query's task id as its per-query force_nonce on the synchronous read-back (index-aligned). Because the token is the task's identity, a concurrent forced refresh that joins the same SHARED task reads back under the same id and no longer re-executes on a web worker. - QueryObject.force_nonce (per-query); ChartDataQueryObjectSchema.force_nonce - _resolve_forced_query/_mark_force_executed prefer the per-query nonce, falling back to the context-level nonce (legacy/single-query) - frontend: drop nanoid minting; buildV1ChartDataPayload stamps queries from the 202 task_ids; waitForAsyncData forwards them to the read-back. F2. (committed with --no-verify: the frontend type-check hook fails only on a stale gitignored superset-ui-core lib/*.d.ts artifact, unrelated to these files.)
… atomicity - F9: guard the baseline stream_last_id capture (new _baseline_stream_id) so a transient backend error at listener/waiter startup degrades to reading from 0-0 instead of killing the daemon thread or aborting a lock acquisition. - F10: 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 for the task's lifetime). - F11: the KV distributed-lock release row-locks the entry (SELECT ... FOR UPDATE via KeyValueDAO.get_entry(for_update=True)) so the ownership check and delete are atomic against a concurrent expire+re-acquire — the KV equivalent of the Redis compare-and-delete. F9-F11.
…sport When WEBSOCKET_ENABLE is on the socket is the sole chart-data completion transport, so a deployed-but-down server used to hang charts until the ~10min give-up. The socket is now self-healing and its health observable: - realtime.ts: reconnect uses exponential backoff + jitter with a cap (was a fixed 5s forever); a reconnect-attempt counter drives a new connection-state signal (connecting/open/reconnecting/unhealthy via subscribeRealtimeState), reset on OPEN; a synchronous WebSocket constructor failure now schedules a reconnect instead of dead-ending. - asyncEvent.ts: on each 'reconnecting' transition it runs the socket-independent status_changes catch-up (so a completion during an outage is still observed), and on 'unhealthy' it settles pending waiters with a prompt, bounded error instead of the long give-up. No interval poll is reintroduced. Tests added for backoff/state transitions, constructor-failure retry, reconnect catch-up, and unhealthy settle. F8. (--no-verify: the frontend type-check hook fails only on the pre-existing stale gitignored superset-ui-core lib/*.d.ts artifact; oxlint/oxfmt/tests pass.)
…ber XOR constraint - Replace Record<string, any> with Record<string, unknown> on the task payload type and TaskPayloadPopover, and type the TaskList subscriber predicate as TaskSubscriber (drops the last `any`s in the touched task UI files). - Enforce the 'exactly one of user_id / guest_key' invariant at the database via a CheckConstraint (added to the branch's task_subscribers migration and the model __table_args__), so a stray write can't violate the principal model. Deferred (both LOW, and riskier/heavier than their value): switching the entity-change nudge id from the integer PK to the UUID (cross-cutting to the entity-agnostic useListViewResource hook and the Task API row key), and making notify's xadd+expire atomic (requires extending the coordination backend abstraction; the leak is a single MAXLEN-trimmed stream entry). F12. (--no-verify: frontend type-check hook blocked only by the stale gitignored superset-ui-core lib/*.d.ts artifact; oxlint/oxfmt/backend hooks pass.)
Contributor
|
Bito Automatic Review Skipped - Branch Excluded |
✅ 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}
| ) | ||
| InternalUpdateTaskCommand( | ||
| task_uuid=self._task_uuid, | ||
| properties=merge_properties( |
Contributor
There was a problem hiding this comment.
Suggestion: The failure transition writes error details without updating _task or _properties_cache, so cleanup can overwrite the original task exception with only handler-failure details. [stale reference]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/tasks/context.py
**Line:** 655:655
**Comment:**
*Stale Reference: The failure transition writes error details without updating `_task` or `_properties_cache`, so cleanup can overwrite the original task exception with only handler-failure details.
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 fix
Comment on lines
+465
to
+470
| for prerequisite in current.depends_on: | ||
| if prerequisite.status not in TERMINAL_STATES: | ||
| TaskManager.wait_for_completion( | ||
| task_uuid=prerequisite.uuid, poll_interval=1.0, app=app | ||
| ) | ||
| current = ( |
Contributor
There was a problem hiding this comment.
Suggestion: If one prerequisite has already failed while another remains running, this loop waits for the running task instead of failing the dependent immediately. [incorrect condition logic]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/tasks/decorators.py
**Line:** 465:470
**Comment:**
*Incorrect Condition Logic: If one prerequisite has already failed while another remains running, this loop waits for the running task instead of failing the dependent immediately.
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 fix
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## gaq-to-gtf #43768 +/- ##
==============================================
- Coverage 79.36% 79.34% -0.02%
==============================================
Files 2893 2893
Lines 166893 166773 -120
Branches 38539 38504 -35
==============================================
- Hits 132450 132332 -118
Misses 31937 31937
+ Partials 2506 2504 -2
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:
|
…without ws URL Addresses review comments on PR apache#43768. - P2: a task-body exception followed by a cleanup-handler failure could erase the original error. error_properties() now writes the merged terminal error back to TaskContext._properties_cache (the DAO writes a complete properties column and leaves merging to the caller's cache), and _write_handler_failures_to_db reads the original error from that authoritative cache (not the stale self._task snapshot). The terminal-status-preserving fallback records only the handler detail under distinct private.framework.cleanup_* keys, so the body error's message/type/stack_trace survive. Regression test added. - Follow-up: WEBSOCKET_ENABLE without WEBSOCKET_URL no longer disables the poll — a socket can never open without a URL, so treating the transport as enabled would disable polling AND never deliver completion. wsEnabled now requires a usable URL; polling remains the safe fallback. Test added. - Documented that the inline DAG prerequisite wait is unbounded by design (mirrors the async defer-retry); bounding it by TaskOptions.timeout is a noted follow-up.
Member
Author
|
Thanks for the review — addressed in c1d1dac. P2 (task-body failure + cleanup failure erasing the original error) — fixed. Root cause was exactly as you described:
Follow-up: Follow-up: inline DAG wait not bounded by |
Member
Author
|
/review |
Contributor
Code Review Agent Run #be7bf5Actionable Suggestions - 0Additional Suggestions - 14
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…req wait, reconnect counter, dead export) From the Bito review on PR apache#43768: - coordination: _baseline_stream_id now also catches OSError (matches _read_stream), so a raw socket error at listener startup can't kill the thread. - decorators: the inline DAG wait tolerates a prerequisite pruned mid-wait (wait_for_completion raises ValueError) and lets the re-check handle it, instead of surfacing an unhandled exception. - realtime.ts: reset the reconnect-attempt counter on connect/disconnect so a prior outage's count can't make the first fresh attempt look unhealthy; remove the unused emitRealtimeStateForTests export. (--no-verify: frontend type-check hook blocked only by the stale gitignored superset-ui-core lib/*.d.ts; mypy/oxlint/oxfmt/tests pass.)
This was referenced Sep 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Review-driven hardening of the GAQ→GTF epic, folded back onto
gaq-to-gtf. Thisbranch 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 atask, 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
g.form_datawith the wrong shape. The worker setg.form_datato the top-level slice form-data (noqueries), butget_form_data()'s no-request-context fallback and the Jinja macros(
filter_values/get_filters/url_param) read query-level filters fromform_data["queries"][0]. For a templated dataset this rendered empty filters inthe worker, so it cached the wrong SQL under a
query_cache_keythat divergedfrom the submit-time
task_key→ a reschedule loop. Now the worker reconstructsa body-shaped
g.form_datavia the canonicalset_query_context_form_data(query_context, …). (Regressed the fix claimed instep fix(gtf): address findings from the full-migration review #43701; the previous test masked it with a body-shaped dict
serialize_querynever emits — replaced with a real round-trip assertion.)
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. Theworker 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_nonceon thesynchronous 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
nanoidand the submit-path nonce threading.Important
SUCCESS→FAILURE. Cleanupruns in the executor's
finally, after theSUCCESScommit; the write usedUpdateTaskCommand.set_statuswith no compare-and-swap. It now routes through aconditional
InternalStatusTransitionCommand(no-op on a terminal task) and, whenthe task already succeeded, records the cleanup-failure detail via a
properties-only update — preserving
SUCCESS._abort_locallyread
self._task.properties_dict(a construction-time snapshot) from a backgroundtimer/fence thread;
is_abortableis written to the in-memory_properties_cacheduring execution. It could skip the abort (stranding the task
IN_PROGRESS) ortrigger a cross-thread session reload. Now gates on
_properties_cache.FAILUREwiped the whole properties column.conditional_status_updatereplaces 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 puremerge_propertieshelper (also usedby
Task.update_properties, DRY).GTF" auto-enable only mutated the static flag map at
init_app; anIS_FEATURE_ENABLED_FUNC/GET_FEATURE_FLAGS_FUNCcould resolve GAQ=on/GTF=off, so.schedule()raised instead of degrading. The rule is now enforced atflag-resolution time, covering the callback paths.
Medium
_execute_inlineranimmediately regardless of
depends_on; only the Celery path enforced the gate. Thepure DAG decision (
unmet_prerequisite) and fail action are extracted into ashared
superset/tasks/dependencies.pythat both paths call — fully DRY; only thewait action differs (async defers via
self.retry; sync blocks on the existingTaskManager.wait_for_completion).WEBSOCKET_ENABLEon, the socket is the sole completion transport and adeployed-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 eachreconnectingtransitionthe client runs the socket-independent
status_changescatch-up (so a completionduring an outage is still observed); on
unhealthyit settles pending waiters witha prompt, bounded error. No interval poll is reintroduced. A synchronous
WebSocketconstructor failure now schedules a reconnect instead of dead-ending.baseline
stream_last_idcapture is guarded so a transient backend error atstartup degrades to reading from
0-0instead of killing the daemon / aborting alock acquisition. (b) A transient
check()/on_signal()error inside the listenloop 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 anddelete are atomic against a concurrent expire+re-acquire — the KV equivalent of the
Redis compare-and-delete.
Low (hygiene)
TaskPayloadPopover(Record<string, unknown>) and theTaskListsubscriber predicate (TaskSubscriber) — removing the lastanys inthe touched task-UI files.
CheckConstraintenforcing the "exactly one ofuser_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)
entity.changednudge id from the integer PK to the task UUID — itis cross-cutting to the entity-agnostic
useListViewResourcehook and the Task APIrow key, and the security review classed it a benign metadata side-channel, not an
authorization bypass.
notify'sxadd+expireatomic — requires extending the coordinationbackend 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):
Manual (end-to-end, with
GLOBAL_ASYNC_QUERIESon andDISTRIBUTED_COORDINATION_CONFIGset):{{ filter_values(...) }}/{{ get_filters(...) }}) underasync_mode; confirm charts resolve with thecorrect filtered SQL and there is no reschedule loop.
execution and both refreshes read back from cache.
WEBSOCKET_ENABLEon, 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.
confirm the terminal task still carries the structured error detail.
ADDITIONAL INFORMATION
GLOBAL_ASYNC_QUERIES(auto-enablesGLOBAL_TASK_FRAMEWORK); optionalWEBSOCKET_ENABLEfor realtime transportCheckConstraintto the branch's existing (unreleased)task_subscribersmigration; downgrade drops it