Fix heap leak: WildcardMatchingQuery retains SearchLookup via query cache

[bowenlan-amzn]

Description

WildcardMatchingQuery stored a SearchLookup field and a valueFetcherSupplier lambda that captured QueryShardContext. When Lucene's LRUQueryCache cached these queries, the heavy per-request objects (SearchLookup, QueryShardContext, IndexSearcher, segment readers) were pinned indefinitely — ~15 MB per cached pattern.

Root cause: The lambda () -> fieldType.valueFetcher(context, lookup, null) closes over both context (QueryShardContext) and lookup (SearchLookup). Since LRUQueryCache retains the WildcardMatchingQuery object, and the query holds this lambda, both heavy objects survive GC for the lifetime of the cache entry.

Fix: Replace with fieldType.valueFetcherSupplier(context) which eagerly resolves only lightweight, index-level data (IndexFieldData for doc-values path, or Set<String> source paths) at construction time. The scorer creates a local SourceLookup per leaf, breaking the retention chain while preserving thread safety and cache correctness.

Verified with a manual cluster test (200 distinct wildcard queries, 6 reps each):

Metric	Before	After
SearchLookup instances	400	0
QueryShardContext instances	200	0
Cache entries	200	200
Cache hits	200	200

Resolves #22419

Check List

• New functionality includes testing
• New functionality has been documented
• API changes companion pull request - N/A
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 0a948ed)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Missing null check In valueFetcherSupplier, when hasDocValues() is false and normalizer() returns non-null, the captured norm is used, but when hasDocValues() is true the DocValueFetcher path is taken. However, if context is null in the public constructor path but fieldType is non-null (or vice versa), the code branches on context != null only. If a caller passes non-null context but null fieldType, fieldType.valueFetcherSupplier(context) will NPE. Previously the same risk existed via fieldType.valueFetcher(...), so this may be pre-existing, but worth confirming callers always pair them. if (context != null) { this.valueFetcherSupplier = fieldType.valueFetcherSupplier(context); } else { this.valueFetcherSupplier = null; } } SourceLookup source reset per doc SourceLookup.setSegmentAndDocument(context, docId) is now used instead of the shared LeafSearchLookup.source(). This creates a fresh SourceLookup per scorer with no shared source cache across queries on the same doc, which could increase _source parsing cost when multiple wildcard queries evaluate the same document. Functionally correct, but a potential performance regression worth measuring for queries that previously benefited from the shared per-request source cache. SourceLookup sourceLookup = new SourceLookup(); final ValueFetcher valueFetcher = valueFetcherSupplier.get(); valueFetcher.setNextReader(context); TwoPhaseIterator twoPhaseIterator = new TwoPhaseIterator(approximation) { @Override public boolean matches() throws IOException { sourceLookup.setSegmentAndDocument(context, approximation.docID()); List<?> values = valueFetcher.fetchValues(sourceLookup);

[github-actions]

Persistent review updated to latest commit ecf8c8e

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 0a948ed

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid docvalue fetcher that may format values Using a DocValueFetcher here will try to read values via SourceLookup.source() path replaced with a plain SourceLookup that has no reader/document source — but DocValueFetcher.fetchValues ignores the SourceLookup and uses the leaf reader set via setNextReader, which is fine. However, DocValueFetcher returns formatted values (e.g., BytesRef formatted as base64 via DocValueFormat.RAW for binary docvalues), which may not match the string wildcard predicate. Consider using a source-based fetcher to preserve prior matching semantics, or verify the docvalue-returned string form matches what secondPhaseMatcher expects. server/src/main/java/org/opensearch/index/mapper/WildcardFieldMapper.java [366-370] Supplier<ValueFetcher> valueFetcherSupplier(QueryShardContext context) { - if (hasDocValues()) { - IndexFieldData<?> ifd = context.getForField(this); - return () -> new DocValueFetcher(DocValueFormat.RAW, ifd); - } + Set<String> sourcePaths = context.sourcePath(name()); + Object nv = nullValue; + int above = ignoreAbove; + NamedAnalyzer norm = normalizer(); + String fieldName = name(); + return () -> new SourceValueFetcher(sourcePaths, nv) { + @Override + protected String parseSourceValue(Object value) { + String keywordValue = value.toString(); + if (keywordValue.length() > above) { + return null; + } + if (norm == null) { + return keywordValue; + } + try { + return normalizeValue(norm, fieldName, keywordValue); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + }; +} Suggestion importance[1-10]: 7 __ Why: The concern is valid: DocValueFetcher with DocValueFormat.RAW may return BytesRef values whose toString() differs from source string values, potentially breaking secondPhaseMatcher semantics compared to the original source-based fetching. This could cause correctness regressions.	Medium
General	Note loss of source cache sharing A fresh SourceLookup per scorer will not benefit from the shared source cache that LeafSearchLookup.source() provided, potentially re-parsing _source for the same doc when multiple queries touch it. If maintaining source cache sharing matters for performance, consider obtaining a SourceLookup from a per-request context rather than always allocating a new one; at minimum, document the tradeoff so callers know source parsing is no longer cached across queries. server/src/main/java/org/opensearch/index/mapper/WildcardFieldMapper.java [872-874] SourceLookup sourceLookup = new SourceLookup(); final ValueFetcher valueFetcher = valueFetcherSupplier.get(); valueFetcher.setNextReader(context); -TwoPhaseIterator twoPhaseIterator = new TwoPhaseIterator(approximation) { - @Override - public boolean matches() throws IOException { - sourceLookup.setSegmentAndDocument(context, approximation.docID()); - Suggestion importance[1-10]: 3 __ Why: A reasonable performance observation about losing shared source caching from LeafSearchLookup, but it's a minor concern and the suggestion doesn't propose a concrete fix.	Low

---

Previous suggestions

Suggestions up to commit 6412ae1

Category	Suggestion	Impact
Possible issue	Guard against null fieldType to avoid NPE When context is non-null but fieldType is null (as allowed by the other constructor path this(fieldName, firstPhaseQuery, s -> true, patternString, (QueryShardContext) null, null) in reverse scenarios), this will NPE. Guard against fieldType == null here to prevent a NullPointerException. server/src/main/java/org/opensearch/index/mapper/WildcardFieldMapper.java [802-806] -if (context != null) { +if (context != null && fieldType != null) { this.valueFetcherSupplier = fieldType.valueFetcherSupplier(context); } else { this.valueFetcherSupplier = null; } Suggestion importance[1-10]: 5 __ Why: Adding a null-check for fieldType is a reasonable defensive measure, though in practice callers passing a non-null context likely pass a non-null fieldType. Minor robustness improvement.	Low
General	Avoid retaining shard state via IndexFieldData Returning a supplier that uses DocValueFetcher still requires setNextReader to be called per-leaf, which is fine, but IndexFieldData obtained from QueryShardContext.getForField may retain references to shard-level state and defeat the purpose of avoiding heap retention via the query cache. Consider capturing only the minimal information needed (e.g., resolve the fielddata lazily inside the supplier using a lighter mechanism) or verify IndexFieldData is safe to hold in cached queries. server/src/main/java/org/opensearch/index/mapper/WildcardFieldMapper.java [366-370] +Supplier<ValueFetcher> valueFetcherSupplier(QueryShardContext context) { + if (hasDocValues()) { + IndexFieldData<?> ifd = context.getForField(this); + return () -> new DocValueFetcher(DocValueFormat.RAW, ifd); + } - Suggestion importance[1-10]: 3 __ Why: The suggestion is speculative and does not provide a concrete fix; the existing_code and improved_code are identical, only asking for verification.	Low

Suggestions up to commit ecf8c8e

Category	Suggestion	Impact
Possible issue	Avoid DocValueFetcher with SourceLookup mismatch Using a DocValueFetcher here will cause fetchValues to be called with a SourceLookup in the query's matches() method, but DocValueFetcher requires setNextReader and reads from doc values via the doc ID set on the fetcher, not from SourceLookup. Also, DocValueFetcher.fetchValues typically ignores the passed lookup and instead relies on an internally tracked docId, which is never advanced by sourceLookup.setSegmentAndDocument. Ensure the fetcher's doc ID is advanced (e.g., via a DocValueFetcher that accepts the current docID) or continue using a source-based fetcher path regardless of hasDocValues(). server/src/main/java/org/opensearch/index/mapper/WildcardFieldMapper.java [366-370] Supplier<ValueFetcher> valueFetcherSupplier(QueryShardContext context) { - if (hasDocValues()) { - IndexFieldData<?> ifd = context.getForField(this); - return () -> new DocValueFetcher(DocValueFormat.RAW, ifd); - } + // Always use a source-based fetcher here since matches() advances a SourceLookup, not a DocValueFetcher's internal docId. + Set<String> sourcePaths = context.sourcePath(name()); Suggestion importance[1-10]: 7 __ Why: The concern is valid: DocValueFetcher.fetchValues uses its internal docId set via setNextReader/setNextDocId, not the passed SourceLookup. If the fetcher's docId isn't advanced, it will return stale/wrong values. However, verification of actual behavior is needed as DocValueFetcher may still work if it tracks docId differently.	Medium
	Guard against null approximate scorer approximateScorer may be null if the first phase produces no documents; dereferencing .iterator() on it would throw NPE. Guard against null before proceeding. server/src/main/java/org/opensearch/index/mapper/WildcardFieldMapper.java [876-880] Scorer approximateScorer = firstPhaseSupplier.get(leadCost); +if (approximateScorer == null) { + return null; +} DocIdSetIterator approximation = approximateScorer.iterator(); SourceLookup sourceLookup = new SourceLookup(); final ValueFetcher valueFetcher = valueFetcherSupplier.get(); valueFetcher.setNextReader(context); Suggestion importance[1-10]: 3 __ Why: The null check is a defensive measure, but this pre-existing code path was not introduced by this PR and the scorer being non-null is generally guaranteed after firstPhaseSupplier check. Minor robustness improvement.	Low

[github-actions]

❌ Gradle check result for ecf8c8e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 6412ae1

[github-actions]

❌ Gradle check result for 6412ae1: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 0a948ed

[github-actions]

✅ Gradle check result for 0a948ed: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 11.53846% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.48%. Comparing base (dbad8f5) to head (0a948ed).

Files with missing lines	Patch %	Lines
...g/opensearch/index/mapper/WildcardFieldMapper.java	13.63%	18 Missing and 1 partial ⚠️
...rg/opensearch/index/mapper/SourceValueFetcher.java	0.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22433      +/-   ##
============================================
- Coverage     73.50%   73.48%   -0.02%     
+ Complexity    76487    76480       -7     
============================================
  Files          6104     6104              
  Lines        346567   346585      +18     
  Branches      49883    49886       +3     
============================================
- Hits         254733   254697      -36     
- Misses        71550    71585      +35     
- Partials      20284    20303      +19     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.