Fix flaky tests for issue 19725

[liuguoqingfz]

Description

Fix flaky tests when on the first failure callback, prev is null (because the AtomicReference starts empty), and Throwable.addSuppressed(null) throws: “Cannot suppress a null exception.” That exception then becomes an uncaught exception on the test thread.

The failure in testCircuitBreakerTriggersBeforeBatchedReduce() is also coming from testProgressListenerExceptionsAreCaught()

Related Issues

Resolves #19725

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.

Summary by CodeRabbit

• Bug Fixes
  • Fixed null-handling logic in search result accumulation to prevent potential exceptions and ensure proper error suppression chaining.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

A null-guard fix in QueryPhaseResultConsumerTests prevents NullPointerException when the accumulator's first invocation receives a null current exception. The code now safely returns the previous exception and conditionally chains suppressed exceptions only when both are present.

Changes

Cohort / File(s)	Change Summary
Null-safety fix server/src/test/java/org/opensearch/action/search/QueryPhaseResultConsumerTests.java	Added null guards to accumulator logic: returns prev when curr is null, and only calls curr.addSuppressed(prev) when prev is not null. Prevents NullPointerException on first invocation and preserves exception chain suppression.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

• Small, localized fix addressing null safety in exception accumulation logic
• Single file change with straightforward guard clause additions
• No API or behavioral changes to public interfaces

Suggested labels

bug, flaky-test

Suggested reviewers

• sachinpkale
• andrross
• dbwiddis
• msfroh
• shwetathareja
• cwperks
• mch2

Poem

🐰 A null was lurking in the test's dark corner,
Causing NPEs like an unwelcome mourner,
With guards now in place, exceptions behave,
The accumulator's safe—flaky tests we'll save!
✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing flaky tests for issue 19725, which directly aligns with the file-level summary about NullPointerException fixes in test code.
Description check	✅ Passed	The PR description includes the required sections: description of what the fix achieves (guarding against null exceptions from addSuppressed), related issues (Resolves #19725), and checklist. All essential elements are present.
Linked Issues check	✅ Passed	The code changes directly address the root cause identified in issue #19725: preventing NullPointerException when curr is null by guarding calls to addSuppressed(prev), which resolves the flaky test failures in QueryPhaseResultConsumerTests.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the NullPointerException issue in QueryPhaseResultConsumerTests. No unrelated modifications or out-of-scope changes are present in the changeset.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e798353 and 15d2f1a.

📒 Files selected for processing (1)

• server/src/test/java/org/opensearch/action/search/QueryPhaseResultConsumerTests.java (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)

• GitHub Check: gradle-check
• GitHub Check: assemble (25, windows-latest)
• GitHub Check: assemble (21, ubuntu-latest)
• GitHub Check: assemble (21, windows-latest)
• GitHub Check: assemble (25, ubuntu-24.04-arm)
• GitHub Check: assemble (21, ubuntu-24.04-arm)
• GitHub Check: assemble (25, ubuntu-latest)
• GitHub Check: detect-breaking-change
• GitHub Check: precommit (21, ubuntu-24.04-arm)
• GitHub Check: precommit (21, windows-2025, true)
• GitHub Check: precommit (21, windows-latest)
• GitHub Check: precommit (25, windows-latest)
• GitHub Check: precommit (25, macos-15)
• GitHub Check: precommit (25, macos-15-intel)
• GitHub Check: precommit (21, macos-15-intel)
• GitHub Check: precommit (25, ubuntu-24.04-arm)
• GitHub Check: precommit (21, ubuntu-latest)
• GitHub Check: precommit (21, macos-15)
• GitHub Check: precommit (25, ubuntu-latest)
• GitHub Check: Analyze (java)

🔇 Additional comments (1)

server/src/test/java/org/opensearch/action/search/QueryPhaseResultConsumerTests.java (1)

135-144: LGTM! Null guards correctly fix the flaky test issue.

The fix properly addresses the root cause described in issue #19725. The null checks prevent the NPE that occurred when addSuppressed(null) was called on the first invocation (when prev is null). The logic correctly handles all cases:

• First invocation: prev is null, returns curr without chaining
• Subsequent invocations: chains exceptions via addSuppressed when both are non-null
• Defensive case: if curr is null, preserves prev

The comment on line 136 clearly documents the issue.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

✅ Gradle check result for 15d2f1a: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.25%. Comparing base (930ae74) to head (15d2f1a).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20259      +/-   ##
============================================
+ Coverage     73.20%   73.25%   +0.05%     
+ Complexity    71745    71742       -3     
============================================
  Files          5795     5795              
  Lines        328304   328304              
  Branches      47283    47283              
============================================
+ Hits         240334   240502     +168     
+ Misses        68663    68504     -159     
+ Partials      19307    19298       -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.