fix(sqllab): mark early-cancelled query stopped, not stuck running (#43889)

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

docs/developer_docs/contributing/overview.md

@@ -93,7 +93,7 @@ Look through the GitHub issues. Issues tagged with
 
 Superset could always use better documentation,
 whether as part of the official Superset docs,
-in docstrings, `docs/*.rst` or even on the web as blog posts or
+in docstrings, Markdown files in `docs/`, or even on the web as blog posts or
 articles. See [Documentation](./howtos.md#contributing-to-documentation) for more details.
 
 ### Add Translations

superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx

@@ -16,6 +16,8 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+
+/** @jsxImportSource @emotion/react */
 import {
   Children,
   cloneElement,
@@ -286,9 +288,28 @@ function StickyWrap({
       </colgroup>
     );
 
-    const headerContainerWidth = hasVerticalScroll
-      ? maxWidth - scrollBarSize
-      : maxWidth;
+    // Below, `width: maxWidth` is applied unconditionally (never reduced by
+    // subtracting a separately-measured scrollbar width, unlike this file's
+    // previous `maxWidth - scrollBarSize`). That's the load-bearing part of
+    // this fix: the shared colgroup (computed from the sizer below, whose
+    // own clientWidth can only ever be <= maxWidth) can never need more
+    // width than that, so a header/footer wrapper that's never narrowed
+    // below maxWidth can never clip it, regardless of whether any
+    // JS-measured scrollbar size agrees with what the sizer/body actually
+    // reserve in a given browser.
+    //
+    // `scrollbarGutter`/`scrollBarStyles` below are a separate, secondary
+    // measure -- matching an actual clip boundary is not what they're for
+    // (an `overflow: hidden` box's clip boundary sits at its real
+    // border-box edge regardless of `scrollbar-gutter`, which only affects
+    // what `clientWidth` reports). They keep header/footer's reported
+    // `clientWidth` consistent with body's so that, when both a vertical
+    // and a horizontal scrollbar are present, the horizontal `scrollLeft`
+    // synced from body (see `onScroll` below) reveals the same slice of the
+    // row in header/footer as is actually visible in body.
+    const headerFooterGutter: CSSProperties = {
+      scrollbarGutter: hasVerticalScroll ? 'stable' : undefined,
+    };
 
     headerTable = (
       <div
@@ -296,9 +317,11 @@ function StickyWrap({
         ref={scrollHeaderRef}
         style={{
           overflow: 'hidden',
-          width: headerContainerWidth,
+          width: maxWidth,
           boxSizing: 'border-box',
+          ...headerFooterGutter,
         }}
+        css={scrollBarStyles}
         role="presentation"
       >
         {cloneElement(
@@ -317,9 +340,11 @@ function StickyWrap({
         ref={scrollFooterRef}
         style={{
           overflow: 'hidden',
-          width: headerContainerWidth,
+          width: maxWidth,
           boxSizing: 'border-box',
+          ...headerFooterGutter,
         }}
+        css={scrollBarStyles}
         role="presentation"
       >
         {cloneElement(

superset-frontend/plugins/plugin-chart-table/test/DataTable/hooks/useSticky.test.tsx

@@ -0,0 +1,205 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useCallback } from 'react';
+import { useTable, Column } from 'react-table';
+import { render } from '@superset-ui/core/spec';
+import useSticky from '../../../src/DataTable/hooks/useSticky';
+
+// A value distinguishable from any real scrollbar width, so the width
+// assertions below can detect whether header/footer's wrapper width was
+// computed by subtracting this JS-measured probe from `maxWidth` (the old,
+// removed `maxWidth - scrollBarSize` behavior) rather than always being the
+// unconditional `maxWidth` the fix uses. If that subtraction is ever
+// reintroduced, header/footer's `style.width` would read
+// `${MAX_WIDTH - MOCKED_SCROLLBAR_PROBE_SIZE}px`, an unmistakably wrong
+// value given how large this mock is.
+const MOCKED_SCROLLBAR_PROBE_SIZE = 42;
+
+jest.mock('../../../src/DataTable/utils/getScrollBarSize', () => ({
+  __esModule: true,
+  CUSTOM_SCROLLBAR_SIZE: 8,
+  default: () => 0,
+  getCustomScrollBarSize: () => MOCKED_SCROLLBAR_PROBE_SIZE,
+}));
+
+const MAX_WIDTH = 300;
+const MAX_HEIGHT = 120; // small enough that the mocked content forces a vertical scroll
+
+const TOTAL_HEADER_HEIGHT = 30;
+const TOTAL_FOOTER_HEIGHT = 30;
+// Larger than `MAX_HEIGHT - TOTAL_HEADER_HEIGHT - TOTAL_FOOTER_HEIGHT`, so the
+// sticky layout effect computes `hasVerticalScroll: true`.
+const FULL_TABLE_HEIGHT = 400;
+
+function mockMeasurements() {
+  jest
+    .spyOn(HTMLElement.prototype, 'clientHeight', 'get')
+    .mockImplementation(function mockClientHeight(this: HTMLElement) {
+      if (this.tagName === 'THEAD') return TOTAL_HEADER_HEIGHT;
+      if (this.tagName === 'TFOOT') return TOTAL_FOOTER_HEIGHT;
+      if (this.tagName === 'TABLE') return FULL_TABLE_HEIGHT;
+      return 0;
+    });
+  jest
+    .spyOn(HTMLElement.prototype, 'getBoundingClientRect')
+    .mockImplementation(function mockRect(this: HTMLElement) {
+      const width = this.tagName === 'TH' ? 60 : 0;
+      return {
+        width,
+        height: 0,
+        top: 0,
+        left: 0,
+        right: width,
+        bottom: 0,
+        x: 0,
+        y: 0,
+        toJSON: () => {},
+      } as DOMRect;
+    });
+}
+
+type Row = { category: string; amount: string };
+
+const columns: Column<Row>[] = [
+  { Header: 'Category', accessor: 'category' },
+  { Header: 'SUM(amount)', accessor: 'amount' },
+];
+
+const data: Row[] = Array.from({ length: 8 }, (_, i) => ({
+  category: `Category ${i}`,
+  amount: `${1234567.891234 + i}`,
+}));
+
+function StickyTableHarness() {
+  const getTableSize = useCallback(
+    () => ({ width: MAX_WIDTH, height: MAX_HEIGHT }),
+    [],
+  );
+  const { getTableProps, headerGroups, rows, prepareRow, wrapStickyTable } =
+    useTable<Row>(
+      {
+        columns,
+        data,
+        getTableSize,
+      },
+      useSticky,
+    );
+
+  const renderTable = () => (
+    <table {...getTableProps()}>
+      <thead>
+        {headerGroups.map(hg => (
+          <tr {...hg.getHeaderGroupProps()} key={hg.id}>
+            {hg.headers.map(col => (
+              <th {...col.getHeaderProps()} key={col.id}>
+                {col.render('Header')}
+              </th>
+            ))}
+          </tr>
+        ))}
+      </thead>
+      <tbody>
+        {rows.map(row => {
+          prepareRow(row);
+          return (
+            <tr {...row.getRowProps()} key={row.id}>
+              {row.cells.map(cell => (
+                <td {...cell.getCellProps()} key={cell.column.id}>
+                  {cell.render('Cell')}
+                </td>
+              ))}
+            </tr>
+          );
+        })}
+      </tbody>
+      <tfoot>
+        <tr key="footer">
+          <th>Summary</th>
+          <td>
+            <strong>14814904.694808</strong>
+          </td>
+        </tr>
+      </tfoot>
+    </table>
+  );
+
+  return <div data-test="sticky-root">{wrapStickyTable(renderTable)}</div>;
+}
+
+test('sticky header/footer width matches the body, independent of the scrollbar-size probe', () => {
+  mockMeasurements();
+
+  const { container } = render(<StickyTableHarness />);
+
+  const root = container.querySelector('[data-test="sticky-root"] > div');
+  expect(root).not.toBeNull();
+  const [headerDiv, bodyDiv, footerDiv] = Array.from(
+    root!.children,
+  ) as HTMLDivElement[];
+
+  expect(bodyDiv.style.width).toBe(`${MAX_WIDTH}px`);
+
+  // This is the load-bearing assertion for the reported bug. Before the fix
+  // these read `${MAX_WIDTH - MOCKED_SCROLLBAR_PROBE_SIZE}px` (258px) --
+  // genuinely narrower than the body, from a real CSS `width` subtraction
+  // (`maxWidth - scrollBarSize`), not just a smaller reported `clientWidth`.
+  // A wrapper that's actually narrower than the shared, fixed-layout
+  // colgroup it has to display gets genuinely clipped by its own
+  // `overflow: hidden` (verified with real hit-testing in a real browser --
+  // this is not true of the `scrollbarGutter` assertions below). The fix
+  // makes header/footer always exactly `maxWidth`, which the colgroup
+  // (bounded by the sizer's `clientWidth`, itself bounded by `maxWidth`)
+  // can never exceed.
+  expect(headerDiv.style.width).toBe(`${MAX_WIDTH}px`);
+  expect(footerDiv.style.width).toBe(`${MAX_WIDTH}px`);
+
+  // Secondary, not itself load-bearing for preventing clipping: real
+  // hit-testing shows `scrollbar-gutter` on an `overflow: hidden` box
+  // changes what `clientWidth` reports without moving where it actually
+  // clips, so this doesn't guard against the reported bug by itself. It's
+  // asserted because header/footer's reported `clientWidth` still needs to
+  // match body's `clientWidth` for their programmatically
+  // synced `scrollLeft` (see `onScroll` in `useSticky.tsx`) to reveal the
+  // same slice of the row body actually shows, when a horizontal scrollbar
+  // is present alongside a vertical one.
+  expect(headerDiv.style.scrollbarGutter).toBe(bodyDiv.style.scrollbarGutter);
+  expect(footerDiv.style.scrollbarGutter).toBe(bodyDiv.style.scrollbarGutter);
+  expect(bodyDiv.style.scrollbarGutter).toBe('stable');
+
+  // Pin the `css={scrollBarStyles}` addition to header/footer directly (part
+  // of the same secondary consistency measure as the `scrollbarGutter`
+  // assertions above, not the clipping fix). This component carries
+  // `/** @jsxImportSource @emotion/react */`, which makes
+  // Babel route its `css` prop through Emotion's jsx runtime instead of
+  // passing `css` straight through as an inert DOM attribute (the default in
+  // this repo's Jest/Babel setup, which -- unlike the webpack/SWC build --
+  // doesn't set `importSource: '@emotion/react'` globally). With the pragma
+  // in place, an applied `css` prop is observable as a real, non-empty
+  // className, so this assertion actually fails without the fix instead of
+  // passing regardless of whether `scrollBarStyles` is wired up.
+  //
+  // Before `css={scrollBarStyles}` was added to header/footer, they had no
+  // emotion-generated class at all (`className === ''`) while the body kept
+  // its own -- so this fails pre-fix and passes post-fix.
+  expect(headerDiv.className).not.toBe('');
+  expect(headerDiv.className).toBe(bodyDiv.className);
+  expect(footerDiv.className).toBe(bodyDiv.className);
+
+  jest.restoreAllMocks();
+});

superset-frontend/plugins/plugin-chart-table/test/DataTable/utils/getScrollBarSize.test.ts

@@ -45,8 +45,8 @@ test('getCustomScrollBarSize measures the probe using the shared custom scrollba
 });
 
 test('CUSTOM_SCROLLBAR_SIZE matches the custom scrollbar width rendered in the sticky table', () => {
-  // useSticky.tsx's scrollBarStyles must stay in sync with this constant so
-  // the sticky header's shrink amount always matches the body's real
-  // scrollbar width.
+  // useSticky.tsx's scrollBarStyles sets `::-webkit-scrollbar { width: ... }`
+  // from this constant, so it must stay in sync with it or the real
+  // scrollbar body/sizer render won't match what this constant claims.
   expect(CUSTOM_SCROLLBAR_SIZE).toBe(8);
 });

superset/constants.py

@@ -42,6 +42,12 @@ NO_TIME_RANGE = "No filter"
 
 QUERY_CANCEL_KEY = "cancel_query"
 QUERY_EARLY_CANCEL_KEY = "early_cancel_query"
+# Set once execute_sql_statements() has opened a DB connection and asked the
+# engine spec for a cancel handle, regardless of whether one came back. Lets
+# cancel_query() tell "hasn't been dispatched to the engine yet" (safe to
+# fabricate a stop) apart from "this engine just has no cancel support"
+# (must fail honestly) when no cancel ID is on record.
+QUERY_DISPATCHED_KEY = "query_dispatched"
 
 LRU_CACHE_MAX_SIZE = 256
 

superset/daos/query.py

@@ -28,6 +28,7 @@ from superset.queries.filters import QueryFilter
 from superset.queries.saved_queries.filters import SavedQueryFilter
 from superset.utils.core import get_user_id
 from superset.utils.dates import now_as_float
+from superset.utils.decorators import transaction
 
 logger = logging.getLogger(__name__)
 
@@ -59,6 +60,7 @@ class QueryDAO(BaseDAO[Query]):
         )
 
     @staticmethod
+    @transaction()
     def stop_query(client_id: str) -> None:
         query = (
             db.session.query(Query)
@@ -81,6 +83,11 @@ class QueryDAO(BaseDAO[Query]):
         if not sql_lab.cancel_query(query):
             raise SupersetCancelQueryException("Could not cancel query")
 
+        # cancel_query() may have staged an early-cancel flag on query.extra
+        # without committing it (see its docstring/comments); the
+        # @transaction decorator commits it together with status=STOPPED
+        # below in one transaction, closing the window where another
+        # request could observe the flag set but the status still RUNNING.
         query.status = QueryStatus.STOPPED
         query.end_time = now_as_float()
 

superset/sql_lab.py

@@ -39,7 +39,11 @@ from superset import (
     security_manager,
 )
 from superset.common.db_query_status import QueryStatus
-from superset.constants import QUERY_CANCEL_KEY, QUERY_EARLY_CANCEL_KEY
+from superset.constants import (
+    QUERY_CANCEL_KEY,
+    QUERY_DISPATCHED_KEY,
+    QUERY_EARLY_CANCEL_KEY,
+)
 from superset.dataframe import df_to_records
 from superset.db_engine_specs import BaseEngineSpec
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
@@ -99,6 +103,39 @@ def handle_query_error(
 ) -> dict[str, Any]:
     """Local method handling error while processing the SQL"""
     payload = payload or {}
+
+    # A stop request may have already committed STOPPED status while this
+    # exception was being raised/propagated -- this function is the general
+    # catch-all for failures anywhere in execute_sql_statements (connection
+    # setup, cancel-ID acquisition, parsing, or a per-block failure), not
+    # just ones caused by the stop itself. A terminal stop must stay
+    # terminal, so don't let an unrelated error overwrite it with FAILED.
+    #
+    # Deliberately NOT a flush()-then-refresh(query) here, unlike the other
+    # STOPPED-preservation checks in this module: the exception that got us
+    # here may itself have already set query.status (or other attributes)
+    # locally (e.g. SoftTimeLimitExceeded's own handler sets TIMED_OUT
+    # without committing). Flushing first would push that stale local state
+    # to the DB, clobbering a concurrently-committed STOPPED before this
+    # check ever gets to observe it.
+    #
+    # A targeted refresh(attribute_names=["status"]) alone isn't enough:
+    # verified empirically that even though it expires and reloads only the
+    # named attribute (so a dirty `status` itself is correctly discarded
+    # rather than written), the reload's own SELECT still triggers a normal
+    # autoflush of any OTHER dirty attribute on the session first -- e.g. a
+    # pending query.tmp_table_name or query.executed_sql set earlier would
+    # still get written before the status read. no_autoflush suppresses
+    # that: verified it emits only the targeted SELECT, with no UPDATE
+    # beforehand, and leaves other pending attributes exactly as dirty as
+    # they were (to be flushed normally by this function's own commit()
+    # below, once we're past the STOPPED check).
+    with db.session.no_autoflush:
+        db.session.refresh(query, attribute_names=["status"])
+    if query.status == QueryStatus.STOPPED:
+        payload.update({"status": query.status})
+        return payload
+
     msg = f"{prefix_message} {str(ex)}".strip()
     query.error_message = msg
     query.tmp_table_name = None
@@ -412,6 +449,21 @@ def execute_sql_statements(  # noqa: C901
 
     query = get_query(query_id=query_id)
     payload: dict[str, Any] = {"query_id": query_id}
+
+    # A stop request may have landed before this worker even started (e.g.
+    # the request was queued and the user clicked Stop before a worker
+    # picked it up). Honor it here, mirroring the per-block stopped-check
+    # further down, instead of unconditionally overwriting it back to
+    # RUNNING and dispatching the statement anyway.
+    #
+    # Same disclosed, unfixed TOCTOU residual as the other status checks in
+    # this function (see the longer comment above the pre-payload check
+    # further down): a stop committed strictly between this check and the
+    # `query.status = RUNNING` commit a few lines below is still missed.
+    if query.status == QueryStatus.STOPPED:
+        payload.update({"status": query.status})
+        return payload
+
     database = query.database
     db_engine_spec = database.db_engine_spec
     db_engine_spec.patch()
@@ -509,9 +561,14 @@ def execute_sql_statements(  # noqa: C901
         cursor = conn.cursor()
 
         cancel_query_id = db_engine_spec.get_cancel_query_id(cursor, query)
+        # Recorded unconditionally -- even when no cancel ID comes back --
+        # so cancel_query() can tell "hasn't reached the engine yet" (still
+        # safe to fabricate a stop) apart from "this engine has no cancel
+        # support" (must fail honestly) once we get here.
+        query.set_extra_json_key(QUERY_DISPATCHED_KEY, True)
         if cancel_query_id is not None:
             query.set_extra_json_key(QUERY_CANCEL_KEY, cancel_query_id)
-            db.session.commit()
+        db.session.commit()
 
         block_count = len(blocks)
         for i, block in enumerate(blocks):
@@ -564,6 +621,41 @@ def execute_sql_statements(  # noqa: C901
         if parsed_script.has_mutation() or query.select_as_cta:
             conn.commit()
 
+    # A stop request may have landed after the last per-block check but
+    # before the final statement finished (there's no next iteration to
+    # catch it on for the last block). Check again before building a SUCCESS
+    # payload or writing results to the backend -- both would otherwise
+    # disagree with the row. The results-backend-write-failure branch below
+    # has its own second check for the same reason (a stop landing while
+    # that specific write is in flight).
+    #
+    # KNOWN, DELIBERATELY UNFIXED RESIDUAL: this codebase has no DB-level
+    # locking, so every "check status, then later commit something based on
+    # what was read" pattern in this function -- this one, the
+    # results-backend-write-failure check below, the startup check before
+    # `query.status = RUNNING` is committed a few lines later, and
+    # cancel_query()'s own QUERY_DISPATCHED_KEY read/commit gap (see the
+    # disclosure comment there) -- has the same fundamental TOCTOU window: a
+    # stop committed strictly between the check and the later commit is
+    # still missed. Each check narrows its window as much as reasonably
+    # possible without locking; none of them claim to close it. Closing any
+    # of them for real needs real DB-level row locking (e.g.
+    # SELECT ... FOR UPDATE) or optimistic-concurrency versioning on the
+    # query row, neither of which is meaningfully verifiable against the
+    # sqlite backend this codebase tests against, and is deliberately not
+    # attempted here.
+    #
+    # flush() first: refresh() does NOT autoflush -- without this, any
+    # pending, uncommitted attribute set earlier in this iteration (e.g.
+    # query.executed_sql, set just before execute_query() ran) would be
+    # silently discarded and reloaded back to its previous committed value
+    # instead of surviving to the function's own later commits.
+    db.session.flush()
+    db.session.refresh(query)
+    if query.status == QueryStatus.STOPPED:
+        payload.update({"status": query.status})
+        return payload
+
     # Success, updating the query entry in database
     query.rows = result_set.size
     query.progress = 100
@@ -652,6 +744,36 @@ def execute_sql_statements(  # noqa: C901
                 # For async queries (not returning results inline), mark as FAILED
                 # because results are inaccessible to the user
                 if not return_results:
+                    # A stop request may have landed and committed STOPPED
+                    # while this (potentially slow) results-backend write was
+                    # in flight. Refresh before marking FAILED -- a terminal
+                    # STOPPED must stay terminal, not be overwritten just
+                    # because the backend write also failed to complete
+                    # around the same time.
+                    #
+                    # flush() first: refresh() does NOT autoflush -- without
+                    # this, the result metadata already set earlier in this
+                    # function (rows, progress, extra "columns", select_sql,
+                    # end_time) plus the results_key = None set just above
+                    # would be silently discarded and reloaded back to their
+                    # previous (pre-execution) values instead of surviving to
+                    # this branch's own commit below.
+                    db.session.flush()
+                    db.session.refresh(query)
+                    if query.status == QueryStatus.STOPPED:
+                        # A fresh, minimal payload -- not `payload.update()`.
+                        # By this point `payload` already has the full
+                        # SUCCESS shape baked in from earlier (result data, a
+                        # nested query["state"] == SUCCESS, and a resultsKey
+                        # for a write that just failed), so patching only the
+                        # top-level "status" key would return a payload that
+                        # simultaneously claims STOPPED while still carrying
+                        # SUCCESS data and a resultsKey pointing at nothing
+                        # actually stored. Matches the shape the other
+                        # STOPPED-preservation return sites in this function
+                        # use (a plain {"query_id", "status"} pair).
+                        return {"query_id": query_id, "status": query.status}
+
                     query.status = QueryStatus.FAILED
                     query.error_message = (
                         "Failed to store query results in the results backend. "
@@ -676,8 +798,24 @@ def execute_sql_statements(  # noqa: C901
                     key,
                 )
 
-    # Only set SUCCESS if we didn't already set FAILED above
-    if query.status != QueryStatus.FAILED:
+    # Only set SUCCESS if we didn't already set FAILED above, and don't
+    # clobber a STOPPED status a concurrent stop request may have committed
+    # since the check above -- a terminal stop must stay terminal. This is a
+    # backstop for the DB row specifically (the payload/results-write
+    # consistency check already happened above); it doesn't reopen or
+    # re-narrow the same disclosed race window from that check.
+    #
+    # flush() first: refresh() does NOT autoflush -- without this, every
+    # result field set on the success path above (rows, progress, extra
+    # "columns", select_sql, end_time, results_key) would be silently
+    # discarded and reloaded back to their pre-execution (typically None)
+    # values on EVERY successful query, since nothing before this point
+    # commits them. This was a real regression caught by CI integration
+    # tests across all three DB backends (sqlite/mysql/postgres) that the
+    # unit-test suite driving this fix never exercised.
+    db.session.flush()
+    db.session.refresh(query)
+    if query.status not in (QueryStatus.FAILED, QueryStatus.STOPPED):
         query.status = QueryStatus.SUCCESS
     db.session.commit()
 
@@ -747,7 +885,41 @@ def cancel_query(query: Query) -> bool:
 
     cancel_query_id = query.extra.get(QUERY_CANCEL_KEY)
     if cancel_query_id is None:
-        return False
+        # KNOWN LIMITATION (deliberately not fixed here): this read of
+        # QUERY_DISPATCHED_KEY and execute_sql_statements()'s own commit of
+        # that same flag (see the "Recorded unconditionally" comment where
+        # it's set) are two independent transactions with no lock between
+        # them. A stop request can still land in the narrow window where
+        # this read has already happened -- deciding "not dispatched yet,
+        # safe to fabricate a stop" -- but the worker's dispatch commit
+        # lands immediately after, so the statement still gets sent to the
+        # engine even though the row was just marked STOPPED. Closing this
+        # for real needs DB-level row locking (e.g. SELECT ... FOR UPDATE)
+        # or optimistic-concurrency versioning on the query row; neither is
+        # meaningfully verifiable against the sqlite backend this codebase's
+        # tests run against, so it's out of scope here rather than a
+        # false claim of safety.
+        if query.extra.get(QUERY_DISPATCHED_KEY):
+            # execute_sql_statements() already opened a connection and asked
+            # this engine spec for a cancel handle, and still got nothing --
+            # this engine genuinely has no way to cancel a query once it's
+            # running. That's a real failure, not a race window; report it
+            # honestly rather than fabricating a stop the engine can't back.
+            return False
+        # No cancel handle has been recorded and execution hasn't reached the
+        # engine yet, so "no ID" here can only mean "too early to have one" --
+        # record the same early-cancel intent Trino's own
+        # prepare_cancel_query() records for its harder case (ID only
+        # obtainable after execution starts), so the stopped check at the top
+        # of the statement-block loop honors the request instead of leaving
+        # the query stuck at RUNNING with no avenue to ever stop it.
+        #
+        # Not committed here: the caller (QueryDAO.stop_query) commits this
+        # together with status=STOPPED in one transaction, so another
+        # request can never observe the flag set but the status still
+        # RUNNING.
+        query.set_extra_json_key(QUERY_EARLY_CANCEL_KEY, True)
+        return True
 
     with query.database.get_sqla_engine(
         catalog=query.catalog,

tests/unit_tests/dao/queries_test.py

@@ -146,6 +146,11 @@ def test_query_dao_stop_query_not_found(
 
     db.session.add(database)
     db.session.add(query_obj)
+    # Committed (not just autoflushed) since QueryDAO.stop_query() is now
+    # wrapped in @transaction, which rolls back the session on the
+    # QueryNotFoundException raised below -- an uncommitted insert would be
+    # discarded along with it.
+    db.session.commit()
 
     mocker.patch("superset.sql_lab.cancel_query", return_value=False)
 
@@ -228,6 +233,11 @@ def test_query_dao_stop_query_failed(
 
     db.session.add(database)
     db.session.add(query_obj)
+    # Committed (not just autoflushed) since QueryDAO.stop_query() is now
+    # wrapped in @transaction, which rolls back the session on the
+    # SupersetCancelQueryException raised below -- an uncommitted insert
+    # would be discarded along with it.
+    db.session.commit()
 
     mocker.patch("superset.sql_lab.cancel_query", return_value=False)
 
@@ -314,6 +324,11 @@ def test_query_dao_stop_query_wrong_user(
 
     db.session.add(database)
     db.session.add(query_obj)
+    # Committed (not just autoflushed) since QueryDAO.stop_query() is now
+    # wrapped in @transaction, which rolls back the session on the
+    # QueryNotFoundException raised below -- an uncommitted insert would be
+    # discarded along with it.
+    db.session.commit()
 
     # Simulate a different user (user 2) attempting to stop user 1's query
     mocker.patch("superset.daos.query.get_user_id", return_value=2)

tests/unit_tests/sql_lab_test.py

@@ -483,6 +483,11 @@ def test_get_sql_results_oauth2(mocker: MockerFixture, app) -> None:
         mocker.patch("superset.daos.key_value.KeyValueDAO.delete_expired_entries")
         mocker.patch("superset.daos.key_value.KeyValueDAO.create_entry")
         mocker.patch("superset.db_engine_specs.base.db.session.commit")
+        # handle_query_error() refreshes `query` from the DB to check for a
+        # concurrently-committed STOPPED status before overwriting it with
+        # FAILED; `query` here is a MagicMock, not a real persistent ORM
+        # instance, so the real refresh() would error introspecting it.
+        mocker.patch("superset.sql_lab.db.session.refresh", return_value=None)
 
         g = mocker.patch("superset.db_engine_specs.base.g")
         g.user = mocker.MagicMock()