Fix Flaky Test Report for Netty4Http3IT

[reta]

Description

The nature of flakyness is understood: under normal conditions, the HTTP/1.1, HTTP/2 and HTTP/3 transports are bound to the same port (but different protocol). At very rare occasion, the HTTP/1.1, HTTP/2 port selection could run into conflict and under AbstractHttpServerTransport::bindAddress algorithm, the next free port is going to be taken from the range. HTTP/3 however may not conflict and could pick the first port from the range.

Related Issues

Closes #20654

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

🔀 Multiple PR themes Sub-PR theme: Expose transports() accessor in Netty4CompositeHttpServerTransport Relevant files: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4CompositeHttpServerTransport.java Sub-PR theme: Fix flaky test by selecting transport address based on protocol type Relevant files: modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http3IT.java

⚡ Recommended focus areas for review Swapped Alt-Svc Values The Alt-Svc header assertions appear to have the expected values swapped. When using http3() client (HTTP/3), the tuple is ("h2=", HttpVersion.HTTP_3_0) and the assertion checks for "h2=" in the Alt-Svc header. When using https() client (HTTP/2), the tuple is ("h3=", HttpVersion.HTTP_2_0) and checks for "h3=". This seems counterintuitive: an HTTP/3 client connecting should advertise "h3=" in Alt-Svc (advertising HTTP/3 availability), not "h2=". Please verify that the Alt-Svc string values are correctly assigned to each protocol client. final Tuple<Netty4HttpClient, Tuple<String, HttpVersion>> client = randomFrom( Tuple.tuple(Netty4HttpClient.http3().withLogger(logger), Tuple.tuple("h2=", HttpVersion.HTTP_3_0)), Tuple.tuple(Netty4HttpClient.https().withLogger(logger), Tuple.tuple("h3=", HttpVersion.HTTP_2_0)) ); Package-Private Exposure The transports() method is package-private, which is acceptable for test access within the same package. However, it directly exposes the internal transports array, allowing callers to potentially mutate it. Consider returning a copy or an unmodifiable view to prevent accidental modification. AbstractHttpServerTransport[] transports() { return transports; } Missing Import The import for AbstractHttpServerTransport is added but it is only used inside the new private helper method randomFrom. Verify that Netty4Http3ServerTransport and Netty4HttpServerTransport are also properly imported, as they are referenced in the helper method but not visible in the diff's import section. final AbstractHttpServerTransport httpServerTransport = Arrays.stream(transport.transports()).filter(t -> { if (protocol == HttpVersion.HTTP_3_0) { return t instanceof Netty4Http3ServerTransport; } else { return t instanceof Netty4HttpServerTransport; } }).findAny().orElseThrow();

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix swapped Alt-Svc header labels The HTTP version labels appear to be swapped. http3() client should advertise "h3=" in the Alt-Svc header (not "h2="), and https() (HTTP/2) client should advertise "h2=" (not "h3="). This mismatch would cause the Alt-Svc header assertion to fail. modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http3IT.java [91-94] final Tuple<Netty4HttpClient, Tuple<String, HttpVersion>> client = randomFrom( - Tuple.tuple(Netty4HttpClient.http3().withLogger(logger), Tuple.tuple("h2=", HttpVersion.HTTP_3_0)), - Tuple.tuple(Netty4HttpClient.https().withLogger(logger), Tuple.tuple("h3=", HttpVersion.HTTP_2_0)) + Tuple.tuple(Netty4HttpClient.http3().withLogger(logger), Tuple.tuple("h3=", HttpVersion.HTTP_3_0)), + Tuple.tuple(Netty4HttpClient.https().withLogger(logger), Tuple.tuple("h2=", HttpVersion.HTTP_2_0)) ); Suggestion importance[1-10]: 8 __ Why: The Alt-Svc labels appear swapped: http3() client connects via HTTP/3 and should check for "h3=", while https() (HTTP/2) should check for "h2=". This would cause the containsString assertions to fail in both test methods.	Medium
General	Return defensive copy of internal array The method returns a direct reference to the internal transports array, which allows callers to mutate the array contents. Return a defensive copy to preserve encapsulation and prevent unintended modifications. modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4CompositeHttpServerTransport.java [76-78] AbstractHttpServerTransport[] transports() { - return transports; + return Arrays.copyOf(transports, transports.length); } Suggestion importance[1-10]: 4 __ Why: Returning a defensive copy of transports is a good encapsulation practice, but since this is a package-private method used only in tests, the risk of unintended mutation is low, making this a minor improvement.	Low

[reta]

@andrross would appreciate a look, this time the fix is for real (I was able to find the cause, reproduce and fix), thanks!

[github-actions]

✅ Gradle check result for d758213: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.36%. Comparing base (436e4a6) to head (d758213).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...ttp/netty4/Netty4CompositeHttpServerTransport.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21360      +/-   ##
============================================
+ Coverage     73.34%   73.36%   +0.01%     
- Complexity    74223    74287      +64     
============================================
  Files          5958     5958              
  Lines        337309   337350      +41     
  Branches      48664    48687      +23     
============================================
+ Hits         247408   247492      +84     
+ Misses        70188    70172      -16     
+ Partials      19713    19686      -27     

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

[cwperks]

TY for the fix @reta! Was there a reproducible seed that always produced test failure?

[reta]

TY for the fix @reta! Was there a reproducible seed that always produced test failure?

Thanks @cwperks , sadly seed is not reproducible: in previous attempts we added some more logs in key places, it helped to understand the issues. This is really coming from very rare port conflicts, either because of other builds on the same agent or other processes.