Fix a flaky test for issue 20040

[liuguoqingfz]

Description

Fix a test where there is almost always a REST client resource leak in the test: the test calls getRestClient().performRequest(...) many times but never close the returned Response objects and it also don’t close the Response embedded in ResponseException. The RestCancellableNodeClient tracks open HTTP channels, leaving even one Response unclosed can leave one channel “still being tracked” during cluster cleanup.

Related Issues

Resolves #20040

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

• Tests
  • Improved resource management in smoke tests with enhanced cleanup procedures for HTTP responses, ensuring proper handling of test fixtures and preventing resource leaks.

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

[coderabbitai]

Walkthrough

A helper method consumeEntity(Response) was added to properly drain and close HTTP response entities in tests, with defensive null checks and error handling. Test methods were updated to wrap REST calls in try-finally blocks using this helper to ensure responses are always consumed and cleaned up, addressing resource leaks causing test flakiness.

Changes

Cohort / File(s)	Summary
Resource cleanup helper and test fixes qa/smoke-test-http/src/test/java/org/opensearch/http/ShardIndexingPressureRestIT.java	Added consumeEntity(Response) helper method to drain and safely close HTTP response entities. Updated test methods to wrap REST calls in try-finally blocks using the helper and added try-with-resources for response content parsing to ensure proper resource cleanup and prevent stream leaks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Verify that all REST response objects are properly consumed using the new helper method
• Confirm try-finally and try-with-resources blocks are correctly implemented across all test methods
• Check that the consumeEntity helper handles edge cases (null responses, null entity) correctly
• Ensure no additional response objects are created without corresponding cleanup

Suggested labels

bug, flaky-test

Suggested reviewers

• dbwiddis
• cwperks
• sachinpkale
• shwetathareja
• msfroh
• kotwanikunal

Poem

🐰 Response streams flowing, cleanup's a must,

With finally blocks, we rebuild the trust,

No leaks shall haunt tests that once did fail,

Resources consumed—our fix won't pale! 🌿✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% 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 'Fix a flaky test for issue 20040' clearly describes the main change—fixing a flaky test related to issue 20040, which aligns with the PR objectives.
Description check	✅ Passed	The PR description fully explains the problem (REST client resource leak in tests), the solution (closing Response objects), and references the related issue #20040, meeting template requirements.
Linked Issues check	✅ Passed	The code changes directly address the flaky test issue by adding proper resource cleanup for Response objects, which resolves the REST client resource leak documented in issue #20040.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the flaky test in ShardIndexingPressureRestIT by adding a helper method and wrapping REST calls with proper resource cleanup, directly addressing issue #20040.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

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

---

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

qa/smoke-test-http/src/test/java/org/opensearch/http/ShardIndexingPressureRestIT.java (1)

367-368: Apply try-finally pattern for consistency.

While functionally safe here, wrapping the final response handling in a try-finally block would maintain the consistent resource management pattern established throughout the test and provide defensive cleanup.

Apply this diff for consistency:

-        final Response resetResponse = getRestClient().performRequest(updateSettingRequest);
-        consumeEntity(resetResponse);
+        final Response resetResponse = getRestClient().performRequest(updateSettingRequest);
+        try {
+            assertThat(resetResponse.getStatusLine().getStatusCode(), equalTo(OK.getStatus()));
+        } finally {
+            consumeEntity(resetResponse);
+        }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d7f787 and 9ed5106.

📒 Files selected for processing (1)

• qa/smoke-test-http/src/test/java/org/opensearch/http/ShardIndexingPressureRestIT.java (7 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-12-02T22:44:14.799Z

Learnt from: prudhvigodithi
Repo: opensearch-project/OpenSearch PR: 20112
File: server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java:73-81
Timestamp: 2025-12-02T22:44:14.799Z
Learning: In OpenSearch integration tests extending OpenSearchIntegTestCase, using `LuceneTestCase.SuppressCodecs("*")` triggers special handling that selects a random production codec from the CODECS array, while `SuppressCodecs("Asserting")` or other specific codec suppressions still allow Lucene's default codec randomization which may include the asserting codec. Use `SuppressCodecs("*")` when you need to completely avoid asserting codecs (e.g., for cross-thread StoredFieldsReader usage) while maintaining production codec test coverage.

Applied to files:

• qa/smoke-test-http/src/test/java/org/opensearch/http/ShardIndexingPressureRestIT.java

⏰ 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: precommit (25, ubuntu-24.04-arm)
• GitHub Check: precommit (21, windows-2025, true)
• GitHub Check: precommit (21, ubuntu-latest)
• GitHub Check: precommit (25, macos-15-intel)
• GitHub Check: precommit (21, windows-latest)
• GitHub Check: precommit (21, ubuntu-24.04-arm)
• GitHub Check: precommit (21, macos-15-intel)
• GitHub Check: precommit (21, macos-15)
• GitHub Check: precommit (25, macos-15)
• GitHub Check: precommit (25, ubuntu-latest)
• GitHub Check: precommit (25, windows-latest)
• GitHub Check: assemble (21, windows-latest)
• GitHub Check: assemble (25, ubuntu-24.04-arm)
• GitHub Check: assemble (21, ubuntu-latest)
• GitHub Check: assemble (25, ubuntu-latest)
• GitHub Check: assemble (21, ubuntu-24.04-arm)
• GitHub Check: Analyze (java)
• GitHub Check: assemble (25, windows-latest)
• GitHub Check: detect-breaking-change

🔇 Additional comments (3)

qa/smoke-test-http/src/test/java/org/opensearch/http/ShardIndexingPressureRestIT.java (3)

19-19: LGTM: Import added for new helper method.

The InputStream import is required for the resource management logic in the new consumeEntity helper method.

---

51-66: Excellent resource cleanup implementation.

The helper method correctly addresses the root cause of the flaky test by ensuring HTTP response entities are fully consumed. The defensive null checks and best-effort exception handling make this robust for test cleanup scenarios.

---

74-316: Consistent and correct resource management pattern applied throughout.

All REST responses are now properly wrapped in try-finally blocks with the consumeEntity helper, ensuring HTTP channels are released even when assertions fail. The combination of try-with-resources for content parsing and finally-block cleanup is particularly well-structured.

[github-actions]

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

[andrross]

@liuguoqingfz If I understand correctly, this test failure (and the other "RestIT" failures) all started around the same time after #19985 was merged. I believe this exposed a latent bug with the reactor netty implementation, which was fixed by #20106. I don't see a failure for this test after #20106 was merged. I believe it is okay to leave the response objects unclosed because the test framework will ensure the underlying client instances get closed and clean up any remaining http channels.