Fix client-v2: cancelTransportRequest lost between two retry attempts - #2990
Open
polyglotAI-bot wants to merge 6 commits into
Open
Fix client-v2: cancelTransportRequest lost between two retry attempts#2990polyglotAI-bot wants to merge 6 commits into
polyglotAI-bot wants to merge 6 commits into
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.
| return true; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Cancel lost before new attempt
Medium Severity
Cancellation for a retried operation is stored only on the current TransportRequest in ongoingRequests. After failIfCancelled passes at the start of a retry iteration, a concurrent cancelTransportRequest can mark that entry cancelled, but the next registerTransportReq replaces it with a fresh request, so the retry loop may still send another HTTP attempt despite the documented operation-wide cancel.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 43cdc47. Configure here.
Collaborator
Author
There was a problem hiding this comment.
Thanks — the window is real, but I don't think it should be closed the way it first looks, so let me lay out what I checked.
Confirmed: between failIfCancelled(queryId, i, lastException) at the top of an iteration and the registerTransportReq(queryId, ...) a few lines later there is only httpClientHelper.createRequest(...). A cancelTransportRequest(queryId) landing inside that window marks the previous attempt's request, and the subsequent put replaces it, so the new attempt goes out. That is exactly the residual I flagged in the previous round ("query-id-keyed registry, put overwrites, no sticky flag").
But it is a pure thread race, not a reachable code path. Every deterministic between-attempts hook this client exposes — notably DataStreamWriter#onRetry() — runs at the end of the previous iteration, i.e. strictly before failIfCancelled, so it is caught by the guard (that is what TransportBaseTests#testCancelBetweenRetryAttempts pins). createRequest invokes no caller code: on the stream/POJO insert paths the output-stream lambda is only run later, from executeRequest. So there is no way to reach this window other than a concurrent canceller hitting a few microseconds of request construction, and no way to write a non-flaky regression test for it.
The obvious fix is wrong. Carrying cancellation over on registration (compute(queryId, (k, prev) -> prev != null && prev.isCancelled() ? prev : tr)) reintroduces precisely the defect you raised in the last round ("First-attempt cancel guard misfires", r3694834108): an unrelated operation's cancelled request left under the same query id would abort a brand-new operation. Closing this window correctly needs the swap to be identity-checked against this operation's own previous request and made atomic against cancelTransportRequest (both sides going through ConcurrentHashMap.compute/computeIfPresent so the cancel and the swap can't interleave):
// cancel side
ongoingRequests.computeIfPresent(queryId, (k, req) -> { req.cancel(); return req; });
// register side, attempts > 0 only
ongoingRequests.compute(queryId, (k, cur) -> (cur == previousAttemptRequest && cur.isCancelled()) ? cur : tr);That is a real design change to the registry (each loop must carry its previous request; three call sites gain an abort branch), and @chernser's direction on this PR was explicitly to simplify — keep TransportRequest in the registry and just move unregistration to the outer try/finally. It also adds branches that no deterministic test can cover, which is what tripped the new-code coverage gate earlier on this PR.
So I'm deliberately not changing code here and leaving this thread open for @chernser / @mshustov to call: the PR as it stands makes cancellation effective for every deterministically reachable between-attempts case, and this remaining microsecond race is orthogonal to the bug in #2989. Happy to implement the identity-checked atomic registry above — in this PR or a follow-up — if you'd like it.
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. |
Comment on lines
+1921
to
+1924
| private void failIfCancelled(String queryId, int attempt, RuntimeException lastException) { | ||
| if (attempt > 0 && !requestIsNotCancelled(queryId)) { | ||
| throw new TransportException("Request was cancelled on client side", lastException, queryId); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Correct, and the doc mismatch is on us: the PR description still described an earlier revision of this change (the OngoingOperation holder with a sticky cancelled flag), which was removed in favour of the small failIfCancelled guard after @chernser asked to simplify. The description has been rewritten to describe the shipped design; docs/features.md and the CHANGELOG entry were already worded in terms of what the code does ("the request of an attempt stays registered until the operation is over, and the cancellation is checked before every attempt", best-effort), so no over-claim there — but please re-read the description, it was the stale part.
On the remaining race: this is the same finding @cursor[bot] raised at the guard site (r3699162510), answered there in detail. Summary: the window between failIfCancelled and registerTransportReq contains only httpClientHelper.createRequest(...) and no caller-visible hook (DataStreamWriter#onRetry() runs at the end of the previous iteration, strictly before the guard — that is what testCancelBetweenRetryAttempts pins), so it is not deterministically reachable and no non-flaky regression test can cover it.
The holder shape you propose is what this PR originally had, and it is the right end state — but note the naive carry-over version is not safe on its own: preserving a cancelled entry across registerTransportReq re-introduces the previous Bugbot finding on this PR ("First-attempt cancel guard misfires", r3694834108), where a foreign cancelled entry under the same query id aborts a brand-new operation. Doing it correctly means an identity-checked, atomic registry (compute/remove(key, value) on both the register and cancel sides), which also subsumes the aliasing point in your other comment.
That is a registry redesign with branches no deterministic test can exercise (the new-code coverage gate on this PR already went red once), so it is scoped out of this bug fix and now listed as known residuals 2 and 3 in the PR description. Leaving this thread unresolved so @chernser / @mshustov can decide whether they want it here or as a follow-up — I am happy to write it either way.
Comment on lines
+1513
to
+1516
| } finally { | ||
| // The request of the last attempt stays registered until the operation is over, so a cancellation | ||
| // landing between two attempts is not lost. | ||
| unregisterTransportReq(queryId); |
Collaborator
Author
There was a problem hiding this comment.
Thanks — the mismatch you spotted is real, and it is in the PR description, not the code: the description still described an earlier revision of this PR (an OngoingOperation holder with a sticky flag, removed with remove(key, value)), which was dropped in favour of the much smaller guard after @chernser asked to simplify. I have rewritten the description to match what is actually shipped, so it no longer promises identity-based removal.
On the substance: removing by query id only is pre-existing on main — unregisterTransportReq there is also ongoingRequests.remove(queryId), and registerTransportReq is a plain put, so two concurrent operations sharing a query id already alias each other today (the second put orphans the first entry, and either remove clears whichever entry is current). This PR does not introduce that; it only lengthens the window on the two insert paths, from one attempt to the whole operation, which is exactly what makes a between-attempts cancel visible at all.
I deliberately did not close it here:
- A correct close is not just
remove(queryId, request)in thefinally— it also needs the registration side to be identity-checked and atomic againstcancelTransportRequest(each loop carrying its own previous request,compute/computeIfPresenton both sides), i.e. a registry redesign across all three call sites. - None of its branches are reachable by a deterministic test (they require two operations racing on the same query id at microsecond granularity), so it would add uncovered new lines — the new-code coverage gate on this PR already went red once for that reason.
- It runs against the simplification direction asked for in review.
It is now listed explicitly as a known residual in the PR description (item 3) rather than left implicit. Happy to implement the identity-checked registry — here or as a separate PR — if @chernser / @mshustov want it; leaving this thread open for that call.
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)