Remove redundant synchronized from ShardCoreKeyMap#getShardId

[uchiha-asha]

Description

Against ShardCoreKeyMap#getShardId, the synchronized modifier is redundant: the only state it reads, coreKeyToShard, is a ConcurrentHashMap.

This is not a cold path. The map is node-global (one per IndicesQueryCache) and the getter is reached once per (query, segment) on every query-cache lookup, via IndicesQueryCache.OpenSearchLRUQueryCache#onHit / #onMiss, so the monitor serialises search threads across the whole node.

Why this is safe

• add() already performs its fast-path containsKey read of coreKeyToShard outside the monitor, and has done since the class was introduced. Lock-free reads of this map are not new behaviour.
• The writer publishes coreKeyToShard.put(...) as the last operation of its critical section, deliberately and with an in-source comment saying so, after the closed-listener registration. A reader that observes a key is therefore guaranteed to see a fully-registered entry.
• The closed listener removes from coreKeyToShard first, so a lock-free reader never sees an entry whose reader has already been torn down.
• getShardId never touches indexToCoreKey, which remains HashMap-backed and monitor-guarded in every method that reads or mutates it. getCoreKeysForIndex, size and assertSize are unchanged and still synchronized.

Behavioural note: a reader racing an in-flight add() may now observe null where it would previously have blocked and then seen the entry. That is already the method's documented contract — it returns null for any segment it does not track.

Testing: :server:compileJava, :benchmarks:compileJava, :server:spotlessJavaCheck, :benchmarks:spotlessJavaCheck and ShardCoreKeyMapTests (3/3) all pass locally.

Ported from elastic/elasticsearch#156902.

Related Issues

N/A — no tracking issue. Upstream equivalent: elastic/elasticsearch#156902

Check List

• Functionality includes testing. (Existing ShardCoreKeyMapTests covers the changed class and passes; a JMH benchmark for the affected path is added in this PR.)
• API changes companion pull request created, if applicable. (N/A — no API change.)
• Public documentation issue/PR created, if applicable. (N/A — internal implementation detail.)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit d8f7725.

⛔ Hard block: Issues at Medium severity or above will block this PR from merging.

Path	Line	Severity	Description
server/src/main/java/org/opensearch/common/lucene/ShardCoreKeyMap.java	146	medium	Removal of 'synchronized' from getShardId() changes the thread-safety contract of this method. If coreKeyToShard is a plain HashMap (not ConcurrentHashMap), concurrent reads interleaved with writes from add() or the close listener can cause data races, returning stale/null values or triggering undefined behavior. While the accompanying benchmark strongly suggests this is an intentional performance optimization under evaluation rather than malicious sabotage, the change deliberately weakens a safety guarantee on a node-global, concurrently-accessed data structure. Maintainers should verify that coreKeyToShard has been migrated to a thread-safe map type or that the access pattern guarantees no concurrent mutation before merging.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 1 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 4863222)

Here are some key observations to aid the review process:

🧪 No relevant 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 4863222
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Unsynchronized read of non-thread-safe map is unsafe Removing synchronized from getShardId while other methods (add, and likely remove) still mutate coreKeyToShard under synchronization creates a data race, since HashMap is not thread-safe for concurrent reads during writes and can result in corrupted reads or infinite loops. Either change coreKeyToShard to a ConcurrentHashMap (or a volatile immutable snapshot) to make lock-free reads safe, or keep the synchronized modifier. server/src/main/java/org/opensearch/common/lucene/ShardCoreKeyMap.java [146] -public ShardId getShardId(Object coreKey) { +public synchronized ShardId getShardId(Object coreKey) { return coreKeyToShard.get(coreKey); } Suggestion importance[1-10]: 9 __ Why: This is a critical concern: removing synchronized from getShardId while add still mutates the underlying HashMap under synchronization creates a data race that can lead to corrupted reads or infinite loops. The suggestion correctly identifies a significant thread-safety issue.	High

---

Previous suggestions

Suggestions up to commit e8819cb

Category	Suggestion	Impact
Possible issue	Unsafe concurrent read on non-thread-safe map Removing synchronized exposes reads to a HashMap that is mutated under the class monitor in add(). Concurrent reads against a non-thread-safe HashMap while writes occur can cause infinite loops, corrupted results, or NullPointerException. Either change coreKeyToShard to a ConcurrentHashMap (or volatile reference to an immutable copy) or keep the synchronized modifier to preserve the happens-before relationship with writers. server/src/main/java/org/opensearch/common/lucene/ShardCoreKeyMap.java [146-147] -public ShardId getShardId(Object coreKey) { +public synchronized ShardId getShardId(Object coreKey) { return coreKeyToShard.get(coreKey); } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a serious concurrency issue: removing synchronized from getShardId while add() mutates a non-thread-safe HashMap under the class monitor can lead to data corruption, infinite loops, or NPEs. This is a critical correctness concern that warrants attention, though the fix may require broader design changes (e.g., ConcurrentHashMap) rather than simply reverting.	High

[github-actions]

❌ Gradle check result for e8819cb: 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 4863222

[github-actions]

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

[iprithv]

since the safety of dropping synchronized here depends on CacheKey using identity-based equals/hashCode (which it does, Lucene documents this), would it make sense to change the parameter type from Object to IndexReader.CacheKey?
that way the compiler enforces that only CacheKey instances get passed in, instead of relying on callers to do the right thing. right now any Object with overridden equals/hashCode could sneak in and break the assumptions.

[uchiha-asha]

Hi, Thanks for suggestion. Lucene’s LRUQueryCache callbacks expose readerCoreKey as Object, so changing this parameter to CacheKey would require casts. Also, removing synchronized is safe because the backing map is a ConcurrentHashMap; identity semantics only affect key matching. I think we can retain Object here.

[uchiha-asha]

Hi @iprithv, can you please review