Fix client-v2: cancelTransportRequest lost between two retry attempts - #2990
Merged
Conversation
…ween retry attempts
Cancellation state lived on the per-attempt TransportRequest, so a
cancelTransportRequest(queryId) landing between two attempts of a retried
operation was silently dropped: on the query path the next attempt overwrote the
registry entry with a fresh un-cancelled request, and on both insert paths the
entry was unregistered after every attempt, so the cancel was a complete no-op.
The retry guard then saw an un-cancelled (or missing) request and the operation
retried and could succeed.
Cancellation is now tracked per operation: the registry holds an OngoingOperation
with a sticky cancelled flag, registration lives for the whole operation on all
three retry loops, a request attached to an already cancelled operation is
cancelled right away, and every retry iteration re-checks the flag before issuing
another request. A cancelled operation now fails with the same
TransportException("Request was cancelled on client side") that an in-flight
cancellation produces.
Fixes: #2989
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
…, not when its first request is created
With useAsyncRequests(true) the operation body runs on the shared executor, so an
operation registered itself in the cancellation registry only once the executor
picked it up. A cancelTransportRequest(queryId) issued right after the call
returned found no entry, was silently dropped, and the operation then ran to
completion - the front edge of the same window that was closed between attempts.
Operations are now registered on the calling thread, before being submitted, and
the cancellation guard runs before every attempt (including the first), so a
cancelled operation fails with TransportException("Request was cancelled on
client side") without sending a request. A registration is dropped again when the
submission is rejected, and the registry is cleared on close() because an
operation still queued when the executor is shut down never runs.
Fixes: #2989
Collaborator
Author
|
Added a follow-up commit closing the front edge of the same cancellation window. With CompletableFuture<QueryResponse> f = client.query(sql, settingsWithQueryId);
client.cancelTransportRequest(queryId); // could land before the supplier registeredChanges in that commit:
Test:
|
…ry and insert retry loops The retry decision after a failed attempt was written three times, once per retried operation (POJO insert, stream insert, query), so the cancellation fix of this PR had to touch all three copies - which SonarCloud reported as duplication on new code. The decision is now taken by one private helper: it rethrows the wrapped failure when no further request may be issued (a non-retryable failure, or an operation cancelled on the client side) and otherwise returns the endpoint of the next attempt. Behaviour is unchanged: the same exception instance is rethrown, and on the last attempt the node is still rotated without changing the endpoint. To keep the helper's parameter list short, an operation now carries the query id and the settings its attempts run with, and is therefore always created - it is only put into the cancellation registry when it has a query id to be addressed by.
Collaborator
Author
|
Pushed The duplicated lines were the retry decision after a failed attempt, which existed three times in
Behaviour is unchanged — same exception instance rethrown, and on the last attempt the node is still rotated without changing the endpoint (as before, where the returned endpoint was discarded). To keep the helper's parameter list short, an operation now carries its query id and the settings its attempts run with; it is always created and only put into the cancellation registry when it has a query id to be addressed by. Verified in a devbox against a live 25.8 server: |
chernser
reviewed
Aug 1, 2026
chernser
requested changes
Aug 1, 2026
Addresses review feedback on #2990: the registry stays a ConcurrentHashMap<String, TransportRequest> and the OngoingOperation holder is removed. Instead the request of an attempt is unregistered in the operation scoped outer try/finally rather than per attempt, so it is still reachable in the window between two attempts, and the cancellation is checked at the top of every retry iteration. The cancellation that lands before the first request of an asynchronous operation was fixed by a second commit on this PR; that fix was built on the holder and is reverted here, to be raised separately.
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
… one place
The identical guard at the top of the POJO insert, stream insert and query
retry loops is replaced by a single failIfCancelled(queryId, lastException)
helper, in the same spirit as logRetryAndSelectNextNode(...). Behaviour is
unchanged: the operation still fails with
TransportException("Request was cancelled on client side") keeping the
failure of the last attempt as cause.
Collaborator
Author
|
Pushed Sonar reported 7 new lines + 6 new conditions to cover in So instead of adding two racy tests, the three copies are now one helper, Verified locally against ClickHouse 25.8: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The registry is keyed by query id, so before the first attempt of an operation a registered request can only belong to another operation that is still running: its cancellation must not stop the operation that is starting.
Member
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 43cdc47. Configure here.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets a client-v2 correctness bug where Client.cancelTransportRequest(queryId) could be lost if it arrived between retry attempts, allowing the next attempt to be issued and the operation to complete successfully despite the caller cancellation.
Changes:
- Adjusts retry loops in
Clientto check for cancellation before issuing a retry attempt, and keeps the last attempt’s request registered until the operation ends. - Adds integration tests covering cancellation issued deterministically between attempts via
DataStreamWriter#onRetry(), plus a regression test for query-id reuse behavior. - Updates the cancellation behavior contract in
docs/features.mdand documents the fix inCHANGELOG.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
client-v2/src/main/java/com/clickhouse/client/api/Client.java |
Adds pre-retry cancellation guard and changes request registration lifetime for retries. |
client-v2/src/test/java/com/clickhouse/client/api/transport/TransportBaseTests.java |
Adds integration coverage for cancellation between retry attempts and query-id reuse scenarios. |
docs/features.md |
Updates documented semantics of client-side cancellation during retries. |
CHANGELOG.md |
Adds a bug-fix entry for issue #2989. |
…between-retry-attempts
|
chernser
approved these changes
Aug 18, 2026
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2989.
Cancellation state lives on the per-attempt
TransportRequest(ConcurrentHashMap<String, TransportRequest> ongoingRequests, keyed by query id), while a retried operation creates a new request per attempt. AClient.cancelTransportRequest(queryId)that landed in the window between two attempts was therefore silently dropped: on both insert paths the entry was unregistered at the end of every attempt, socancelTransportRequestwas a complete no-op (getreturnednull); on the query path the entry held the already-completed request of the previous attempt and nothing re-checked it before the next attempt was created. Either way the operation issued another request and could complete successfully — the caller's cancellation was lost. On the stream-insert path that window is reachable deterministically through public API, because the client callsDataStreamWriter#onRetry()exactly there.The fix keeps the registration alive for the whole operation and re-checks it before every retry: the request of an attempt stays registered until the operation is over (the per-attempt
unregisterTransportReqon the two insert paths is replaced by a single outerfinally, which the query path already had), and each retry iteration starts withfailIfCancelled(queryId, i, lastException), which throwsTransportException("Request was cancelled on client side")— the same type and message an in-flight cancellation produces, with the last attempt's failure kept as cause — instead of creating the next request. The check runs only fori > 0: before the first attempt this operation has nothing registered, so an entry found under the same query id can only belong to a different operation and must not stop this one.Changes
client-v2/.../api/Client.javatry/finallyand the stream-insert per-attemptunregisterTransportReqis dropped, so the attempt's request stays registered for the whole operation on all three paths (the query path already did this).failIfCancelled(String queryId, int attempt, RuntimeException lastException), called at the top of each iteration of the three retry loops: forattempt > 0, if the registered request was cancelled it throws the cancellationTransportExceptioninstead of issuing another request.cancelTransportRequestjavadoc states that a cancellation landing between two attempts is effective.docs/features.md: the client-side cancellation entry now states that a retried operation stops instead of issuing another request when the cancellation lands between two attempts.CHANGELOG.md: bug-fix entry with the issue link.Test
client-v2/.../api/transport/TransportBaseTests.java, next to the existing cancel/retry tests and reusing their WireMock helpers:testCancelBetweenRetryAttempts(TestNG@DataProvider, two rows). The mocked server returns a retryable503(X-ClickHouse-Exception-Code: 202) on the first request and200afterwards; the cancel is issued fromDataStreamWriter#onRetry(), i.e. deterministically between the two attempts.cancelled: the operation must fail as cancelled (TransportException) and the mock must observe exactly one request. Onmainthe insert completes successfully and the mock records two requests, so the test fails there for the right reason. The same query id is then reused by a second insert that must succeed, pinning that the cancellation does not outlive the operation.other-query-id(contrast): cancelling an unrelated query id must leave the operation alone — it recovers on the retry with exactly two requests, so the new guard does not blanket-stop retries.testFirstAttemptNotStoppedByAnotherCancelledOperation: holds a first stream insert in flight (its request stays registered), cancels that query id, then starts a second operation reusing the same query id — the second operation must still reach the server. This pins theattempt > 0condition of the guard: without it, a foreign cancelled entry under the same query id aborts a brand-new operation.Runs (devbox, ClickHouse 25.8):
TransportBaseTestsgreen;client-v2unit tests 529/529 green;InsertTests+QueryTests126/126 green. No existing test modified.Known residual windows (not closed here)
These are properties of the query-id-keyed
ongoingRequestsregistry, unchanged by this PR; they are listed for reviewers rather than hidden:registerTransportReq— a concurrent cancel can mark the previous attempt's request in the few microsecondshttpClientHelper.createRequest(...)takes, and the subsequentputreplaces it, so that attempt goes out. It is not reachable from any caller hook (DataStreamWriter#onRetry()runs at the end of the previous iteration, strictly before the guard), so no non-flaky regression test can pin it.registerTransportReqoverwrites andunregisterTransportReqremoves by key. This is pre-existing onmain(ongoingRequests.remove(queryId)there too); this PR only lengthens the insert-path window from one attempt to the whole operation.Closing 2 and 3 properly needs the same change: an identity-checked registry (each loop keeps its own previous request, swap/remove with
remove(key, value)/compute, atomic againstcancelTransportRequest), or an operation-level holder with a stickycancelledflag. That is a registry redesign touching all three call sites with branches no deterministic test can cover, and it goes against the simplification direction asked for in review, so it is deliberately left out of this fix — happy to do it here or as a follow-up if maintainers prefer.docs/changes_checklist.mdtestRetriesAndSucceedsAfterRetryableServerErrorfor all three paths). No defaults, config keys or per-request overrides change. Retry logging is untouched: the stop happens before the next attempt, so no extra retry WARN is emitted. Documented behavior is updated indocs/features.md.TransportException,isRetryable = false), so cancellation stays classified as non-retryable. Paths that are not cancelled keep throwing exactly what they threw before.i > 0(a retry), so the first attempt is unchanged; anullquery id makesrequestIsNotCancelled(null)returntrue, so the guard is a no-op exactly as before.lastExceptionis always assigned before an iteration can continue, so it is nevernullat the guard.Pre-PR validation gate
main, passes with the fix, same command)mainAGENTS.md/docs/changes_checklist.md/docs/features.mdCHANGELOG.mdupdatedClient#insertentry point end-to-end)