Use point-in-time key snapshot for fielddata cache cleanup

[jwils]

Description

The fielddata cache cleanup sweep iterated Cache.keys(), which walks
the LRU linked list without holding the LRU lock; a concurrent cache
hit relinking the current entry to the head can silently skip entries,
so per-index/per-field clears could miss stale entries and retain them
indefinitely. Add Cache.keysSnapshot(), a weakly consistent
point-in-time copy taken one segment at a time under each segment's
read lock (never blocking cache reads and never holding a global lock
for O(n) work), and sweep that snapshot with exact-key invalidation
instead. Reader-close cleanup already invalidates its exact key
synchronously (#22491) and does not rely on the sweep.

Verification

Leak reproduction through the real clear() path. Harness: node-level cache with 505k entries (5k belonging to a target index), 4 threads hammering cache.get() across all entries to simulate search traffic, 30 rounds of clear(targetIndex) + sweep, counting target entries that survive. Same harness binary run against each build:

	Before	After (this PR)
Rounds with leaked entries	30/30	0/30
Worst single sweep	300/5,000 entries retained (6%)	0
Leaked entries persist through later no-mark sweeps	yes	n/a

Iterator coverage under concurrent promotion (1M-entry Cache, 4 reader threads): the live keys() walk missed keys in 200/200 iterations — the worst walk observed only 168,881 of 1,000,000 keys — while keysSnapshot() missed none in 200 iterations.

Snapshot cost (quiesced cache): 1.6 ms at 100k keys, 10.7 ms at 1M keys, spread across per-segment read locks.

Search latency impact (single node, 30 indices x 999 fielddata fields ≈ 124k keys / 121 MB, 12 search threads, 1s cleanup interval, 25 clear-and-sweep rounds): parity with the base branch.

Search latency (ms)	Base p50 / p99	This PR p50 / p99
Quiet control window	12.6 / 26.4	13.2 / 28.0
During clear rounds	14.2 / 54.5	14.5 / 54.4

Related Issues

Supersedes #22491 (see review discussion there and in this PR).

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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.

Update (2026-07-29)

• Made invalidateKey(Key) package-private per review and kept its removal-failure coverage in the same package without widening production visibility.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 08ebb96)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 08ebb96

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid cache reentry from removal listener Running the hook inside onRemoval triggers cache.get(tailTargetKey) while the cache's segment write lock (held by the invalidation) is likely still active on the current thread, which can deadlock or reenter unexpectedly depending on lock reentrancy semantics. Consider verifying the reentrancy is safe, or trigger the promotion from a separate thread synchronized via a latch to more faithfully simulate the concurrent-search scenario. server/src/test/java/org/opensearch/indices/fielddata/cache/IndicesFieldDataCacheTests.java [117-127] private IndicesFieldDataCache newFieldDataCache(AtomicReference<Runnable> onFirstRemoval) { return new IndicesFieldDataCache(Settings.EMPTY, new IndexFieldDataCache.Listener() { @Override public void onRemoval(ShardId shardId, String fieldName, boolean wasEvicted, long sizeInBytes) { Runnable hook = onFirstRemoval.getAndSet(null); if (hook != null) { - hook.run(); + // Run in a separate thread to avoid reentering the cache under its own write lock + Thread t = new Thread(hook); + t.start(); + try { t.join(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); } } } }, null, null); } Suggestion importance[1-10]: 3 __ Why: The concern about reentrancy under segment write lock is speculative; the existing test appears to work as designed to reproduce the LRU promotion scenario. The suggested change may not accurately simulate the intended concurrent scenario either.	Low
General	Avoid pre-sizing snapshot with unreliable count The field count is not defined/visible in the shown scope, and even if it exists, pre-sizing the ArrayList with a stale/approximate count can cause under- or over-allocation. Use a default capacity or sum the per-segment sizes under their read locks to get an accurate initial capacity. server/src/main/java/org/opensearch/common/cache/Cache.java [753-761] public List<K> keysSnapshot() { - List<K> keys = new ArrayList<>(count); + List<K> keys = new ArrayList<>(); for (CacheSegment<K, V> segment : segments) { try (ReleasableLock ignored = segment.readLock.acquire()) { keys.addAll(segment.map.keySet()); } } return keys; } Suggestion importance[1-10]: 2 __ Why: The count field is a member of the Cache class (used elsewhere in the file), so the claim that it's undefined is incorrect. Using it as an initial capacity hint is reasonable even if approximate; the suggestion offers only marginal improvement.	Low

---

Previous suggestions

Suggestions up to commit d38afcd

Category	Suggestion	Impact
General	Use a reliable size for initial capacity The field count is not declared/initialized in the visible diff context and appears to be used as an initial capacity hint. If count is not maintained as a live total size across segments, this will either fail to compile or produce an incorrect capacity. Consider initializing with a safe default (e.g., count() method or 0) to avoid relying on an ambiguous field. server/src/main/java/org/opensearch/common/cache/Cache.java [754] public List<K> keysSnapshot() { - List<K> keys = new ArrayList<>(count); + List<K> keys = new ArrayList<>(count()); for (CacheSegment<K, V> segment : segments) { try (ReleasableLock ignored = segment.readLock.acquire()) { keys.addAll(segment.map.keySet()); } } return keys; } Suggestion importance[1-10]: 2 __ Why: The count field is likely an existing member of the Cache class (as evidenced by the code compiling in context), so this suggestion questions an existing declaration outside the visible diff. Even if valid, it's a minor style/correctness concern with low impact.	Low

Suggestions up to commit 1ae96ef

Category	Suggestion	Impact
General	Avoid using stale count for list sizing The field count is not defined/used elsewhere as a total-size counter in Cache, and even if it were, reading it without synchronization to pre-size an ArrayList can produce a stale value and cause unnecessary resizing or over-allocation. Initialize the list with the default capacity or compute size from segment maps to avoid depending on a potentially stale/unsafe field. server/src/main/java/org/opensearch/common/cache/Cache.java [754] public List<K> keysSnapshot() { - List<K> keys = new ArrayList<>(count); + List<K> keys = new ArrayList<>(); for (CacheSegment<K, V> segment : segments) { try (ReleasableLock ignored = segment.readLock.acquire()) { keys.addAll(segment.map.keySet()); } } return keys; } Suggestion importance[1-10]: 2 __ Why: The count field does exist in Cache class (used elsewhere for tracking cache size). Using it as an initial capacity hint is a reasonable optimization; even if slightly stale, ArrayList will resize as needed. The suggestion's premise about count not existing is likely incorrect, and the impact of the change is minimal.	Low

Suggestions up to commit 73971a0

Category	Suggestion	Impact
General	Avoid unsynchronized read for list sizing The count field is accessed without synchronization to size the ArrayList. Since count is a plain long updated under LRU lock, reading it here is a data race and may return a stale/torn value. Consider initializing with a small default capacity (or (int) count after a volatile read) to avoid relying on unsynchronized state and prevent potential NegativeArraySizeException or truncated-to-int overflow issues. server/src/main/java/org/opensearch/common/cache/Cache.java [753-761] public List<K> keysSnapshot() { - List<K> keys = new ArrayList<>(count); + List<K> keys = new ArrayList<>(); for (CacheSegment<K, V> segment : segments) { try (ReleasableLock ignored = segment.readLock.acquire()) { keys.addAll(segment.map.keySet()); } } return keys; } Suggestion importance[1-10]: 4 __ Why: The concern about reading count without synchronization is valid but minor: count is a long field, and while a torn read is theoretically possible, in practice it's used elsewhere in the class similarly. The suggestion's impact is low as ArrayList handles growth automatically, and the initial capacity is only an optimization hint.	Low

Suggestions up to commit f66fcee

