Delete cache entries that are removed before they are promoted

[jwils]

Description

A cache segment maps each key to a CompletableFuture, and computeIfAbsent installs that future in the segment map before the value is loaded, so there is a window in which a key is in the segment map while its entry is not yet linked into the LRU list.

Any keyed removal can land in that window. It removes the future from the map, then calls delete(), whose unlink() finds state == NEW, returns false and drops the removal notification. The loading thread then promotes the entry, linking it into the LRU list even though its key is already gone from the map. What is left is an entry that

• is counted in count()/weight(),
• is unreachable via get(),
• can no longer be invalidated by key, because invalidate() looks in the segment map, and
• never reaches the removal listener.

For the node-level fielddata cache that means the circuit breaker is never credited back for the entry, and since indices.fielddata.cache.size is unbounded by default there is no weight-based eviction path to reclaim it either.

This is a gap in Cache rather than in any one caller -- the public invalidate(key, value) has always been able to hit it. It became reachable from the fielddata cache cleanup sweep in #22499, which switched the sweep from iterator.remove() to exact-key invalidation; iterator.remove() only removes entries reached through the LRU list, so it never saw an unpromoted one.

delete() now handles the not-yet-linked state: mark the entry DELETED so the pending promote() hits its existing case DELETED no-op, and fire the removal notification so listeners can account for the value the loader produced. There is no count/weight adjustment to make, because the entry was never linked. Every other delete() caller passes an entry reached through the LRU list, which is always EXISTING, so nothing else changes behaviour.

Verification

testInvalidateBeforePromoteLeavesUnreclaimableEntry forces the interleaving rather than racing for it. The weigher runs inside linkAtHead() while the LRU lock is held, which gives the test thread a point where it holds that lock, so the loading thread cannot promote its entry until the test thread has finished invalidating it. On main it fails deterministically:

TARGET should not be left in the LRU list expected:<[stall]> but was:<[target, stall]>

and with the fix the entry is reclaimed, the listener is notified once with INVALIDATED, and count()/weight() are correct.

To confirm the interleaving is reachable without that scaffolding, I also ran a version that races two threads for the LRU lock naturally (a third thread stalled in the weigher to hold the lock, the loader parked on it, and the invalidator spinning on-CPU so it can barge ahead of the parked loader). Over 300 attempts against main, invalidate() beat promote() 24 times and all 24 produced a leaked entry; with the fix the same run hit the interleaving 21 times and leaked none. That test is not included here, since as a permanent regression test it is timing-dependent and much harder to read than the deterministic one.

testInvalidateBlocksOnInFlightLoad covers the surrounding behaviour that made this reachable: keysSnapshot() returns keys whose load is still in flight, and invalidate() on such a key parks the caller until the load completes, because the invalidation consumer calls future.get() on the incomplete future with no timeout. That blocking is a separate concern -- the fielddata sweep can stall on an in-flight load while holding its monitor -- and fixing it means making invalidate() asynchronous for in-flight keys, which is a semantics change worth deciding on its own. This PR only fixes the leak; the test documents the current behaviour.

common.cache.*, IndicesFieldDataCacheTests and IndexFieldDataServiceTests all pass (83 tests), and the new tests pass over 25 iterations.

Related Issues

Follow-up to #22499, from this review comment by @sgup432 (thank you).

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 🔍

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 ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Assert NEW entries are unlinked before claiming If delete() is called for an EVICTED entry that is still in State.NEW (evicted before promotion), firing a removal notification with reason EVICTED while the caller in evictEntry also expects eviction semantics is fine, but the removed = true path skips any count/weight bookkeeping that unlink would normally do. Verify that this branch is only reachable for entries that were never linked; otherwise the NEW-check should also confirm the entry has no prev/next links to avoid silently losing accounting on a linked entry whose state was somehow still NEW. server/src/main/java/org/opensearch/common/cache/Cache.java [959-971] final boolean removed; if (entry.state == State.NEW) { - // The entry was removed from the segment map before the thread that loaded it acquired the LRU lock to - // promote it. Claim it by marking it DELETED so that the pending promote() becomes a no-op: otherwise the - // entry gets linked into the LRU list while absent from the segment map, where it is counted in - // count()/weight() but is unreachable via get() and can no longer be invalidated by key. There is no - // count/weight adjustment to make here because the entry was never linked, but the removal notification - // must still fire so that listeners can account for the value the loader produced. + assert entry.prev == null && entry.next == null : "NEW entry unexpectedly linked into LRU"; entry.state = State.DELETED; removed = true; } else { removed = unlink(entry); } Suggestion importance[1-10]: 4 __ Why: Adding an assertion for the invariant that NEW entries are not linked in the LRU list is a reasonable defensive check that could aid debugging, but it's a minor improvement since the invariant is already implied by the state machine.	Low