Fix cache stats race during dimension removal

[GovindBalaji-S-Glean]

Description

DefaultCacheStatsHolder.removeDimensions() takes a snapshot of a cache entry before subtracting its stats from ancestor nodes. If a concurrent cache-removal callback decrements the same entry first, the stale snapshot is subtracted again, causing aggregate counters to become negative and _nodes/stats serialization to fail. Same can happen during reset().

This change serializes dimension removal with counter updates and adds a deterministic regression test for the race. Tests to reproduce are in the PR.

Related Issues

N/A. Plmk if I should open an issue for this.

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 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Lock acquisition inside try In internalIncrement, lock.writeLock().lock() is called inside the try block. If lock() throws (unlikely for ReentrantReadWriteLock, but a general anti-pattern), the finally block will call unlock() on a lock that was never acquired, throwing IllegalMonitorStateException and masking the original exception. Move the lock() call before the try block, as is done correctly in removeDimensions and reset. if (createNodesIfAbsent) { try { lock.writeLock().lock(); internalIncrementHelper(dimensionValues, statsRoot, 0, adder, true); } finally { lock.writeLock().unlock(); } Concurrency regression for increments Previously, increments on existing nodes were lock-free and node-creation used a mutex. Now every increment acquires the read lock of a ReentrantReadWriteLock, which contends on shared internal state and can noticeably reduce throughput under heavy concurrent cache access (the hot path for hits/misses). Consider whether the read lock is strictly necessary for increments on existing leaf nodes, since counter updates on Node fields appear to already be thread-safe. Uncertain: without seeing the full Node implementation, this may be an intentional trade-off to synchronize with tree structure changes. protected void internalIncrement(List<String> dimensionValues, Consumer<Node> adder, boolean createNodesIfAbsent) { assert dimensionValues.size() == dimensionNames.size(); lock.readLock().lock(); try { // First try to increment without creating nodes. if (internalIncrementHelper(dimensionValues, statsRoot, 0, adder, false)) { return; } } finally { lock.readLock().unlock(); } // If we failed to increment because nodes had to be created, obtain the write lock and run again. if (createNodesIfAbsent) { try { lock.writeLock().lock(); internalIncrementHelper(dimensionValues, statsRoot, 0, adder, true); } finally { lock.writeLock().unlock(); } }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Acquire lock outside try block Move lock.writeLock().lock() outside the try block. If lock() throws (e.g., due to thread interruption in some lock implementations or subclass overrides), the finally block would attempt to unlock a lock that was never acquired, causing an IllegalMonitorStateException that masks the original exception. This is the standard idiom for lock usage. server/src/main/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolder.java [135-141] if (createNodesIfAbsent) { + lock.writeLock().lock(); try { - lock.writeLock().lock(); internalIncrementHelper(dimensionValues, statsRoot, 0, adder, true); } finally { lock.writeLock().unlock(); } } Suggestion importance[1-10]: 6 __ Why: Valid standard idiom concern: acquiring the lock inside the try block risks unlocking an unheld lock if lock() throws. While ReentrantReadWriteLock.writeLock().lock() doesn't typically throw, following the standard idiom improves correctness and consistency.	Low

[github-actions]

✅ Gradle check result for 813dee3: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.44%. Comparing base (e8e618b) to head (813dee3).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22647      +/-   ##
============================================
+ Coverage     71.42%   71.44%   +0.01%     
+ Complexity    76853    76815      -38     
============================================
  Files          6148     6148              
  Lines        358003   358008       +5     
  Branches      52179    52180       +1     
============================================
+ Hits         255718   255788      +70     
+ Misses        81972    81867     -105     
- Partials      20313    20353      +40     

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