Fix cancellation race in CancellableRateLimitedFluxIterator by nicktindall · Pull Request #141974 · elastic/elasticsearch

nicktindall · 2026-02-06T06:13:54Z

This PR ensures that if the consumer and the producer update the done state independently:

Errors from the producer take priority over errors from the consumer
Errors always take priority over completions

elasticsearchmachine · 2026-02-06T06:14:20Z

Pinging @elastic/es-distributed (Team:Distributed)

elasticsearchmachine · 2026-02-06T06:14:43Z

Hi @nicktindall, I've created a changelog YAML for you.

Copilot

Pull request overview

This PR fixes a race condition in CancellableRateLimitedFluxIterator where concurrent cancellation and error events could lead to incorrect error propagation. The fix ensures that producer errors take precedence over consumer cancellation errors.

Changes:

Modified CancellableRateLimitedFluxIterator to track error source (producer vs consumer) and ensure producer errors are preserved even after cancellation
Added test coverage for cancellation after error and completion after cancellation scenarios
Updated existing test to verify CancellationException is properly wrapped

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`modules/repository-azure/src/main/java/org/elasticsearch/repositories/azure/CancellableRateLimitedFluxIterator.java`	Changed doneState from volatile to AtomicReference, added fromProducer tracking to DoneState record, implemented updateDoneState method to atomically manage state transitions with priority for producer errors
`modules/repository-azure/src/test/java/org/elasticsearch/repositories/azure/CancellableRateLimitedFluxIteratorTests.java`	Enhanced existing cancellation test to verify CancellationException wrapping, added testCancellationAfterError and testCompleteAfterCancellation to verify race condition fixes
`docs/changelog/141974.yaml`	Added changelog entry documenting the bug fix

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…itories/azure/CancellableRateLimitedFluxIteratorTests.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

ywangd · 2026-02-09T00:58:33Z

+            // If the existing error is not from the producer, allow it to be overwritten by one from the producer
+            if (existing.error() != null && existing.fromProducer() == false && newState.isErrorFromProducer()) {
+                return newState;
+            }


Why do we want to override the error if it is from the producer? Do we depend on this behaviour anywhere? I wonder whether it is necessary. Or maybe we can add the new error as suppressed?

No, we only override the error if it's not from the producer (i.e. it's the cancellation error or the IllegalStateException). There are tests that ensure a producer error is apparent if it occurs after a cancellation, so this should be consistent with the existing behaviour.

