You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/10/07 23:45:37 UTC
(superset) 03/07: fix(clickhouse): confirm KILL QUERY actually terminated before reporting success
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-clickhouse-stop-query-cancel
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 68a11500cfc54aa5136d81d6c04bc1e02f84422e
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Wed Oct 7 22:29:42 2026 +0000
fix(clickhouse): confirm KILL QUERY actually terminated before reporting success
cancel_query() returned True whenever cursor.execute("KILL QUERY WHERE
query_id = ...") didn't raise, without checking whether anything was
actually killed. ClickHouse's KILL QUERY never raises for a non-matching
id -- it silently does nothing -- so a stale/raced/incorrect query_id
would still report a successful cancel while the query kept running.
Switches to KILL QUERY ... SYNC, which blocks until the server confirms
the outcome, and inspects the returned kill_status: "finished" is the
only value that confirms termination; the documented non-confirming
states (waiting/cant_cancel/pending/unknown_status) now return False
since they mean a real process matched but termination wasn't confirmed.
A row with no recognizable kill_status (no process matched -- most
likely it already finished on its own) is treated as success, since the
desired end state already holds; this also covers a clickhouse-connect
driver quirk where a non-matching KILL QUERY's empty HTTP response gets
turned into a synthetic stats row rather than an empty result set.
Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
---
superset/db_engine_specs/clickhouse.py | 48 ++++++++++++++-
.../unit_tests/db_engine_specs/test_clickhouse.py | 69 +++++++++++++++++++++-
2 files changed, 114 insertions(+), 3 deletions(-)
diff --git a/superset/db_engine_specs/clickhouse.py b/superset/db_engine_specs/clickhouse.py
index d637d3d8ac8..f392803f2fd 100644
--- a/superset/db_engine_specs/clickhouse.py
+++ b/superset/db_engine_specs/clickhouse.py
@@ -736,6 +736,17 @@ class ClickHouseConnectEngineSpec(BasicParametersMixin, ClickHouseEngineSpec):
except Exception as ex:
raise cls.get_dbapi_mapped_exception(ex) from ex
+ # ClickHouse's own enum for the `kill_status` column `KILL QUERY SYNC`
+ # returns per matched process (https://clickhouse.com/docs/sql-reference/
+ # statements/kill): "finished" is the only value confirming the query
+ # was actually terminated. The other four are documented failure/
+ # indeterminate states -- a real process matched, but termination wasn't
+ # confirmed -- and must not be reported as a successful cancel.
+ _KILL_STATUS_CONFIRMED = "finished"
+ _KILL_STATUS_KNOWN_NOT_CONFIRMED = frozenset(
+ {"waiting", "cant_cancel", "pending", "unknown_status"}
+ )
+
@classmethod
def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool:
"""
@@ -754,8 +765,43 @@ class ClickHouseConnectEngineSpec(BasicParametersMixin, ClickHouseEngineSpec):
return False
try:
- cursor.execute(f"KILL QUERY WHERE query_id = '{cancel_query_id}'") # noqa: S608
+ # SYNC blocks until ClickHouse confirms the outcome. The default
+ # ASYNC mode returns immediately with kill_status="waiting" --
+ # before termination is confirmed -- which would make this
+ # method's return value meaningless.
+ cursor.execute( # noqa: S608
+ f"KILL QUERY WHERE query_id = '{cancel_query_id}' SYNC"
+ )
+ rows = cursor.fetchall()
except Exception: # pylint: disable=broad-except
return False
+ kill_status = rows[0][0] if rows and rows[0] else None
+ if kill_status == cls._KILL_STATUS_CONFIRMED:
+ return True
+ if kill_status in cls._KILL_STATUS_KNOWN_NOT_CONFIRMED:
+ # A real process matched our query_id, but ClickHouse itself
+ # couldn't confirm termination -- there's positive evidence the
+ # query may still be running, so don't report success.
+ return False
+
+ # No recognizable kill_status landed in the first column. Verified
+ # empirically against a real server, this covers two cases that
+ # can't be told apart from this response alone:
+ # 1. No process matched `query_id` at all -- most likely the query
+ # had already finished on its own between the stop request and
+ # this KILL QUERY reaching the server: ClickHouse returns a
+ # genuinely empty HTTP body for a non-matching `KILL QUERY`
+ # (0 result_rows per its own X-ClickHouse-Summary header). The
+ # desired end state -- query not running -- already holds.
+ # 2. clickhouse-connect==1.9.0's DB-API cursor synthesizes a
+ # 1-row "stats" result from that same X-ClickHouse-Summary
+ # header whenever the real HTTP body is empty, so `rows` is
+ # never actually `[]` here even when nothing matched; its
+ # first column is an int (a row count), never one of the
+ # known kill_status strings above.
+ # Both report "nothing to confirm", not "confirmed failure" --
+ # treating this as success avoids stranding the SQL Lab UI on a
+ # query that has already finished on its own. See RCA.md for the
+ # full reasoning and empirical verification.
return True
diff --git a/tests/unit_tests/db_engine_specs/test_clickhouse.py b/tests/unit_tests/db_engine_specs/test_clickhouse.py
index 2b7b38bfc54..f01b2366fdd 100644
--- a/tests/unit_tests/db_engine_specs/test_clickhouse.py
+++ b/tests/unit_tests/db_engine_specs/test_clickhouse.py
@@ -830,7 +830,13 @@ def test_execute_with_cursor_omits_settings_when_no_cancel_id_recorded() -> None
cursor_mock.execute.assert_called_once_with("SELECT 1", settings=None)
-def test_cancel_query_issues_kill_query_for_the_recorded_id() -> None:
+def test_cancel_query_issues_kill_query_sync_for_the_recorded_id() -> None:
+ """
+ SYNC is required: the default ASYNC mode returns immediately with
+ kill_status="waiting", before ClickHouse has confirmed the query is
+ actually gone -- verified empirically against a real server (see
+ RCA.md). Without SYNC, cancel_query's return value would be meaningless.
+ """
from superset.db_engine_specs.clickhouse import ( # noqa: N813
ClickHouseConnectEngineSpec as spec,
)
@@ -838,13 +844,72 @@ def test_cancel_query_issues_kill_query_for_the_recorded_id() -> None:
cursor_mock = Mock()
cancel_query_id = "d14b9e5b-47ec-478e-bc7c-f8b56be98576"
+ cursor_mock.fetchall.return_value = [
+ ["finished", cancel_query_id, "default", "SELECT 1"]
+ ]
assert spec.cancel_query(cursor_mock, Query(), cancel_query_id) is True
cursor_mock.execute.assert_called_once_with(
- f"KILL QUERY WHERE query_id = '{cancel_query_id}'"
+ f"KILL QUERY WHERE query_id = '{cancel_query_id}' SYNC"
)
+def test_cancel_query_returns_false_when_kill_status_not_confirmed() -> None:
+ """
+ A real process matched our query_id, but ClickHouse itself reports it
+ couldn't confirm termination (e.g. `cant_cancel`) -- there's positive
+ evidence the query may still be running, so this must not report
+ success just because `cursor.execute()` didn't raise.
+ """
+ from superset.db_engine_specs.clickhouse import ( # noqa: N813
+ ClickHouseConnectEngineSpec as spec,
+ )
+ from superset.models.sql_lab import Query
+
+ cancel_query_id = "d14b9e5b-47ec-478e-bc7c-f8b56be98576"
+
+ for kill_status in ("waiting", "cant_cancel", "pending", "unknown_status"):
+ cursor_mock = Mock()
+ cursor_mock.fetchall.return_value = [
+ [kill_status, cancel_query_id, "default", "SELECT 1"]
+ ]
+ assert spec.cancel_query(cursor_mock, Query(), cancel_query_id) is False, (
+ f"kill_status={kill_status!r} must not report success"
+ )
+
+
+def test_cancel_query_returns_true_when_nothing_matched() -> None:
+ """
+ No row with a recognizable kill_status -- either a genuinely empty
+ result (no process matched, e.g. the query already finished on its
+ own) or clickhouse-connect==1.9.0's own synthesized "stats" row (its
+ DB-API cursor returns a 1-row summary derived from the
+ X-ClickHouse-Summary response header instead of an empty list when the
+ real HTTP body is empty -- verified empirically against a real server,
+ see RCA.md). Both mean "nothing to confirm", which is treated as
+ success so SQL Lab doesn't get stuck believing an already-finished
+ query is still running.
+ """
+ from superset.db_engine_specs.clickhouse import ( # noqa: N813
+ ClickHouseConnectEngineSpec as spec,
+ )
+ from superset.models.sql_lab import Query
+
+ cancel_query_id = "d14b9e5b-47ec-478e-bc7c-f8b56be98576"
+
+ # Case 1: a genuinely empty result.
+ cursor_mock = Mock()
+ cursor_mock.fetchall.return_value = []
+ assert spec.cancel_query(cursor_mock, Query(), cancel_query_id) is True
+
+ # Case 2: clickhouse-connect's synthesized X-ClickHouse-Summary stats
+ # row -- same 9-column shape observed against a real server, first
+ # column is an int (a row count), never a kill_status string.
+ cursor_mock = Mock()
+ cursor_mock.fetchall.return_value = [[1, 834, 0, 0, 0, 0, 0, 1686330, "some-id"]]
+ assert spec.cancel_query(cursor_mock, Query(), cancel_query_id) is True
+
+
def test_cancel_query_rejects_non_uuid_ids() -> None:
"""
Defense-in-depth against injection via a tampered/forged cancel id --