Make ICache keys() iteration safe under concurrent mutation

[jwils]

Description

Builds on Cache#keysSnapshot(), introduced in #22499, which has now merged — this PR is rebased onto main and is down to the single new commit (Make ICache keys() iteration safe under concurrent mutation). Not stacked on #22498, which has since been reworked to remove the async BitsetCacheCleaner and no longer touches Cache.

Third instance of the Cache.keys() iteration hazard fixed in #22491 / #22499 (fielddata cache) and #22498 (bitset filter cache), this time reaching the request cache through the pluggable cache layer:

• OpenSearchOnHeapCache#keys() returned the live Cache#keys() LRU iteration through the ICache interface. The live iterator walks the LRU linked list without holding the LRU lock; a concurrent cache hit relinks a not-yet-visited entry to the head of the list, behind the iterator's cursor, and iteration silently skips it.
• IndicesRequestCache's cleanup sweep (cleanCache) consumes keysToClean before scanning cache.keys(). An entry skipped by the sweep is not re-marked — a reader closes only once — so a stale entry promoted mid-sweep by a concurrent request survives the sweep.

Impact, stated plainly: unlike the fielddata cache, the request cache is size-bounded (indices.requests.cache.size, default 1% of heap), so a skipped entry is retained until size-based eviction reaches it rather than indefinitely. The consequences are wasted cache capacity holding an entry no future request can reach, and a staleKeysCount that never decrements for the skipped entry (the decrement is driven by the removal notification). That leaves the count permanently inflated, so canSkipCacheCleanup stops skipping sweeps that then return early at the empty-keysToClean check. This is a correctness bug worth fixing, but it is not the unbounded growth that motivated #22491 — I have no production measurements for this path.

Change:

• OpenSearchOnHeapCache#keys() returns a point-in-time copy built on Cache#keysSnapshot(). The copy is wrapped unmodifiable so a caller using Iterator#remove — which would silently no-op against a copy — fails fast instead.
• That behavior — point-in-time copy, iterator removal unsupported — is documented on OpenSearchOnHeapCache#keys() rather than on the ICache interface, so the ICache SPI is left untouched (per review feedback). The other implementations already iterate safely (TieredSpilloverCache composes per-tier keys(); ehcache's iterator is weakly consistent) and keep their existing removal behavior.
• The request-cache sweep removes matches with exact-key invalidate(key) instead of Iterator#remove(). IndicesRequestCache is the only caller in server/src/main that removed while iterating ICache#keys().

Behavior change for the tiered cache

Switching the sweep from Iterator#remove() to invalidate(key) is not a no-op under TieredSpilloverCache. ConcatenatedIterator#remove() delegates to the current tier's iterator, removing the entry from only the tier being iterated; TieredSpilloverCache#invalidate(key) invalidates in both tiers under the write lock. For stale-reader cleanup, removing from both tiers is the intended outcome — a key whose reader has closed should not survive on disk — and a key present in both tiers was yielded (and removed) twice by the concatenated iterator anyway. Flagging it explicitly rather than claiming semantics are unchanged. The INVALIDATED removal notification and stats accounting path are unchanged, and invalidating a concurrently removed key is a no-op.

Verification

Two new deterministic regression tests (no threads, no races — the promotion is triggered at a controlled point mid-iteration):

• OpenSearchOnHeapCacheTests#testKeysIsSafeToIterateUnderConcurrentPromotion — begins iterating keys(), then hits the LRU-tail entry (relinking it at the head behind the cursor), and asserts every inserted key is still observed. Against the live view the promoted key vanishes from iteration (4 of 5 keys observed); with the snapshot all 5 are observed.
• IndicesRequestCacheTests#testCleanCacheIsNotDefeatedByCacheHitPromotingEntryMidSweep — four entries across two shards, one shard's entries marked for cleanup, and a cache hit fired from the first swept entry's entity lookup promotes the not-yet-visited stale entry. Against the live view the promoted stale entry survives the sweep (3 entries remain instead of 2, and the suite's teardown leak checks trip); with the snapshot the sweep removes both stale entries.
• OpenSearchOnHeapCacheTests#testKeysIteratorDoesNotSupportRemoval pins the fail-fast contract.

IndicesRequestCacheTests, OpenSearchOnHeapCacheTests, and CacheTests pass, as does :modules:cache-common:test (50 tests, including TieredSpilloverCacheTests 40/40) — re-verified after rebasing onto main.

Related Issues