(see org.elasticsearch.repositories.azure.CancellableRateLimitedFluxIteratorTests#testErrorAfterCancellation)

I think the producer error would have more information in it, so we should prefer it if both occur. The cancellation and illegal state exception are merely to prevent the consumer from thinking it reached the end of the stream without error.

My original comment was unclear. I did mean why a producer error should override a consumer one. I am OK with override. I wonder whether there is value in adding the one being overriden as suppressed. I might be overthinking if it has no use case.

Yeah I think the client initiated errors aren't that interesting, they're kind-of synthetic and are really just to mark the iterator as failed.

ywangd

LGTM

ywangd · 2026-02-09T03:20:07Z


-        if (doneState.done() && doneState.error() != null) {
+        final var currentDoneState = doneState.get();
+        if (currentDoneState.done() && currentDoneState.error() != null) {


Can we encapsulate these two conditions with a method on DoneState?

Done in 73d8181

ywangd · 2026-02-09T03:20:46Z

+            }
+
+            // If the existing error is not from the producer, allow it to be overwritten by one from the producer
+            if (existing.error() != null && existing.fromProducer() == false && newState.isErrorFromProducer()) {


Similarly can we extract a method on DoneState for existing.error() != null && existing.fromProducer() == false?

Also in 73d8181

…roducer_errors

…141974) (cherry picked from commit 1948d32)

nicktindall · 2026-03-10T22:10:10Z

💚 All backports created successfully

Status	Branch	Result
✅	9.3
✅	9.2

Questions ?

Please refer to the Backport tool documentation

…141974) (cherry picked from commit 1948d32)

…#143977) (cherry picked from commit 1948d32)

…#143976) (cherry picked from commit 1948d32)

Allow consumer errors to be overwritten by producer errors

809fa23

nicktindall added >bug :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Feb 6, 2026

elasticsearchmachine added Team:Distributed Meta label for distributed team. v9.4.0 labels Feb 6, 2026

Update docs/changelog/141974.yaml

d822c28

nicktindall requested a review from Copilot February 6, 2026 06:14

Copilot started reviewing on behalf of nicktindall February 6, 2026 06:15 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

Comment thread .../test/java/org/elasticsearch/repositories/azure/CancellableRateLimitedFluxIteratorTests.java Outdated

Comment thread .../test/java/org/elasticsearch/repositories/azure/CancellableRateLimitedFluxIteratorTests.java Outdated

nicktindall added 3 commits February 6, 2026 17:28

Errors always overwrite non-errors

ea8bafa

Tidy

329b086

Tidy

fd510bb

nicktindall requested a review from Copilot February 6, 2026 06:44

Copilot started reviewing on behalf of nicktindall February 6, 2026 06:44 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

Comment thread .../test/java/org/elasticsearch/repositories/azure/CancellableRateLimitedFluxIteratorTests.java Outdated

Update modules/repository-azure/src/test/java/org/elasticsearch/repos…

90b619c

…itories/azure/CancellableRateLimitedFluxIteratorTests.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

nicktindall requested a review from ywangd February 6, 2026 06:57

ywangd reviewed Feb 9, 2026

View reviewed changes

ywangd approved these changes Feb 9, 2026

View reviewed changes

nicktindall added 4 commits February 9, 2026 14:54

Encapsulate done state checks

73d8181

Merge branch 'main' into allow_consumer_errors_to_be_overwritten_by_p…

f126014

…roducer_errors

Fix randomisation

d5fe0af

Fix randomisation (better)

3ab96f5

nicktindall merged commit 1948d32 into elastic:main Feb 9, 2026
34 of 35 checks passed

nicktindall deleted the allow_consumer_errors_to_be_overwritten_by_producer_errors branch February 9, 2026 09:40

This was referenced Mar 9, 2026

[9.3] Fix CancellableRateLimitedFluxIteratorTests, fix potential race condition (#141896) #143913

Merged

[9.2] Fix CancellableRateLimitedFluxIteratorTests, fix potential race condition (#141896) #143914

Merged

lkts mentioned this pull request Mar 10, 2026

[CI] CancellableRateLimitedFluxIteratorTests testErrorPath failing #143879

Closed

nicktindall added a commit to nicktindall/elasticsearch that referenced this pull request Mar 10, 2026

Fix cancellation race in CancellableRateLimitedFluxIterator (elastic#…

d44dfba

…141974) (cherry picked from commit 1948d32)

This was referenced Mar 10, 2026

[9.3] Fix cancellation race in CancellableRateLimitedFluxIterator (#141974) #143976

Merged

[9.2] Fix cancellation race in CancellableRateLimitedFluxIterator (#141974) #143977

Merged

nicktindall added a commit to nicktindall/elasticsearch that referenced this pull request Mar 10, 2026

Fix cancellation race in CancellableRateLimitedFluxIterator (elastic#…

71d30ce

…141974) (cherry picked from commit 1948d32)

elasticsearchmachine pushed a commit that referenced this pull request Mar 10, 2026

Fix cancellation race in CancellableRateLimitedFluxIterator (#141974) (…

0dc66cc

…#143977) (cherry picked from commit 1948d32)

elasticsearchmachine pushed a commit that referenced this pull request Mar 11, 2026

Fix cancellation race in CancellableRateLimitedFluxIterator (#141974) (…

5a6bad2

…#143976) (cherry picked from commit 1948d32)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cancellation race in CancellableRateLimitedFluxIterator#141974

Fix cancellation race in CancellableRateLimitedFluxIterator#141974
nicktindall merged 10 commits intoelastic:mainfrom
nicktindall:allow_consumer_errors_to_be_overwritten_by_producer_errors

nicktindall commented Feb 6, 2026 •

edited

Loading

elasticsearchmachine commented Feb 6, 2026

elasticsearchmachine commented Feb 6, 2026

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

ywangd Feb 9, 2026

nicktindall Feb 9, 2026 •

edited

Loading

nicktindall Feb 9, 2026

ywangd Feb 9, 2026

nicktindall Feb 9, 2026

ywangd left a comment

ywangd Feb 9, 2026

nicktindall Feb 9, 2026

ywangd Feb 9, 2026

nicktindall Feb 9, 2026

Uh oh!

nicktindall commented Mar 10, 2026

Labels

4 participants

Conversation

nicktindall commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Feb 6, 2026

elasticsearchmachine commented Feb 6, 2026

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

nicktindall Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

nicktindall commented Mar 10, 2026

💚 All backports created successfully

Questions ?

Labels

4 participants

nicktindall commented Feb 6, 2026 •

edited

Loading

nicktindall Feb 9, 2026 •

edited

Loading