Skip to content

Commit a0ea369

Browse files
drempapiselasticsearchmachine
drempapis
and
elasticsearchmachine
authored
[9.2] Fix circuit breaker leak in percolator query construction (elastic#144827) (elastic#144999)
* Fix circuit breaker leak in percolator query construction (elastic#144827) (cherry picked from commit b36fdbc) # Conflicts: # modules/percolator/src/test/java/org/elasticsearch/percolator/QueryBuilderStoreTests.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/ExpressionQueryList.java * [CI] Auto commit changes from spotless * Update ExpressionQueryList.java * Delete x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/enrich/ExpressionQueryListCircuitBreakerTests.java * update code * update * update * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>
1 parent 5175770 commit a0ea369

File tree

6 files changed

+527
-2
lines changed

6 files changed

+527
-2
lines changed

‎docs/changelog/144827.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
area: Search
2+
issues: []
3+
pr: 144827
4+
summary: Fix circuit breaker leak in percolator query construction
5+
type: bug

‎modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -674,6 +674,21 @@ public <IFD extends IndexFieldData<?>> IFD getForField(
674674
public void addNamedQuery(String name, Query query) {
675675
delegate.addNamedQuery(name, query);
676676
}
677+
678+
@Override
679+
public void addCircuitBreakerMemory(long bytes, String label) {
680+
delegate.addCircuitBreakerMemory(bytes, label);
681+
}
682+
683+
@Override
684+
public long getQueryConstructionMemoryUsed() {
685+
return delegate.getQueryConstructionMemoryUsed();
686+
}
687+
688+
@Override
689+
public void releaseQueryConstructionMemory() {
690+
delegate.releaseQueryConstructionMemory();
691+
}
677692
};
678693
}
679694

‎modules/percolator/src/test/java/org/elasticsearch/percolator/QueryBuilderStoreTests.java‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,28 +19,45 @@
1919
import org.apache.lucene.search.TermQuery;
2020
import org.apache.lucene.store.Directory;
2121
import org.elasticsearch.TransportVersion;
22+
import org.elasticsearch.cluster.metadata.IndexMetadata;
23+
import org.elasticsearch.common.breaker.CircuitBreaker;
2224
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
2325
import org.elasticsearch.common.settings.Settings;
26+
import org.elasticsearch.common.unit.ByteSizeValue;
2427
import org.elasticsearch.core.CheckedFunction;
28+
import org.elasticsearch.index.IndexMode;
29+
import org.elasticsearch.index.IndexSettings;
2530
import org.elasticsearch.index.IndexVersion;
31+
import org.elasticsearch.index.fielddata.FieldDataContext;
32+
import org.elasticsearch.index.fielddata.IndexFieldData;
2633
import org.elasticsearch.index.fielddata.plain.BytesBinaryIndexFieldData;
2734
import org.elasticsearch.index.mapper.BinaryFieldMapper;
2835
import org.elasticsearch.index.mapper.DocumentParserContext;
2936
import org.elasticsearch.index.mapper.KeywordFieldMapper;
3037
import org.elasticsearch.index.mapper.MappedFieldType;
3138
import org.elasticsearch.index.mapper.MapperBuilderContext;
39+
import org.elasticsearch.index.mapper.MapperMetrics;
40+
import org.elasticsearch.index.mapper.Mapping;
41+
import org.elasticsearch.index.mapper.MappingLookup;
3242
import org.elasticsearch.index.mapper.TestDocumentParserContext;
43+
import org.elasticsearch.index.query.QueryBuilder;
44+
import org.elasticsearch.index.query.RegexpQueryBuilder;
3345
import org.elasticsearch.index.query.SearchExecutionContext;
3446
import org.elasticsearch.index.query.TermQueryBuilder;
47+
import org.elasticsearch.index.query.WildcardQueryBuilder;
3548
import org.elasticsearch.search.SearchModule;
3649
import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
3750
import org.elasticsearch.test.ESTestCase;
3851
import org.elasticsearch.xcontent.NamedXContentRegistry;
52+
import org.elasticsearch.xcontent.XContentParserConfiguration;
3953
import org.mockito.Mockito;
4054

4155
import java.io.IOException;
4256
import java.util.Collections;
57+
import java.util.List;
58+
import java.util.function.BiFunction;
4359

60+
import static org.hamcrest.Matchers.greaterThan;
4461
import static org.mockito.Mockito.mock;
4562
import static org.mockito.Mockito.when;
4663

@@ -108,4 +125,105 @@ public void testStoringQueryBuilders() throws IOException {
108125
}
109126
}
110127
}
128+
129+
public void testCircuitBreakerReleasedAfterPerDocumentQueryConstruction() throws IOException {
130+
CircuitBreaker circuitBreaker = newLimitedBreaker(ByteSizeValue.ofMb(100));
131+
132+
String fieldName = "keyword_field";
133+
QueryBuilder[] queryBuilders = new QueryBuilder[] {
134+
new WildcardQueryBuilder(fieldName, "test*pattern*with*wildcards"),
135+
new RegexpQueryBuilder(fieldName, ".*test.*regexp.*pattern.*"),
136+
new WildcardQueryBuilder(fieldName, "another*wildcard*query"),
137+
new RegexpQueryBuilder(fieldName, "prefix[0-9]+suffix"), };
138+
139+
try (Directory directory = newDirectory()) {
140+
IndexWriterConfig config = new IndexWriterConfig(new WhitespaceAnalyzer());
141+
config.setMergePolicy(NoMergePolicy.INSTANCE);
142+
BinaryFieldMapper fieldMapper = PercolatorFieldMapper.Builder.createQueryBuilderFieldBuilder(
143+
MapperBuilderContext.root(false, false)
144+
);
145+
146+
IndexVersion indexVersion = IndexVersion.current();
147+
try (IndexWriter indexWriter = new IndexWriter(directory, config)) {
148+
for (QueryBuilder queryBuilder : queryBuilders) {
149+
DocumentParserContext documentParserContext = new TestDocumentParserContext();
150+
PercolatorFieldMapper.createQueryBuilderField(
151+
indexVersion,
152+
TransportVersion.current(),
153+
fieldMapper,
154+
queryBuilder,
155+
documentParserContext
156+
);
157+
indexWriter.addDocument(documentParserContext.doc());
158+
}
159+
}
160+
161+
NamedWriteableRegistry writeableRegistry = writableRegistry();
162+
XContentParserConfiguration parserConfig = parserConfig();
163+
Settings indexSettingsSettings = indexSettings(indexVersion, 1, 1).build();
164+
IndexSettings indexSettings = new IndexSettings(
165+
IndexMetadata.builder("test").settings(indexSettingsSettings).build(),
166+
Settings.EMPTY
167+
);
168+
169+
KeywordFieldMapper keywordMapper = new KeywordFieldMapper.Builder(fieldName, indexSettings.getIndexVersionCreated()).build(
170+
MapperBuilderContext.root(false, false)
171+
);
172+
MappingLookup mappingLookup = MappingLookup.fromMappers(
173+
Mapping.EMPTY,
174+
List.of(keywordMapper),
175+
Collections.emptyList(),
176+
IndexMode.STANDARD
177+
);
178+
179+
BytesBinaryIndexFieldData fieldData = new BytesBinaryIndexFieldData(fieldMapper.fullPath(), CoreValuesSourceType.KEYWORD);
180+
BiFunction<MappedFieldType, FieldDataContext, IndexFieldData<?>> indexFieldDataLookup = (mft, fdc) -> fieldData;
181+
182+
SearchExecutionContext baseContext = new SearchExecutionContext(
183+
0,
184+
0,
185+
indexSettings,
186+
null,
187+
indexFieldDataLookup,
188+
null,
189+
mappingLookup,
190+
null,
191+
null,
192+
parserConfig,
193+
writeableRegistry,
194+
null,
195+
null,
196+
System::currentTimeMillis,
197+
null,
198+
null,
199+
() -> true,
200+
null,
201+
Collections.emptyMap(),
202+
null,
203+
MapperMetrics.NOOP
204+
);
205+
SearchExecutionContext searchExecutionContext = new SearchExecutionContext(baseContext, circuitBreaker);
206+
207+
PercolateQuery.QueryStore queryStore = PercolateQueryBuilder.createStore(fieldMapper.fieldType(), searchExecutionContext);
208+
209+
try (IndexReader indexReader = DirectoryReader.open(directory)) {
210+
LeafReaderContext leafContext = indexReader.leaves().get(0);
211+
CheckedFunction<Integer, Query, IOException> queries = queryStore.getQueries(leafContext);
212+
assertEquals(queryBuilders.length, leafContext.reader().numDocs());
213+
214+
long baselineUsed = circuitBreaker.getUsed();
215+
for (int i = 0; i < queryBuilders.length; i++) {
216+
queries.apply(i);
217+
assertThat(
218+
"CB bytes should still be tracked (not leaked) after document " + i,
219+
circuitBreaker.getUsed(),
220+
greaterThan(baselineUsed)
221+
);
222+
}
223+
224+
searchExecutionContext.releaseQueryConstructionMemory();
225+
assertEquals("All CB bytes must be released after the request-end release", baselineUsed, circuitBreaker.getUsed());
226+
}
227+
}
228+
}
111229
}

