Skip to content

feat(gaq): chart-data cutover — per-QueryObject GTF tasks + task-status polling - #43424

Merged
villebro merged 15 commits into
apache:gaq-to-gtffrom
villebro:villebro/gtf-chart-data
Aug 22, 2026
Merged

feat(gaq): chart-data cutover — per-QueryObject GTF tasks + task-status polling#43424
villebro merged 15 commits into
apache:gaq-to-gtffrom
villebro:villebro/gtf-chart-data

Conversation

@villebro

@villebro villebro commented Aug 22, 2026

Copy link
Copy Markdown
Member

SUMMARY

Step 4 of the GAQ→GTF epic (#43407): the chart-data cutover. Async chart-data requests now run entirely on the Global Task Framework instead of the bespoke Global Async Queries (GAQ) plumbing — one GTF task per QueryObject, a lightweight status poll, and a client re-request on completion. This subsumes the old "frontend re-request" and "remove query-context wrapper" steps into one cutover.

Because the whole gaq-to-gtf branch merges to master as a unit, this takes the breaking simplifications now rather than carrying compatibility shims.

Backend

  • Fan-out: /chart/data (async) schedules one SHARED GTF task per QueryObject, keyed by its query_cache_key (safe cross-user dedup — the key encodes RLS/impersonation). Contribution queries depends_on the totals query's task and read its cached result to normalize. Dedicated task type superset.query_object_v1. The 202 body is {"task_ids": [...]}.
  • No coordinator task, no qc-<hash> wrapper: the client aggregates the query tasks' own honest statuses itself. Removed the /api/v1/chart/data/<cache_key> replay endpoint, QueryContextCacheLoader, and result_url.
  • New poll primitive GET /api/v1/task/status_changes: returns {statuses: {uuid: {status, progress}}, cursor} for tasks the caller can see (TaskFilter-scoped), changed since an opaque server-issued cursor (changed_on >=, so no transition is missed; re-delivery is idempotent). No cursor = baseline (empty + current watermark; never dumps all tasks). Optional task_type filter.
  • Embedded-guest visibility: guests have no ab_user id, so they subscribe to tasks by a stable, token-derived guest_key (HMAC over the guest token's authorization-relevant claims — user, resources, datasets, rls_rules, etc. — keyed with SECRET_KEY); TaskFilter honors it, and cancellation is guest-aware. Survives SHARED-scope dedup (two equivalent guests collapse to one task and both subscribe). Adds a nullable task_subscribers.guest_key column (folded into the task-dependencies migration; user_id relaxed to nullable — a subscriber is exactly one of user_id/guest_key).
  • GAQ rip-out: deleted AsyncQueryManager, the /api/v1/async_event/ REST API, the factory, and the legacy load_chart_data_into_cache Celery task. Removed the GAQ JWT/cookie/transport/stream config and the dedicated GLOBAL_ASYNC_QUERIES_CACHE_BACKEND — coordination now runs on DISTRIBUTED_COORDINATION_CONFIG exclusively. Kept the GLOBAL_ASYNC_QUERIES flag and GLOBAL_ASYNC_QUERIES_POLLING_DELAY.

Frontend

  • Rewrote asyncEvent.ts off the GAQ firehose (poll + WebSocket + result_url) to a single shared cursor-poll of status_changes filtered to superset.query_object_v1. waitForAsyncData(job, refetch, signal) awaits the job's task_ids to all reach SUCCESS, then calls refetch() (re-issues the original request — served synchronously from the now-warm per-query DATA cache); any terminal non-success rejects; abort cancels the tasks via /api/v1/task/<uuid>/cancel. Each task id maps to a set of waiters so a deduplicated SHARED task settles every concurrent chart awaiting it.
  • Threaded the re-request through chartAction.exploreJSON, FilterValue, FiltersConfigForm, DrillByModal, ChartVersionPreview, and StatefulChart's handleAsyncChartData hook.

Auth / transport notes

  • Polling and cancel use the caller's normal Superset session (@protect()); embedded guests use their existing guest token. The GAQ JWT is no longer needed.
  • Real-time WebSocket push (previously via the GAQ firehose) is intentionally retired here; async chart-data is polling-only on the branch. A GTF-native WebSocket transport returns in a later epic step with its own channel-token service.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — no user-visible UI change (async chart loading behaves the same; the transport underneath changed).

TESTING INSTRUCTIONS

With GLOBAL_ASYNC_QUERIES enabled (which also enables GLOBAL_TASK_FRAMEWORK) and DISTRIBUTED_COORDINATION_CONFIG pointed at Redis/Valkey:

  • Load a dashboard with multi-query + contribution charts; confirm each chart resolves (tasks appear in the Task List as superset.query_object_v1, then the chart renders).
  • Confirm a second load short-circuits to a synchronous 200 (warm cache).
  • Press Stop mid-load; confirm the outstanding tasks are cancelled.
  • Load an embedded (guest-token) dashboard; confirm async charts resolve.

Automated: pytest tests/unit_tests/tasks tests/unit_tests/coordination tests/unit_tests/daos/test_tasks.py; frontend npm run test -- asyncEvent chartActions StatefulChart. Migration up/down/up verified reversible on SQLite.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags: GLOBAL_ASYNC_QUERIES (auto-enables GLOBAL_TASK_FRAMEWORK in a later step)
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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 (/api/v1/task/status_changes; per-QueryObject async tasks)
  • Removes existing feature or API (/api/v1/async_event/, AsyncQueryManager, qc-<hash> replay, GLOBAL_ASYNC_QUERIES_CACHE_BACKEND + GAQ JWT/transport config)

Review follow-ups addressed in this PR: multi-waiter registry for deduplicated shared tasks; rls_rules bound into the guest key; contribution task now fails (rather than silently succeeding un-normalized) when the prerequisite totals cache is missing; guest-aware task cancellation; abort signal threaded through the version-preview wait; restored the cancel route; frontend type fixes.

Deferred (noted, not blocking): FilterValue's async default-value load lacks an abort/stale guard — a pre-existing gap that needs an effect refactor (its isRefreshing state is both set inside the effect and in its dependency array, so a naive abort-on-cleanup would abort the in-flight request the moment setIsRefreshing(true) re-runs the effect). Also: rip the now-dead cache_query_context/qc-<hash> cache_key() write path from QueryContextProcessor (touches the /chart/data response shape, so done separately); async_mode per-request opt-in + GLOBAL_ASYNC_QUERIESGLOBAL_TASK_FRAMEWORK auto-enable (next step); GTF-native WebSocket transport.

Targets the gaq-to-gtf feature branch (part of #43407), not master.

Replace the single load_chart_data_into_cache Celery job with a GTF fan-out:
one SHARED task per QueryObject (keyed by query_cache_key for cross-user dedup)
plus a per-job coordinator that joins them and emits the completion event onto
the firehose. Contribution queries depend_on the totals query's task and read
its cached result to normalize. The 202 body carries the coordinator task UUID
so the client polls/cancels via the GTF task API.

- add submit_chart_data_query_tasks orchestrator in tasks/async_queries.py
- QueryContext.prepare_contribution_totals() public delegate
- ChartDataCommand.query_context accessor; _run_async passes the built context
- CreateAsyncChartDataJobCommand.run takes a QueryContext
- fix @task/.schedule() typing (overloads + explicit options param) so the
  first real source caller type-checks
Minimal-IO polling primitive returning {uuid: status} for accessible tasks
changed since a cursor (TaskFilter-scoped: subscribed tasks for regular users,
all for admins), plus the next cursor to poll with (server-observed changed_on
watermark, so the client never computes timestamps). Uses changed_on >= cursor
so no boundary-straddling transition is missed; re-delivery is idempotent.

Backs the async chart-data completion poll (replacing the /async_event/
firehose) and the future realtime task-list transport.
…ctly

The chart-data async path is now purely GTF-native:
- no coordinator task — the 202 returns {task_ids: [...]} and the client polls
  /api/v1/task/status_changes and aggregates the query tasks' own statuses (all
  SUCCESS -> re-request served from cache; any terminal non-success -> error)
- dedicated task type superset.query_object_v1 (the atomic QueryObject unit,
  versioned); status_changes gains a task_type filter so a client tracks only
  its own kind of task, each value carrying {status, progress}
- remove the qc-<hash> descriptor replay: /api/v1/chart/data/<cache_key>
  (data_from_cache), _load_query_context_form_from_cache, QueryContextCacheLoader
- inline the trivial CreateAsyncChartDataJobCommand into _run_async and delete it

GTF owns completion emission (per-task, via the coordination service), so the
websocket transport will subscribe to GTF rather than a GAQ-specific stream.
Replace the GAQ firehose (poll + WS + result_url fetch) with a GTF-native
cursor poll of /api/v1/task/status_changes (filtered to superset.query_object_v1):
- asyncEvent.ts: single shared poll loop; waitForAsyncData(job, refetch, signal)
  awaits the job's task_ids to all reach SUCCESS then calls refetch() (which
  re-issues the original request, now served from the warm per-query cache);
  any terminal non-success rejects; abort cancels tasks via /task/<uuid>/cancel.
  Baseline cursor fetched at init before any query is triggered.
- handleChartDataResponse(response, json, refetch?, signal?): 202 body is the
  {task_ids} job; delegates to waitForAsyncData with a caller-supplied refetch.
- thread refetch through exploreJSON, FilterValue, FiltersConfigForm,
  DrillByModal, ChartVersionPreview, and StatefulChart's handleAsyncChartData
  hook (ChartProps signature updated).
- rewrite asyncEvent.test.ts for the new transport; update chartActions +
  StatefulChart tests for the new call shape.
Embedded guests have no ab_user id, so they could not poll their own async
chart-data tasks once the GAQ guest-channel path was removed. Give guests real
subscription-based visibility:
- task_subscribers.guest_key (nullable) + user_id relaxed to nullable; a
  subscriber is exactly one of user_id or guest_key (migration + unique
  (task_id, guest_key), reversible)
- superset/tasks/guest.py get_current_guest_subscriber_key(): stable HMAC over
  the guest token's identifying claims, keyed with SECRET_KEY
- SubmitTaskCommand subscribes guests by guest_key on create and on SHARED-task
  join; TaskDAO.create_task/add_guest_subscriber + Task.has_guest_subscriber
- TaskFilter grants guests visibility of tasks carrying their guest_key, so
  status_changes / the task API scope correctly for embedded dashboards

Survives SHARED-scope dedup: two equivalent guests collapse to one task and
both subscribe to it.
… backend

With async chart data fully on GTF, remove the orphaned GAQ machinery entirely
(breaking change — no deprecation window):
- delete superset/async_events/{api,async_query_manager,async_query_manager_factory}.py
  (keep cache_backend.py — its Redis classes back the coordination service)
- remove the /api/v1/async_event/ REST API + its registration; remove the
  AsyncQueryManager proxy/factory + init wiring + the check_async_query_secret
  startup check
- config: drop GLOBAL_ASYNC_QUERY_MANAGER_CLASS, all GLOBAL_ASYNC_QUERIES_JWT_*/
  _REDIS_STREAM_*/_TRANSPORT/_WEBSOCKET_URL/_REGISTER_REQUEST_HANDLERS, and the
  dedicated GLOBAL_ASYNC_QUERIES_CACHE_BACKEND — coordination uses only
  DISTRIBUTED_COORDINATION_CONFIG now. Keep GLOBAL_ASYNC_QUERIES flag +
  GLOBAL_ASYNC_QUERIES_POLLING_DELAY (frontend polls at this interval)
- drop GAQ transport keys from the frontend bootstrap conf + the GAQ JWT secret
- remove the legacy load_chart_data_into_cache celery task; async_queries.py is
  now pure GTF
- delete/rewrite tests bound to the removed layer; new unit tests cover the
  fan-out orchestrator

Client auth for polling/cancel is the normal Superset session; guest visibility
is SECRET_KEY-derived (superset/tasks/guest.py), so the GAQ JWT is unneeded.
@dosubot dosubot Bot added api Related to the REST API change:backend Requires changing the backend change:frontend Requires changing the frontend global:async-query Related to Async Queries feature risk:breaking-change Issues or PRs that will introduce breaking changes labels Aug 22, 2026
@bito-code-review

bito-code-review Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Bito Automatic Review Skipped - Branch Excluded

Bito didn't auto-review because the source or target branch is excluded from automatic reviews.
No action is needed if you didn't intend for the agent to review it. Otherwise, to manually trigger a review, type /review in a comment and save.
You can change the branch exclusion settings here, or contact your Bito workspace admin at evan@preset.io.

@codecov

codecov Bot commented Aug 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.58741% with 107 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.87%. Comparing base (4dea578) to head (4bf78b4).

Files with missing lines Patch % Lines
superset/daos/tasks.py 28.57% 24 Missing and 1 partial ⚠️
superset/tasks/async_queries.py 62.26% 20 Missing ⚠️
superset/tasks/api.py 47.05% 9 Missing ⚠️
...tend/src/components/Chart/DrillBy/DrillByModal.tsx 33.33% 8 Missing ⚠️
superset-frontend/src/middleware/asyncEvent.ts 90.54% 7 Missing ⚠️
...set-ui-core/src/chart/components/StatefulChart.tsx 16.66% 5 Missing ⚠️
superset/commands/tasks/cancel.py 58.33% 3 Missing and 2 partials ⚠️
superset/tasks/guest.py 64.28% 4 Missing and 1 partial ⚠️
...erset-frontend/src/components/Chart/chartAction.ts 72.72% 3 Missing ⚠️
...veFilters/FilterBar/FilterControls/FilterValue.tsx 57.14% 3 Missing ⚠️
... and 8 more
Additional details and impacted files
@@              Coverage Diff               @@
##           gaq-to-gtf   #43424      +/-   ##
==============================================
- Coverage       78.88%   78.87%   -0.02%     
==============================================
  Files            2883     2884       +1     
  Lines          164733   165065     +332     
  Branches        38028    38010      -18     
==============================================
+ Hits           129956   130191     +235     
- Misses          32331    32434     +103     
+ Partials         2446     2440       -6     
Flag Coverage Δ
hive 38.16% <29.81%> (+0.05%) ⬆️
javascript 74.20% <75.20%> (-0.03%) ⬇️
mysql 57.74% <36.64%> (-0.07%) ⬇️
postgres 57.77% <36.64%> (-0.07%) ⬇️
presto 40.10% <29.81%> (+0.06%) ⬆️
python 83.50% <52.79%> (-0.09%) ⬇️
sqlite 57.46% <36.64%> (-0.07%) ⬇️
superset-extensions-cli 90.57% <ø> (?)
unit 73.57% <47.82%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@netlify

netlify Bot commented Aug 22, 2026

Copy link
Copy Markdown

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 9e0da41
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6a89d7ed33352e0008ae7581
😎 Deploy Preview https://deploy-preview-43424--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

'Async chart-data response (202) received without a refetch handler',
);
}
return waitForAsyncData(json as unknown as AsyncJob, refetch, signal);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Shared GTF tasks can be returned to multiple chart requests, but waitForAsyncData stores only one waiter per task ID. When this call registers a second request for an already-awaited shared task, it overwrites the first waiter, so only the latest chart request is resolved and the earlier chart remains pending indefinitely. The waiter registry must support multiple waiters per task ID or deduplicate completion notifications without overwriting subscribers. [race condition]