Category	Suggestion	Impact
Possible issue	Verify key equality semantics for invalidation Cache.invalidate(K) typically requires equals/hashCode semantics on Key. Since the original deferred approach compared by readerKey, ensure Key.equals/hashCode are based on indexCache + readerKey (ignoring the listeners field passed as null), otherwise this synchronous invalidation will not find the entry inserted during load and will silently leak the cached fielddata. server/src/main/java/org/opensearch/indices/fielddata/cache/IndicesFieldDataCache.java [401] // Don't call cache.refresh() here as it would have bad performance implications. +// Requires Key.equals/hashCode to be based on (indexCache, readerKey) only. nodeLevelCache.getCache().invalidate(new Key(this, key, null)); Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about Key.equals/hashCode semantics being critical for correct synchronous invalidation, but it only asks to verify and doesn't provide a concrete code change (improved_code is essentially the same as existing_code with a comment).	Low
General	Guard ArrayList initial capacity from overflow count is a long field but ArrayList's constructor takes an int; if count ever exceeds Integer.MAX_VALUE or is negative due to a race, this will throw. Also, count may be stale (not read under lock), so it's only a hint anyway — cap or clamp it to avoid an unexpected IllegalArgumentException. server/src/main/java/org/opensearch/common/cache/Cache.java [753-761] public List<K> keysSnapshot() { - List<K> keys = new ArrayList<>(count); + List<K> keys = new ArrayList<>((int) Math.min(Math.max(count, 0L), Integer.MAX_VALUE)); for (CacheSegment<K, V> segment : segments) { try (ReleasableLock ignored = segment.readLock.acquire()) { keys.addAll(segment.map.keySet()); } } return keys; } Suggestion importance[1-10]: 4 __ Why: Valid defensive concern since count is a long passed to ArrayList(int), which could theoretically overflow or be negative under races. In practice, cache sizes rarely exceed Integer.MAX_VALUE, so the impact is minor.	Low

[github-actions]

❌ Gradle check result for f66fcee: null

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?

[sgup432]

@jwils Thanks for the PR. Can you remove this from draft state if this is ready?
Also do we need the original PR now? As this PR solves all the leaks happening within field data cache and not just with the stale segment reader. I would prefer to merge this in.

[jwils]

This does not fix any issue. Let's fix and do patches the memory leak first.

[sgup432]

This does not fix any issue

And can you explain me why? I see the original issue was that we are iterating over a live LRU list which can reshuffle and we might miss a entry during a certain pass. This completely avoids it.

You have also a test you wrote apparently which you can run against this approach and verify.

[sgup432]

Tagging maintainers here for a second eye here - @andrross @reta @msfroh @jainankitk

[jwils]

My frustration here is my AWS TAM is saying you don't believe the first PR fixes the memory leak. If you can confirm you just prefer the style of this PR vs the original PR I can adjust the style here.

I'd like to be explicit that the first PR as written when opened shows and fixes a memory leak in Opensearch 3.3.

[github-actions]

Persistent review updated to latest commit 73971a0

[sgup432]

@jwils This is an opensource community not just managed by AWS but by others as well. So lets keep the AWS specific conversation out of here. If you need help with AWS specific issues, better to engage via other internal channel.

From my side, I agree this is a real issue. I'm just looking for the right solution. The earlier PR partially fixes it for one major case, whereas this PR covers all of them. And by exposing a keysSnapshot method, this change also lets anyone else reuse it if needed.

[kkhatua]

Will you concede that the other fix is a complete fix for the memory leak?

@jwils
I don't believe @sgup432 is denying that the original fix #22491 is not applicable, but that your current PR is a better approach. The original FDC was a buggy implementation lingering in the code base for more than a decade. Hence, it is prudent to ensure that any changes to such infrequently touched but widely impacting components be made with sufficient time for deliberations.

@sgup432 's request for test cases is something that is reasonable and asked of every contributor to the project. If you need guidance for this, the community (including us) are always willing to help.

