BitsetFilterCache refactor: Change it to a node level cache with a size limit

[sgup432]

Description

Earlier, BitsetFilterCache was created per index and without any size limit for each or all of the caches combined.

• This PR changes this cache to a node level cache shared across the segment, and then puts a size limit. Defaults to 5% of the total heap available on the node, also a static node level setting.

• It is also changing(or flattening) this cache structure from Cache<K, Cache<K1, V1>> to Cache<CK, V> where CK is a composite key <Segment, Query>. This logic was needed to be able to put a size limit on the cache.
  Other way to handle this would have been to implement our own custom cache and manage our own logic to calculate the size and impose a limit, but that's a even bigger change compared to the current approach taken.

We have now IndicesBitsetFilterCache, a node level cache. BitsetFilterCache is still used at an index level, but it eventually uses IndicesBitsetFilterCache to store results, so it is only a thin layer now. Kept it to avoid backward compatibility issues.

Related Issues

#21025

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 73f28fb)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Core: Introduce node-level IndicesBitsetFilterCache and refactor BitsetFilterCache Relevant files: server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java server/src/main/java/org/opensearch/index/cache/bitset/BitsetFilterCache.java server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java server/src/main/java/org/opensearch/index/IndexModule.java server/src/main/java/org/opensearch/index/IndexService.java server/src/main/java/org/opensearch/indices/IndicesService.java Sub-PR theme: Tests: Update all test infrastructure to use IndicesBitsetFilterCache Relevant files: server/src/test/java/org/opensearch/index/cache/bitset/BitSetFilterCacheTests.java server/src/test/java/org/opensearch/indices/IndicesBitsetFilterCacheTests.java server/src/test/java/org/opensearch/index/IndexModuleTests.java server/src/test/java/org/opensearch/search/internal/ContextIndexSearcherTests.java server/src/test/java/org/opensearch/search/sort/AbstractSortTestCase.java test/framework/src/main/java/org/opensearch/search/aggregations/AggregatorTestCase.java test/framework/src/main/java/org/opensearch/test/AbstractBuilderTestCase.java server/src/test/java/org/opensearch/search/aggregations/bucket/nested/NestedAggregatorTests.java modules/percolator/src/test/java/org/opensearch/percolator/PercolatorQuerySearchTests.java

