Delay circuit breaker release until fetch response is sent by drempapis · Pull Request #139243 · elastic/elasticsearch

drempapis · 2025-12-09T13:55:28Z

Circuit breaker bytes were being released too early in the fetch phase. The bytes were released immediately after building SearchHits in FetchPhase.buildSearchHits(), but before the response was serialized and sent to the coordinator node. This created a timing window where:

`SearchHits are still in memory, consuming resources
Circuit breaker has already released the bytes, believing the memory is freed
This leads to circuit breaker undercounting, potentially allowing OOM conditions, especially for small setups with low heap resources.

This PR fixes the timing issue by delaying circuit breaker release until after the response has been successfully sent to the coordinator. We achieve this by:

Storing circuit breaker bytes in FetchSearchResult when hits are built
Releasing after transmission via listener wrappers that free bytes after listener.onResponse() completes
Handling exceptions safely by releasing immediately if an error occurs before the result is created

elasticsearchmachine · 2025-12-09T13:57:40Z

Pinging @elastic/es-search-foundations (Team:Search Foundations)

drempapis · 2025-12-09T16:08:39Z

@elasticmachine run elasticsearch-ci/part-1

drempapis · 2025-12-10T08:21:03Z

@elasticmachine run elasticsearch-ci/part-2

drempapis · 2025-12-15T15:52:23Z

@elasticmachine run elasticsearch-ci/part-1

andreidan

Thanks for working on this Dimi (and wow, sorry for the late review 🙏 )

I had an initial look over this and left a few comments.

Did you test this locally with a running Elasticsearch? (i.e. issue some requests without the fix and see Elasticsearch OOM, then the same with the fix in place and see Elasticsearch 429 instead?)

andreidan · 2026-01-13T15:51:44Z

.../src/internalClusterTest/java/org/elasticsearch/search/fetch/FetchPhaseCircuitBreakerIT.java

+    private void createSmallIndex() throws IOException {
+        createIndex(INDEX_SMALL);
+        populateIndex(INDEX_SMALL, 50, 10_000);
+    }
+
+    private void createLargeIndex() throws IOException {
+        createIndex(INDEX_LARGE);
+        populateIndex(INDEX_LARGE, 50, 100_000);
+    }
+
+    private void createIndex(String indexName) {
+        assertAcked(
+            prepareCreate(indexName).setMapping(
+                SORT_FIELD,
+                "type=long",
+                "text",
+                "type=text,store=true",
+                "large_text_1",
+                "type=text,store=false",
+                "large_text_2",
+                "type=text,store=false",
+                "large_text_3",
+                "type=text,store=false",
+                "keyword",
+                "type=keyword"
+            )
+        );
+    }
+
+    private void populateIndex(String indexName, int nDocs, int textSize) throws IOException {


I think we usually have private methods at the bottom to enhance readability

andreidan · 2026-01-13T15:53:03Z

.../src/internalClusterTest/java/org/elasticsearch/search/fetch/FetchPhaseCircuitBreakerIT.java

+        ensureSearchable(INDEX_SMALL);
+    }
+
+    private void createSmallIndex() throws IOException {


nit: I think this rather shallow method hinders readability (and it's only used once)
Would it be easier to just call the code directly?

Correct, updated

andreidan · 2026-01-13T15:55:46Z

.../src/internalClusterTest/java/org/elasticsearch/search/fetch/FetchPhaseCircuitBreakerIT.java

+        populateIndex(INDEX_SMALL, 50, 10_000);
+    }
+
+    private void createLargeIndex() throws IOException {


I think this isn't used

andreidan · 2026-01-13T15:57:45Z

.../src/internalClusterTest/java/org/elasticsearch/search/fetch/FetchPhaseCircuitBreakerIT.java

+    }
+
+    private long getRequestBreakerUsed() {
+        CircuitBreakerService breakerService = internalCluster().getInstance(CircuitBreakerService.class);


I think this gets the CircuitBreakerService from a random node in the cluster so it might create test flakiness?
Do we need to be more specific about which CircuitBreakerService are we retrieving?

That's a good point!!

I updated the code to use

@ClusterScope(scope = TEST, numDataNodes = 0, numClientNodes = 0)

to start with an empty cluster and explicitly start a data node and coordinator node per test.

So now I pass the data node name to getRequestBreakerUsed(String node) to retrieve the circuit breaker from the specific node where shards are allocated and fetch phase executes.

andreidan · 2026-01-13T16:09:31Z

server/src/main/java/org/elasticsearch/search/fetch/FetchSearchResult.java


    private SearchHits hits;
+
+    private transient long circuitBreakerBytes = 0L;


Shall we name this something that indicates what the field is? (i.e. searchHitsSizeBytes or something like that? - it's currently named based on what it is used for, not what it is)

Updated, ty!

andreidan · 2026-01-13T16:27:41Z

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

+    private static class SearchHitsWithBreakerBytes {
+        final SearchHits hits;
+        final long circuitBreakerBytes;
+
+        SearchHitsWithBreakerBytes(SearchHits hits, long circuitBreakerBytes) {
+            this.hits = hits;
+            this.circuitBreakerBytes = circuitBreakerBytes;
+        }


should we rename this to SearchHitsAndBytesSize ?

Updated, ty!

andreidan · 2026-01-13T16:29:04Z

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

                ? Profiler.NOOP
                : Profilers.startProfilingFetchPhase();
        SearchHits hits = null;
+        long circuitBreakerBytes = 0L;


Suggested change

long circuitBreakerBytes = 0L;

long searchHitsBytesSize = 0L;

That's a good suggestion, updated!

andreidan · 2026-01-13T16:29:50Z

server/src/test/java/org/elasticsearch/action/search/SearchServiceCircuitBreakerTests.java

+    /**
+     * Test the circuit breaker release helper for QueryFetchSearchResult.
+     */


nit: these comments are a bit confusing for me (what helper?) I'd prefer if we remove them and bake the logic of what we're testing in the test name

Removed the comments

andreidan · 2026-01-13T16:30:55Z

.../src/internalClusterTest/java/org/elasticsearch/search/fetch/FetchPhaseCircuitBreakerIT.java

+        assertThat(
+            "Circuit breaker should not grow after multiple searches (no leaks)",
+            getRequestBreakerUsed(),
+            lessThanOrEqualTo(initialBreaker + ByteSizeValue.ofKb(100).getBytes()) // Allow small variance


would the variance not mask a leak here?

You’re right; I removed that and added it within an assertBusy block

andreidan · 2026-01-13T16:31:30Z

.../src/internalClusterTest/java/org/elasticsearch/search/fetch/FetchPhaseCircuitBreakerIT.java

+    /**
+     * Test circuit breaker release with scroll search (scroll fetch path).
+     */


test name is descriptive enough IMO

I added the comment to stay consistent with the rest of the tests. Do you think we should remove these comments across all tests for consistency? The tests code is fairly straightforward to read.

Yes please, unless you find them useful (they currently mostly just restate what the test method names are saying)

drempapis · 2026-01-14T11:57:06Z

Did you test this locally with a running Elasticsearch? (i.e. issue some requests without the fix and see Elasticsearch OOM, then the same with the fix in place and see Elasticsearch 429 instead?)

@andreidan thank you for the review.

I tested this locally against a running Elasticsearch cluster during implementation. Before applying the request circuit breaker, I was able to reproduce the problematic behavior by issuing large fetch requests, which caused the data node to run out of memory during the fetch phase. I used a profiler to monitor heap usage while the fetch was executing and observed the OOM.

With the fix applied, the same requests fail with a CircuitBreakingException and are mapped to 429, without destabilizing the node. In this PR, I added integration tests that intentionally lower indices.breaker.request.limit and assert that large fetches now trip the request breaker and return 429s, as well as that breaker usage is properly released afterward.

andreidan

Thanks for working on this Dimi.

LGTM

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

Co-authored-by: Andrei Dan <andrei.dan@elastic.co>

drempapis added 5 commits December 9, 2025 10:57

add cb lifecycle for fetch phase

fdad427

optimize code

36bdbd3

update code for testing

066a455

add tests

4c21adc

add unit tests

70e2461

elasticsearchmachine added needs:triage Requires assignment of a team area label v9.3.0 labels Dec 9, 2025

drempapis added >non-issue Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch :Search Foundations/Search Catch all for Search Foundations and removed needs:triage Requires assignment of a team area label labels Dec 9, 2025

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

f1bbdd7

drempapis changed the title ~~Fix/fetch phase breaker lifecycle~~ Delay circuit breaker release until fetch response is sent Dec 9, 2025

[CI] Auto commit changes from spotless

d8ba8e4

drempapis mentioned this pull request Dec 9, 2025

Fix SearchContext CB memory accounting #138002

Merged

9 tasks

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

4d1f65d

drempapis added 6 commits December 10, 2025 14:21

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

54d04c7

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

955439d

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

e6df481

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

d3e2814

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

cd8264e

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

89f8979

drempapis added 2 commits December 16, 2025 09:14

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

fe581ad

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

9000571

elasticsearchmachine added v9.4.0 and removed v9.3.0 labels Dec 17, 2025

drempapis added 5 commits December 18, 2025 12:43

fix test

0bda191

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

2d2190f

fix LEAK

68a7d42

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

0fff2a8

upate after review

c3d9c98

drempapis requested review from andreidan and luigidellaquila December 18, 2025 15:17

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

c22ff5c

andreidan reviewed Jan 13, 2026

View reviewed changes

drempapis and others added 4 commits January 14, 2026 12:49

update after review

d31a5ff

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

a46a599

update

f539668

[CI] Auto commit changes from spotless

54cb4c6

drempapis added 4 commits January 14, 2026 14:13

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

9e4b919

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

456a435

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

e6ef186

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

d7e61ce

andreidan approved these changes Jan 21, 2026

View reviewed changes

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java Outdated Show resolved Hide resolved

drempapis and others added 4 commits January 26, 2026 10:08

Apply suggestion from @andreidan

f248137

Co-authored-by: Andrei Dan <andrei.dan@elastic.co>

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

fd78fc1

update after review

5313045

Merge branch 'main' into fix/fetch-phase-breaker-lifecycle

abd1022

drempapis merged commit ea94623 into elastic:main Jan 26, 2026
35 checks passed

This was referenced Mar 17, 2026

Defer circuit breaker release until transport write completes #143136

Merged

Improve Memory Accounting and Stability in FetchPhase and Multi-Search #139132

Open

drempapis mentioned this pull request Apr 1, 2026

Remove empirical x2 multiplier for source memory accounting in FetchPhase #145389

Merged


		private SearchHits hits;

		private transient long circuitBreakerBytes = 0L;

	long circuitBreakerBytes = 0L;
	long searchHitsBytesSize = 0L;

Conversation

drempapis commented Dec 9, 2025

Uh oh!

elasticsearchmachine commented Dec 9, 2025

Uh oh!

drempapis commented Dec 9, 2025

Uh oh!

drempapis commented Dec 10, 2025

Uh oh!

drempapis commented Dec 15, 2025

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drempapis commented Jan 14, 2026

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants