[Bugfix] Fixes IRC NPE bug for timed-out cacheable queries

[peteralfonsi]

Description

When a cacheable query times out, the IRC lets it enter the cache and then invalidates it in the same search thread. In 2.13, invalidation was changed to require a readerCacheKeyId string. invalidate() only got this if the DirectoryReader was an OpenSearchDirectoryReader and otherwise it was left null. When the reader was an ExitableDirectoryReader, this caused an NPE, which was returned to the user. It also means the timed-out query response incorrectly remained in the cache and could be returned the next time the user made the query.

Fix this by getting readerCacheKeyId from reader.getReaderCacheHelper() without checking which class reader is. This is safe, as it's the same method used in IndicesRequestCache.getOrCompute(). Now the user correctly sees the timed-out response and it is invalidated from the cache.

Tested in an IT and manually with the java debugger.

Related Issues

Resolves #15175

Check List

• Functionality includes testing.
  - [N/A] API changes companion pull request created, if applicable.
  - [N/A] 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]

✅ Gradle check result for 43c0ef4: SUCCESS

[codecov]

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 71.35%. Comparing base (dac6460) to head (a8f2521).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...va/org/opensearch/indices/IndicesRequestCache.java	0.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #15327      +/-   ##
============================================
- Coverage     71.83%   71.35%   -0.49%     
+ Complexity    63174    62709     -465     
============================================
  Files          5224     5224              
  Lines        296078   296077       -1     
  Branches      42763    42762       -1     
============================================
- Hits         212702   211272    -1430     
- Misses        65902    67312    +1410     
- Partials      17474    17493      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

❕ Gradle check result for a8f2521: UNSTABLE

• TEST FAILURES:

      2 org.opensearch.common.util.concurrent.QueueResizableOpenSearchThreadPoolExecutorTests.classMethod
      1 org.opensearch.common.util.concurrent.QueueResizableOpenSearchThreadPoolExecutorTests.testResizeQueueDown

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[peteralfonsi]

Seems codecov thinks lines 314-316 of IndicesRequestCache (the invalidate() method) aren't covered... this isn't the case though, I can verify with the java debugger that the new IT hits it. The test would also have failed if that code didn't run, but it passed in the gradle check. @sgup432 @jainankitk do you know why this might be, or if we can override this check?

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-15327-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 beda6164c09f39ceb4dd832e3debdbcb7f7c8f3e
# Push it to GitHub
git push --set-upstream origin backport/backport-15327-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-15327-to-2.x.

[jainankitk]

@peteralfonsi - Can you create backport PR for this?

[peteralfonsi]

Assuming I should also backport to 2.13-2.16 branches right?

[jainankitk]

Assuming I should also backport to 2.13-2.16 branches right?

Depends how critical the bug is. To me, it seems the NPE is for timed-out queries?

[sgup432]

I think we can just backport this to 2.x branch

[dblock]

We only make patch releases for security fixes so backport to 2.x only.

[jainankitk]

We only make patch releases for security fixes so backport to 2.x only.

Thanks @dblock for further clarification!