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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