⚡ Recommended focus areas for review Race Condition In purgeStaleEntries(), there is a potential race condition: after taking a snapshot of staleCacheKeys, new keys could be added to staleCacheKeys before removeAll is called. Additionally, iterating over cache.keys() while invalidating entries may not be safe if the cache's key iterator does not support concurrent modification. The two-set approach (staleCacheKeys + registeredKeys) with non-atomic operations could lead to missed purges or double-registration. public void purgeStaleEntries() { if (staleCacheKeys.isEmpty()) { return; } Set<IndexReader.CacheKey> staleSnapshot = new HashSet<>(staleCacheKeys); for (BitsetCacheKey key : cache.keys()) { if (staleSnapshot.contains(key.readerCacheKey)) { cache.invalidate(key); } } staleCacheKeys.removeAll(staleSnapshot); registeredKeys.removeAll(staleSnapshot); } Listener Stats Mismatch The Value object stores the BitsetFilterCache.Listener at cache-insertion time. If the same (readerCacheKey, query) pair is requested with a different listener (e.g., from a different shard or index), computeIfAbsent returns the cached value with the original listener, so onCache is never called for the new listener and onRemoval will fire for the original listener only. This can cause shard-level stats to be incorrect. return cache.computeIfAbsent(cacheKey, key -> { final BitSet bitSet = bitsetFromQuery(query, context); Value value = new Value(bitSet, shardId, listener); listener.onCache(shardId, value.bitset); return value; }).bitset; Silent Failure getBitSetProducer throws IllegalStateException when indicesCache is null, but the deprecated no-arg constructor sets indicesCache to null. Any code using the deprecated constructor and then calling getBitSetProducer will get a runtime exception rather than a compile-time warning. This is a behavioral regression compared to the old code which always worked. public BitSetProducer getBitSetProducer(Query query) { if (indicesCache != null) { return indicesCache.getBitSetProducer(query, listener); } throw new IllegalStateException("IndicesBitsetFilterCache is not available"); } Resource Leak The ThreadPool tp and IndicesBitsetFilterCache indicesCache are created inside the test method but only cleaned up at the end of the specific test block. If the test fails before reaching the cleanup lines, these resources will be leaked. They should be managed with try-finally or as class-level fields with proper teardown. ThreadPool tp = new TestThreadPool("test"); IndicesBitsetFilterCache indicesCache = new IndicesBitsetFilterCache(Settings.EMPTY, tp); BitsetFilterCache cache = new BitsetFilterCache(settings, indicesCache, listener); Query roleQuery = new TermQuery(new Term("allowed", "yes")); BitSet bitSet = cache.getBitSetProducer(roleQuery).getBitSet(reader.leaves().get(0)); if (sparse) { assertThat(bitSet, instanceOf(SparseFixedBitSet.class)); } else { assertThat(bitSet, instanceOf(FixedBitSet.class)); } DocumentSubsetDirectoryReader filteredReader = new DocumentSubsetDirectoryReader(reader, cache, roleQuery); SearchContext searchContext = mock(SearchContext.class); IndexShard indexShard = mock(IndexShard.class); when(searchContext.indexShard()).thenReturn(indexShard); SearchOperationListener searchOperationListener = new SearchOperationListener() { }; when(indexShard.getSearchOperationListener()).thenReturn(searchOperationListener); when(searchContext.bucketCollectorProcessor()).thenReturn(SearchContext.NO_OP_BUCKET_COLLECTOR_PROCESSOR); ContextIndexSearcher searcher = new ContextIndexSearcher( filteredReader, IndexSearcher.getDefaultSimilarity(), IndexSearcher.getDefaultQueryCache(), IndexSearcher.getDefaultQueryCachingPolicy(), true, null, searchContext ); for (LeafReaderContext context : searcher.getIndexReader().leaves()) { assertThat(context.reader(), instanceOf(SequentialStoredFieldsLeafReader.class)); SequentialStoredFieldsLeafReader lf = (SequentialStoredFieldsLeafReader) context.reader(); assertNotNull(lf.getSequentialStoredFieldsReader()); } // Assert wrapping assertEquals(ExitableDirectoryReader.class, searcher.getIndexReader().getClass()); for (LeafReaderContext lrc : searcher.getIndexReader().leaves()) { assertEquals(ExitableLeafReader.class, lrc.reader().getClass()); assertNotEquals(ExitableTerms.class, lrc.reader().terms("foo").getClass()); assertNotEquals(ExitablePointValues.class, lrc.reader().getPointValues("point").getClass()); } searcher.addQueryCancellation(() -> {}); for (LeafReaderContext lrc : searcher.getIndexReader().leaves()) { assertEquals(ExitableTerms.class, lrc.reader().terms("foo").getClass()); assertEquals(ExitablePointValues.class, lrc.reader().getPointValues("point").getClass()); } // Searching a non-existing term will trigger a null scorer assertEquals(0, searcher.count(new TermQuery(new Term("non_existing_field", "non_existing_value")))); assertEquals(1, searcher.count(new TermQuery(new Term("foo", "bar")))); // make sure scorers are created only once, see #1725 assertEquals(1, searcher.count(new CreateScorerOnceQuery(new MatchAllDocsQuery()))); TopDocs topDocs = searcher.search(new BoostQuery(new ConstantScoreQuery(new TermQuery(new Term("foo", "bar"))), 3f), 1); assertEquals(1, topDocs.totalHits.value()); assertEquals(1, topDocs.scoreDocs.length); assertEquals(3f, topDocs.scoreDocs[0].score, 0); IOUtils.close(reader, w, dir, indicesCache); ThreadPool.terminate(tp, 10, java.util.concurrent.TimeUnit.SECONDS); Scheduler Leak In the constructor, threadPool.schedule(cacheCleaner, cleanInterval, ThreadPool.Names.SAME) is called before the object is fully constructed and before any error handling. If an exception occurs after this line, the cleaner task will keep running against a partially-initialized or abandoned cache. Also, if close() is called before the first scheduled run completes, there could be a window where the cleaner runs after clear() has been called. public IndicesBitsetFilterCache(Settings settings, ThreadPool threadPool) { long sizeInBytes = INDICES_BITSET_FILTER_CACHE_SIZE_SETTING.get(settings).getBytes(); CacheBuilder<BitsetCacheKey, Value> cacheBuilder = CacheBuilder.<BitsetCacheKey, Value>builder().removalListener(this); if (sizeInBytes > 0) { cacheBuilder.setMaximumWeight(sizeInBytes).weigher(new BitsetWeigher()); } this.cache = cacheBuilder.build(); TimeValue cleanInterval = INDICES_BITSET_FILTER_CACHE_CLEAN_INTERVAL_SETTING.get(settings); this.cacheCleaner = new BitsetCacheCleaner(this, threadPool, cleanInterval); threadPool.schedule(cacheCleaner, cleanInterval, ThreadPool.Names.SAME); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 3af0257

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard against null bitset before invoking cache listener When bitsetFromQuery returns null (no matching documents), listener.onCache is called with a null accountable, which can cause a NullPointerException in listener implementations that call accountable.ramBytesUsed() (as seen in BitSetProducerWarmer). Add a null check before invoking listener.onCache. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [151-156] -BitSet getAndLoadIfNotPresent(final Query query, final LeafReaderContext context, final BitsetFilterCache.Listener listener) - throws ExecutionException { - ... - return cache.computeIfAbsent(cacheKey, key -> { - final BitSet bitSet = bitsetFromQuery(query, context); - Value value = new Value(bitSet, shardId, listener); +return cache.computeIfAbsent(cacheKey, key -> { + final BitSet bitSet = bitsetFromQuery(query, context); + Value value = new Value(bitSet, shardId, listener); + if (value.bitset != null) { listener.onCache(shardId, value.bitset); - return value; - }).bitset; -} + } + return value; +}).bitset; Suggestion importance[1-10]: 7 __ Why: When bitsetFromQuery returns null (no matching documents), calling listener.onCache(shardId, value.bitset) passes a null accountable, which can cause NullPointerException in listeners that call accountable.ramBytesUsed(). This is a real bug that could cause runtime failures.	Medium
General	Ensure thread pool is terminated even on test failure The ThreadPool tp is created locally but is not closed if an exception occurs before ThreadPool.terminate is reached. The indicesCache is closed via IOUtils.close, but tp is terminated after that in a separate statement. If IOUtils.close throws, tp will leak. Consider using a try-finally block or adding tp to the IOUtils.close call (if it implements Closeable). server/src/test/java/org/opensearch/search/internal/ContextIndexSearcherTests.java [262-324] ThreadPool tp = new TestThreadPool("test"); IndicesBitsetFilterCache indicesCache = new IndicesBitsetFilterCache(Settings.EMPTY, tp); -BitsetFilterCache cache = new BitsetFilterCache(settings, indicesCache, listener); -... -IOUtils.close(reader, w, dir, indicesCache); -ThreadPool.terminate(tp, 10, java.util.concurrent.TimeUnit.SECONDS); +try { + BitsetFilterCache cache = new BitsetFilterCache(settings, indicesCache, listener); + // ... rest of test ... + IOUtils.close(reader, w, dir, indicesCache); +} finally { + ThreadPool.terminate(tp, 10, java.util.concurrent.TimeUnit.SECONDS); +} Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that tp could leak if IOUtils.close throws before ThreadPool.terminate is called. Using a try-finally block is a valid improvement for test resource cleanup reliability.	Low
	Prevent unbounded growth of registered keys set There is a race condition: between taking staleSnapshot and calling staleCacheKeys.removeAll(staleSnapshot), new keys could be added to staleCacheKeys by onClose. Using staleCacheKeys.removeAll(staleSnapshot) is correct only if no new entries were added for the same key after the snapshot. However, a more critical issue is that registeredKeys is never cleaned up when entries are evicted by size limit (only during purgeStaleEntries and clear), causing registeredKeys to grow unboundedly and the closed-listener to be added multiple times if a key is re-registered after eviction. The registeredKeys.add check in getAndLoadIfNotPresent prevents re-registration, but after purgeStaleEntries removes the key from registeredKeys, a subsequent access will re-add the closed listener, which is correct. However, if the cache evicts an entry by size but the reader is still open, registeredKeys still holds the key, preventing re-registration of the closed listener on the next access — this is actually fine. The real issue is that registeredKeys is never cleaned up on size-based eviction, so it can grow without bound. Consider removing from registeredKeys in onRemoval when the last entry for a reader key is evicted. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [177-183] -public void purgeStaleEntries() { - if (staleCacheKeys.isEmpty()) { +@Override +public void onRemoval(RemovalNotification<BitsetCacheKey, Value> notification) { + Value value = notification.getValue(); + if (value == null || value.listener == null) { return; } - Set<IndexReader.CacheKey> staleSnapshot = new HashSet<>(staleCacheKeys); - - for (BitsetCacheKey key : cache.keys()) { - if (staleSnapshot.contains(key.readerCacheKey)) { - cache.invalidate(key); + value.listener.onRemoval(value.shardId, value.bitset); + // Clean up registeredKeys if no more entries exist for this reader key + BitsetCacheKey removedKey = notification.getKey(); + if (removedKey != null) { + boolean hasMore = false; + for (BitsetCacheKey k : cache.keys()) { + if (k.readerCacheKey == removedKey.readerCacheKey) { + hasMore = true; + break; + } + } + if (!hasMore) { + registeredKeys.remove(removedKey.readerCacheKey); } } - - staleCacheKeys.removeAll(staleSnapshot); - registeredKeys.removeAll(staleSnapshot); } Suggestion importance[1-10]: 3 __ Why: The concern about registeredKeys growing unboundedly on size-based eviction is valid, but the proposed fix iterates over all cache keys in onRemoval which is called on every eviction — this would be very expensive. The suggestion identifies a real issue but the improved code has significant performance implications.	Low
	Avoid passing null listener to IndexWarmer When indicesBitsetFilterCache is null, the warmer is created with a null listener. If IndexWarmer does not handle a null listener gracefully, this will cause a NullPointerException at warm time. Verify that IndexWarmer accepts a null listener, or provide a no-op listener as a fallback to avoid potential NPEs. server/src/main/java/org/opensearch/index/IndexService.java [316-320] -this.warmer = new IndexWarmer( - threadPool, - indexFieldData, - indicesBitsetFilterCache != null ? indicesBitsetFilterCache.createListener(threadPool) : null -); +IndexWarmer.Listener warmerListener = indicesBitsetFilterCache != null + ? indicesBitsetFilterCache.createListener(threadPool) + : IndexWarmer.Listener.NO_WAIT_LISTENER; // or a no-op implementation +this.warmer = new IndexWarmer(threadPool, indexFieldData, warmerListener); Suggestion importance[1-10]: 3 __ Why: The suggestion asks to verify that IndexWarmer handles a null listener, but the improved code references a non-existent IndexWarmer.Listener.NO_WAIT_LISTENER constant, making it inaccurate. The concern is valid but the fix is not directly applicable.	Low

---

Previous suggestions

Suggestions up to commit 94a1f9f

Category	Suggestion	Impact
Possible issue	Fix stale key removal to avoid dropping newly stale entries The cache.keys() iteration and subsequent cache.invalidate(key) calls are not atomic. New entries for stale keys could be inserted between the iteration and invalidation, causing them to remain in the cache indefinitely. Additionally, keys added to staleCacheKeys after the snapshot is taken but before removeAll are silently dropped from the stale set without being purged. Consider using a more robust approach such as filtering keys during iteration or re-checking after removal. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [185-199] public void purgeStaleEntries() { if (staleCacheKeys.isEmpty()) { return; } Set<IndexReader.CacheKey> staleSnapshot = new HashSet<>(staleCacheKeys); for (BitsetCacheKey key : cache.keys()) { if (staleSnapshot.contains(key.readerCacheKey)) { cache.invalidate(key); } } - staleCacheKeys.removeAll(staleSnapshot); - registeredKeys.removeAll(staleSnapshot); + // Only remove keys that were in our snapshot to avoid dropping newly stale keys + for (IndexReader.CacheKey staleKey : staleSnapshot) { + staleCacheKeys.remove(staleKey); + registeredKeys.remove(staleKey); + } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that staleCacheKeys.removeAll(staleSnapshot) could drop keys added after the snapshot. The improved code iterates over the snapshot to remove only those keys, which is a more correct approach. The improved_code accurately reflects the change and addresses a real concurrency concern.	Low
	Fix race condition in closed listener registration There is a race condition between registeredKeys.add and cacheHelper.addClosedListener. Two threads could both pass the add check before either registers the listener, resulting in the closed listener being registered twice. Use a proper concurrent check-then-act pattern or synchronize the registration to ensure the listener is added exactly once. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [146-156] if (registeredKeys.add(coreCacheReader)) { - cacheHelper.addClosedListener(this); + try { + cacheHelper.addClosedListener(this); + } catch (Exception e) { + registeredKeys.remove(coreCacheReader); + throw e; + } } final BitsetCacheKey cacheKey = new BitsetCacheKey(coreCacheReader, query); return cache.computeIfAbsent(cacheKey, key -> { final BitSet bitSet = bitsetFromQuery(query, context); Value value = new Value(bitSet, shardId, listener); listener.onCache(shardId, value.bitset); return value; }).bitset; Suggestion importance[1-10]: 5 __ Why: The suggestion identifies a potential race condition between registeredKeys.add and cacheHelper.addClosedListener. However, the improved_code only adds rollback on exception, not a true atomic check-then-act, so it doesn't fully solve the described race. The issue is real but the fix is incomplete, warranting a moderate score.	Low
General	Avoid clearing registered keys on cache clear Clearing registeredKeys without removing the closed listeners from the underlying IndexReader.CacheHelper means that after a clear(), if the same reader is still open and later closes, onClose will be called and add the key to staleCacheKeys again, but no new entry will ever be cached for it since registeredKeys was cleared. This is a minor inconsistency, but more critically, after clear() new cache entries for already-registered readers will not re-register the closed listener (since registeredKeys is empty but the listener is still attached), potentially causing double onClose calls. Consider not clearing registeredKeys in clear(), or re-registering listeners as needed. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [170-174] public void clear() { cache.invalidateAll(); staleCacheKeys.clear(); - registeredKeys.clear(); + // Do not clear registeredKeys: existing readers still have their closed listeners attached. + // Clearing registeredKeys would cause double-registration on next cache access for open readers. } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about clearing registeredKeys potentially causing double-registration of closed listeners. However, the scenario is somewhat edge-case and the improved_code only removes one line with a comment, making it a minor improvement with limited practical impact.	Low
	Guard against null warmer listener causing NPE When indicesBitsetFilterCache is null, the warmer is created with a null listener, which may cause NullPointerException inside IndexWarmer if it unconditionally invokes the listener. Verify that IndexWarmer handles a null listener gracefully, or provide a no-op listener as a fallback to avoid potential NPEs at runtime. server/src/main/java/org/opensearch/index/IndexService.java [315-320] this.bitsetFilterCache = new BitsetFilterCache(indexSettings, indicesBitsetFilterCache, new BitsetCacheListener(this)); -this.warmer = new IndexWarmer( - threadPool, - indexFieldData, - indicesBitsetFilterCache != null ? indicesBitsetFilterCache.createListener(threadPool) : null -); +IndexWarmer.Listener bitsetWarmerListener = indicesBitsetFilterCache != null + ? indicesBitsetFilterCache.createListener(threadPool) + : IndexWarmer.Listener.NO_OP; // or null only if IndexWarmer explicitly supports it +this.warmer = new IndexWarmer(threadPool, indexFieldData, bitsetWarmerListener); Suggestion importance[1-10]: 4 __ Why: The suggestion asks to verify that IndexWarmer handles a null listener, which is a valid concern. However, the improved_code references IndexWarmer.Listener.NO_OP which may not exist in the codebase, making the suggested fix potentially incorrect. The concern is valid but the proposed solution is speculative.	Low

Suggestions up to commit 73f28fb

Category	Suggestion	Impact
Possible issue	Fix race condition in closed listener registration There is a race condition between registeredKeys.add and cacheHelper.addClosedListener. Two threads could both pass the add check before either registers the listener, but more critically, if the reader closes between the add and addClosedListener calls, the onClose callback will never fire and the entry will never be marked stale. Consider using a lock or a computeIfAbsent-style atomic operation to ensure the listener is registered atomically with the key insertion. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [146-157] -if (registeredKeys.add(coreCacheReader)) { - cacheHelper.addClosedListener(this); +// Use computeIfAbsent to atomically register the closed listener +// only on first encounter of this cache key. +final BitsetCacheKey cacheKey = new BitsetCacheKey(coreCacheReader, query); +// Register closed listener atomically before inserting into cache +synchronized (registeredKeys) { + if (registeredKeys.add(coreCacheReader)) { + cacheHelper.addClosedListener(this); + } } - -final BitsetCacheKey cacheKey = new BitsetCacheKey(coreCacheReader, query); return cache.computeIfAbsent(cacheKey, key -> { final BitSet bitSet = bitsetFromQuery(query, context); Value value = new Value(bitSet, shardId, listener); listener.onCache(shardId, value.bitset); return value; }).bitset; Suggestion importance[1-10]: 6 __ Why: The race condition between registeredKeys.add and cacheHelper.addClosedListener is a valid concern — if a reader closes between these two operations, the stale entry will never be purged. However, the suggested fix using synchronized on a ConcurrentSet is not idiomatic and may introduce other issues. The core issue is real but the fix needs more careful design.	Low
General	Prevent unbounded growth of registered keys set After close() is called, cacheCleaner is stopped but purgeStaleEntries() can still be called externally (e.g., from tests). More critically, clear() clears staleCacheKeys and registeredKeys, but purgeStaleEntries() only removes the snapshot subset from staleCacheKeys — if new stale keys arrive concurrently during purge, they could be lost. Additionally, registeredKeys is never cleaned up for keys that were evicted by the size-based LRU eviction (only stale keys are removed), causing registeredKeys to grow unboundedly over time. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [185-199] public void purgeStaleEntries() { if (staleCacheKeys.isEmpty()) { return; } Set<IndexReader.CacheKey> staleSnapshot = new HashSet<>(staleCacheKeys); for (BitsetCacheKey key : cache.keys()) { if (staleSnapshot.contains(key.readerCacheKey)) { cache.invalidate(key); } } staleCacheKeys.removeAll(staleSnapshot); registeredKeys.removeAll(staleSnapshot); } +@Override +public void onRemoval(RemovalNotification<BitsetCacheKey, Value> notification) { + Value value = notification.getValue(); + if (value == null || value.listener == null) { + return; + } + value.listener.onRemoval(value.shardId, value.bitset); + // Clean up registeredKeys for LRU-evicted entries so it doesn't grow unboundedly + if (notification.getKey() != null) { + IndexReader.CacheKey readerKey = notification.getKey().readerCacheKey; + // Only remove from registeredKeys if no other cache entries reference this reader key + boolean stillReferenced = false; + for (BitsetCacheKey k : cache.keys()) { + if (k.readerCacheKey == readerKey) { + stillReferenced = true; + break; + } + } + if (!stillReferenced) { + registeredKeys.remove(readerKey); + } + } +} + Suggestion importance[1-10]: 5 __ Why: The concern about registeredKeys growing unboundedly when entries are LRU-evicted (not just stale-purged) is valid. However, the proposed improved_code is essentially the same as the existing_code for purgeStaleEntries with an entirely new onRemoval method appended — the onRemoval method already exists in the PR code, making the suggestion's structure misleading. The underlying issue is real but the fix is incomplete and potentially expensive (iterating cache keys on every eviction).	Low
	Document breaking behavior of deprecated constructor usage When indicesCache is null (e.g., when the deprecated no-arg constructor is used), getBitSetProducer throws IllegalStateException. However, the deprecated constructor BitsetFilterCache(IndexSettings, Listener) delegates to this(indexSettings, null, listener), so any caller using the old API will get a runtime exception instead of a graceful fallback. This is a breaking behavioral change for existing callers of the deprecated constructor. Consider providing a fallback implementation or documenting this clearly. server/src/main/java/org/opensearch/index/cache/bitset/BitsetFilterCache.java [116-121] public BitSetProducer getBitSetProducer(Query query) { if (indicesCache != null) { return indicesCache.getBitSetProducer(query, listener); } - throw new IllegalStateException("IndicesBitsetFilterCache is not available"); + // Fallback: create a non-caching producer for backwards compatibility + // when no node-level cache is available. + throw new IllegalStateException( + "IndicesBitsetFilterCache is not available. Use BitsetFilterCache(IndexSettings, IndicesBitsetFilterCache, Listener) constructor." + ); } Suggestion importance[1-10]: 4 __ Why: The concern is valid — callers using the deprecated BitsetFilterCache(IndexSettings, Listener) constructor will now get an IllegalStateException at runtime when calling getBitSetProducer. However, the improved_code is essentially identical to the existing_code (just adds a slightly more descriptive message), making the suggestion's impact minimal.	Low
	Ensure thread pool is terminated on test failure The ThreadPool tp is created inside the test method but is not terminated if an exception is thrown before the ThreadPool.terminate call at the end. The indicesCache is closed via IOUtils.close, but tp is only terminated in the happy path. Use a try-finally block to ensure tp is always terminated, preventing thread leaks on test failures. server/src/test/java/org/opensearch/search/internal/ContextIndexSearcherTests.java [262-324] ThreadPool tp = new TestThreadPool("test"); -IndicesBitsetFilterCache indicesCache = new IndicesBitsetFilterCache(Settings.EMPTY, tp); -BitsetFilterCache cache = new BitsetFilterCache(settings, indicesCache, listener); -Query roleQuery = new TermQuery(new Term("allowed", "yes")); -BitSet bitSet = cache.getBitSetProducer(roleQuery).getBitSet(reader.leaves().get(0)); -... -IOUtils.close(reader, w, dir, indicesCache); -ThreadPool.terminate(tp, 10, java.util.concurrent.TimeUnit.SECONDS); +try { + IndicesBitsetFilterCache indicesCache = new IndicesBitsetFilterCache(Settings.EMPTY, tp); + BitsetFilterCache cache = new BitsetFilterCache(settings, indicesCache, listener); + Query roleQuery = new TermQuery(new Term("allowed", "yes")); + BitSet bitSet = cache.getBitSetProducer(roleQuery).getBitSet(reader.leaves().get(0)); + ... + IOUtils.close(reader, w, dir, indicesCache); +} finally { + ThreadPool.terminate(tp, 10, java.util.concurrent.TimeUnit.SECONDS); +} Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that tp could leak if an exception is thrown before ThreadPool.terminate. Using a try-finally block is the right approach. However, this is a test-only concern with limited production impact.	Low

Suggestions up to commit 864718f

Category	Suggestion	Impact
Possible issue	Fix race condition in closed-listener registration There is a race condition between registeredKeys.add and cacheHelper.addClosedListener. Two threads could both call registeredKeys.add returning true and both attempt to register the closed listener, or one thread could miss the registration. Use a concurrent map (e.g., ConcurrentHashMap.computeIfAbsent) to atomically register the listener exactly once per coreCacheReader. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [145-155] -if (registeredKeys.add(coreCacheReader)) { +registeredKeys.computeIfAbsent(coreCacheReader, k -> { cacheHelper.addClosedListener(this); -} + return Boolean.TRUE; +}); final BitsetCacheKey cacheKey = new BitsetCacheKey(coreCacheReader, query); return cache.computeIfAbsent(cacheKey, key -> { final BitSet bitSet = bitsetFromQuery(query, context); Value value = new Value(bitSet, shardId, listener); listener.onCache(shardId, value.bitset); return value; }).bitset; Suggestion importance[1-10]: 7 __ Why: The registeredKeys.add + cacheHelper.addClosedListener pattern has a TOCTOU race where two threads could both see add return true and register the listener twice, or miss registration. The improved code using computeIfAbsent would fix this, but it requires changing registeredKeys from a Set to a ConcurrentHashMap (covered by suggestion 2), making these two suggestions interdependent.	Medium
	Avoid concurrent modification during stale entry purge cache.keys() returns a live view; invalidating entries while iterating over it can cause ConcurrentModificationException or missed entries depending on the Cache implementation. Collect the keys to invalidate into a separate list first, then invalidate them after the iteration is complete. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [184-198] public void purgeStaleEntries() { if (staleCacheKeys.isEmpty()) { return; } Set<IndexReader.CacheKey> staleSnapshot = new HashSet<>(staleCacheKeys); + List<BitsetCacheKey> toInvalidate = new ArrayList<>(); for (BitsetCacheKey key : cache.keys()) { if (staleSnapshot.contains(key.readerCacheKey)) { - cache.invalidate(key); + toInvalidate.add(key); } + } + for (BitsetCacheKey key : toInvalidate) { + cache.invalidate(key); } staleCacheKeys.removeAll(staleSnapshot); - registeredKeys.removeAll(staleSnapshot); + registeredKeys.keySet().removeAll(staleSnapshot); } Suggestion importance[1-10]: 7 __ Why: Invalidating cache entries while iterating cache.keys() can cause ConcurrentModificationException or missed entries depending on the Cache implementation. Collecting keys to invalidate first and then invalidating them is a safer pattern that avoids this issue.	Medium
	Replace set with concurrent map for atomic listener registration registeredKeys is a Set but needs atomic check-and-add semantics to safely register the closed listener exactly once. Replace it with a ConcurrentHashMap<IndexReader.CacheKey, Boolean> so computeIfAbsent can be used for atomic registration, eliminating the TOCTOU race. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [98-99] private final Set<IndexReader.CacheKey> staleCacheKeys = ConcurrentCollections.newConcurrentSet(); -private final Set<IndexReader.CacheKey> registeredKeys = ConcurrentCollections.newConcurrentSet(); +private final java.util.concurrent.ConcurrentHashMap<IndexReader.CacheKey, Boolean> registeredKeys = new java.util.concurrent.ConcurrentHashMap<>(); Suggestion importance[1-10]: 6 __ Why: Changing registeredKeys to a ConcurrentHashMap is the prerequisite for fixing the race condition identified in suggestion 1. The change is valid but is a supporting change rather than a standalone fix.	Low
General	Document or guard null indicesCache in deprecated path The deprecated no-arg constructor BitsetFilterCache(IndexSettings, Listener) sets indicesCache to null, so any call to getBitSetProducer on such an instance will throw IllegalStateException at runtime. This silently breaks existing callers that use the deprecated constructor. Either document this clearly or provide a fallback (e.g., a local in-memory cache) so the deprecated path remains functional. server/src/main/java/org/opensearch/index/cache/bitset/BitsetFilterCache.java [116-121] public BitSetProducer getBitSetProducer(Query query) { if (indicesCache != null) { return indicesCache.getBitSetProducer(query, listener); } - throw new IllegalStateException("IndicesBitsetFilterCache is not available"); + throw new IllegalStateException( + "IndicesBitsetFilterCache is not available. Use BitsetFilterCache(IndexSettings, IndicesBitsetFilterCache, Listener) constructor." + ); } Suggestion importance[1-10]: 3 __ Why: The improved code only changes the exception message text, which is a minor documentation improvement. The core issue of the deprecated constructor breaking callers is real, but the suggestion doesn't actually fix it — it just improves the error message slightly.	Low

Suggestions up to commit 66619c5

Category	Suggestion	Impact
Possible issue	Avoid concurrent modification during stale entry purge Iterating over cache.keys() while calling cache.invalidate(key) inside the loop can cause a ConcurrentModificationException or skip entries depending on the underlying iterator implementation. Collect the keys to invalidate first, then invalidate them in a separate pass to avoid modifying the collection during iteration. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [184-198] public void purgeStaleEntries() { if (staleCacheKeys.isEmpty()) { return; } Set<IndexReader.CacheKey> staleSnapshot = new HashSet<>(staleCacheKeys); + List<BitsetCacheKey> toInvalidate = new ArrayList<>(); for (BitsetCacheKey key : cache.keys()) { if (staleSnapshot.contains(key.readerCacheKey)) { - cache.invalidate(key); + toInvalidate.add(key); } + } + for (BitsetCacheKey key : toInvalidate) { + cache.invalidate(key); } staleCacheKeys.removeAll(staleSnapshot); registeredKeys.removeAll(staleSnapshot); } Suggestion importance[1-10]: 6 __ Why: Modifying a cache while iterating over its keys can cause issues depending on the underlying Cache implementation. Collecting keys to invalidate first and then invalidating them in a separate pass is a valid defensive improvement that avoids potential ConcurrentModificationException or skipped entries.	Low
	Fix race condition in closed-listener registration There is a race condition between registeredKeys.add and cacheHelper.addClosedListener. Two threads could both call registeredKeys.add concurrently; the set is a ConcurrentSet so only one will return true, but between the check and addClosedListener the reader could close without the listener being registered. Additionally, if computeIfAbsent loads a new entry but the reader closes before addClosedListener is called, the stale entry will never be purged. The listener registration should happen unconditionally before the cache lookup, or use a proper lock/CAS to ensure atomicity. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [145-155] +// Always register the closed listener before inserting into the cache +// to avoid missing close events. if (registeredKeys.add(coreCacheReader)) { cacheHelper.addClosedListener(this); } final BitsetCacheKey cacheKey = new BitsetCacheKey(coreCacheReader, query); return cache.computeIfAbsent(cacheKey, key -> { final BitSet bitSet = bitsetFromQuery(query, context); Value value = new Value(bitSet, shardId, listener); listener.onCache(shardId, value.bitset); return value; }).bitset; Suggestion importance[1-10]: 5 __ Why: The suggestion identifies a potential race condition between registeredKeys.add and cacheHelper.addClosedListener, but the 'improved_code' is essentially identical to the 'existing_code' with only a comment added. The actual fix described (ensuring atomicity) is not implemented in the improved code, making the suggestion's impact minimal.	Low
General	Warn when cache size resolves to unbounded When sizeInBytes is 0 (e.g., the setting resolves to 0 bytes), no weigher is set and the cache becomes unbounded, silently ignoring the configured limit. The condition if (sizeInBytes > 0) should be validated more explicitly, or the setting should enforce a minimum positive value to prevent an accidentally unbounded cache. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [102-113] public IndicesBitsetFilterCache(Settings settings, ThreadPool threadPool) { long sizeInBytes = INDICES_BITSET_FILTER_CACHE_SIZE_SETTING.get(settings).getBytes(); CacheBuilder<BitsetCacheKey, Value> cacheBuilder = CacheBuilder.<BitsetCacheKey, Value>builder().removalListener(this); if (sizeInBytes > 0) { cacheBuilder.setMaximumWeight(sizeInBytes).weigher(new BitsetWeigher()); + } else { + logger.warn( + "Bitset filter cache size is 0 or negative ({}); cache will be unbounded. " + + "Set [{}] to a positive value to enforce a size limit.", + sizeInBytes, + INDICES_BITSET_FILTER_CACHE_SIZE_SETTING.getKey() + ); } this.cache = cacheBuilder.build(); TimeValue cleanInterval = INDICES_BITSET_FILTER_CACHE_CLEAN_INTERVAL_SETTING.get(settings); this.cacheCleaner = new BitsetCacheCleaner(this, threadPool, cleanInterval); threadPool.schedule(cacheCleaner, cleanInterval, ThreadPool.Names.SAME); } Suggestion importance[1-10]: 4 __ Why: Adding a warning log when the cache size is 0 is a reasonable operational improvement to help administrators detect misconfiguration. However, since INDICES_BITSET_FILTER_CACHE_SIZE_SETTING defaults to "5%" of memory, a 0-byte scenario is unlikely in practice, limiting the impact.	Low
	Document breaking behavior of deprecated constructor The deprecated no-arg constructor BitsetFilterCache(IndexSettings, Listener) sets indicesCache to null, so any caller using that constructor and then calling getBitSetProducer will get an IllegalStateException at runtime. Since the deprecated constructor is still public and used in tests, this is a silent breaking change. Consider returning a no-op or stub BitSetProducer instead, or at minimum document this clearly in the deprecated constructor's Javadoc. server/src/main/java/org/opensearch/index/cache/bitset/BitsetFilterCache.java [116-121] public BitSetProducer getBitSetProducer(Query query) { if (indicesCache != null) { return indicesCache.getBitSetProducer(query, listener); } - throw new IllegalStateException("IndicesBitsetFilterCache is not available"); + // Legacy path: no node-level cache available; callers using the deprecated + // constructor will get an empty producer. + throw new IllegalStateException( + "IndicesBitsetFilterCache is not available. Use BitsetFilterCache(IndexSettings, IndicesBitsetFilterCache, Listener) constructor." + ); } Suggestion importance[1-10]: 3 __ Why: The 'improved_code' is functionally identical to the 'existing_code' — it only changes the exception message text. While the concern about the deprecated constructor causing IllegalStateException is valid, the suggestion doesn't actually fix the problem, making its impact low.	Low

Suggestions up to commit 6c5c879

Category	Suggestion	Impact
Possible issue	Fix stale key loss during concurrent purge Stale keys are removed from staleCacheKeys before the cache entries are actually invalidated. If a reader closes and adds to staleCacheKeys between the removeAll and the cache invalidation loop, those keys will be lost and their entries will never be purged. Move staleCacheKeys.removeAll(staleSnapshot) to after the cache invalidation loop to avoid this window. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [184-197] public void purgeStaleEntries() { if (staleCacheKeys.isEmpty()) { return; } Set<IndexReader.CacheKey> staleSnapshot = new HashSet<>(staleCacheKeys); - staleCacheKeys.removeAll(staleSnapshot); - registeredKeys.removeAll(staleSnapshot); for (BitsetCacheKey key : cache.keys()) { if (staleSnapshot.contains(key.readerCacheKey)) { cache.invalidate(key); } } + + staleCacheKeys.removeAll(staleSnapshot); + registeredKeys.removeAll(staleSnapshot); } Suggestion importance[1-10]: 7 __ Why: This is a valid concurrency concern: removing stale keys from staleCacheKeys before invalidating cache entries creates a window where newly-closed readers could have their keys lost. Moving the removeAll calls after the invalidation loop is a straightforward and correct fix.	Medium
	Prevent null bitset cache causing NullPointerException When indicesBitsetFilterCache is null, bitsetFilterCache is set to null and passed to IndexCache. Any subsequent code that calls indexCache.bitsetFilterCache() and then invokes methods on it (e.g., getBitSetProducer) will throw a NullPointerException. The original code always created a BitsetFilterCache; passing null here is a regression that will break nested queries and other features relying on the bitset cache. server/src/main/java/org/opensearch/index/IndexService.java [309-317] -this.bitsetFilterCache = indicesBitsetFilterCache != null - ? new BitsetFilterCache(indexSettings, indicesBitsetFilterCache, new BitsetCacheListener(this)) - : null; +this.bitsetFilterCache = new BitsetFilterCache( + indexSettings, + indicesBitsetFilterCache, + new BitsetCacheListener(this) +); this.warmer = new IndexWarmer( threadPool, indexFieldData, indicesBitsetFilterCache != null ? indicesBitsetFilterCache.createListener(threadPool) : null ); this.indexCache = new IndexCache(indexSettings, queryCache, bitsetFilterCache); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that setting bitsetFilterCache to null when indicesBitsetFilterCache is null could cause NPEs downstream. However, the BitsetFilterCache constructor already handles null indicesCache gracefully (throwing IllegalStateException on getBitSetProducer), so the actual risk depends on usage patterns. The improved code always creates a BitsetFilterCache, which is safer.	Medium
	Fix race condition in closed-listener registration There is a race condition between registeredKeys.add and cacheHelper.addClosedListener. Two threads could both pass registeredKeys.add before either calls addClosedListener, resulting in the listener being registered twice. Use a concurrent map with computeIfAbsent or synchronize the check-then-act to ensure the listener is registered exactly once. server/src/main/java/org/opensearch/indices/IndicesBitsetFilterCache.java [145-155] -if (registeredKeys.add(coreCacheReader)) { +registeredKeys.computeIfAbsent(coreCacheReader, k -> { cacheHelper.addClosedListener(this); -} + return Boolean.TRUE; +}); final BitsetCacheKey cacheKey = new BitsetCacheKey(coreCacheReader, query); return cache.computeIfAbsent(cacheKey, key -> { final BitSet bitSet = bitsetFromQuery(query, context); Value value = new Value(bitSet, shardId, listener); listener.onCache(shardId, value.bitset); return value; }).bitset; Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a TOCTOU race between registeredKeys.add and cacheHelper.addClosedListener. However, registeredKeys is a Set (from ConcurrentCollections.newConcurrentSet()), not a Map, so the proposed computeIfAbsent fix on a Set is not directly applicable without changing the type. The issue is real but the improved_code doesn't compile as-is.	Low
General	Avoid runtime exception from deprecated constructor usage The deprecated no-arg constructor BitsetFilterCache(IndexSettings, Listener) sets indicesCache to null, so any caller using the old constructor and then calling getBitSetProducer will get an IllegalStateException at runtime instead of a graceful degradation. Since the deprecated constructor is still public and used in tests, this is a silent behavioral break. Consider logging a warning or providing a fallback rather than throwing unconditionally. server/src/main/java/org/opensearch/index/cache/bitset/BitsetFilterCache.java [112-117] public BitSetProducer getBitSetProducer(Query query) { if (indicesCache != null) { return indicesCache.getBitSetProducer(query, listener); } - throw new IllegalStateException("IndicesBitsetFilterCache is not available"); + logger.warn("IndicesBitsetFilterCache is not available; bitset producer will return null bitsets"); + return context -> null; } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about the deprecated constructor leading to IllegalStateException, but returning context -> null as a fallback could silently break nested queries. The IllegalStateException is arguably more appropriate as it makes the misconfiguration explicit rather than hiding it.	Low

[github-actions]

❌ Gradle check result for a77d8b9: 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 7db9af0

[github-actions]

Persistent review updated to latest commit dd3178c

[github-actions]

❌ Gradle check result for dd3178c: 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 5ceb33c

[github-actions]

Persistent review updated to latest commit 21f368a

[github-actions]

Persistent review updated to latest commit 6c5c879

[github-actions]

✅ Gradle check result for 6c5c879: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.31%. Comparing base (c93eb90) to head (3af0257).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/main/java/org/opensearch/index/IndexModule.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21179      +/-   ##
============================================
+ Coverage     73.22%   73.31%   +0.09%     
- Complexity    73325    73541     +216     
============================================
  Files          5910     5911       +1     
  Lines        334422   334789     +367     
  Branches      48207    48231      +24     
============================================
+ Hits         244880   245464     +584     
+ Misses        69964    69738     -226     
- Partials      19578    19587       +9     

☔ View full report in Codecov by Sentry.
📢 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.

[jainankitk]

I noticed that code reviewer added some concerns here - #21179 (comment), in addition to my comments. Also, I am wondering if it is worthwhile breaking this down into 2 PRs (3 might be too much)?

1. Sub-PR theme: Introduce IndicesBitsetFilterCache node-level cache with size limit
2. Sub-PR theme: Wire IndicesBitsetFilterCache into IndexService and IndicesService + Sub-PR theme: Update test infrastructure to use IndicesBitsetFilterCache

[github-actions]

Persistent review updated to latest commit 66619c5

[github-actions]

❌ Gradle check result for 66619c5: 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 864718f

[github-actions]

❌ Gradle check result for 864718f: 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 73f28fb

[github-actions]

✅ Gradle check result for 73f28fb: SUCCESS

[github-actions]

❌ Gradle check result for 94a1f9f: 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]

❕ Gradle check result for 3af0257: 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.