Severity Level: Major ⚠️
- ❌ Concurrent charts sharing a task can remain indefinitely pending.
- ⚠️ Affected charts never receive cached query results.
- ⚠️ Shared task polling loses an earlier subscriber.

Use CodeAnt Skill

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/src/components/Chart/chartAction.ts
**Line:** 663:663
**Comment:**
	*Race Condition: Shared GTF tasks can be returned to multiple chart requests, but `waitForAsyncData` stores only one waiter per task ID. When this call registers a second request for an already-awaited shared task, it overwrites the first waiter, so only the latest chart request is resolved and the earlier chart remains pending indefinitely. The waiter registry must support multiple waiters per task ID or deduplicate completion notifications without overwriting subscribers.

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 thread superset/tasks/guest.py
Comment on lines +54 to +60
"user": token.get("user"),
"resources": token.get("resources"),
"iat": token.get("iat"),
"exp": token.get("exp"),
"aud": token.get("aud"),
"datasets": token.get("datasets"),
"rev": token.get("rev"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The derived identity omits the token's rls_rules claim, even though RLS rules are part of the guest's effective authorization scope. Two tokens for the same user/resources but different RLS rules therefore receive the same guest_key, allowing one guest to pass the task visibility filter and observe the other guest's task status and metadata. Include all authorization-relevant claims, especially rls_rules, in the HMAC input. [security]

Severity Level: Minor 🧹
- ⚠️ Guest task status metadata crosses RLS scopes.
- ⚠️ `/api/v1/task/status_changes` exposes another guest’s progress.
- ⚠️ Different row-level scopes share one task visibility identity.

Use CodeAnt Skill

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset/tasks/guest.py
**Line:** 54:60
**Comment:**
	*Security: The derived identity omits the token's `rls_rules` claim, even though RLS rules are part of the guest's effective authorization scope. Two tokens for the same user/resources but different RLS rules therefore receive the same `guest_key`, allowing one guest to pass the task visibility filter and observe the other guest's task status and metadata. Include all authorization-relevant claims, especially `rls_rules`, in the HMAC input.

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
👍 | 👎

…migration

Remove the standalone guest_key migration and add the column + user_id-nullable
relaxation + unique (task_id, guest_key) index to the existing
task_dependencies migration on the feature branch, so the branch carries a
single task-schema migration. Fixes the RAT license-header failure on the
now-deleted standalone file (the folded migration already has the ASF header).
Up/down/up verified reversible.
Comment on lines 92 to 96
SupersetClient.post({
endpoint: `/api/v1/async_event/${jobId}/cancel`,
endpoint: `/api/v1/task/${taskId}/cancel`,
}).catch(error => {
logging.warn('Failed to cancel async job', jobId, error);
logging.warn('Failed to cancel task', taskId, error);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Aborting an embedded guest request calls the generic task cancellation endpoint, but guest authorization is based on guest_key while the cancellation command validates only get_user_id() and has_subscriber(user_id). The cancellation therefore fails for guest-created shared tasks, leaving the worker running despite the client abandoning the request. Guest cancellation must use the guest subscriber identity or avoid claiming cancellation succeeded. [api mismatch]

Severity Level: Major ⚠️
- ⚠️ Embedded guest cancellations leave warehouse queries running.
- ⚠️ Abandoned guest requests continue consuming GTF capacity.
- ❌ Guest cancellation cannot unsubscribe or abort shared tasks.

Use CodeAnt Skill

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/src/middleware/asyncEvent.ts
**Line:** 92:96
**Comment:**
	*Api Mismatch: Aborting an embedded guest request calls the generic task cancellation endpoint, but guest authorization is based on `guest_key` while the cancellation command validates only `get_user_id()` and `has_subscriber(user_id)`. The cancellation therefore fails for guest-created shared tasks, leaving the worker running despite the client abandoning the request. Guest cancellation must use the guest subscriber identity or avoid claiming cancellation succeeded.

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
👍 | 👎

settle(waiter);
return;
}
taskIds.forEach(taskId => waitersByTaskId.set(taskId, waiter));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The task registry stores only one Waiter per task ID. Because shared GTF tasks can be returned to multiple concurrent requests, registering a later waiter overwrites the earlier one; when the task completes, only the latest request is settled and the earlier request remains pending indefinitely. Store a set/list of waiters per task ID and settle all of them. [race condition]

Severity Level: Critical 🚨
- ❌ Concurrent charts sharing tasks can remain permanently loading.
- ❌ Earlier requests never refetch completed chart data.
- ⚠️ Shared-task deduplication becomes user-visible request starvation.

Use CodeAnt Skill

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/src/middleware/asyncEvent.ts
**Line:** 188:188
**Comment:**
	*Race Condition: The task registry stores only one `Waiter` per task ID. Because shared GTF tasks can be returned to multiple concurrent requests, registering a later waiter overwrites the earlier one; when the task completes, only the latest request is settled and the earlier request remains pending indefinitely. Store a set/list of waiters per task ID and settle all of them.

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 thread superset/tasks/async_queries.py Outdated
Comment on lines 88 to 96
if not cache.is_loaded or cache.df is None:
# The depends_on prerequisite guarantees the totals task succeeded, so a
# miss here is unexpected; leave the query as-is (the contribution op will
# fall back to its own totals) rather than failing the whole chart.
logger.warning(
"Totals result not cached under %s; contribution left un-normalized",
totals_cache_key,
)
return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: When the totals cache is unavailable, this branch marks the contribution task as successful and executes it without injecting contribution_totals. The reconstructed context contains only the contribution query, so it cannot perform the synchronous path's ensure_totals_available operation; the result is therefore unnormalized or fails later while the task is still reported as successful. Treat a missing prerequisite cache as task failure instead of silently returning. [incomplete implementation]

Severity Level: Major ⚠️
- ❌ Contribution charts can display incorrectly normalized percentages.
- ⚠️ Successful task status hides missing prerequisite data.
- ⚠️ Frontend re-request returns incorrect cached chart results.

Use CodeAnt Skill

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset/tasks/async_queries.py
**Line:** 88:96
**Comment:**
	*Incomplete Implementation: When the totals cache is unavailable, this branch marks the contribution task as successful and executes it without injecting `contribution_totals`. The reconstructed context contains only the contribution query, so it cannot perform the synchronous path's `ensure_totals_available` operation; the result is therefore unnormalized or fails later while the task is still reported as successful. Treat a missing prerequisite cache as task failure instead of silently returning.

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 +299 to +304
waitForAsyncData(json as unknown as AsyncJob, () =>
requestFilterData(true).then(
({ json: cachedJson }) =>
cachedJson.result as ChartDataResponseResult[],
),
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The asynchronous filter request has no abort or stale-request guard. If filter state changes while waitForAsyncData is polling, the captured newFormData request can eventually refetch and call setState and handleFilterLoadFinish after a newer filter request has started, allowing old options to overwrite current values and clearing the newer request's loading state. Create an effect-scoped AbortController and ignore or cancel completion from superseded requests. [stale reference]

Severity Level: Major ⚠️
- ⚠️ Native filter options can briefly reflect obsolete selections.
- ⚠️ Old completion can clear the current filter's loading state.
- ⚠️ Async default-value loading has the same pattern.

Use CodeAnt Skill

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
**Line:** 299:304
**Comment:**
	*Stale Reference: The asynchronous filter request has no abort or stale-request guard. If filter state changes while `waitForAsyncData` is polling, the captured `newFormData` request can eventually refetch and call `setState` and `handleFilterLoadFinish` after a newer filter request has started, allowing old options to overwrite current values and clearing the newer request's loading state. Create an effect-scoped `AbortController` and ignore or cancel completion from superseded requests.

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 +182 to +186
const { response, json } = await requestPreviewData();
const result = await handleChartDataResponse(response, json, () =>
requestPreviewData().then(({ response: r, json: j }) =>
handleChartDataResponse(r, j),
),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The preview's async wait is started without an AbortSignal, so changing versions or unmounting the preview only prevents the final state update via fetchId; it does not stop polling or cancel the outstanding GTF tasks. Repeated preview changes can therefore retain waiters and continue warehouse work until each task finishes. Pass a lifecycle-scoped signal through the response handler and abort it when the preview request is superseded or unmounted. [resource leak]

Severity Level: Major ⚠️
- ⚠️ Abandoned version previews retain polling waiters.
- ⚠️ Outstanding GTF chart tasks continue after preview changes.
- ⚠️ Repeated preview changes increase unnecessary warehouse work.

Use CodeAnt Skill

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/src/features/versionHistory/ChartVersionPreview.tsx
**Line:** 182:186
**Comment:**
	*Resource Leak: The preview's async wait is started without an `AbortSignal`, so changing versions or unmounting the preview only prevents the final state update via `fetchId`; it does not stop polling or cancel the outstanding GTF tasks. Repeated preview changes can therefore retain waiters and continue warehouse work until each task finishes. Pass a lifecycle-scoped signal through the response handler and abort it when the preview request is superseded or unmounted.

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
👍 | 👎

A SHARED chart-data task can be returned to multiple concurrent chart requests
(GTF dedup by query_cache_key), but the waiter registry stored one Waiter per
task id, so a later request overwrote the earlier one and the earlier chart hung
forever. Key each task id to a Set of waiters and settle them all on completion,
with centralized unregister on settle/abort so no waiter leaks. Adds a
regression test.
get_current_guest_subscriber_key omitted the token's rls_rules claim, so two
guest tokens differing only in row-level scope derived the same guest_key and
could see each other's task status/metadata via /api/v1/task/status_changes.
Include rls_rules (and all authorization-relevant claims) in the HMAC input.
_inject_contribution_totals logged a warning and returned when the totals cache
was missing, so the contribution query ran un-normalized yet the task still
reported SUCCESS and the client re-requested silently-wrong percentages. Raise
instead: the single-query task can't reproduce the sync ensure_totals_available.
CancelTaskCommand validated only user_id + has_subscriber(user_id), so a guest
(no ab_user id) could never cancel/unsubscribe a shared task it created.
Honor the guest's token-derived guest_key in the permission check and
unsubscribe path; add TaskDAO.remove_guest_subscriber (shared
_remove_subscription helper with remove_subscriber).
The status_changes endpoint addition accidentally consumed the cancel view's
@expose decorator, so the cancel route stopped registering (cancel-by-uuid and
admin-cancel integration tests 404'd; the not-found test passed only because a
routing 404 is indistinguishable from its expected response_404). Restore the
@expose("/<task_uuid>/cancel", methods=("POST",)).
ChartRenderer's local ChartHooks.handleAsyncChartData type still had the old
(response, json, signal?) shape; align it with the (response, json, refetch?,
signal?) contract. Import QueryData from @superset-ui/core in chartActions.test
(it isn't re-exported from chartAction).
…uperseded

ChartVersionPreview only suppressed the final state update via fetchId; pass an
effect-scoped AbortController signal through handleChartDataResponse and abort it
on cleanup so polling stops and the GTF tasks are cancelled when the preview
changes or unmounts. (Effect deps are [dispatch, entityUuid, versionUuid] — none
set inside the effect — so cleanup only fires on a genuine supersede/unmount.)
- initialization_test: drop the removed configure_async_queries from the
  init_app_in_ctx patch set
- test_chart_data_api: rewrite test_run_async_does_not_project_timing_onto_a_job_response
  to patch submit_chart_data_query_tasks and assert the 202 body is {task_ids}
  (the async command + result_url/channel_id shape are gone)
@villebro

Copy link
Copy Markdown
Member Author

/review

@bito-code-review

bito-code-review Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #2e3ad0

Actionable Suggestions - 0
Additional Suggestions - 7
  • superset-frontend/src/middleware/asyncEvent.test.ts - 2
    • Missing test coverage for refetch throw path · Line 67-82
      The existing test suite only stubs `refetch` with `mockResolvedValue` — no test exercises the case where `refetch()` itself throws after tasks resolve. This means the handling gap in the main function could go undetected. Rule [6262]: tests must verify actual business logic, not just render/success paths.
    • Missing result assertion in test · Line 98-98
      The `await` on line 96 resolves without result verification. Test at line 81 includes `expect(result).toEqual([{ rows: 1 }])`; this test lacks the analogous check for its return value.
  • tests/integration_tests/charts/data/api_tests.py - 1
    • Security coverage gap: JWT auth test removed · Line 924-937
      Removed `test_chart_data_async_invalid_token` checks JWT auth on async chart data requests (cookie "foo" → 401). If `GLOBAL_ASYNC_QUERIES_JWT_COOKIE_NAME` auth is still a supported path, removing this test creates a coverage gap. Other JWT invalid-token tests exist in the codebase.
  • superset/commands/tasks/cancel.py - 1
    • Duplicate inline import · Line 285-285
      Duplicate inline import at line 285 inside `_do_unsubscribe`. Same issue as line 194 — move to module level.
  • superset-frontend/src/middleware/asyncEvent.ts - 1
    • Misleading comment about error handling · Line 120-120
      The comment on line 120 attributes error surfacing to `getClientErrorObject`, but that function was removed in this diff and is not called in `waitForAsyncData`. The error is thrown directly as `new Error('One or more chart-data queries failed')`. This misleading comment could cause a future developer to search for or refactor the non-existent call path.
  • superset/daos/tasks.py - 1
    • Missing guest subscriber tests · Line 401-414
      The new `remove_guest_subscriber` method has no unit tests. Per organizational testing standards, every new tool/method needs coverage for success, error, and not-found paths. Existing `test_remove_subscriber` (line 362) provides a ready template.
  • superset/commands/tasks/submit.py - 1
    • Misleading comment in guest subscribe block · Line 126-126
      The comment at line 127 says 'an equivalent guest created' which is ambiguous — it could suggest we're checking some equivalence relation. We are checking `guest_key` (the current guest's own key) against the existing task's subscribers. Use phrasing that clarifies this is the current guest joining a shared task.
Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

  • tests/integration_tests/charts/data/api_tests.py - 1
  • superset/daos/tasks.py - 1
    • CWE-20: Naive datetime instead of UTC · Line 99-99
Review Details
  • Files reviewed - 51 · Commit Range: d2e6e26..4bf78b4
    • superset-core/src/superset_core/tasks/models.py
    • superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.test.tsx
    • superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.tsx
    • superset-frontend/packages/superset-ui-core/src/chart/models/ChartProps.ts
    • superset-frontend/src/components/Chart/ChartRenderer.tsx
    • superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx
    • superset-frontend/src/components/Chart/chartAction.ts
    • superset-frontend/src/components/Chart/chartActions.test.ts
    • superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
    • superset-frontend/src/features/versionHistory/ChartVersionPreview.tsx
    • superset-frontend/src/middleware/asyncEvent.test.ts
    • superset-frontend/src/middleware/asyncEvent.ts
    • superset/async_events/api.py
    • superset/async_events/async_query_manager.py
    • superset/async_events/async_query_manager_factory.py
    • superset/charts/data/api.py
    • superset/charts/data/query_context_cache_loader.py
    • superset/commands/chart/data/create_async_job_command.py
    • superset/commands/chart/data/get_data_command.py
    • superset/commands/tasks/cancel.py
    • superset/commands/tasks/submit.py
    • superset/common/query_context.py
    • superset/config.py
    • superset/constants.py
    • superset/coordination/__init__.py
    • superset/coordination/base.py
    • superset/daos/tasks.py
    • superset/extensions/__init__.py
    • superset/initialization/__init__.py
    • superset/migrations/versions/2026-08-21_12-00_7e2c9a4f1b83_create_task_dependencies_table.py
    • superset/models/task_subscribers.py
    • superset/models/tasks.py
    • superset/tasks/api.py
    • superset/tasks/async_queries.py
    • superset/tasks/decorators.py
    • superset/tasks/filters.py
    • superset/tasks/guest.py
    • superset/views/base.py
    • tests/integration_tests/async_events/api_tests.py
    • tests/integration_tests/charts/data/api_tests.py
    • tests/integration_tests/superset_test_config.py
    • tests/integration_tests/tasks/async_queries_tests.py
    • tests/unit_tests/async_events/async_query_manager_tests.py
    • tests/unit_tests/charts/test_chart_data_api.py
    • tests/unit_tests/commands/chart/create_async_job_command_test.py
    • tests/unit_tests/coordination/test_service.py
    • tests/unit_tests/initialization/check_async_query_secret_test.py
    • tests/unit_tests/initialization/check_encryption_engine_test.py
    • tests/unit_tests/initialization_test.py
    • tests/unit_tests/tasks/test_async_queries.py
  • Files skipped - 0
  • Tools
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Eslint (Linter) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers an incremental AI Review.

  • /review full - Manually triggers a full AI Review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@villebro
villebro merged commit fb20230 into apache:gaq-to-gtf Aug 22, 2026
70 checks passed
@villebro
villebro deleted the villebro/gtf-chart-data branch August 22, 2026 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Related to the REST API change:backend Requires changing the backend change:frontend Requires changing the frontend global:async-query Related to Async Queries feature packages risk:breaking-change Issues or PRs that will introduce breaking changes risk:db-migration PRs that require a DB migration size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant