Send a failure response when the REST response body cannot be serialized

[asimmahmood1]

Summary

Adds HTTP-level integration test coverage for the dropped-failure-response regression in #22311, and carries the fix from #22356 so the tests are green.

Commits 1-3 are @venkateshwaracholan's fix from #22356, unchanged (authorship preserved). Commit 4 is the new integration test. Raised as a separate PR because the test needs the fix to pass; happy to have it folded into #22356 instead if maintainers prefer.

Fixes #22311.

---

Why an integration test

The unit tests in #22356 verify the new guard helper and that close() no longer throws, but none of them exercise the customer-visible symptom: zero bytes flushed, no status line, connection held open until the load balancer times out. That symptom only appears once a real HTTP channel is involved:

1. RestController.ResourceHandlingHttpChannel#sendResponse closes the channel, then delegates.
2. DefaultRestChannel#sendResponse resolves RestResponse#content() as its last step before writing to the wire. If that throws, nothing is written.
3. RestActionListener#onFailure then tries to send the error over the same channel. close() threw IllegalStateException("Channel is already closed"), the error was swallowed and logged as failed to send failure response, and the client got nothing.

Step 3 only reaches that state when the failure happens in an asynchronous listener callback, after prepareRequest has returned — RestController.dispatchRequest's catch-all is no longer on the stack, so RestActionListener is the only thing left that can answer the client. A unit test with a stubbed channel cannot observe this.

What was added

qa/smoke-test-http, following the existing TestResponseHeaderPlugin pattern:

• TestResponseFailurePlugin / TestResponseFailureRestAction — a handler that completes through RestResponseListener (the same listener the search path uses, and the frame present in the reported stack) and returns a response whose content() throws ArithmeticException, mirroring what Lucene's UnicodeUtil#maxUTF8Length throws on overflow. This reproduces the failure shape without allocating an oversized response, so the test is cheap and needs no special heap.
• RestResponseFailureIT#testFailureResponseIsSentWhenResponseBodyCannotBeSerialized — asserts the client receives HTTP 500 with an arithmetic_exception body rather than a connection that never answers.
• RestResponseFailureIT#testInFlightRequestsBreakerIsReleasedWhenResponseBodyCannotBeSerialized — asserts the request's in-flight-requests breaker reservation is released exactly once, now that the failure response travels over an already-closed channel. Compared against a baseline reading rather than zero, because the transport layer shares that breaker and reserves bytes for the node stats request used to read it.

Verification

Both tests pass on this branch:

./gradlew :qa:smoke-test-http:integTest --tests "org.opensearch.http.RestResponseFailureIT"
BUILD SUCCESSFUL

Reverting only the close() change in RestController (both ResourceHandlingHttpChannel and StreamHandlingHttpChannel) makes both tests fail, and the node logs the exact line from the field report:

RestResponseFailureIT > testFailureResponseIsSentWhenResponseBodyCannotBeSerialized FAILED
    Unexpected exception type, expected ResponseException but got
    java.net.SocketTimeoutException: 30000 MILLISECONDS

[ERROR][o.o.r.a.RestResponseListener] [node_s0] failed to send failure response

./gradlew :qa:smoke-test-http:precommit also passes.

Notes for reviewers

• Scope of the Bug A guard. The overflow guard covers BytesArray(String), so it protects the plain-text/String response path. Responses built through BytesRestResponse(RestStatus, XContentBuilder) never go through it, so a large JSON search response in a stock OpenSearch build does not hit this guard. That is why the integration test drives the String path through a plugin handler rather than through _search. A configurable maximum response size covering every path is the more complete fix and is worth a separate issue.
• close() is now a full no-op on repeat calls, which means a handler that sends twice will now write two responses instead of failing loudly. A narrower alternative is to track responseSent separately from closed and only permit the second send when the first one failed. Happy to change it if that is preferred.

Check List

• Functionality includes testing.
• API changes companion pull request not required.
• Public documentation issue/PR not required.

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

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 943fd78)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Potentially Flaky Assertion testInFlightRequestsBreakerIsReleasedWhenResponseBodyCannotBeSerialized compares in-flight-requests breaker readings before and after the failing request via NodesStatsResponse. Since the breaker is shared with the transport layer and stats requests themselves reserve bytes, two consecutive readings may not be equal even without leaks if any background transport activity reserves/releases bytes concurrently. The assertBusy may mitigate this, but the equality check remains sensitive to concurrent transport traffic on the test cluster. public void testInFlightRequestsBreakerIsReleasedWhenResponseBodyCannotBeSerialized() throws Exception { final long baseline = inFlightRequestsEstimatedBytes(); expectThrows(ResponseException.class, () -> getRestClient().performRequest(responseFailureRequest())); assertBusy(() -> assertThat(inFlightRequestsEstimatedBytes(), equalTo(baseline))); } Incomplete Overflow Guard ensureUTF16LengthIsValidForUTF8Encoding uses MAX_UTF8_BYTES_PER_CHAR = 3, but Lucene's UnicodeUtil.MAX_UTF8_BYTES_PER_CHAR is actually 4 (for surrogate pair encoding). Using 3 leaves a range of UTF-16 lengths (Integer.MAX_VALUE/4 < len <= Integer.MAX_VALUE/3) that will pass the guard but can still overflow inside UnicodeUtil.maxUTF8Length when computing len * 4. The Javadoc claim to "mirror" UnicodeUtil#MAX_UTF8_BYTES_PER_CHAR is inaccurate. /** * Mirrors {@code UnicodeUtil#MAX_UTF8_BYTES_PER_CHAR}. */ private static final int MAX_UTF8_BYTES_PER_CHAR = 3; private static final int MAX_UTF16_LENGTH_FOR_UTF8 = Integer.MAX_VALUE / MAX_UTF8_BYTES_PER_CHAR;

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 943fd78
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Narrow overflow-to-413 mapping scope overflowGuardFailureToRequestEntityTooLarge returns an OpenSearchStatusException which is a RuntimeException, but the constructors calling toBytesArray don't declare that any error path exists. More importantly, this converts every IllegalArgumentException from the BytesArray(String) constructor into HTTP 413, even ones that may not be UTF-16 overflow related in the future. Narrow the catch by checking the guard explicitly (e.g., via ensureUTF16LengthIsValidForUTF8Encoding before constructing) so that unrelated IllegalArgumentExceptions aren't misclassified as 413. server/src/main/java/org/opensearch/rest/BytesRestResponse.java [92-98] static BytesArray toBytesArray(String content) { try { - return new BytesArray(content); + BytesArray.ensureUTF16LengthIsValidForUTF8Encoding(content.length()); } catch (IllegalArgumentException e) { throw overflowGuardFailureToRequestEntityTooLarge(e); } + return new BytesArray(content); } Suggestion importance[1-10]: 5 __ Why: The suggestion has merit in that catching all IllegalArgumentException from BytesArray construction could over-map to 413, and using the explicit guard would be more precise. However, currently BytesArray(String) only throws IllegalArgumentException from this exact guard, so the practical impact is limited.	Low
	Avoid silently allowing duplicate responses Making close() silently idempotent hides genuine double-sendResponse bugs elsewhere in the codebase, since callers can now send two response bodies over the same channel without any signal. Consider making sendResponse itself the idempotent guard (drop the second response, optionally log) while keeping close() a single-shot bookkeeping operation, so accidental double sends are surfaced rather than merged. server/src/main/java/org/opensearch/rest/RestController.java [664-667] -private void close() { +public void sendResponse(RestResponse response) { if (closed.compareAndSet(false, true)) { inFlightRequestsBreaker(circuitBreakerService).addWithoutBreaking(-contentLength); + delegate.sendResponse(response); + } else { + logger.warn("attempted to send response on already-closed channel; dropping"); } } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a legitimate design concern about masking double-sendResponse bugs, but the proposed alternative changes the semantics significantly and may not align with the PR's intent to fix the specific bug where the failure path legitimately calls sendResponse twice. Moderate impact, debatable correctness.	Low

---

Previous suggestions

Suggestions up to commit 14aa92d

Category	Suggestion	Impact
General	Preserve visibility of double-close events Making close() silently idempotent removes the double-close detection that previously signalled a bug in the response lifecycle. Consider logging a warning (or emitting an assertion) on the second close so accidental double-sendResponse invocations are still observable, while keeping the breaker release single-shot. server/src/main/java/org/opensearch/rest/RestController.java [664-667] private void close() { if (closed.compareAndSet(false, true)) { inFlightRequestsBreaker(circuitBreakerService).addWithoutBreaking(-contentLength); + } else { + assert false : "Channel is already closed"; + logger.warn("Channel is already closed"); } } Suggestion importance[1-10]: 4 __ Why: Adding logging/assertion on double-close could aid debugging, but the PR intentionally makes close idempotent to fix the bug. The suggestion is a minor observability enhancement.	Low
	Document/guard exception thrown from constructor BytesRestResponse constructors are declared to return a response object, not to throw checked/unchecked status exceptions. Throwing OpenSearchStatusException from within a constructor call chain (this(status, contentType, toBytesArray(content))) can propagate out of error-path code (e.g. building a 500 response) and be swallowed or mis-mapped. Consider handling the overflow at the caller level or documenting this behavior explicitly, and ensure callers that build failure responses cannot themselves fail with this exception, causing recursive failure handling. server/src/main/java/org/opensearch/rest/BytesRestResponse.java [92-98] +static BytesArray toBytesArray(String content) { + try { + return new BytesArray(content); + } catch (IllegalArgumentException e) { + throw overflowGuardFailureToRequestEntityTooLarge(e); + } +} - Suggestion importance[1-10]: 3 __ Why: The improved_code is identical to existing_code, so no concrete change is proposed. The concern is a valid observation but not actionable.	Low

[asimmahmood1]

@bowenlan-amzn or @jainankitk Can I ask you to review?

[github-actions]

Persistent review updated to latest commit 943fd78

[github-actions]

✅ Gradle check result for 943fd78: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.59%. Comparing base (24a14b9) to head (943fd78).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...in/java/org/opensearch/rest/BytesRestResponse.java	71.42%	2 Missing ⚠️
.../main/java/org/opensearch/rest/RestController.java	50.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22840      +/-   ##
============================================
+ Coverage     71.58%   71.59%   +0.01%     
- Complexity    77353    77360       +7     
============================================
  Files          6170     6170              
  Lines        359700   359707       +7     
  Branches      52459    52459              
============================================
+ Hits         257493   257535      +42     
+ Misses        81808    81775      -33     
+ Partials      20399    20397       -2     

☔ View full report in Codecov by Harness.
📢 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.

[bowenlan-amzn]

Today one flag answers two different questions. First, did we release the breaker reservation? Second, did we already send a response? After a failed send those two answers differ. That difference is the bug in #22311.

This change keeps the first answer and drops the second. DefaultRestChannel#sendResponse holds no send-once guard of its own. So a handler that sends twice now writes two HTTP responses onto one connection. The new test testResourceHandlingChannelCloseIsIdempotent asserts responseCount == 2. That makes the double write the expected behaviour.

Can we use two flags instead of one? The existing flag guards the release only. A new responseSent flag guards the answer, and we set it after the delegate call returns. Then we allow a second send only when the first send failed. This keeps the fix for #22311. A handler that sends twice also still fails loudly.

[bowenlan-amzn]

A naming nit, since this PR already rewrites the method. close() closes nothing here. It closes no socket, no stream, and no file. It only subtracts a number from a counter. The old message Channel is already closed was wrong twice over. The channel was not closed, and nothing closed it.

Can you rename the method to releaseRequestBytes() and the field to released? An accurate name would make this bug visible on sight.

[bowenlan-amzn]

This test never calls toBytesArray. It calls the guard and the exception factory separately. Then it asserts the factory returns 413, which is what the factory constructs by definition.

So the test still passes if someone unwires the guard from BytesRestResponse(RestStatus, String, String). That constructor is the line that matters. Can the test assert through the public constructor instead? The same change also covers the four lines codecov reports.

[bowenlan-amzn]

This has the same shape as the BytesRestResponseTests case. The name says testStringConstructor, but the body calls the static guard. It never calls new BytesArray(String). So nothing here fails if the constructor stops calling the guard.

Please call the constructor with a string of the maximum allowed length. Or rename the test to match what it checks.

[bowenlan-amzn]

This line asserts Lucene's internal behaviour, not ours. It already broke once on Java 25, in 354a74e4. Please drop it. The assertion on our own guard is the contract we control.

[bowenlan-amzn]

This method is public on a libs/core class. No caller exists outside BytesArray and its own tests. Private keeps the core API surface unchanged.