rusackas opened a new pull request, #44552:
URL: https://github.com/apache/superset/pull/44552
### SUMMARY
Adopted from #36350 by @X-arshiya-X, with @Vab-170, @ssadras and
@Infernashwin. That PR could not be rebased in place — its head branch lives on
an org-owned fork with `maintainerCanModify: false`, which GitHub does not let
maintainers override — and it had drifted ~1,300 commits behind `master`. The
idea is theirs; this re-derives it against current `master`, which has grown
machinery that did not exist when it was written. Original authors credited as
co-authors on the commit.
Refs #35347 — see **Engine coverage** below; this fixes the reported
behavior for most engines but *not yet* for Trino, so I've deliberately not
written `Fixes`.
**The bug.** Clicking **Stop** on a running chart query in Explore aborts
the in-flight HTTP request, but the database keeps executing the query. A heavy
chart query keeps burning compute long after nobody is waiting for the result.
**The fix.** Each chart run now carries a client-generated `client_id`.
While the query runs, the existing chart-data cancellation seam in
`superset/tasks/query_cancel.py` captures the engine cancel id off the live
cursor and publishes it to the cache. A new `POST /api/v1/chart/data/stop`
resolves that handle and kills the backend session over a fresh connection —
the same `db_engine_spec.cancel_query` contract SQL Lab uses.
That seam already existed: it was built for the async (GTF) path, where a
task abort needs to kill the warehouse query. It was only ever wired to Celery
tasks. All this PR does is let the *synchronous* Explore path reach it, so the
Stop button gets the cancellation that async queries already had.
### Engine coverage — please read before approving
The capture happens on the seam that runs *before* the blocking execute, so
only engines that can produce a cancel handle at that point participate:
| | Engines | Stop now cancels? |
|---|---|---|
| `get_cancel_query_id` before execute | Postgres, MySQL, Redshift,
Snowflake, SingleStore, Ocient | **Yes** |
| id only available *during* execution | Trino, Presto, Hive, Impala | Not
yet |
So the engine named in #35347 (Trino) is **not** covered by this PR. Trino's
query id only appears on the cursor once execution has started, and it is
captured by `handle_cursor`, which only runs under `execute_with_cursor` — a
threaded poll that `Database._execute_sql_with_mutation_and_logging` does not
currently use (it calls `db_engine_spec.execute` directly). Covering Trino
means routing chart execution through `execute_with_cursor` when a cancel sink
is active, and publishing the handle from the callback as soon as
`handle_cursor` sets it.
I left that out on purpose: it changes the code path that executes *every*
chart query in Superset, and I had no Trino instance to validate the threaded
path against. It is a well-defined follow-up rather than something to guess at
here. (#36350 did attempt this, in `models/core.py`.) Happy to do it as a
follow-up PR, or fold it in here if you'd rather it land together — say which
you prefer.
### Notes for reviewers
**Authorization.** Cancelling someone else's query is a real authorization
surface, and `client_id` is untrusted, client-supplied input. The cache key
embeds the requesting user's id (`chart-query-cancel:{user_id}:{client_id}`)
rather than storing the owner as a field compared after lookup, so a
`client_id` is only ever resolvable inside the namespace of the user who
registered it. Passing another user's `client_id` simply misses — it cannot
read, cancel, or overwrite their entry. There are dedicated regression tests
for both directions of that
(`test_cancel_chart_query_for_user_cannot_reach_another_users_query`,
`test_cancellable_chart_query_cannot_overwrite_another_users_handle`).
Anonymous requests register nothing at all: an anonymous viewer of a public
dashboard has no user id to scope a handle to, and an unscoped handle would be
cancellable by any other anonymous visitor. They keep the existing client-side
abort.
The endpoint maps to `can_read on Chart`, the same permission that running a
chart query needs — cancelling a query you started requires no privilege beyond
starting it, and it needs no permission migration. Deliberately *not* reusing
SQL Lab's `/api/v1/query/stop`, which is gated on `can_stop_query on Query`: a
Gamma user with chart access but no SQL Lab access would have been 403'd on
their own chart's Stop button.
**Divergence from #36350.** The original made chart queries cancellable by
inserting a real SQL Lab `Query` row per chart render, keyed by the
client-supplied `client_id`, and reusing `/api/v1/query/stop`. I did not carry
that forward, for three reasons:
1. `query.client_id` is `unique=True` **globally, across all users**, and
the original's reuse lookup (`filter_by(client_id=client_id).one_or_none()`)
was not scoped by user. A request naming another user's `client_id` would have
resolved to *their* `Query` row and then handed it to the execution path, where
`handle_cursor` overwrites `extra_json[cancel_query]` — clobbering a running
SQL Lab query's cancel handle, so its owner's Stop would cancel the wrong
query. That is the blocker this rewrite removes.
2. It wrote a metadata-DB row plus a commit for **every chart render**,
forever. The follow-on `Query.sql IS NOT NULL` filters it added to
`queries/filters.py` and `daos/query.py` existed only to hide those rows from
Query History.
3. It threaded a `query=` parameter through `get_df` → `ExploreMixin.query`
→ `get_query_result` → the `Explorable` protocol, which needed an
`inspect.signature` guard because `AnnotationDatasource.query()` has a
different arity.
Routing through `query_cancel.py` instead needs no `Query` rows, no
migration, no history filtering, and no signature threading — so
`models/core.py`, `models/helpers.py`, `explorables/base.py`,
`connectors/sqla/models.py`, `daos/query.py`, `queries/filters.py`,
`sql_lab.py` and `sql/execution/executor.py` are all untouched here. I also
dropped the original's broad `except Exception: return False` wrappers around
`sql_lab.cancel_query` / `executor._cancel_query`, which silently converted
engine errors into "could not cancel".
Also dropped as unrelated drift from the stale branch: `CLAUDE.md`,
`GEMINI.md`, `GPT.md`, `.github/copilot-instructions.md`,
`.storybook/main.mjs`, `number-format/README.md`,
`playwright/embedded-app/index.html`,
`playwright/generators/docs/screenshot-manifest.yaml`, and a `DatabaseSelector`
null-guard.
**Scope.** `client_id` is sent for dashboard chart renders too, so their
queries register handles; there is just no per-chart Stop control in the
dashboard UI today to call the endpoint. Engines that cannot produce a cancel
id before execution capture nothing and stay non-cancellable, exactly as
before. Async (GTF) chart queries keep using their own abort path.
The one-line annotation in `superset/charts/api.py` widens
`openapi_spec_component_schemas` to `tuple[type[Schema], ...]` — matching how
`BaseSupersetApi` already declares it — so a subclass can extend it without
mypy flagging the override against an inferred fixed-length tuple.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No visual change. The Stop button looks and behaves the same in the UI; what
changes is that the query actually dies in the database.
### TESTING INSTRUCTIONS
Automated:
```bash
pytest tests/unit_tests/tasks/test_query_cancel.py
tests/unit_tests/charts/test_chart_data_api.py
cd superset-frontend && npm run test --
src/components/Chart/chartReducers.test.ts
src/explore/exploreUtils/exploreUtils.test.tsx
```
Manual, against a covered engine (Postgres, MySQL, Redshift, Snowflake,
SingleStore, Ocient):
1. Point Superset at the database and open its query monitoring UI.
2. In Explore, build a chart whose query runs for a while (a heavy
aggregation, or `SELECT ... FROM huge_table` via a virtual dataset).
3. Run the chart and confirm the query shows as running in the database.
4. Click **Stop**.
5. The query should transition to cancelled/killed in the database, not keep
running to completion. (Use Postgres/MySQL/Snowflake/Redshift here — per
**Engine coverage** above, Trino and Presto are not covered yet.)
6. Regression check: SQL Lab's own Stop button still works unchanged.
To verify the authorization boundary by hand: note the `client_id` in a
running chart's `/api/v1/chart/data` request body, then as a *different* user
`POST /api/v1/chart/data/stop` with that `client_id`. It returns `{"result":
{"stopped": false}}` and the first user's query keeps running.
### ADDITIONAL INFORMATION
- [x] Has associated issue: #35347 (partially — see Engine coverage)
- [ ] Required feature flags:
- [x] 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
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
Supersedes #36350.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