[jwils]

My read is we now agree on the mechanism for a memory leak in opensearch. The live keys() iteration could skip entries when the LRU reshuffled mid-sweep, and on the reader-close path a miss was permanent. #22491 fixed that permanent leak with exact-key invalidation; #22499 fixes the same leak plus the per-index/per-field sweep misses by removing the unsafe iteration entirely, and is the approach we're merging and while the implementation may have been buggy for a while this issue was caused by #19116. The regression test fails on main and passes with either fix. @sgup432 is that a fair summary?

[github-actions]

Persistent review updated to latest commit 1ae96ef

[jwils]

@kkhatua adding a test is not a problem. Done.

[jwils]

You all of course get to make the decision here, but let me also give one more recommendation as to why I believe #22491 is the better branch to backport to 3.3 and a good independent fix. It fixes the memory leak by reverting the invalidation to the previous behavior that runs in O(1) and does not introduce new allocations or locking mechanisms.

[github-actions]

❌ Gradle check result for 1ae96ef: 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?

[jwils]

Can't help myself, another datapoint after a day of running the other patch. Happy to deploy this change as well to compare, but looking at our last blue green deployment of 3.3 on June 17 you can see old gen jvm heap rose to about 60% of the heap. This rise was rapid and I doubt accounted for by the memory leak as it happened pretty quickly and we see the later rise is much slower. You can see one of those clear leaks occur right at the end of the graph.

Note on this graph we were going from a 64GB heap i8g.4xlarge, so the heap usage before the B/G shouldn't be compared directly.

This graph shows the current deployment of #22491 which appears to be extremely stable at 45% old gen heap. Given we run with a custom larger memory instance I imagine this difference would be even more pronounced for those with high write load running with 32 GB or less of heap.

I'd be open to working with AWS to test this fix out as well so we can compare the old gen heap usage between the two patches. The difference in old gen usage is significantly more than I would have expected.

[sgup432]

@jwils Thanks for sharing that, looks good. I am happy to take #22491 in, along with testing this change out as well if possible. Accordingly we can decide to keep the desired change.

[jwils]

Thanks @sgup432 — and apologies for getting a bit off track across these two PRs. I appreciate the careful review of the data here and will aim to hold my responses to the same standard.

[jwils]

Another interesting datapoint, this time comparing steady-state fielddata size under production query load. East is running #22491; west is unpatched 3.3, sampled ~1 hour after a fielddata cache clear to hopefully minimize the contribution of already-leaked entries. Given our high cache turnover, you can see the west cache runs almost twice as large under the 1-min sweep interval — sawtoothing between ~70–100 MB while east holds ~52–64 MB.

It's currently hard to track and not sure if there is a way, but I'd be interested in pinning down how much heap actually gets retained here. If it's really just ~50 MB of fielddata it's probably not a big issue, but if those entries pin substantially more than the reported memory_size, that could account for some of the old-gen difference in the graphs above.

[andrross]

I've merged #22491.

My two cents (caveat I'm not really a deep expert on all this caching code):

• Iterating the list while it is being modified is clearly wrong and we should make sure that never happens (that's what this PR intends to fix)
• Replacing an async path with a synchronous path is almost always a simplification. Async programming is hard and if you can do something synchronously you probably should (that's what Fix stale fielddata retention by invalidating exact key on reader close #22491 does).

In other words doing both PRs is probably worthwhile.

[github-actions]

Persistent review updated to latest commit d38afcd

[jwils]

Thank you. I agree with that approach. I've rebased this pr and switched back to that method.

[github-actions]

✅ Gradle check result for d38afcd: SUCCESS

[kkhatua]

Tagging @jainankitk for final review and merge after @sgup432 's review

[sgup432]

Overall LGTM, just one minor comment

[github-actions]

Persistent review updated to latest commit 08ebb96

[jwils]

Done and rebased.

[github-actions]

❕ Gradle check result for 08ebb96: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.