‎server/src/main/java/org/elasticsearch/search/suggest/phrase/PhraseSuggester.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,8 +135,12 @@ public Suggestion<? extends Entry<? extends Option>> innerExecute(
135135
.createParser(searchExecutionContext.getParserConfig(), querySource)
136136
) {
137137
QueryBuilder innerQueryBuilder = AbstractQueryBuilder.parseTopLevelQuery(parser);
138-
final ParsedQuery parsedQuery = searchExecutionContext.toQuery(innerQueryBuilder);
139-
collateMatch = Lucene.exists(searcher, parsedQuery.query());
138+
try {
139+
final ParsedQuery parsedQuery = searchExecutionContext.toQuery(innerQueryBuilder);
140+
collateMatch = Lucene.exists(searcher, parsedQuery.query());
141+
} finally {
142+
searchExecutionContext.releaseQueryConstructionMemory();
143+
}
140144
}
141145
}
142146
if (collateMatch == false && collatePrune == false) {

‎server/src/test/java/org/elasticsearch/index/query/InnerHitContextBuilderCircuitBreakerTests.java‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the "Elastic License
4+
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
5+
* Public License v 1"; you may not use this file except in compliance with, at
6+
* your election, the "Elastic License 2.0", the "GNU Affero General Public
7+
* License v3.0 only", or the "Server Side Public License, v 1".
8+
*/
9+
package org.elasticsearch.index.query;
10+
11+
import org.apache.lucene.analysis.core.WhitespaceAnalyzer;
12+
import org.apache.lucene.index.DirectoryReader;
13+
import org.apache.lucene.index.IndexWriter;
14+
import org.apache.lucene.index.IndexWriterConfig;
15+
import org.apache.lucene.search.IndexSearcher;
16+
import org.apache.lucene.store.ByteBuffersDirectory;
17+
import org.apache.lucene.store.Directory;
18+
import org.elasticsearch.cluster.metadata.IndexMetadata;
19+
import org.elasticsearch.common.breaker.CircuitBreaker;
20+
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
21+
import org.elasticsearch.common.settings.Settings;
22+
import org.elasticsearch.common.unit.ByteSizeValue;
23+
import org.elasticsearch.index.IndexMode;
24+
import org.elasticsearch.index.IndexSettings;
25+
import org.elasticsearch.index.IndexVersion;
26+
import org.elasticsearch.index.mapper.KeywordFieldMapper;
27+
import org.elasticsearch.index.mapper.MapperBuilderContext;
28+
import org.elasticsearch.index.mapper.MapperMetrics;
29+
import org.elasticsearch.index.mapper.Mapping;
30+
import org.elasticsearch.index.mapper.MappingLookup;
31+
import org.elasticsearch.search.SearchModule;
32+
import org.elasticsearch.search.fetch.subphase.InnerHitsContext;
33+
import org.elasticsearch.search.internal.SearchContext;
34+
import org.elasticsearch.test.ESTestCase;
35+
import org.elasticsearch.xcontent.NamedXContentRegistry;
36+
37+
import java.io.IOException;
38+
import java.util.Collections;
39+
import java.util.List;
40+
41+
import static org.hamcrest.Matchers.equalTo;
42+
import static org.hamcrest.Matchers.greaterThan;
43+
44+
public class InnerHitContextBuilderCircuitBreakerTests extends ESTestCase {
45+
46+
@Override
47+
protected NamedWriteableRegistry writableRegistry() {
48+
SearchModule searchModule = new SearchModule(Settings.EMPTY, Collections.emptyList());
49+
return new NamedWriteableRegistry(searchModule.getNamedWriteables());
50+
}
51+
52+
@Override
53+
protected NamedXContentRegistry xContentRegistry() {
54+
SearchModule searchModule = new SearchModule(Settings.EMPTY, Collections.emptyList());
55+
return new NamedXContentRegistry(searchModule.getNamedXContents());
56+
}
57+
58+
public void testCBTrackedDuringInnerHitsAndReleasedAtRequestEnd() throws IOException {
59+
Directory dir = new ByteBuffersDirectory();
60+
// Empty index – we just need a valid IndexSearcher for the context.
61+
try (IndexWriter writer = new IndexWriter(dir, new IndexWriterConfig(new WhitespaceAnalyzer()))) {
62+
// intentionally empty
63+
}
64+
65+
try (DirectoryReader reader = DirectoryReader.open(dir)) {
66+
IndexSearcher searcher = new IndexSearcher(reader);
67+
68+
IndexVersion indexVersion = IndexVersion.current();
69+
Settings indexSettingsSettings = indexSettings(indexVersion, 1, 1).build();
70+
IndexSettings indexSettings = new IndexSettings(
71+
IndexMetadata.builder("test").settings(indexSettingsSettings).build(),
72+
Settings.EMPTY
73+
);
74+
KeywordFieldMapper fieldMapper = new KeywordFieldMapper.Builder("field", indexSettings.getIndexVersionCreated()).build(
75+
MapperBuilderContext.root(false, false)
76+
);
77+
MappingLookup mappingLookup = MappingLookup.fromMappers(
78+
Mapping.EMPTY,
79+
List.of(fieldMapper),
80+
Collections.emptyList(),
81+
IndexMode.STANDARD
82+
);
83+
84+
SearchExecutionContext baseCtx = new SearchExecutionContext(
85+
0,
86+
0,
87+
indexSettings,
88+
null,
89+
null,
90+
null,
91+
mappingLookup,
92+
null,
93+
null,
94+
parserConfig(),
95+
writableRegistry(),
96+
null,
97+
searcher,
98+
System::currentTimeMillis,
99+
null,
100+
null,
101+
() -> true,
102+
null,
103+
Collections.emptyMap(),
104+
null,
105+
MapperMetrics.NOOP
106+
);
107+
108+
CircuitBreaker cb = newLimitedBreaker(ByteSizeValue.ofMb(100));
109+
SearchExecutionContext ctx = new SearchExecutionContext(baseCtx, cb);
110+
QueryBuilder innerQuery = new WildcardQueryBuilder("field", "*test*pattern*");
111+
InnerHitBuilder innerHitBuilder = new InnerHitBuilder("test_inner");
112+
InnerHitContextBuilder builder = new InnerHitContextBuilder(innerQuery, innerHitBuilder, Collections.emptyMap()) {
113+
@Override
114+
protected void doBuild(SearchContext parentSearchContext, InnerHitsContext innerHitsContext) {}
115+
};
116+
117+
InnerHitsContext.InnerHitSubContext subContext = org.mockito.Mockito.mock(InnerHitsContext.InnerHitSubContext.class);
118+
119+
long baselineUsed = cb.getUsed();
120+
121+
int iterations = 5;
122+
for (int i = 0; i < iterations; i++) {
123+
builder.setupInnerHitsContext(ctx, subContext);
124+
assertThat(
125+
"CB bytes must still be tracked (not released early) after iteration " + i,
126+
cb.getUsed(),
127+
greaterThan(baselineUsed)
128+
);
129+
}
130+
131+
ctx.releaseQueryConstructionMemory();
132+
assertThat("All CB bytes must be released after the request-end release", cb.getUsed(), equalTo(baselineUsed));
133+
}
134+
}
135+
}

0 commit comments

Comments
 (0)