Same defect class as #22522; follow-up to #22491, #22499 and #22498.

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit a0b8647)

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 a0b8647

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Guard against invalidating re-inserted entries Since cache.keys() now returns a point-in-time snapshot, a key visited here may already have been invalidated and re-inserted by a concurrent request between snapshot time and the invalidate(key) call. Calling invalidate on such a re-inserted (still-live) entry would incorrectly remove a valid cache entry. Consider re-checking liveness (e.g. via a conditional remove) before invalidating. server/src/main/java/org/opensearch/indices/IndicesRequestCache.java [798-803] +CacheEntity cacheEntity = cacheEntityLookup.apply(delegatingKey.shardId).orElse(null); +if (cacheEntity == null) { + // If cache entity is null, it means that index or shard got deleted/closed meanwhile. + // So we will delete this key. + dimensionListsToDrop.add(key.dimensions); + cache.invalidate(key); +} else { - Suggestion importance[1-10]: 3 __ Why: The concern about invalidating a re-inserted entry is theoretically valid, but the suggestion's improved_code is identical to the existing_code and provides no concrete fix. Additionally, in a request cache context, invalidating a rarely-hit re-inserted entry is a minor correctness concern with limited practical impact.	Low

---

Previous suggestions

Suggestions up to commit 1b23984

Category	Suggestion	Impact
General	Document or mitigate snapshot memory cost Materializing the entire key set into a list on every keys() call can be memory-expensive for large caches and produces a strongly-referenced snapshot that may pin many keys. Consider returning an Iterable backed by a lazy snapshot iterator or documenting the memory cost so callers avoid invoking keys() in tight loops. server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java [145] +@Override +public Iterable<ICacheKey<K>> keys() { + return Collections.unmodifiableList(cache.keysSnapshot()); +} - Suggestion importance[1-10]: 2 __ Why: The existing_code and improved_code are identical, offering no concrete change. The suggestion is merely advisory and the existing Javadoc already explains the snapshot behavior.	Low

Suggestions up to commit b3ac091

Category	Suggestion	Impact
General	Avoid null ClusterService/ThreadPool in test Passing null for both ClusterService and ThreadPool may cause NPEs during construction or teardown of IndicesFieldDataCache (e.g., scheduling the refresh task or reading cluster settings). Provide test doubles or a TestThreadPool and a mocked ClusterService to prevent flaky/failing tests on unrelated code changes. server/src/test/java/org/opensearch/indices/fielddata/cache/IndicesFieldDataCacheTests.java [80-90] 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(); } } - }, null, null); + }, mock(ClusterService.class), new TestThreadPool(getTestName())); } Suggestion importance[1-10]: 5 __ Why: Passing null for ClusterService and ThreadPool could cause NPEs depending on the IndicesFieldDataCache constructor behavior; providing test doubles is a reasonable robustness improvement, though the test may already pass as-is.	Low
	Verify removal-notification semantics after invalidate switch Switching to cache.invalidate(key) will now fire a removal notification whose reason may differ from the previous iterator-based removal (e.g., INVALIDATED vs. EXPLICIT). Verify that downstream listeners (stats/staleness accounting in onRemoval) still classify these removals correctly; otherwise stale-key counters or eviction stats may drift. server/src/main/java/org/opensearch/indices/IndicesRequestCache.java [792-810] for (ICacheKey<Key> key : cache.keys()) { Key delegatingKey = key.key; Tuple<ShardId, Integer> shardIdInfo = new Tuple<>(delegatingKey.shardId, delegatingKey.indexShardHashCode); if (cleanupKeysFromFullClean.contains(shardIdInfo) || cleanupKeysFromClosedShards.contains(shardIdInfo)) { cache.invalidate(key); } else { CacheEntity cacheEntity = cacheEntityLookup.apply(delegatingKey.shardId).orElse(null); if (cacheEntity == null) { - // If cache entity is null, it means that index or shard got deleted/closed meanwhile. - // So we will delete this key. dimensionListsToDrop.add(key.dimensions); cache.invalidate(key); } else { CleanupKey cleanupKey = new CleanupKey(cacheEntity, delegatingKey.readerCacheKeyId); if (cleanupKeysFromOutdatedReaders.contains(cleanupKey)) { cache.invalidate(key); } } } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about removal notification reasons after switching from iterator.remove() to invalidate(), which could affect stats accounting, but it only asks to verify without concrete evidence of a defect.	Low
	Avoid sizing snapshot with unsafe count field The count field is volatile/estimated and read without any lock, so using it to size the ArrayList can under-allocate (causing resize) or over-allocate. More importantly, referencing count here assumes it's an accessible field of the enclosing Cache — if it isn't, this will not compile. Prefer sizing via segments.length-based estimation or omit the initial capacity to avoid coupling to internal counters. 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]: 3 __ Why: The count field exists in Cache (used elsewhere in the class), so the code compiles. Using it as an initial capacity hint is a reasonable estimate even if slightly stale; the concern is minor.	Low

Suggestions up to commit 86a0946

Category	Suggestion	Impact
General	Avoid passing null dependencies in test Passing null for the ClusterService and ThreadPool arguments risks a NullPointerException inside IndicesFieldDataCache's constructor or during close()/refresh() if those dependencies are dereferenced. Provide non-null mocks (e.g., via mock(ClusterService.class) and a TestThreadPool) and shut down the thread pool in a finally block to keep the tests robust to internal changes. server/src/test/java/org/opensearch/indices/fielddata/cache/IndicesFieldDataCacheTests.java [138-148] 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(); } } - }, null, null); + }, mock(ClusterService.class), mock(ThreadPool.class)); } Suggestion importance[1-10]: 4 __ Why: Passing null for ClusterService and ThreadPool works only if they aren't dereferenced; using mocks would make the test more robust to future changes. Minor test-quality improvement.	Low
	Reconsider synchronization scope for sweep The synchronized (this) block around this loop is now questionable: since the snapshot is taken and invalidate(key) is per-key thread-safe, holding the monitor for the entire sweep can cause unnecessary contention (and the comment update says correctness no longer depends on it). Consider removing the synchronized block or scoping it only around the snapshot acquisition, to avoid unnecessary serialization of concurrent clear() callers. server/src/main/java/org/opensearch/indices/fielddata/cache/IndicesFieldDataCache.java [250-263] +for (Key key : getCache().keysSnapshot()) { + if (indicesToClearCopy.contains(key.indexCache.index)) { + removeKey(key); + continue; + } + Set<String> fieldsOfIndexToClear = fieldsToClearCopy.get(key.indexCache.index); + if (fieldsOfIndexToClear != null && fieldsOfIndexToClear.contains(key.indexCache.fieldName)) { + removeKey(key); + continue; + } + if (cacheKeysToClearCopy.contains(key.readerKey)) { + removeKey(key); + } +} - Suggestion importance[1-10]: 4 __ Why: Valid observation about potential unnecessary contention after switching to a snapshot-based sweep, but the existing_code and improved_code are identical, so no concrete change is provided.	Low
	Guard initial capacity against negative count The count field is not synchronized and may be read as a stale/negative-ish value or a value larger than actually needed; more importantly, new ArrayList<>(count) uses the field directly which could be misleading. Since count here refers to the outer Cache's field, ensure it is read once and clamped to a non-negative value to avoid IllegalArgumentException when count is transiently negative due to concurrent updates. server/src/main/java/org/opensearch/common/cache/Cache.java [753-761] public List<K> keysSnapshot() { - List<K> keys = new ArrayList<>(count); + int initialCapacity = Math.max(0, count); + List<K> keys = new ArrayList<>(initialCapacity); for (CacheSegment<K, V> segment : segments) { try (ReleasableLock ignored = segment.readLock.acquire()) { keys.addAll(segment.map.keySet()); } } return keys; } Suggestion importance[1-10]: 3 __ Why: The count field in Cache is a long (typically an AtomicLong or similar), and while a transiently stale value is possible, ArrayList<>(int) throws only on negative capacity. This is a minor defensive improvement with limited real-world impact.	Low

[github-actions]

❌ Gradle check result for 86a0946: 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?

[github-actions]

Persistent review updated to latest commit b3ac091

[github-actions]

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

[kkhatua]

@jwils
Noticed this pending on #22499 which is pending on a minor test-related comment. @sgup432 can shepherd this in once that is done.

Thanks for helping with this contribution towards improving cache efficiency.

[github-actions]

Persistent review updated to latest commit a91607d

[github-actions]

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

[sgup432]

Overall LGTM, one minor comment

[sgup432]

@jwils I found one issue with keySnapshot() logic which I have mentioned here - #22499 (comment). It doesn't block this PR, just adding a FYI.

If you wanna take it up, feel free otherwise I can also fix it.

[github-actions]

Persistent review updated to latest commit 1b23984

[github-actions]

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

[sgup432]

@jwils Can you check on the failed unit tests? Seems like it is related to this change.

[github-actions]

Persistent review updated to latest commit a0b8647

[github-actions]

✅ Gradle check result for a0b8647: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.53%. Comparing base (0240b35) to head (a0b8647).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22542      +/-   ##
============================================
+ Coverage     71.48%   71.53%   +0.05%     
- Complexity    77022    77052      +30     
============================================
  Files          6156     6156              
  Lines        358482   358481       -1     
  Branches      52246    52246              
============================================
+ Hits         256255   256443     +188     
+ Misses        81810    81685     -125     
+ Partials      20417    20353      -64     

☔ 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.