Honor query cancellation in waiting for quorum in ReplicatedMergeTree by nickitat · Pull Request #107513 · ClickHouse/ClickHouse · GitHub
Skip to content

Honor query cancellation in waiting for quorum in ReplicatedMergeTree#107513

Merged
tavplubix merged 6 commits into
masterfrom
fix-quorum-wait-honor-cancellation
Jun 16, 2026
Merged

Honor query cancellation in waiting for quorum in ReplicatedMergeTree#107513
tavplubix merged 6 commits into
masterfrom
fix-quorum-wait-honor-cancellation

Conversation

@nickitat

@nickitat nickitat commented Jun 15, 2026

Copy link
Copy Markdown
Member

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Query cancellation is now better tracked while waiting for the quorum in ReplicatedMergeTree.

Version info

  • Merged into: 26.6.1.875

nickitat and others added 2 commits June 15, 2026 12:55
`waitForQuorum` blocked in a single `event->tryWait(quorum_timeout_ms)`
that ignores query cancellation. The quorum watch is only signalled when
the quorum node changes, so when a quorum INSERT is killed
(`KILL QUERY ... ASYNC`, which sets `is_killed`/`is_cancelled`), nothing
wakes the waiter and the killed query keeps blocking for the whole
`insert_quorum_timeout`.

Under the stress test (which has a background random query killer and
randomizes `insert_quorum_timeout` to very large values, e.g. 6000000 ms),
a killed async-insert flush into a `ReplicatedMergeTree` quorum table
lingered in the processlist for ~100 minutes inside
`ReplicatedMergeTreeSink::waitForQuorum`, far past the hung-check
threshold, surfacing as `Hung check failed, possible deadlock found`.

Fix: wait for the quorum watch in bounded steps and re-check
`getProcessListElement()->isKilled()` between steps, throwing
`QUERY_WAS_CANCELLED` promptly when the query was killed. The overall
`quorum_timeout_ms` deadline and the existing timeout/satisfied-quorum
behavior are preserved. This mirrors the cancellation check the
surrounding `ZooKeeperRetriesControl` already performs between retries
(`query_status->checkTimeLimit()`), closing the gap inside the blocking
wait. The exception is not a `KeeperException`, so it propagates out of
the retry loop immediately instead of being retried.

A new stateless test reproduces the hang: a quorum=2 INSERT into a
replica whose peer never fetches (`SYSTEM STOP FETCHES`) blocks in
`waitForQuorum`; after `KILL QUERY` it must terminate promptly rather
than after the full `insert_quorum_timeout`. The test fails (lingers for
the full 600s timeout) without the fix and passes (terminates in <1s)
with it.

CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107467&sha=13537af226f37d86bfaf51214ab59187fd5473f6&name_0=PR&name_1=Stress%20test%20%28arm_ubsan%29

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Refines the previous commit. Instead of only checking isKilled(), the
bounded quorum wait now calls getProcessListElement()->checkTimeLimit(),
which throws QUERY_WAS_CANCELLED on a KILL QUERY and TIMEOUT_EXCEEDED when
max_execution_time is exceeded. This matches the check that the
surrounding ZooKeeperRetriesControl already performs between retries, and
makes the cancellation/time-limit responsiveness testable without a second
connection.

The test is rewritten as a pure SQL test: a quorum = 2 INSERT into a
replica whose peer never fetches (SYSTEM STOP FETCHES) blocks in
waitForQuorum; with a large insert_quorum_timeout and a small
max_execution_time the INSERT must be interrupted promptly with
TIMEOUT_EXCEEDED rather than hanging for the whole timeout. The test fails
(the INSERT hangs, ignoring max_execution_time) without the fix and passes
(throws TIMEOUT_EXCEEDED in ~4s) with it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

@clickhouse-gh clickhouse-gh Bot added the pr-improvement Pull request with some product improvements label Jun 15, 2026
Comment thread src/Storages/MergeTree/ReplicatedMergeTreeSink.cpp
Review feedback: `QueryStatus::checkTimeLimit()` only throws for an actual
`KILL QUERY` or for `timeout_overflow_mode = 'throw'`; with
`timeout_overflow_mode = 'break'` it returns false. The previous version
ignored that return value, so a quorum INSERT with `max_execution_time`
and `timeout_overflow_mode = 'break'` kept waiting until
`insert_quorum_timeout` — the same long wait this change removes.

`CancellationChecker` deliberately does not hard-cancel queries in 'break'
mode (it only calls `checkTimeLimit()`), so the framework would otherwise
let such a query finish as a successful graceful "break". A quorum INSERT
must not report a partial success while the quorum status is unknown, so on
a false return we escalate to a hard cancellation — the same
`cancelQuery(CancelReason::TIMEOUT)` call `CancellationChecker` uses for
'throw' mode — and the query fails with `TIMEOUT_EXCEEDED` instead of
hanging or silently succeeding.

