You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/09/23 01:51:46 UTC
[PR] fix(explore): make the Stop button cancel the running chart query [superset]
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44552:
URL: https://github.com/apache/superset/pull/44552#issuecomment-5787593126
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4118875136
##########
superset/charts/schemas.py:
##########
@@ -1657,6 +1657,18 @@ def rename_deprecated_fields(
)
+class ChartDataStopSchema(Schema):
Review Comment:
Added, matches `ChartDataTimingSchema`'s one-liner.
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,142 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ from flask import current_app
+
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
+
+
+def _publish_cancel_handle(
+ user_id: int, client_id: str, database_id: int, cancel_query_id: str
+) -> None:
+ """Publish a cancel handle for the owning user. Best-effort."""
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.set(
+ _registry_key(user_id, client_id),
+ {"database_id": database_id, "cancel_query_id": cancel_query_id},
+ timeout=_registry_ttl(),
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ # Forfeits cancellability for this query; never breaks its execution.
+ logger.warning("Could not publish chart query cancel handle", exc_info=True)
+
+
+def _discard_cancel_handle(user_id: int, client_id: str) -> None:
+ """Drop a cancel handle once its query is no longer running. Best-effort."""
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.delete(_registry_key(user_id, client_id))
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not discard chart query cancel handle", exc_info=True)
+
+
+def cancel_chart_query_for_user(client_id: str) -> bool:
+ """Cancel the requesting user's running chart query, if it is cancellable.
+
+ :returns: True if the engine reported the query cancelled. False covers both
+ "no such in-flight query for this user" and "the engine declined" — the
+ caller cannot distinguish them, which is deliberate: it keeps the
+ endpoint from confirming whether a given ``client_id`` exists.
+ """
+ from superset.daos.database import DatabaseDAO
+ from superset.extensions import cache_manager
+ from superset.utils.core import get_user_id
Review Comment:
Traced it, and these turn out to be load-bearing rather than sloppy. test_query_cancel.py patches get_user_id/cache_manager at their defining modules (superset.utils.core, superset.extensions), and a module-level `from ... import X` binds X once at import time, before any patch runs, so the patch would silently never take effect. Confirmed by actually hoisting all six and watching 4 of the 24 tests fail before reverting.
current_app was the one exception, nothing patches it, so that one's moved to a normal top-level import. Added a comment at each remaining site explaining why it stays inline.
##########
superset-frontend/src/explore/types.ts:
##########
@@ -61,6 +61,8 @@ export interface ChartState {
latestQueryFormData: LatestQueryFormData;
sliceFormData: QueryFormData | null;
queryController: AbortController | null;
+ /** Client-generated id of the in-flight query, used to cancel it server-side. */
+ latestQueryId?: string;
Review Comment:
Traced this one, and I don't think it's reachable: RunQueryButton only renders Stop while chartStatus === 'loading' (the Run/Update button otherwise), and CHART_UPDATE_STARTED is exactly what sets latestQueryId, right as that same loading window opens. So by the time Stop could actually be clicked, latestQueryId always names the query that's currently loading, there's no path where it survives to name a finished one, since finishing swaps the button away before anything else could read it. Agreed the field would be misleading if something else ever read it after the fact, but nothing does today, so I'd rather not add dead-code clearing for it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4318741888
##########
superset/tasks/query_cancel.py:
##########
@@ -168,3 +177,150 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
+
+
+def _publish_cancel_handle(
+ user_id: int, client_id: str, database_id: int, cancel_query_id: str
+) -> None:
+ """Publish a cancel handle for the owning user. Best-effort."""
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.set(
+ _registry_key(user_id, client_id),
+ {"database_id": database_id, "cancel_query_id": cancel_query_id},
+ timeout=_registry_ttl(),
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ # Forfeits cancellability for this query; never breaks its execution.
+ logger.warning("Could not publish chart query cancel handle", exc_info=True)
+
+
+def _discard_cancel_handle(user_id: int, client_id: str) -> None:
+ """Drop a cancel handle once its query is no longer running. Best-effort."""
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.delete(_registry_key(user_id, client_id))
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not discard chart query cancel handle", exc_info=True)
+
+
+def cancel_chart_query_for_user(client_id: str) -> bool:
+ """Cancel the requesting user's running chart query, if it is cancellable.
+
+ :returns: True if the engine reported the query cancelled. False covers both
+ "no such in-flight query for this user" and "the engine declined" — the
+ caller cannot distinguish them, which is deliberate: it keeps the
+ endpoint from confirming whether a given ``client_id`` exists.
+ """
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.daos.database import DatabaseDAO
+ from superset.extensions import cache_manager
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if user_id is None:
+ return False
+
+ try:
+ handle = cache_manager.cache.get(_registry_key(user_id, client_id))
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not read chart query cancel handle", exc_info=True)
+ return False
+
+ if not handle:
+ return False
+
+ database = DatabaseDAO.find_by_id(handle["database_id"])
+ if database is None:
+ return False
+
+ cancelled = cancel_chart_query(database, handle["cancel_query_id"])
+ if cancelled:
+ _discard_cancel_handle(user_id, client_id)
Review Comment:
After Stop cancels the active query, `get_payload_result` still starts the remaining query objects; a Stop before/between queries is lost entirely. Preventing that requires cancellation intent shared by the endpoint and execution loop.
##########
superset/common/query_context_processor.py:
##########
@@ -328,7 +329,15 @@ def get_df_payload_result(
)
)
- query_result = self.get_query_result(query_obj)
+ # Make the warehouse query cancellable by its owner for the span
+ # of its execution, so Explore's Stop button kills the query
+ # rather than just abandoning the response. No-op for engines
+ # without cancel support, and for requests carrying no client_id.
+ with cancellable_chart_query(
Review Comment:
Contribution totals run through `ensure_totals_available` before this capture scope, so Stop cannot cancel that warehouse query even on covered engines. Its execution needs a separate capture scope outside this commentable span.
##########
superset/common/query_context_processor.py:
##########
@@ -328,7 +329,15 @@ def get_df_payload_result(
)
)
- query_result = self.get_query_result(query_obj)
+ # Make the warehouse query cancellable by its owner for the span
+ # of its execution, so Explore's Stop button kills the query
+ # rather than just abandoning the response. No-op for engines
+ # without cancel support, and for requests carrying no client_id.
+ with cancellable_chart_query(
+ self._query_context.client_id,
+ getattr(self._qc_datasource, "database", None),
+ ):
+ query_result = self.get_query_result(query_obj)
Review Comment:
Cancelling an early grouping-set fallback level returns a FAILED frame, but later levels overwrite its status and the partial result is cached as SUCCESS. Cancellation must propagate through the fallback and cache paths; a one-span fix is insufficient.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4117663911
##########
superset/charts/schemas.py:
##########
@@ -1657,6 +1657,18 @@ def rename_deprecated_fields(
)
+class ChartDataStopSchema(Schema):
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing class docstring</b></div>
<div id="fix">
New class `ChartDataStopSchema` has no docstring; AGENTS.md and .cursor/rules/dev-standard.mdc require docstrings on new classes. A one-liner noting it is the request body for POST /api/v1/chart/data/stop keeps the file consistent with documented classes like `ChartDataTimingSchema`.
</div>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a href="https://github.com/apache/superset/blob/d3508a2/AGENTS.md#L146">AGENTS.md:146</a>
</li>
<li>
Rule Violated: <a href="https://github.com/apache/superset/blob/d3508a2/.cursor/rules/dev-standard.mdc#L118">dev-standard.mdc:118</a>
</li>
</ul>
</details>
<small><i>Code Review Run #93932e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/types.ts:
##########
@@ -61,6 +61,8 @@ export interface ChartState {
latestQueryFormData: LatestQueryFormData;
sliceFormData: QueryFormData | null;
queryController: AbortController | null;
+ /** Client-generated id of the in-flight query, used to cancel it server-side. */
+ latestQueryId?: string;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Stale query id lifecycle</b></div>
<div id="fix">
`latestQueryId` is set on CHART_UPDATE_STARTED but never cleared on STOPPED/FAILED/SUCCEEDED (chartReducer.ts:78). After a query finishes, `onStop` (ExploreViewContainer/index.tsx:605) still reads the last-started id and POSTs `/api/v1/chart/data/stop` with a stale `client_id`. Backend scoping makes this a harmless cache miss, but the field's documented meaning ('in-flight query') is misleading. Clear it when the query stops/fails.
</div>
</div>
<small><i>Code Review Run #93932e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,142 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ from flask import current_app
+
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
+
+
+def _publish_cancel_handle(
+ user_id: int, client_id: str, database_id: int, cancel_query_id: str
+) -> None:
+ """Publish a cancel handle for the owning user. Best-effort."""
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.set(
+ _registry_key(user_id, client_id),
+ {"database_id": database_id, "cancel_query_id": cancel_query_id},
+ timeout=_registry_ttl(),
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ # Forfeits cancellability for this query; never breaks its execution.
+ logger.warning("Could not publish chart query cancel handle", exc_info=True)
+
+
+def _discard_cancel_handle(user_id: int, client_id: str) -> None:
+ """Drop a cancel handle once its query is no longer running. Best-effort."""
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.delete(_registry_key(user_id, client_id))
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not discard chart query cancel handle", exc_info=True)
+
+
+def cancel_chart_query_for_user(client_id: str) -> bool:
+ """Cancel the requesting user's running chart query, if it is cancellable.
+
+ :returns: True if the engine reported the query cancelled. False covers both
+ "no such in-flight query for this user" and "the engine declined" — the
+ caller cannot distinguish them, which is deliberate: it keeps the
+ endpoint from confirming whether a given ``client_id`` exists.
+ """
+ from superset.daos.database import DatabaseDAO
+ from superset.extensions import cache_manager
+ from superset.utils.core import get_user_id
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inline imports lack justification</b></div>
<div id="fix">
The new functions use six inline imports (`get_user_id` at 219, `cache_manager` at 258 and 273, `DatabaseDAO`/`cache_manager`/`get_user_id` at 289-291, `current_app` at 196) with no explanatory comment. BITO adaptive rule 12745 allows inline imports only to prevent circular dependencies and requires a justifying comment; none of these sites has one. `superset.extensions` and `superset.utils.core` are import-safe singletons/util modules, and the module already imports `flask` symbols at top level elsewhere. Prefer module-level imports, or document the cycle each deferred import avoids.
</div>
</div>
<small><i>Code Review Run #93932e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/common/query_context_processor.py:
##########
@@ -305,7 +306,15 @@ def get_df_payload_result(
)
)
- query_result = self.get_query_result(query_obj)
+ # Make the warehouse query cancellable by its owner for the span
+ # of its execution, so Explore's Stop button kills the query
+ # rather than just abandoning the response. No-op for engines
+ # without cancel support, and for requests carrying no client_id.
+ with cancellable_chart_query(
+ self._query_context.client_id,
+ getattr(self._qc_datasource, "database", None),
+ ):
+ query_result = self.get_query_result(query_obj)
annotation_data = self.get_annotation_data(query_obj)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Annotation query not cancellable</b></div>
<div id="fix">
The change wraps only `self.get_query_result(query_obj)` in `cancellable_chart_query`, but `self.get_annotation_data(query_obj)` on the next line runs a second warehouse query for the same request outside the guard, so it stays non-cancellable and its cursor is never registered with `notify_cursor`. If the intent is that Explore's Stop kills this request's warehouse work, include the annotation query in the same `with` block.
</div>
</div>
<small><i>Code Review Run #93932e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4333474718
##########
superset/common/query_context_processor.py:
##########
@@ -560,8 +574,36 @@ def get_query_result(self, query_object: QueryObject) -> QueryResult:
"""
if query_object.grouping_sets and not self._supports_grouping_sets():
return self._grouping_sets_fallback(query_object)
+ return self._execute_query_object(query_object)
+
+ def _execute_query_object(self, query_object: QueryObject) -> QueryResult:
+ """Run one statement on the datasource, unless its owner asked to stop.
+
+ Every statement of a chart-data run passes through here, so a Stop
+ that lands before the run starts, or between two of its statements,
+ is honoured at the next one instead of being lost. Cancelling the
+ statement executing right now is ``cancellable_chart_query``'s job.
+ """
+ if chart_query_cancel_requested(self._query_context.client_id):
+ return self._stopped_query_result()
return self._qc_datasource.get_query_result(query_object)
Review Comment:
A Stop that kills a time-offset subquery leaves an empty comparison frame, but the parent still reports SUCCESS and caches it. Recheck the stop intent after the datasource call so cancelled results cannot become cache hits.
```suggestion
if chart_query_cancel_requested(self._query_context.client_id):
return self._stopped_query_result()
result = self._qc_datasource.get_query_result(query_object)
if chart_query_cancel_requested(self._query_context.client_id):
return self._stopped_query_result()
return result
```
##########
superset/tasks/query_cancel.py:
##########
@@ -168,3 +177,266 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _cancel_requested_key(user_id: int, client_id: str) -> str:
+ """Cache key recording that a user asked to stop a chart query run.
+
+ Scoped by user for the same reason as ``_registry_key``: a ``client_id``
+ can only ever flag the requester's own run.
+ """
+ return f"chart-query-cancel-requested:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
Review Comment:
A Stop during connection setup records intent and misses the unpublished handle. This sink then publishes without checking that intent, so the statement still executes; internal time-offset statements bypass the only check too.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44552:
URL: https://github.com/apache/superset/pull/44552#issuecomment-5787641128
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44552?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `35.55556%` with `58 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.18%. Comparing base ([`350dd0d`](https://app.codecov.io/gh/apache/superset/commit/350dd0d60ece9f1aff08d3b89b16c89aa4c1fe8a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`cccdd4a`](https://app.codecov.io/gh/apache/superset/commit/cccdd4a0242e9b63ac0c7fe5e5853ea2716fdab4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44552?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/tasks/query\_cancel.py](https://app.codecov.io/gh/apache/superset/pull/44552?src=pr&el=tree&filepath=superset%2Ftasks%2Fquery_cancel.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdGFza3MvcXVlcnlfY2FuY2VsLnB5) | 17.46% | [51 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/44552?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/charts/data/api.py](https://app.codecov.io/gh/apache/superset/pull/44552?src=pr&el=tree&filepath=superset%2Fcharts%2Fdata%2Fapi.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY2hhcnRzL2RhdGEvYXBpLnB5) | 64.70% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44552?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (350dd0d) and HEAD (cccdd4a). Click for more details.
>
> <details><summary>HEAD has 25 uploads less than BASE</summary>
>
>| Flag | BASE (350dd0d) | HEAD (cccdd4a) |
>|------|------|------|
>|python|21|2|
>|presto|4|1|
>|hive|4|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44552 +/- ##
==========================================
- Coverage 66.20% 57.18% -9.02%
==========================================
Files 2948 2949 +1
Lines 177295 177456 +161
Branches 41089 41109 +20
==========================================
- Hits 117376 101487 -15889
- Misses 57416 74993 +17577
+ Partials 2503 976 -1527
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44552/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44552/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.91% <27.77%> (-0.02%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44552/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44552/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44552/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.86% <35.55%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44552/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.90% <35.55%> (-17.59%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44552/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44552?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4118875874
##########
superset/charts/schemas.py:
##########
@@ -1657,6 +1657,18 @@ def rename_deprecated_fields(
)
+class ChartDataStopSchema(Schema):
Review Comment:
<!-- Bito Reply -->
The suggestion to add a docstring to the `ChartDataStopSchema` class is appropriate. Adding a docstring ensures consistency with existing classes like `ChartDataTimingSchema` and adheres to the project's documentation standards.
**superset/charts/schemas.py**
```
+class ChartDataStopSchema(Schema):
+ """Schema for the stop request body for POST /api/v1/chart/data/stop."""
```
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,142 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ from flask import current_app
+
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
+
+
+def _publish_cancel_handle(
+ user_id: int, client_id: str, database_id: int, cancel_query_id: str
+) -> None:
+ """Publish a cancel handle for the owning user. Best-effort."""
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.set(
+ _registry_key(user_id, client_id),
+ {"database_id": database_id, "cancel_query_id": cancel_query_id},
+ timeout=_registry_ttl(),
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ # Forfeits cancellability for this query; never breaks its execution.
+ logger.warning("Could not publish chart query cancel handle", exc_info=True)
+
+
+def _discard_cancel_handle(user_id: int, client_id: str) -> None:
+ """Drop a cancel handle once its query is no longer running. Best-effort."""
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.delete(_registry_key(user_id, client_id))
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not discard chart query cancel handle", exc_info=True)
+
+
+def cancel_chart_query_for_user(client_id: str) -> bool:
+ """Cancel the requesting user's running chart query, if it is cancellable.
+
+ :returns: True if the engine reported the query cancelled. False covers both
+ "no such in-flight query for this user" and "the engine declined" — the
+ caller cannot distinguish them, which is deliberate: it keeps the
+ endpoint from confirming whether a given ``client_id`` exists.
+ """
+ from superset.daos.database import DatabaseDAO
+ from superset.extensions import cache_manager
+ from superset.utils.core import get_user_id
Review Comment:
<!-- Bito Reply -->
The reviewer's concern regarding the lack of justification for inline imports is noted. However, these imports are necessary to avoid circular dependencies, as confirmed by testing where module-level imports caused patches to fail. The `current_app` import was moved to the top level as it does not have this issue, and comments have been added to the remaining inline import sites to explain their necessity.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4336410058
##########
superset/tasks/query_cancel.py:
##########
@@ -168,3 +186,275 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _cancel_requested_key(user_id: int, client_id: str) -> str:
+ """Cache key recording that a user asked to stop a chart query run.
+
+ Scoped by user for the same reason as ``_registry_key``: a ``client_id``
+ can only ever flag the requester's own run.
+ """
+ return f"chart-query-cancel-requested:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ # A Stop recorded before this statement had a handle to cancel is
+ # honoured by not running the statement at all.
+ if chart_query_cancel_requested(query_id):
+ raise ChartQueryStoppedError(_("Query was stopped"))
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+ # A Stop landing between the check above and the publish saw no handle
+ # to kill; recheck now that the handle is visible.
+ if chart_query_cancel_requested(query_id):
+ _discard_cancel_handle(owner_id, query_id, cancel_id)
+ raise ChartQueryStoppedError(_("Query was stopped"))
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
+
+
+def _publish_cancel_handle(
+ user_id: int, client_id: str, database_id: int, cancel_query_id: str
+) -> None:
+ """Publish a cancel handle for the owning user. Best-effort."""
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.chart_query_cancel_cache.set(
+ _registry_key(user_id, client_id),
+ {"database_id": database_id, "cancel_query_id": cancel_query_id},
+ timeout=_registry_ttl(),
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ # Forfeits cancellability for this query; never breaks its execution.
+ logger.warning("Could not publish chart query cancel handle", exc_info=True)
+
+
+def _discard_cancel_handle(
+ user_id: int, client_id: str, expected_cancel_query_id: "str | None" = None
+) -> None:
+ """Drop a cancel handle once its query is no longer running. Best-effort.
+
+ When ``expected_cancel_query_id`` is given, the entry is only deleted if it
+ still names that id. Sequential cursors within one query (e.g. the
+ grouping-sets fallback) republish the same key as each statement starts;
+ without this check, discarding a handle resolved just before a newer
+ cursor's republish would delete that newer, still-live entry instead of
+ the stale one that was actually acted on.
+ """
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ key = _registry_key(user_id, client_id)
+ try:
+ if expected_cancel_query_id is not None:
+ current = cache_manager.chart_query_cancel_cache.get(key)
+ if (
+ not current
+ or current.get("cancel_query_id") != expected_cancel_query_id
+ ):
+ return
+ cache_manager.chart_query_cancel_cache.delete(key)
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not discard chart query cancel handle", exc_info=True)
+
+
+def _request_chart_query_cancel(user_id: int, client_id: str) -> None:
+ """Record that the user asked to stop this run. Best-effort.
+
+ Killing the statement that is executing right now is only half of a Stop:
+ a chart-data run can be several statements (contribution totals, the
+ grouping-sets fallback's one-query-per-level, one query object after
+ another), and a Stop that lands before the first starts, or in the gap
+ between two, finds no handle to cancel. The intent recorded here is what
+ the execution loop checks before it starts each statement (see
+ ``chart_query_cancel_requested``), so the rest of the run is skipped
+ instead of carrying on as if nothing happened.
+ """
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.chart_query_cancel_cache.set(
+ _cancel_requested_key(user_id, client_id),
+ True,
+ timeout=_registry_ttl(),
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not record chart query cancel request", exc_info=True)
+
+
+def chart_query_cancel_requested(client_id: "str | None") -> bool:
+ """Whether the requesting user has asked to stop the run named by ``client_id``.
+
+ Checked by the execution loop before each statement of a chart-data run.
+ False without a ``client_id`` or a user (nothing could have been
+ requested), and on a cache failure (never block a query on the registry).
+ """
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or user_id is None:
+ return False
+
+ try:
+ return bool(
+ cache_manager.chart_query_cancel_cache.get(
+ _cancel_requested_key(user_id, client_id)
+ )
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not read chart query cancel request", exc_info=True)
+ return False
+
+
+@contextmanager
+def chart_query_run(client_id: "str | None") -> Iterator[None]:
+ """Scope one chart-data run so its cancel intent is cleared when it ends.
+
+ The intent is deliberately *not* cleared on entry: a Stop that arrives
+ before the run reaches its first statement must still take effect. Each
+ run carries a fresh ``client_id``, so an intent recorded for a run that is
+ already over can never leak into a later one; clearing it here just keeps
+ the registry tidy rather than leaving it to the TTL.
+ """
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or user_id is None:
+ yield
+ return
+
+ try:
+ yield
+ finally:
+ try:
+ cache_manager.chart_query_cancel_cache.delete(
+ _cancel_requested_key(user_id, client_id)
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not clear chart query cancel request", exc_info=True)
+
+
+def cancel_chart_query_for_user(client_id: str) -> bool:
+ """Cancel the requesting user's running chart query, if it is cancellable.
+
+ Always records the cancel intent for the run first, so whatever the run
+ has not started yet is skipped at its next checkpoint even when there is
+ no statement executing right now to kill.
+
+ :returns: True if the engine reported the query cancelled. False covers both
+ "no such in-flight query for this user" and "the engine declined" — the
+ caller cannot distinguish them, which is deliberate: it keeps the
+ endpoint from confirming whether a given ``client_id`` exists.
+ """
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.daos.database import DatabaseDAO
+ from superset.extensions import cache_manager
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if user_id is None:
+ return False
+
+ _request_chart_query_cancel(user_id, client_id)
+
+ try:
+ handle = cache_manager.chart_query_cancel_cache.get(
+ _registry_key(user_id, client_id)
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not read chart query cancel handle", exc_info=True)
+ return False
+
+ if not handle:
+ return False
+
+ database = DatabaseDAO.find_by_id(handle["database_id"])
Review Comment:
With `all_datasource_access` but no per-database grants, chart queries can run but this lookup filters out their database, so Stop never calls engine cancellation. The user-scoped handle already identifies the database of the caller's authorized query.
```suggestion
database = DatabaseDAO.find_by_id(handle["database_id"], skip_base_filter=True)
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4078642368
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -584,9 +585,26 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
);
function onStop() {
- if (props.chart && props.chart.queryController) {
+ // Abort the in-flight HTTP request so the UI stops waiting immediately.
+ if (props.chart?.queryController) {
props.chart.queryController.abort();
}
+
+ // Aborting only drops the response; the database keeps executing the query.
+ // Ask the backend to cancel it too. The backend resolves `client_id` within
+ // the requesting user's own in-flight queries, so this can only ever cancel
+ // our own query. A falsy `stopped` just means there was nothing running to
+ // cancel (or the engine has no cancel support), which is not an error.
+ const clientId = props.chart?.latestQueryId;
+ if (clientId) {
+ SupersetClient.post({
+ endpoint: '/api/v1/chart/data/stop',
+ body: JSON.stringify({ client_id: clientId }),
+ headers: { 'Content-Type': 'application/json' },
+ }).catch(() => {
+ props.addDangerToast(t('Failed to stop query.'));
+ });
+ }
Review Comment:
<!-- Bito Reply -->
The suggestion to extract a shared `stopQuery` helper is appropriate, as it promotes code reuse and ensures consistent handling of CSRF headers across the application. Regarding the error handling, logging the error before displaying the toast is a good practice that improves observability for server-side failures.
##########
tests/unit_tests/tasks/test_query_cancel.py:
##########
@@ -205,3 +206,208 @@ def test_capture_is_noop_without_a_database() -> None:
with _capture_query_cancellation(qc):
assert _cancel_id_sink.get() is None
get_context.assert_not_called()
+
+
+# --- synchronous Explore cancellation registry ----------------------------
+
+
+class _FakeCache:
+ """Minimal dict-backed stand-in for ``cache_manager.cache``.
+
+ Real storage (rather than a MagicMock) so the user-scoping of the registry
+ keys is genuinely exercised end to end.
+ """
+
+ def __init__(self) -> None:
+ self.store: dict[str, object] = {}
+
+ def set(self, key: str, value: object, timeout: int | None = None) -> None:
+ self.store[key] = value
+
+ def get(self, key: str) -> object | None:
+ return self.store.get(key)
+
+ def delete(self, key: str) -> None:
+ self.store.pop(key, None)
+
+
+@contextmanager
+def _registry_env(user_id: int | None, cache: "_FakeCache"):
+ """Run with a given current user and a shared fake cache backend."""
+ # ``cache_manager.cache`` is a read-only property; swap the backing attribute.
+ with (
+ patch("superset.utils.core.get_user_id", return_value=user_id),
+ patch("superset.extensions.cache_manager._cache", cache),
+ patch("superset.tasks.query_cancel._registry_ttl", return_value=60),
+ ):
+ yield
+
+
+def _run_cancellable(client_id, database, cache, user_id, cancel_id="engine-1"):
+ """Drive one cancellable query, returning whether a handle was live inside it."""
+ from superset.tasks.query_cancel import cancellable_chart_query
+
+ with _registry_env(user_id, cache):
+ with patch(
+ "superset.tasks.query_cancel.capture_cancel_query_id",
+ return_value=cancel_id,
+ ):
+ with cancellable_chart_query(client_id, database):
+ # Stands in for Database._execute_sql_with_mutation_and_logging
+ # handing the live cursor to the active sink before executing.
+ notify_cursor(MagicMock())
+ inside = dict(cache.store)
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to annotate local variables in the test file is a stylistic preference. Since the project does not currently enforce explicit type annotations for inferable local variables in tests, you are not required to apply this change. You may choose to ignore this suggestion if it does not align with the existing codebase conventions.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44552:
URL: https://github.com/apache/superset/pull/44552#issuecomment-5787611021
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | cccdd4a0242e9b63ac0c7fe5e5853ea2716fdab4 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ab330b24c886700085a86d5 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44552--superset-docs-preview.netlify.app](https://deploy-preview-44552--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#Dco7DoAgDADQu3S2SSOCxBt4jPJJGFRqxcEY7y7bG94LonWXBgvAAEH5iKWbU5WGomicsYRXq4KxsDY876xPrynLVp819ew4GENhnKL3biYib9m7ZOH7AQ) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44552--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4336708534
##########
superset/tasks/query_cancel.py:
##########
@@ -168,3 +186,275 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _cancel_requested_key(user_id: int, client_id: str) -> str:
+ """Cache key recording that a user asked to stop a chart query run.
+
+ Scoped by user for the same reason as ``_registry_key``: a ``client_id``
+ can only ever flag the requester's own run.
+ """
+ return f"chart-query-cancel-requested:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ # A Stop recorded before this statement had a handle to cancel is
+ # honoured by not running the statement at all.
+ if chart_query_cancel_requested(query_id):
+ raise ChartQueryStoppedError(_("Query was stopped"))
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+ # A Stop landing between the check above and the publish saw no handle
+ # to kill; recheck now that the handle is visible.
+ if chart_query_cancel_requested(query_id):
+ _discard_cancel_handle(owner_id, query_id, cancel_id)
+ raise ChartQueryStoppedError(_("Query was stopped"))
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
+
+
+def _publish_cancel_handle(
+ user_id: int, client_id: str, database_id: int, cancel_query_id: str
+) -> None:
+ """Publish a cancel handle for the owning user. Best-effort."""
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.chart_query_cancel_cache.set(
+ _registry_key(user_id, client_id),
+ {"database_id": database_id, "cancel_query_id": cancel_query_id},
+ timeout=_registry_ttl(),
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ # Forfeits cancellability for this query; never breaks its execution.
+ logger.warning("Could not publish chart query cancel handle", exc_info=True)
+
+
+def _discard_cancel_handle(
+ user_id: int, client_id: str, expected_cancel_query_id: "str | None" = None
+) -> None:
+ """Drop a cancel handle once its query is no longer running. Best-effort.
+
+ When ``expected_cancel_query_id`` is given, the entry is only deleted if it
+ still names that id. Sequential cursors within one query (e.g. the
+ grouping-sets fallback) republish the same key as each statement starts;
+ without this check, discarding a handle resolved just before a newer
+ cursor's republish would delete that newer, still-live entry instead of
+ the stale one that was actually acted on.
+ """
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ key = _registry_key(user_id, client_id)
+ try:
+ if expected_cancel_query_id is not None:
+ current = cache_manager.chart_query_cancel_cache.get(key)
+ if (
+ not current
+ or current.get("cancel_query_id") != expected_cancel_query_id
+ ):
+ return
+ cache_manager.chart_query_cancel_cache.delete(key)
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not discard chart query cancel handle", exc_info=True)
+
+
+def _request_chart_query_cancel(user_id: int, client_id: str) -> None:
+ """Record that the user asked to stop this run. Best-effort.
+
+ Killing the statement that is executing right now is only half of a Stop:
+ a chart-data run can be several statements (contribution totals, the
+ grouping-sets fallback's one-query-per-level, one query object after
+ another), and a Stop that lands before the first starts, or in the gap
+ between two, finds no handle to cancel. The intent recorded here is what
+ the execution loop checks before it starts each statement (see
+ ``chart_query_cancel_requested``), so the rest of the run is skipped
+ instead of carrying on as if nothing happened.
+ """
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.chart_query_cancel_cache.set(
+ _cancel_requested_key(user_id, client_id),
+ True,
+ timeout=_registry_ttl(),
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not record chart query cancel request", exc_info=True)
+
+
+def chart_query_cancel_requested(client_id: "str | None") -> bool:
+ """Whether the requesting user has asked to stop the run named by ``client_id``.
+
+ Checked by the execution loop before each statement of a chart-data run.
+ False without a ``client_id`` or a user (nothing could have been
+ requested), and on a cache failure (never block a query on the registry).
+ """
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or user_id is None:
+ return False
+
+ try:
+ return bool(
+ cache_manager.chart_query_cancel_cache.get(
+ _cancel_requested_key(user_id, client_id)
+ )
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not read chart query cancel request", exc_info=True)
+ return False
+
+
+@contextmanager
+def chart_query_run(client_id: "str | None") -> Iterator[None]:
+ """Scope one chart-data run so its cancel intent is cleared when it ends.
+
+ The intent is deliberately *not* cleared on entry: a Stop that arrives
+ before the run reaches its first statement must still take effect. Each
+ run carries a fresh ``client_id``, so an intent recorded for a run that is
+ already over can never leak into a later one; clearing it here just keeps
+ the registry tidy rather than leaving it to the TTL.
+ """
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or user_id is None:
+ yield
+ return
+
+ try:
+ yield
+ finally:
+ try:
+ cache_manager.chart_query_cancel_cache.delete(
+ _cancel_requested_key(user_id, client_id)
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not clear chart query cancel request", exc_info=True)
+
+
+def cancel_chart_query_for_user(client_id: str) -> bool:
+ """Cancel the requesting user's running chart query, if it is cancellable.
+
+ Always records the cancel intent for the run first, so whatever the run
+ has not started yet is skipped at its next checkpoint even when there is
+ no statement executing right now to kill.
+
+ :returns: True if the engine reported the query cancelled. False covers both
+ "no such in-flight query for this user" and "the engine declined" — the
+ caller cannot distinguish them, which is deliberate: it keeps the
+ endpoint from confirming whether a given ``client_id`` exists.
+ """
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.daos.database import DatabaseDAO
+ from superset.extensions import cache_manager
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if user_id is None:
+ return False
+
+ _request_chart_query_cancel(user_id, client_id)
+
+ try:
+ handle = cache_manager.chart_query_cancel_cache.get(
+ _registry_key(user_id, client_id)
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not read chart query cancel handle", exc_info=True)
+ return False
+
+ if not handle:
+ return False
+
+ database = DatabaseDAO.find_by_id(handle["database_id"])
Review Comment:
Good catch, @aminghadersohi. The handle is user-scoped and only written by an authorized run, so the lookup now uses `skip_base_filter=True`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4118876363
##########
superset/common/query_context_processor.py:
##########
@@ -305,7 +306,15 @@ def get_df_payload_result(
)
)
- query_result = self.get_query_result(query_obj)
+ # Make the warehouse query cancellable by its owner for the span
+ # of its execution, so Explore's Stop button kills the query
+ # rather than just abandoning the response. No-op for engines
+ # without cancel support, and for requests carrying no client_id.
+ with cancellable_chart_query(
+ self._query_context.client_id,
+ getattr(self._qc_datasource, "database", None),
+ ):
+ query_result = self.get_query_result(query_obj)
annotation_data = self.get_annotation_data(query_obj)
Review Comment:
This one's real, and I don't think it's a safe one-line fix. get_viz_annotation_data can run a genuine second warehouse query, against a *different* chart's own datasource than the one cancellable_chart_query was opened for. The context manager's capture sink closes over a single fixed target database (the outer chart's), so naively pulling get_annotation_data into the same `with` block would misattribute any captured cancel id to the wrong database whenever the annotation layer points at a chart on a different one, cancelling nothing, or worse, the wrong session.
Same category as the Trino gap already called out in the PR description, real, not something to guess a fix for here. Adding it to that section so it's not just sitting in a resolved thread. Follow-up would need cancellable_chart_query to accept (or the sink to resolve) a database per capture rather than one fixed at open time.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4078643962
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,140 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ from flask import current_app
+
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ nonlocal captured
+ if captured:
+ return
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `2547ad5`.
The cursor sink now captures and publishes a cancellation handle for every cursor, rather than using a `captured` guard that only allowed the first handle to be stored.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4304741714
##########
tests/unit_tests/tasks/test_query_cancel.py:
##########
@@ -205,3 +206,249 @@ def test_capture_is_noop_without_a_database() -> None:
with _capture_query_cancellation(qc):
assert _cancel_id_sink.get() is None
get_context.assert_not_called()
+
+
+# --- synchronous Explore cancellation registry ----------------------------
+
+
+class _FakeCache:
+ """Minimal dict-backed stand-in for ``cache_manager.cache``.
+
+ Real storage (rather than a MagicMock) so the user-scoping of the registry
+ keys is genuinely exercised end to end.
+ """
+
+ def __init__(self) -> None:
+ self.store: dict[str, object] = {}
+
+ def set(self, key: str, value: object, timeout: int | None = None) -> None:
+ self.store[key] = value
+
+ def get(self, key: str) -> object | None:
+ return self.store.get(key)
+
+ def delete(self, key: str) -> None:
+ self.store.pop(key, None)
+
+
+@contextmanager
+def _registry_env(user_id: int | None, cache: "_FakeCache"):
+ """Run with a given current user and a shared fake cache backend."""
+ # ``cache_manager.cache`` is a read-only property; swap the backing attribute.
+ with (
+ patch("superset.utils.core.get_user_id", return_value=user_id),
+ patch("superset.extensions.cache_manager._cache", cache),
+ patch("superset.tasks.query_cancel._registry_ttl", return_value=60),
+ ):
+ yield
+
+
+def _run_cancellable(client_id, database, cache, user_id, cancel_id="engine-1"):
+ """Drive one cancellable query, returning whether a handle was live inside it."""
+ from superset.tasks.query_cancel import cancellable_chart_query
+
+ with _registry_env(user_id, cache):
+ with patch(
+ "superset.tasks.query_cancel.capture_cancel_query_id",
+ return_value=cancel_id,
+ ):
+ with cancellable_chart_query(client_id, database):
+ # Stands in for Database._execute_sql_with_mutation_and_logging
+ # handing the live cursor to the active sink before executing.
+ notify_cursor(MagicMock())
+ inside = dict(cache.store)
+ return inside
+
+
+def test_registry_key_is_scoped_by_user() -> None:
+ """The same ``client_id`` under two users maps to two distinct keys."""
+ from superset.tasks.query_cancel import _registry_key
+
+ # Same client_id under two users must not collide.
+ assert _registry_key(1, "abc") != _registry_key(2, "abc")
+ assert "abc" in _registry_key(1, "abc")
+
+
+def test_cancellable_chart_query_publishes_then_discards_the_handle() -> None:
+ """A captured handle is live for the query's duration, then removed."""
+ cache = _FakeCache()
+ database = MagicMock()
+ database.id = 5
+
+ inside = _run_cancellable("client-1", database, cache, user_id=1)
+
+ # Live for the duration of the query...
+ assert inside == {
+ "chart-query-cancel:1:client-1": {
+ "database_id": 5,
+ "cancel_query_id": "engine-1",
+ }
+ }
+ # ...and cleaned up once it returns.
+ assert cache.store == {}
+
+
+def test_cancellable_chart_query_republishes_for_each_cursor() -> None:
+ """Every cursor the query opens refreshes the handle to its own session.
+
+ One query object can run several statements in turn (the grouping-sets
+ fallback does), each on whatever pooled connection it gets, so the handle
+ must follow the statement that is executing now rather than the first one.
+ """
+ from superset.tasks.query_cancel import cancellable_chart_query
+
+ cache = _FakeCache()
+ database = MagicMock()
+ database.id = 5
+ seen: list[object] = []
+
+ with _registry_env(1, cache):
+ with patch(
+ "superset.tasks.query_cancel.capture_cancel_query_id",
+ side_effect=["engine-1", "engine-2"],
+ ):
+ with cancellable_chart_query("client-1", database):
+ notify_cursor(MagicMock())
+ seen.append(cache.store["chart-query-cancel:1:client-1"])
+ notify_cursor(MagicMock())
+ seen.append(cache.store["chart-query-cancel:1:client-1"])
+
+ assert seen == [
+ {"database_id": 5, "cancel_query_id": "engine-1"},
+ {"database_id": 5, "cancel_query_id": "engine-2"},
+ ]
+ assert cache.store == {}
+
+
+def test_cancellable_chart_query_is_noop_without_a_client_id() -> None:
+ """Requests that carry no ``client_id`` register nothing."""
+ cache = _FakeCache()
+ database = MagicMock()
+ database.id = 5
+
+ assert _run_cancellable(None, database, cache, user_id=1) == {}
+
+
+def test_cancellable_chart_query_is_noop_without_a_database() -> None:
+ """Datasources without a database (e.g. annotations) register nothing."""
+ cache = _FakeCache()
+
+ assert _run_cancellable("client-1", None, cache, user_id=1) == {}
+
+
+def test_cancellable_chart_query_is_noop_for_an_anonymous_request() -> None:
+ """An anonymous request has no user id to scope a handle to."""
+ # An anonymous viewer has no user id to scope the handle to, and an unscoped
+ # handle would be cancellable by any other anonymous visitor.
+ cache = _FakeCache()
+ database = MagicMock()
+ database.id = 5
+
+ assert _run_cancellable("client-1", database, cache, user_id=None) == {}
+
+
+def test_cancellable_chart_query_publishes_nothing_when_engine_has_no_cancel_id() -> (
+ None
+):
+ """Engines with no pre-execution cancel id leave the query non-cancellable."""
+ cache = _FakeCache()
+ database = MagicMock()
+ database.id = 5
+
+ assert _run_cancellable("client-1", database, cache, 1, cancel_id=None) == {}
+
+
+def test_cancel_chart_query_for_user_cancels_its_own_query() -> None:
+ """Cancelling one's own registered query hits the engine and cleans up."""
+ from superset.tasks.query_cancel import cancel_chart_query_for_user
+
+ cache = _FakeCache()
+ cache.store["chart-query-cancel:1:client-1"] = {
+ "database_id": 5,
+ "cancel_query_id": "engine-1",
+ }
+ database = MagicMock()
+
+ with _registry_env(1, cache):
+ with (
+ patch(
+ "superset.daos.database.DatabaseDAO.find_by_id", return_value=database
+ ),
+ patch(
+ "superset.tasks.query_cancel.cancel_chart_query", return_value=True
+ ) as cancel,
+ ):
+ assert cancel_chart_query_for_user("client-1") is True
+
+ cancel.assert_called_once_with(database, "engine-1")
+ # A cancelled query's handle is dropped rather than left to expire.
+ assert cache.store == {}
+
+
+def test_cancel_chart_query_for_user_cannot_reach_another_users_query() -> None:
+ """Regression test: ``client_id`` is untrusted, client-supplied input.
+
+ User 1 has a running, cancellable query. User 2 asks to cancel that exact
+ ``client_id``. Because the registry key embeds the requesting user, the
+ lookup misses: no engine cancellation is attempted and user 1's handle is
+ left untouched.
+ """
+ from superset.tasks.query_cancel import cancel_chart_query_for_user
+
+ cache = _FakeCache()
+ victim_handle = {"database_id": 5, "cancel_query_id": "engine-1"}
+ cache.store["chart-query-cancel:1:client-1"] = victim_handle
Review Comment:
The literal victim key makes this test pass even if `_registry_key` drops user scoping: the mutated lookup simply misses it. Seed through the production key builder so that regression reaches the cancellation assertions.
```suggestion
from superset.tasks.query_cancel import (
_registry_key,
cancel_chart_query_for_user,
)
cache = _FakeCache()
victim_handle = {"database_id": 5, "cancel_query_id": "engine-1"}
cache.store[_registry_key(1, "client-1")] = victim_handle
```
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,150 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
+
+
+def _publish_cancel_handle(
+ user_id: int, client_id: str, database_id: int, cancel_query_id: str
+) -> None:
+ """Publish a cancel handle for the owning user. Best-effort."""
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.set(
Review Comment:
At `fb560033f051`, default `NullCache` still discards every handle, so Stop never reaches cancellation. A safe fix spans `cache_manager.py`, `config.py` and registry helpers, so no single-span suggestion is possible.
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,150 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
Review Comment:
At `fb560033f051`, if a cursor republishes during a successful cancel, the unconditional delete still erases its handle and Stop cannot reach the live statement. Safe publication/deletion coordination spans multiple sites, so no single-span suggestion is possible.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44552:
URL: https://github.com/apache/superset/pull/44552#issuecomment-6308003828
@aminghadersohi the run-level handling is in. Stop now records the intent itself and every statement checks it before running, so the totals query, fallback levels and later query objects all get skipped rather than carrying on. Specifics on each thread. Mind taking another look?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4078341558
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,140 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ from flask import current_app
+
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ nonlocal captured
+ if captured:
+ return
Review Comment:
**Suggestion:** Grouping-set fallback runs several sequential database queries, but `captured` prevents later cursors from replacing the completed first query's cancel handle.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Incomplete implementation`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=78e5b0d8fada4563a26470aae893fba2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=78e5b0d8fada4563a26470aae893fba2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/tasks/query_cancel.py
**Line:** 234:237
**Comment:**
*Incomplete Implementation: Grouping-set fallback runs several sequential database queries, but `captured` prevents later cursors from replacing the completed first query's cancel handle.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=ec797f881e7eec5ee9352c74d3973efcbe342f1cde94ae4eef9893d9c64ddc23&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=ec797f881e7eec5ee9352c74d3973efcbe342f1cde94ae4eef9893d9c64ddc23&reaction=dislike'>👎</a>
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,140 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ from flask import current_app
+
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ nonlocal captured
+ if captured:
+ return
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
Review Comment:
**Suggestion:** A Stop request can arrive before `_sink` publishes the handle; it then returns false once, while the query continues with no later cancellation attempt.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes` · 🏷️ `Race condition`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7a649d7e08124513a3ac0c85a29c3bd7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7a649d7e08124513a3ac0c85a29c3bd7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/tasks/query_cancel.py
**Line:** 244:246
**Comment:**
*Race Condition: A Stop request can arrive before `_sink` publishes the handle; it then returns false once, while the query continues with no later cancellation attempt.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=ae0aa3033e2f12bd1ac239fb3c160d6ab43e0491b834f428b2c4408f784ee2f0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=ae0aa3033e2f12bd1ac239fb3c160d6ab43e0491b834f428b2c4408f784ee2f0&reaction=dislike'>👎</a>
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,140 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ from flask import current_app
+
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ nonlocal captured
+ if captured:
+ return
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
Review Comment:
**Suggestion:** When two requests reuse the same user and `client_id`, an older query can delete the newer handle during cleanup, so Stop misses the currently running query.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Race condition`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0b261ea3d0484db49301f6b9c82deb7d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0b261ea3d0484db49301f6b9c82deb7d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/tasks/query_cancel.py
**Line:** 247:249
**Comment:**
*Race Condition: When two requests reuse the same user and `client_id`, an older query can delete the newer handle during cleanup, so Stop misses the currently running query.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=2929ba3bfa1c02b0089cbd0f853104c3ade25e43c81d15e7581e1173ae213330&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=2929ba3bfa1c02b0089cbd0f853104c3ade25e43c81d15e7581e1173ae213330&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4078339765
##########
superset-frontend/src/explore/exploreUtils/index.ts:
##########
@@ -329,6 +333,9 @@ export const buildV1ChartDataPayload = async ({
}
});
}
+ if (clientId) {
+ payload.client_id = clientId;
+ }
Review Comment:
**Suggestion:** One `client_id` covers every query in a multi-query payload, so later registrations overwrite earlier handles and Stop cancels only the last warehouse query.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes` · 🏷️ `Logic error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7588c45bfefe4011a47dca02d2c3c213&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7588c45bfefe4011a47dca02d2c3c213&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/exploreUtils/index.ts
**Line:** 336:338
**Comment:**
*Logic Error: One `client_id` covers every query in a multi-query payload, so later registrations overwrite earlier handles and Stop cancels only the last warehouse query.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=18ce3a0672c0a08631e6f9d7144fcb7c5db84759e535105ecbc2556d51b492ae&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=18ce3a0672c0a08631e6f9d7144fcb7c5db84759e535105ecbc2556d51b492ae&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/Chart/chartAction.ts:
##########
@@ -803,7 +816,18 @@ export function exploreJSON(
const setDataMask = (dataMask: DataMask): void => {
dispatch(updateDataMask(formData.slice_id, dataMask));
};
- dispatch(chartUpdateStarted(controller, formData, key as string | number));
+ // Identifies this run to the backend so Stop can cancel the database query
+ // itself, not just abort the in-flight HTTP request. Matches SQL Lab's
+ // client id shape (see `postStopQuery`) and the `query.client_id` column.
+ const clientId = nanoid(11);
+ dispatch(
+ chartUpdateStarted(
+ controller,
+ formData,
+ key as string | number,
+ clientId,
+ ),
+ );
Review Comment:
**Suggestion:** The stop request can arrive before this chart request registers its handle, so an immediate Stop misses and the database query continues uncancelled.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Race condition`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=29f55f54c28649be9d855aeff563b260&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=29f55f54c28649be9d855aeff563b260&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/chartAction.ts
**Line:** 822:830
**Comment:**
*Race Condition: The stop request can arrive before this chart request registers its handle, so an immediate Stop misses and the database query continues uncancelled.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=1302099cf7f46e4166a8f86bbbc733047133a35c4f1f50a7e2d37710cbdc462d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=1302099cf7f46e4166a8f86bbbc733047133a35c4f1f50a7e2d37710cbdc462d&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44552:
URL: https://github.com/apache/superset/pull/44552#issuecomment-5787644821
<!-- Bito Reply -->
The flagged issue describes a race condition where a query cancellation request might be sent before the backend has registered the `client_id` for the running query. To resolve this, the client should ensure the `client_id` is generated and available before the query request is initiated, and the backend should handle the cancellation request gracefully if the `client_id` is not yet found.
Since the provided context does not include the specific implementation of the `stop` action in `ExploreViewContainer/index.tsx`, I recommend verifying that the `client_id` is passed to the backend immediately upon initiating the query and that the frontend waits for confirmation of the query's registration before enabling the stop button.
Would you like me to fetch all comments on this PR to validate the correctness of other issues and implement fixes for them?
**superset-frontend/src/components/Chart/chartAction.ts**
```
export function chartUpdateStarted(
queryController: AbortController,
latestQueryFormData: QueryFormData | LatestQueryFormData,
key: string | number,
latestQueryId?: string,
): ChartUpdateStartedAction {
return {
type: CHART_UPDATE_STARTED,
queryController,
latestQueryFormData,
key,
latestQueryId,
};
}
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4132100583
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,150 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
Review Comment:
The handle is republished on every cursor via `_sink` (for multi-statement query objects like the grouping-sets fallback), but discarded only once here, after the whole `get_query_result` call returns. A Stop request that reads the cache in the gap between one cursor finishing and the next cursor's republish resolves a stale `cancel_query_id`. For Postgres and Redshift, `cancel_query` reports success whenever the terminate statement executes without raising, even when the PID matches no row — so the stale handle is treated as successfully cancelled, `stopped: true` is returned, and this line then deletes the cache entry the *next*, still-running statement just published, while that statement keeps executing to completion. Could the handle carry something that lets discard/cancel confirm it's still the current statement's before acting on it?
##########
superset/charts/data/api.py:
##########
@@ -345,6 +360,63 @@ def data( # noqa: C901
expected_rows=expected_rows,
)
+ @expose("/data/stop", methods=("POST",))
+ @protect()
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.stop_data",
+ log_to_statsd=False,
+ )
+ def stop_data(self) -> Response:
+ """
+ Cancel a running chart-data query.
+ ---
+ post:
+ summary: Cancel a running chart-data query
+ description: >-
+ Cancels the warehouse query started by a chart-data request that
+ carried the given `client_id`, for databases whose engine supports
+ query cancellation. The `client_id` is resolved only within the
+ requesting user's own in-flight queries, so it cannot be used to
+ reach another user's query.
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/ChartDataStopSchema'
+ responses:
+ 200:
+ description: Cancellation outcome
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ type: object
+ properties:
+ stopped:
+ type: boolean
+ description: >-
+ Whether the engine reported the query cancelled.
+ False when there was no such in-flight query for
+ this user, or the engine could not cancel it.
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 500:
Review Comment:
This endpoint is decorated with `@protect()` and mapped to `method_permission_name["stop_data"] = "read"`, so a caller without `can_read` on Chart gets a 403 before this handler runs — but the response spec here only documents 400/401/500. The sibling `data` endpoint above documents 403 for the same permission check. Should 403 be added here too so a client generated from this spec models the permission-denied response?
##########
superset/common/query_context_processor.py:
##########
@@ -305,7 +306,15 @@ def get_df_payload_result(
)
)
- query_result = self.get_query_result(query_obj)
+ # Make the warehouse query cancellable by its owner for the span
+ # of its execution, so Explore's Stop button kills the query
+ # rather than just abandoning the response. No-op for engines
+ # without cancel support, and for requests carrying no client_id.
+ with cancellable_chart_query(
Review Comment:
Under `GLOBAL_ASYNC_QUERIES`, this query reaches the Celery worker through `serialize_query`/`load_serialized_query`, and neither carries `client_id` (only `force_nonce` and `custom_cache_timeout` are threaded through the `SerializedQuery` payload). The worker's reconstructed `QueryContext.client_id` is therefore always `None`, so `cancellable_chart_query` is a permanent no-op here for every async chart query — Stop silently never reaches the warehouse query in this mode. Should `client_id` be added to the serialized payload so async chart queries get the same cancellation coverage as synchronous ones?
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,150 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
+
+
+def _publish_cancel_handle(
+ user_id: int, client_id: str, database_id: int, cancel_query_id: str
+) -> None:
+ """Publish a cancel handle for the owning user. Best-effort."""
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.set(
Review Comment:
Superset's default `CACHE_CONFIG` is `NullCache`, so this `cache_manager.cache.set(...)` call is a no-op there — the cancel handle is never actually stored, the Stop endpoint's lookup always misses, and the warehouse query keeps running after the user clicks Stop, with no error surfaced (the endpoint just returns `stopped: false`, which the UI treats as normal). Should this registry use a cache guaranteed to persist across requests/workers by default, e.g. a dedicated required cache the way `FILTER_STATE_CACHE_CONFIG` is configured, rather than relying on the general `CACHE_CONFIG` an operator may leave at its default?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44552:
URL: https://github.com/apache/superset/pull/44552#issuecomment-6305926901
@aminghadersohi good call on Ocient, its `cancel_query` keys off the `handle_cursor` mapping this path never populates, so it would just decline. Moved it to the not-yet row in the description. Trino as a follow-up works for me.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4118876694
##########
superset-frontend/src/explore/types.ts:
##########
@@ -61,6 +61,8 @@ export interface ChartState {
latestQueryFormData: LatestQueryFormData;
sliceFormData: QueryFormData | null;
queryController: AbortController | null;
+ /** Client-generated id of the in-flight query, used to cancel it server-side. */
+ latestQueryId?: string;
Review Comment:
<!-- Bito Reply -->
The reasoning regarding the lifecycle of the query identifier is sound. Since the UI state transition effectively prevents the stop action from being triggered after a query has finished, the identifier does not persist in a way that would cause functional issues. Given that no other part of the application currently reads this field, avoiding the addition of cleanup logic is an acceptable approach to keep the codebase lean.
**superset-frontend/src/explore/types.ts**
```
/** Client-generated id of the in-flight query, used to cancel it server-side. */
latestQueryId?: string;
```
##########
superset/common/query_context_processor.py:
##########
@@ -305,7 +306,15 @@ def get_df_payload_result(
)
)
- query_result = self.get_query_result(query_obj)
+ # Make the warehouse query cancellable by its owner for the span
+ # of its execution, so Explore's Stop button kills the query
+ # rather than just abandoning the response. No-op for engines
+ # without cancel support, and for requests carrying no client_id.
+ with cancellable_chart_query(
+ self._query_context.client_id,
+ getattr(self._qc_datasource, "database", None),
+ ):
+ query_result = self.get_query_result(query_obj)
annotation_data = self.get_annotation_data(query_obj)
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to include `self.get_annotation_data(query_obj)` within the `cancellable_chart_query` context manager is technically sound in its intent to make the annotation query cancellable. However, as you noted, the current implementation of the context manager relies on a fixed database target resolved at open time. Simply wrapping the annotation call would lead to misattribution if the annotation layer points to a different database, as the capture sink would still be scoped to the outer chart's database.
To address this correctly, the context manager would need to support resolving the database per capture, or `cancellable_chart_query` would need to be updated to handle dynamic database targets. Given the complexity and the risk of misattributing cancel IDs, your decision to treat this as a non-trivial follow-up rather than a simple one-line fix is appropriate.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4078335563
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -584,9 +585,26 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
);
function onStop() {
- if (props.chart && props.chart.queryController) {
+ // Abort the in-flight HTTP request so the UI stops waiting immediately.
+ if (props.chart?.queryController) {
props.chart.queryController.abort();
}
+
+ // Aborting only drops the response; the database keeps executing the query.
+ // Ask the backend to cancel it too. The backend resolves `client_id` within
+ // the requesting user's own in-flight queries, so this can only ever cancel
+ // our own query. A falsy `stopped` just means there was nothing running to
+ // cancel (or the engine has no cancel support), which is not an error.
+ const clientId = props.chart?.latestQueryId;
+ if (clientId) {
+ SupersetClient.post({
+ endpoint: '/api/v1/chart/data/stop',
+ body: JSON.stringify({ client_id: clientId }),
+ headers: { 'Content-Type': 'application/json' },
+ }).catch(() => {
+ props.addDangerToast(t('Failed to stop query.'));
+ });
Review Comment:
**Suggestion:** Stop sends cancellation only once. If pressed before the backend publishes the cancel handle, the lookup misses and the database query continues running.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes` · 🏷️ `Race condition`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5cd8d695d9854e7c8d9074f871200439&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5cd8d695d9854e7c8d9074f871200439&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/ExploreViewContainer/index.tsx
**Line:** 598:606
**Comment:**
*Race Condition: Stop sends cancellation only once. If pressed before the backend publishes the cancel handle, the lookup misses and the database query continues running.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=fd5e4b50fef63bc6f1fa9bb720a1c4ba31e29b14690a153bd74c7d89b7c47bfc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=fd5e4b50fef63bc6f1fa9bb720a1c4ba31e29b14690a153bd74c7d89b7c47bfc&reaction=dislike'>👎</a>
##########
superset/common/query_context_factory.py:
##########
@@ -111,6 +112,7 @@ def create( # pylint: disable=too-many-arguments
result_format=result_format,
force=force,
force_nonce=force_nonce,
+ client_id=client_id,
Review Comment:
**Suggestion:** `client_id` is stored only on `QueryContext`; async serialization does not carry it, so reconstructed tasks cannot register queries for cancellation.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes` · 🏷️ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=308896f1c3574e25b6759d22955c35f4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=308896f1c3574e25b6759d22955c35f4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/common/query_context_factory.py
**Line:** 115:115
**Comment:**
*Api Mismatch: `client_id` is stored only on `QueryContext`; async serialization does not carry it, so reconstructed tasks cannot register queries for cancellation.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=da2f6e7f0485709765e240e9ae8b15319305197003c592d0d76337351378d0bb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44552&comment_hash=da2f6e7f0485709765e240e9ae8b15319305197003c592d0d76337351378d0bb&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44552:
URL: https://github.com/apache/superset/pull/44552#issuecomment-5804394318
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4305286519
##########
tests/unit_tests/tasks/test_query_cancel.py:
##########
@@ -205,3 +206,249 @@ def test_capture_is_noop_without_a_database() -> None:
with _capture_query_cancellation(qc):
assert _cancel_id_sink.get() is None
get_context.assert_not_called()
+
+
+# --- synchronous Explore cancellation registry ----------------------------
+
+
+class _FakeCache:
+ """Minimal dict-backed stand-in for ``cache_manager.cache``.
+
+ Real storage (rather than a MagicMock) so the user-scoping of the registry
+ keys is genuinely exercised end to end.
+ """
+
+ def __init__(self) -> None:
+ self.store: dict[str, object] = {}
+
+ def set(self, key: str, value: object, timeout: int | None = None) -> None:
+ self.store[key] = value
+
+ def get(self, key: str) -> object | None:
+ return self.store.get(key)
+
+ def delete(self, key: str) -> None:
+ self.store.pop(key, None)
+
+
+@contextmanager
+def _registry_env(user_id: int | None, cache: "_FakeCache"):
+ """Run with a given current user and a shared fake cache backend."""
+ # ``cache_manager.cache`` is a read-only property; swap the backing attribute.
+ with (
+ patch("superset.utils.core.get_user_id", return_value=user_id),
+ patch("superset.extensions.cache_manager._cache", cache),
+ patch("superset.tasks.query_cancel._registry_ttl", return_value=60),
+ ):
+ yield
+
+
+def _run_cancellable(client_id, database, cache, user_id, cancel_id="engine-1"):
+ """Drive one cancellable query, returning whether a handle was live inside it."""
+ from superset.tasks.query_cancel import cancellable_chart_query
+
+ with _registry_env(user_id, cache):
+ with patch(
+ "superset.tasks.query_cancel.capture_cancel_query_id",
+ return_value=cancel_id,
+ ):
+ with cancellable_chart_query(client_id, database):
+ # Stands in for Database._execute_sql_with_mutation_and_logging
+ # handing the live cursor to the active sink before executing.
+ notify_cursor(MagicMock())
+ inside = dict(cache.store)
+ return inside
+
+
+def test_registry_key_is_scoped_by_user() -> None:
+ """The same ``client_id`` under two users maps to two distinct keys."""
+ from superset.tasks.query_cancel import _registry_key
+
+ # Same client_id under two users must not collide.
+ assert _registry_key(1, "abc") != _registry_key(2, "abc")
+ assert "abc" in _registry_key(1, "abc")
+
+
+def test_cancellable_chart_query_publishes_then_discards_the_handle() -> None:
+ """A captured handle is live for the query's duration, then removed."""
+ cache = _FakeCache()
+ database = MagicMock()
+ database.id = 5
+
+ inside = _run_cancellable("client-1", database, cache, user_id=1)
+
+ # Live for the duration of the query...
+ assert inside == {
+ "chart-query-cancel:1:client-1": {
+ "database_id": 5,
+ "cancel_query_id": "engine-1",
+ }
+ }
+ # ...and cleaned up once it returns.
+ assert cache.store == {}
+
+
+def test_cancellable_chart_query_republishes_for_each_cursor() -> None:
+ """Every cursor the query opens refreshes the handle to its own session.
+
+ One query object can run several statements in turn (the grouping-sets
+ fallback does), each on whatever pooled connection it gets, so the handle
+ must follow the statement that is executing now rather than the first one.
+ """
+ from superset.tasks.query_cancel import cancellable_chart_query
+
+ cache = _FakeCache()
+ database = MagicMock()
+ database.id = 5
+ seen: list[object] = []
+
+ with _registry_env(1, cache):
+ with patch(
+ "superset.tasks.query_cancel.capture_cancel_query_id",
+ side_effect=["engine-1", "engine-2"],
+ ):
+ with cancellable_chart_query("client-1", database):
+ notify_cursor(MagicMock())
+ seen.append(cache.store["chart-query-cancel:1:client-1"])
+ notify_cursor(MagicMock())
+ seen.append(cache.store["chart-query-cancel:1:client-1"])
+
+ assert seen == [
+ {"database_id": 5, "cancel_query_id": "engine-1"},
+ {"database_id": 5, "cancel_query_id": "engine-2"},
+ ]
+ assert cache.store == {}
+
+
+def test_cancellable_chart_query_is_noop_without_a_client_id() -> None:
+ """Requests that carry no ``client_id`` register nothing."""
+ cache = _FakeCache()
+ database = MagicMock()
+ database.id = 5
+
+ assert _run_cancellable(None, database, cache, user_id=1) == {}
+
+
+def test_cancellable_chart_query_is_noop_without_a_database() -> None:
+ """Datasources without a database (e.g. annotations) register nothing."""
+ cache = _FakeCache()
+
+ assert _run_cancellable("client-1", None, cache, user_id=1) == {}
+
+
+def test_cancellable_chart_query_is_noop_for_an_anonymous_request() -> None:
+ """An anonymous request has no user id to scope a handle to."""
+ # An anonymous viewer has no user id to scope the handle to, and an unscoped
+ # handle would be cancellable by any other anonymous visitor.
+ cache = _FakeCache()
+ database = MagicMock()
+ database.id = 5
+
+ assert _run_cancellable("client-1", database, cache, user_id=None) == {}
+
+
+def test_cancellable_chart_query_publishes_nothing_when_engine_has_no_cancel_id() -> (
+ None
+):
+ """Engines with no pre-execution cancel id leave the query non-cancellable."""
+ cache = _FakeCache()
+ database = MagicMock()
+ database.id = 5
+
+ assert _run_cancellable("client-1", database, cache, 1, cancel_id=None) == {}
+
+
+def test_cancel_chart_query_for_user_cancels_its_own_query() -> None:
+ """Cancelling one's own registered query hits the engine and cleans up."""
+ from superset.tasks.query_cancel import cancel_chart_query_for_user
+
+ cache = _FakeCache()
+ cache.store["chart-query-cancel:1:client-1"] = {
+ "database_id": 5,
+ "cancel_query_id": "engine-1",
+ }
+ database = MagicMock()
+
+ with _registry_env(1, cache):
+ with (
+ patch(
+ "superset.daos.database.DatabaseDAO.find_by_id", return_value=database
+ ),
+ patch(
+ "superset.tasks.query_cancel.cancel_chart_query", return_value=True
+ ) as cancel,
+ ):
+ assert cancel_chart_query_for_user("client-1") is True
+
+ cancel.assert_called_once_with(database, "engine-1")
+ # A cancelled query's handle is dropped rather than left to expire.
+ assert cache.store == {}
+
+
+def test_cancel_chart_query_for_user_cannot_reach_another_users_query() -> None:
+ """Regression test: ``client_id`` is untrusted, client-supplied input.
+
+ User 1 has a running, cancellable query. User 2 asks to cancel that exact
+ ``client_id``. Because the registry key embeds the requesting user, the
+ lookup misses: no engine cancellation is attempted and user 1's handle is
+ left untouched.
+ """
+ from superset.tasks.query_cancel import cancel_chart_query_for_user
+
+ cache = _FakeCache()
+ victim_handle = {"database_id": 5, "cancel_query_id": "engine-1"}
+ cache.store["chart-query-cancel:1:client-1"] = victim_handle
Review Comment:
Good catch @aminghadersohi, both isolation tests now seed through `_registry_key` so a regression in user scoping reaches the assertions.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4333017962
##########
superset/common/query_context_processor.py:
##########
@@ -328,7 +329,15 @@ def get_df_payload_result(
)
)
- query_result = self.get_query_result(query_obj)
+ # Make the warehouse query cancellable by its owner for the span
+ # of its execution, so Explore's Stop button kills the query
+ # rather than just abandoning the response. No-op for engines
+ # without cancel support, and for requests carrying no client_id.
+ with cancellable_chart_query(
Review Comment:
Good catch, the totals query now runs inside the same `cancellable_chart_query` scope as the chart's own queries, so Stop reaches it on the covered engines.
##########
superset/common/query_context_processor.py:
##########
@@ -328,7 +329,15 @@ def get_df_payload_result(
)
)
- query_result = self.get_query_result(query_obj)
+ # Make the warehouse query cancellable by its owner for the span
+ # of its execution, so Explore's Stop button kills the query
+ # rather than just abandoning the response. No-op for engines
+ # without cancel support, and for requests carrying no client_id.
+ with cancellable_chart_query(
+ self._query_context.client_id,
+ getattr(self._qc_datasource, "database", None),
+ ):
+ query_result = self.get_query_result(query_obj)
Review Comment:
Yep, that was a hole. A failed or stopped level now ends the fallback with that failure instead of letting later levels overwrite it, so nothing partial gets cached. Each level also checks the cancel intent before it runs.
##########
superset/tasks/query_cancel.py:
##########
@@ -168,3 +177,150 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
+
+
+def _publish_cancel_handle(
+ user_id: int, client_id: str, database_id: int, cancel_query_id: str
+) -> None:
+ """Publish a cancel handle for the owning user. Best-effort."""
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.set(
+ _registry_key(user_id, client_id),
+ {"database_id": database_id, "cancel_query_id": cancel_query_id},
+ timeout=_registry_ttl(),
+ )
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ # Forfeits cancellability for this query; never breaks its execution.
+ logger.warning("Could not publish chart query cancel handle", exc_info=True)
+
+
+def _discard_cancel_handle(user_id: int, client_id: str) -> None:
+ """Drop a cancel handle once its query is no longer running. Best-effort."""
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.delete(_registry_key(user_id, client_id))
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not discard chart query cancel handle", exc_info=True)
+
+
+def cancel_chart_query_for_user(client_id: str) -> bool:
+ """Cancel the requesting user's running chart query, if it is cancellable.
+
+ :returns: True if the engine reported the query cancelled. False covers both
+ "no such in-flight query for this user" and "the engine declined" — the
+ caller cannot distinguish them, which is deliberate: it keeps the
+ endpoint from confirming whether a given ``client_id`` exists.
+ """
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.daos.database import DatabaseDAO
+ from superset.extensions import cache_manager
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if user_id is None:
+ return False
+
+ try:
+ handle = cache_manager.cache.get(_registry_key(user_id, client_id))
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning("Could not read chart query cancel handle", exc_info=True)
+ return False
+
+ if not handle:
+ return False
+
+ database = DatabaseDAO.find_by_id(handle["database_id"])
+ if database is None:
+ return False
+
+ cancelled = cancel_chart_query(database, handle["cancel_query_id"])
+ if cancelled:
+ _discard_cancel_handle(user_id, client_id)
Review Comment:
Agreed, so Stop now records the intent itself, keyed per user and `client_id` like the handle, and every statement in the run (query objects, fallback levels, the totals query) checks it before hitting the warehouse. A Stop that lands before or between them skips the rest instead of getting lost, and the intent is cleared when the run ends.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #44552:
URL: https://github.com/apache/superset/pull/44552#issuecomment-6112029164
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging because an early Stop can miss the handle and leave the warehouse query running.
- The stop endpoint performs a single lookup, while the chart request registers its handle only when query execution opens a cursor.
**Assessed commit:** `fb560033f051`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44552:
URL: https://github.com/apache/superset/pull/44552#issuecomment-5861054199
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4078498284
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -584,9 +585,26 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
);
function onStop() {
- if (props.chart && props.chart.queryController) {
+ // Abort the in-flight HTTP request so the UI stops waiting immediately.
+ if (props.chart?.queryController) {
props.chart.queryController.abort();
}
+
+ // Aborting only drops the response; the database keeps executing the query.
+ // Ask the backend to cancel it too. The backend resolves `client_id` within
+ // the requesting user's own in-flight queries, so this can only ever cancel
+ // our own query. A falsy `stopped` just means there was nothing running to
+ // cancel (or the engine has no cancel support), which is not an error.
+ const clientId = props.chart?.latestQueryId;
+ if (clientId) {
+ SupersetClient.post({
+ endpoint: '/api/v1/chart/data/stop',
+ body: JSON.stringify({ client_id: clientId }),
+ headers: { 'Content-Type': 'application/json' },
+ }).catch(() => {
+ props.addDangerToast(t('Failed to stop query.'));
+ });
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CSRF token missing on POST</b></div>
<div id="fix">
`SupersetClient.post` here bypasses the CSRF token that `SupersetClient.configure` injects via `postHeaders`/`csrfHeader` — `chartAction.ts` and `postStopQuery` (SqlLab) always send it, so this request can 403 on CSRF validation. It also duplicates the stop-request logic that exists in `chartAction.ts`/`sqlLab.ts`. Extract a shared `stopQuery` helper and reuse it here.
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Swallowed stop-request error</b></div>
<div id="fix">
The `.catch` swallows the actual error object and shows only a generic toast, so server-side failures (403/500/network) leave no diagnostic trace in the console or logger. `logging` is already imported in this file (line 53) — log the error before showing the toast.
</div>
</div>
<small><i>Code Review Run #1ec09f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/tasks/test_query_cancel.py:
##########
@@ -205,3 +206,208 @@ def test_capture_is_noop_without_a_database() -> None:
with _capture_query_cancellation(qc):
assert _cancel_id_sink.get() is None
get_context.assert_not_called()
+
+
+# --- synchronous Explore cancellation registry ----------------------------
+
+
+class _FakeCache:
+ """Minimal dict-backed stand-in for ``cache_manager.cache``.
+
+ Real storage (rather than a MagicMock) so the user-scoping of the registry
+ keys is genuinely exercised end to end.
+ """
+
+ def __init__(self) -> None:
+ self.store: dict[str, object] = {}
+
+ def set(self, key: str, value: object, timeout: int | None = None) -> None:
+ self.store[key] = value
+
+ def get(self, key: str) -> object | None:
+ return self.store.get(key)
+
+ def delete(self, key: str) -> None:
+ self.store.pop(key, None)
+
+
+@contextmanager
+def _registry_env(user_id: int | None, cache: "_FakeCache"):
+ """Run with a given current user and a shared fake cache backend."""
+ # ``cache_manager.cache`` is a read-only property; swap the backing attribute.
+ with (
+ patch("superset.utils.core.get_user_id", return_value=user_id),
+ patch("superset.extensions.cache_manager._cache", cache),
+ patch("superset.tasks.query_cancel._registry_ttl", return_value=60),
+ ):
+ yield
+
+
+def _run_cancellable(client_id, database, cache, user_id, cancel_id="engine-1"):
+ """Drive one cancellable query, returning whether a handle was live inside it."""
+ from superset.tasks.query_cancel import cancellable_chart_query
+
+ with _registry_env(user_id, cache):
+ with patch(
+ "superset.tasks.query_cancel.capture_cancel_query_id",
+ return_value=cancel_id,
+ ):
+ with cancellable_chart_query(client_id, database):
+ # Stands in for Database._execute_sql_with_mutation_and_logging
+ # handing the live cursor to the active sink before executing.
+ notify_cursor(MagicMock())
+ inside = dict(cache.store)
+ return inside
+
+
+def test_registry_key_is_scoped_by_user() -> None:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test docstrings</b></div>
<div id="fix">
Nine new test functions have no docstring: `test_registry_key_is_scoped_by_user`, `..._publishes_then_discards_the_handle`, the three `is_noop` tests, `..._publishes_nothing_when_engine_has_no_cancel_id`, `..._cancels_its_own_query`, and the two `returns_false` tests. BITO.md rule 12148 requires a docstring per new test and says inline comments do not substitute.
</div>
</div>
<small><i>Code Review Run #1ec09f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/tasks/test_query_cancel.py:
##########
@@ -205,3 +206,208 @@ def test_capture_is_noop_without_a_database() -> None:
with _capture_query_cancellation(qc):
assert _cancel_id_sink.get() is None
get_context.assert_not_called()
+
+
+# --- synchronous Explore cancellation registry ----------------------------
+
+
+class _FakeCache:
+ """Minimal dict-backed stand-in for ``cache_manager.cache``.
+
+ Real storage (rather than a MagicMock) so the user-scoping of the registry
+ keys is genuinely exercised end to end.
+ """
+
+ def __init__(self) -> None:
+ self.store: dict[str, object] = {}
+
+ def set(self, key: str, value: object, timeout: int | None = None) -> None:
+ self.store[key] = value
+
+ def get(self, key: str) -> object | None:
+ return self.store.get(key)
+
+ def delete(self, key: str) -> None:
+ self.store.pop(key, None)
+
+
+@contextmanager
+def _registry_env(user_id: int | None, cache: "_FakeCache"):
+ """Run with a given current user and a shared fake cache backend."""
+ # ``cache_manager.cache`` is a read-only property; swap the backing attribute.
+ with (
+ patch("superset.utils.core.get_user_id", return_value=user_id),
+ patch("superset.extensions.cache_manager._cache", cache),
+ patch("superset.tasks.query_cancel._registry_ttl", return_value=60),
+ ):
+ yield
+
+
+def _run_cancellable(client_id, database, cache, user_id, cancel_id="engine-1"):
+ """Drive one cancellable query, returning whether a handle was live inside it."""
+ from superset.tasks.query_cancel import cancellable_chart_query
+
+ with _registry_env(user_id, cache):
+ with patch(
+ "superset.tasks.query_cancel.capture_cancel_query_id",
+ return_value=cancel_id,
+ ):
+ with cancellable_chart_query(client_id, database):
+ # Stands in for Database._execute_sql_with_mutation_and_logging
+ # handing the live cursor to the active sink before executing.
+ notify_cursor(MagicMock())
+ inside = dict(cache.store)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unannotated test locals</b></div>
<div id="fix">
Locals in the new section are unannotated: `inside` here, plus `cache`, `database`, and `victim_handle` throughout the new test bodies. BITO.md rule 13153 requires explicit annotations on all test-file locals even when inferable. Annotate e.g. `inside: dict[str, object]`, `cache: _FakeCache`, `database: MagicMock`, `victim_handle: dict[str, object]`.
</div>
</div>
<small><i>Code Review Run #1ec09f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4078640776
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -584,9 +585,26 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
);
function onStop() {
- if (props.chart && props.chart.queryController) {
+ // Abort the in-flight HTTP request so the UI stops waiting immediately.
+ if (props.chart?.queryController) {
props.chart.queryController.abort();
}
+
+ // Aborting only drops the response; the database keeps executing the query.
+ // Ask the backend to cancel it too. The backend resolves `client_id` within
+ // the requesting user's own in-flight queries, so this can only ever cancel
+ // our own query. A falsy `stopped` just means there was nothing running to
+ // cancel (or the engine has no cancel support), which is not an error.
+ const clientId = props.chart?.latestQueryId;
+ if (clientId) {
+ SupersetClient.post({
+ endpoint: '/api/v1/chart/data/stop',
+ body: JSON.stringify({ client_id: clientId }),
+ headers: { 'Content-Type': 'application/json' },
+ }).catch(() => {
+ props.addDangerToast(t('Failed to stop query.'));
+ });
Review Comment:
The gap is between the request landing and the cursor opening. A miss just returns `stopped: false`, which is exactly what `master` does today. Closing it means the execution path checking a cancel flag before it runs anything, and I would rather not grow this PR that way.
##########
superset/common/query_context_factory.py:
##########
@@ -111,6 +112,7 @@ def create( # pylint: disable=too-many-arguments
result_format=result_format,
force=force,
force_nonce=force_nonce,
+ client_id=client_id,
Review Comment:
That is on purpose. The async path already has its own cancellation via the task abort (`_capture_query_cancellation` in `async_queries.py`), so this registry only backs the synchronous Stop, and an async request just gets `stopped: false` from the endpoint.
##########
superset-frontend/src/explore/exploreUtils/index.ts:
##########
@@ -329,6 +333,9 @@ export const buildV1ChartDataPayload = async ({
}
});
}
+ if (clientId) {
+ payload.client_id = clientId;
+ }
Review Comment:
The queries in a payload run one after another, and each `get_df_payload` publishes its own handle and drops it when it returns, so the key always names whichever query is executing at that moment. Nothing gets overwritten mid-flight.
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,140 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ from flask import current_app
+
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ nonlocal captured
+ if captured:
+ return
Review Comment:
Good catch, the sink now republishes for every cursor it sees instead of stopping at the first, with a test for the two-statement case.
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,140 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ from flask import current_app
+
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ nonlocal captured
+ if captured:
+ return
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
Review Comment:
The ids are a fresh `nanoid` per run, so the only way to collide is a client reusing its own id, and the worst case is that user's own Stop missing. Not worth a compare-and-delete the cache cannot do atomically anyway.
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -584,9 +585,26 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
);
function onStop() {
- if (props.chart && props.chart.queryController) {
+ // Abort the in-flight HTTP request so the UI stops waiting immediately.
+ if (props.chart?.queryController) {
props.chart.queryController.abort();
}
+
+ // Aborting only drops the response; the database keeps executing the query.
+ // Ask the backend to cancel it too. The backend resolves `client_id` within
+ // the requesting user's own in-flight queries, so this can only ever cancel
+ // our own query. A falsy `stopped` just means there was nothing running to
+ // cancel (or the engine has no cancel support), which is not an error.
+ const clientId = props.chart?.latestQueryId;
+ if (clientId) {
+ SupersetClient.post({
+ endpoint: '/api/v1/chart/data/stop',
+ body: JSON.stringify({ client_id: clientId }),
+ headers: { 'Content-Type': 'application/json' },
+ }).catch(() => {
+ props.addDangerToast(t('Failed to stop query.'));
+ });
+ }
Review Comment:
`SupersetClient.post` already sends the CSRF token: `request()` merges `this.headers`, which carries `X-CSRFToken`, into every call, same as every other post in the codebase. I did add a `logging.error` before the toast so a failed stop leaves a trace.
##########
tests/unit_tests/tasks/test_query_cancel.py:
##########
@@ -205,3 +206,208 @@ def test_capture_is_noop_without_a_database() -> None:
with _capture_query_cancellation(qc):
assert _cancel_id_sink.get() is None
get_context.assert_not_called()
+
+
+# --- synchronous Explore cancellation registry ----------------------------
+
+
+class _FakeCache:
+ """Minimal dict-backed stand-in for ``cache_manager.cache``.
+
+ Real storage (rather than a MagicMock) so the user-scoping of the registry
+ keys is genuinely exercised end to end.
+ """
+
+ def __init__(self) -> None:
+ self.store: dict[str, object] = {}
+
+ def set(self, key: str, value: object, timeout: int | None = None) -> None:
+ self.store[key] = value
+
+ def get(self, key: str) -> object | None:
+ return self.store.get(key)
+
+ def delete(self, key: str) -> None:
+ self.store.pop(key, None)
+
+
+@contextmanager
+def _registry_env(user_id: int | None, cache: "_FakeCache"):
+ """Run with a given current user and a shared fake cache backend."""
+ # ``cache_manager.cache`` is a read-only property; swap the backing attribute.
+ with (
+ patch("superset.utils.core.get_user_id", return_value=user_id),
+ patch("superset.extensions.cache_manager._cache", cache),
+ patch("superset.tasks.query_cancel._registry_ttl", return_value=60),
+ ):
+ yield
+
+
+def _run_cancellable(client_id, database, cache, user_id, cancel_id="engine-1"):
+ """Drive one cancellable query, returning whether a handle was live inside it."""
+ from superset.tasks.query_cancel import cancellable_chart_query
+
+ with _registry_env(user_id, cache):
+ with patch(
+ "superset.tasks.query_cancel.capture_cancel_query_id",
+ return_value=cancel_id,
+ ):
+ with cancellable_chart_query(client_id, database):
+ # Stands in for Database._execute_sql_with_mutation_and_logging
+ # handing the live cursor to the active sink before executing.
+ notify_cursor(MagicMock())
+ inside = dict(cache.store)
Review Comment:
There is no `BITO.md` in this repo, and we do not annotate inferable locals in tests anywhere else, so leaving these as they are.
##########
tests/unit_tests/tasks/test_query_cancel.py:
##########
@@ -205,3 +206,208 @@ def test_capture_is_noop_without_a_database() -> None:
with _capture_query_cancellation(qc):
assert _cancel_id_sink.get() is None
get_context.assert_not_called()
+
+
+# --- synchronous Explore cancellation registry ----------------------------
+
+
+class _FakeCache:
+ """Minimal dict-backed stand-in for ``cache_manager.cache``.
+
+ Real storage (rather than a MagicMock) so the user-scoping of the registry
+ keys is genuinely exercised end to end.
+ """
+
+ def __init__(self) -> None:
+ self.store: dict[str, object] = {}
+
+ def set(self, key: str, value: object, timeout: int | None = None) -> None:
+ self.store[key] = value
+
+ def get(self, key: str) -> object | None:
+ return self.store.get(key)
+
+ def delete(self, key: str) -> None:
+ self.store.pop(key, None)
+
+
+@contextmanager
+def _registry_env(user_id: int | None, cache: "_FakeCache"):
+ """Run with a given current user and a shared fake cache backend."""
+ # ``cache_manager.cache`` is a read-only property; swap the backing attribute.
+ with (
+ patch("superset.utils.core.get_user_id", return_value=user_id),
+ patch("superset.extensions.cache_manager._cache", cache),
+ patch("superset.tasks.query_cancel._registry_ttl", return_value=60),
+ ):
+ yield
+
+
+def _run_cancellable(client_id, database, cache, user_id, cancel_id="engine-1"):
+ """Drive one cancellable query, returning whether a handle was live inside it."""
+ from superset.tasks.query_cancel import cancellable_chart_query
+
+ with _registry_env(user_id, cache):
+ with patch(
+ "superset.tasks.query_cancel.capture_cancel_query_id",
+ return_value=cancel_id,
+ ):
+ with cancellable_chart_query(client_id, database):
+ # Stands in for Database._execute_sql_with_mutation_and_logging
+ # handing the live cursor to the active sink before executing.
+ notify_cursor(MagicMock())
+ inside = dict(cache.store)
+ return inside
+
+
+def test_registry_key_is_scoped_by_user() -> None:
Review Comment:
Added docstrings to the new tests.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44552:
URL: https://github.com/apache/superset/pull/44552#issuecomment-5895263421
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4134612484
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,150 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
+
+
+def _publish_cancel_handle(
+ user_id: int, client_id: str, database_id: int, cancel_query_id: str
+) -> None:
+ """Publish a cancel handle for the owning user. Best-effort."""
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.set(
Review Comment:
Good catch. `cache_manager.cache` is the general `CACHE_CONFIG`, which defaults to `NullCache`. Split the registry onto its own `CHART_QUERY_CANCEL_CACHE_CONFIG`, required like `FILTER_STATE_CACHE_CONFIG`, so Stop actually persists a handle by default.
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,150 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
Review Comment:
Good catch, that gap's real for the grouping-sets fallback's sequential cursors, not just a client reusing its own id. Discard now only clears the entry if it still names the id that was actually cancelled, so a stale hit can't wipe a cursor that's already republished.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4288957188
##########
superset/common/query_context_processor.py:
##########
@@ -305,7 +306,15 @@ def get_df_payload_result(
)
)
- query_result = self.get_query_result(query_obj)
+ # Make the warehouse query cancellable by its owner for the span
+ # of its execution, so Explore's Stop button kills the query
+ # rather than just abandoning the response. No-op for engines
+ # without cancel support, and for requests carrying no client_id.
+ with cancellable_chart_query(
Review Comment:
Took another look: `execute_chart_query` wraps `get_df_payload_result` in `_capture_query_cancellation`, which registers its own on_abort cancel independent of `client_id`. Async's covered by that path already, so no serialization change needed here. Resolving.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4078643235
##########
superset/common/query_context_factory.py:
##########
@@ -111,6 +112,7 @@ def create( # pylint: disable=too-many-arguments
result_format=result_format,
force=force,
force_nonce=force_nonce,
+ client_id=client_id,
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require propagating client_id through async serialization; async cancellation is handled by task abort, and the query registry is only for synchronous Stop requests.
**Applied to:**
- `superset/common/query_context_factory.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,140 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ from flask import current_app
+
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ nonlocal captured
+ if captured:
+ return
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag the potential stale-handle cleanup race in query cancellation when each run uses a fresh nanoid and atomic compare-and-delete is unavailable; a client reusing its own id may miss Stop in the worst case.
**Applied to:**
- `superset/tasks/query_cancel.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44552:
URL: https://github.com/apache/superset/pull/44552#issuecomment-5789988834
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44552:
URL: https://github.com/apache/superset/pull/44552#issuecomment-5866381183
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4134615056
##########
superset/common/query_context_processor.py:
##########
@@ -305,7 +306,15 @@ def get_df_payload_result(
)
)
- query_result = self.get_query_result(query_obj)
+ # Make the warehouse query cancellable by its owner for the span
+ # of its execution, so Explore's Stop button kills the query
+ # rather than just abandoning the response. No-op for engines
+ # without cancel support, and for requests carrying no client_id.
+ with cancellable_chart_query(
Review Comment:
Async does not need this registry. `_capture_query_cancellation` in `async_queries.py` gives the worker task its own abort-triggered cancel, wired to `ctx.on_abort`, independent of `client_id` or serialization. This registry only backs the synchronous Stop path.
##########
superset/charts/data/api.py:
##########
@@ -345,6 +360,63 @@ def data( # noqa: C901
expected_rows=expected_rows,
)
+ @expose("/data/stop", methods=("POST",))
+ @protect()
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.stop_data",
+ log_to_statsd=False,
+ )
+ def stop_data(self) -> Response:
+ """
+ Cancel a running chart-data query.
+ ---
+ post:
+ summary: Cancel a running chart-data query
+ description: >-
+ Cancels the warehouse query started by a chart-data request that
+ carried the given `client_id`, for databases whose engine supports
+ query cancellation. The `client_id` is resolved only within the
+ requesting user's own in-flight queries, so it cannot be used to
+ reach another user's query.
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/ChartDataStopSchema'
+ responses:
+ 200:
+ description: Cancellation outcome
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ type: object
+ properties:
+ stopped:
+ type: boolean
+ description: >-
+ Whether the engine reported the query cancelled.
+ False when there was no such in-flight query for
+ this user, or the engine could not cancel it.
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 500:
Review Comment:
Added the 403 response here, same as `data`'s spec since `stop_data` is gated by the identical `can_read` check.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4335083055
##########
superset/common/query_context_processor.py:
##########
@@ -560,8 +574,36 @@ def get_query_result(self, query_object: QueryObject) -> QueryResult:
"""
if query_object.grouping_sets and not self._supports_grouping_sets():
return self._grouping_sets_fallback(query_object)
+ return self._execute_query_object(query_object)
+
+ def _execute_query_object(self, query_object: QueryObject) -> QueryResult:
+ """Run one statement on the datasource, unless its owner asked to stop.
+
+ Every statement of a chart-data run passes through here, so a Stop
+ that lands before the run starts, or between two of its statements,
+ is honoured at the next one instead of being lost. Cancelling the
+ statement executing right now is ``cancellable_chart_query``'s job.
+ """
+ if chart_query_cancel_requested(self._query_context.client_id):
+ return self._stopped_query_result()
return self._qc_datasource.get_query_result(query_object)
Review Comment:
@aminghadersohi done: `_execute_query_object` rechecks the stop intent after the datasource call, so a result cut short by a Stop comes back as the stopped result instead of a cacheable SUCCESS.
##########
superset/tasks/query_cancel.py:
##########
@@ -168,3 +177,266 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _cancel_requested_key(user_id: int, client_id: str) -> str:
+ """Cache key recording that a user asked to stop a chart query run.
+
+ Scoped by user for the same reason as ``_registry_key``: a ``client_id``
+ can only ever flag the requester's own run.
+ """
+ return f"chart-query-cancel-requested:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
Review Comment:
@aminghadersohi done: the cursor sink now checks the stop intent before capturing and again after publishing the handle, and raises so the statement never executes. That covers internal time-offset statements too, since they all pass through `notify_cursor`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4332179773
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,150 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
+
+
+def _publish_cancel_handle(
+ user_id: int, client_id: str, database_id: int, cancel_query_id: str
+) -> None:
+ """Publish a cancel handle for the owning user. Best-effort."""
+ # Inline for test-patchability; see cancellable_chart_query's comment.
+ from superset.extensions import cache_manager
+
+ try:
+ cache_manager.cache.set(
Review Comment:
You're right, that fix got dropped in a rebase along the way, sorry about that. Restored it, so the registry is back on its own required `CHART_QUERY_CANCEL_CACHE_CONFIG` with the metastore cache as the default.
##########
superset/tasks/query_cancel.py:
##########
@@ -163,3 +172,150 @@ def cancel_chart_query(
exc_info=True,
)
return False
+
+
+def _registry_key(user_id: int, client_id: str) -> str:
+ """Cache key for a user's in-flight, cancellable chart query.
+
+ The user id is part of the key rather than a field compared after lookup, so
+ a ``client_id`` is only ever resolvable within the namespace of the user who
+ registered it. A caller passing somebody else's ``client_id`` gets a miss —
+ it cannot read, cancel, or overwrite another user's entry.
+ """
+ return f"chart-query-cancel:{user_id}:{client_id}"
+
+
+def _registry_ttl() -> int:
+ """How long a cancel handle stays resolvable.
+
+ A synchronous chart query cannot outlive the web request running it, so the
+ webserver timeout is the natural upper bound. Entries are discarded as soon
+ as the query returns; this TTL only bounds the leak when a worker dies
+ mid-query.
+ """
+ return int(current_app.config.get("SUPERSET_WEBSERVER_TIMEOUT", 60))
+
+
+@contextmanager
+def cancellable_chart_query(
+ client_id: "str | None", database: "Database | None"
+) -> Iterator[None]:
+ """Let the requesting user cancel this synchronous chart query by ``client_id``.
+
+ Captures the engine cancel id off the live cursor (for engines that expose
+ one before execution) and publishes it so a concurrent Stop request — which
+ lands on a different worker while this one is blocked on the query — can kill
+ the backend session. Engines without cancel support capture nothing and the
+ query stays non-cancellable, exactly as before.
+
+ A no-op without a ``client_id``, without a database (e.g. the annotation
+ datasource, which queries Superset's own metadata DB), or for an
+ unauthenticated request — an anonymous viewer of a public dashboard has no
+ user id to scope the handle to, and an unscoped handle would be cancellable
+ by any other anonymous visitor.
+ """
+ # Inline (also below, and in _publish_cancel_handle/_discard_cancel_handle/
+ # cancel_chart_query_for_user): the unit tests patch get_user_id and
+ # cache_manager at their defining modules (superset.utils.core,
+ # superset.extensions), not here. A module-level `from ... import X` binds
+ # X once at import time, before any test patch runs, so the patch would
+ # silently never take effect; re-importing on every call picks up the
+ # patched object instead.
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not client_id or database is None or user_id is None:
+ yield
+ return
+
+ # Rebound as non-optional locals: mypy does not carry the narrowing above
+ # into the nested function below.
+ owner_id: int = user_id
+ query_id: str = client_id
+ target: "Database" = database
+ database_id = target.id
+ captured = False
+
+ def _sink(cursor: Any) -> None:
+ # Republish for every cursor rather than only the first: one query
+ # object can execute several statements in turn (e.g. the grouping-sets
+ # fallback), each on whatever connection the pool hands out, and the
+ # handle must always name the session that is executing right now.
+ nonlocal captured
+ cancel_id = capture_cancel_query_id(target, cursor)
+ if cancel_id is None:
+ return
+ captured = True
+ _publish_cancel_handle(owner_id, query_id, database_id, cancel_id)
+
+ try:
+ with capture_cancel_id(_sink):
+ yield
+ finally:
+ if captured:
+ _discard_cancel_handle(owner_id, query_id)
Review Comment:
Same story here, the rebase lost it. The guarded discard is back, so a stale hit only clears the entry if it still names the id that was actually cancelled.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): make the Stop button cancel the running chart query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44552:
URL: https://github.com/apache/superset/pull/44552#issuecomment-6444870304
@aminghadersohi added the regression test: Stop for a user with no per-database grants only finds the database when the lookup skips the base filter, so it goes red if that skip is dropped.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org