Honor query cancellation in waiting for quorum in ReplicatedMergeTree#107513
Conversation
`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>
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>
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()) |
There was a problem hiding this comment.
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");
There was a problem hiding this comment.
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
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>
LLVM Coverage ReportChanged 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 |

Changelog category (leave one):
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
26.6.1.875