The test now covers both `timeout_overflow_mode` values (each on its own
pair of replicas, so a finished-but-unsatisfied quorum write does not make
the next INSERT fail early with `UNSATISFIED_QUORUM_FOR_PREVIOUS_WRITE`);
both must fail promptly with `TIMEOUT_EXCEEDED` rather than hang until
`insert_quorum_timeout`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nickitat nickitat changed the title Make ReplicatedMergeTreeSink::waitForQuorum honor query cancellation Honor query cancellation in waiting for quorum in ReplicatedMergeTree Jun 15, 2026
nickitat and others added 2 commits June 15, 2026 15:25
Review feedback: the SQL test covers the max_execution_time paths (throw
and break), but not explicit `KILL QUERY` cancellation, which is a distinct
path (`QUERY_WAS_CANCELLED`) and the scenario that originally tripped the
stress-test hung-check.

This adds a focused `.sh` regression (KILL needs a second connection, so it
cannot be a pure SQL test): start a synchronous quorum INSERT into a replica
whose peer never fetches (`SYSTEM STOP FETCHES`), so it blocks in
`waitForQuorum`; then `KILL QUERY` it and assert it terminates promptly with
`QUERY_WAS_CANCELLED` instead of waiting for `insert_quorum_timeout` (set
very large). Verified: with the fix the killed INSERT returns in ~1s with
`QUERY_WAS_CANCELLED`; without it the query lingers in the processlist until
the timeout. `async_insert = 0` is pinned so the killed query_id is the one
waiting for quorum regardless of settings randomization.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
/// successful "break"), but a quorum INSERT must not report success while the quorum status is unknown.
/// Escalate to a hard cancellation (the same call CancellationChecker uses for 'throw' mode) so the
/// query fails with TIMEOUT_EXCEEDED instead of being silently completed.
if (process_list_element && !process_list_element->checkTimeLimit())

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case you wonder how QUERY_WAS_CANCELLED is propagated in this case:

  1. KILL QUERY ... ASYNC → QueryStatus::cancelQuery(...) sets is_killed = true
  (ProcessList.cpp:542).
  2. The killed INSERT is parked in ReplicatedMergeTreeSink::waitForQuorum's loop. Within
  one quorum_wait_step_ms step (≤1s), the next iteration evaluates:
  // ReplicatedMergeTreeSink.cpp:1301
  if (process_list_element && !process_list_element->checkTimeLimit())
  3. QueryStatus::checkTimeLimit() (ProcessList.cpp:619) calls
  throwProperExceptionIfNeeded() (line 622) before it would return a bool. That sees
  is_killed (line 577), and since the cancel reason is a plain KILL (not TIMEOUT), calls
  throwQueryWasCancelled() (line 585).
  4. throwQueryWasCancelled() throws at ProcessList.cpp:632:
  throw Exception(ErrorCodes::QUERY_WAS_CANCELLED, "Query was cancelled");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that handling "break" in the same way as "throw" is a good idea, the query can fail before it enters this function and then it will silently ignore the exception.

For this is what happened here - https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=105330&sha=cbfb4a10514013862658949ae52d42f1e8d63ed0&name_0=PR&name_1=Stateless%20tests%20%28amd_msan%2C%20WasmEdge%2C%20parallel%2C%202%2F2%29
Here the problem was that on disk reservation took ±3 second
And actually for this one we can handle it in ReplicatedMergeTreeSink::onFinish()

But, there are other places where query may fail

So TL;DR; let's handle "break" as ignoring error

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickitat nickitat added pr-bugfix Pull request with bugfix, not backported by default and removed pr-improvement Pull request with some product improvements labels Jun 15, 2026
The AsyncInsert stateless test configuration forces async_insert = 1, and
async inserts with quorum require insert_quorum_parallel = 1. The test uses
sequential quorum (insert_quorum_parallel = 0), so under that configuration
the INSERT failed early with UNSUPPORTED_PARAMETER (code 2) instead of
reaching waitForQuorum and timing out with TIMEOUT_EXCEEDED (code 159):

  Async inserts with quorum only make sense with enabled insert_quorum_parallel
  setting, either disable quorum or set insert_quorum_parallel=1 or do not use
  async inserts. (UNSUPPORTED_PARAMETER)

Pin async_insert = 0 in both INSERTs so the test is deterministically a
synchronous quorum insert regardless of the run configuration (the companion
04338_kill_quorum_insert already does this).

CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107513&sha=b7ba82febf2281e355b128b58d6a86048bf7001f&name_0=PR&name_1=Stateless%20tests%20%28amd_llvm_coverage%2C%20AsyncInsert%2C%20s3%20storage%2C%20parallel%29
PR: #107513

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 85.20% 85.10% -0.10%
Functions 92.30% 92.30% +0.00%
Branches 77.40% 77.30% -0.10%

Changed lines: Changed C/C++ lines covered by tests: 19/20 (95.00%) | Lost baseline coverage (was covered on master, now uncovered in this PR): 8 line(s) · Uncovered code

Full report · Diff report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants