Add typed ClientChannelCloseException for client-rooted PUT termination

[beijxu]

Summary

When a client's PUT upload terminates because the client disconnected (TCP reset, dropped connection), the readInto(...) callback on the request body reader currently only sees a bare ClosedChannelException with no provenance. Downstream consumers (e.g. AmbryLI's dual-write failure classification) have to fall back to fragile message-string inference to separate genuine client aborts from server/destination failures.

This PR adds a three-tier, typed classification of the exception delivered to readInto(...) on termination, so downstream can distinguish "definitely client," "plausibly client but not provable," and "not client":

Tier	Type	Call site(s)	Confidence
Sure	ClientChannelCloseException (new)	channelInactive() — delivered on both readInto (Path A) and the response-completion path (Path B)	Proven exclusively client-rooted
Possible	PossibleClientChannelCloseException (new)	idle-timeout (userEventTriggered's ALL_IDLE) — delivered on both Path A and Path B; exceptionCaught's IOException branch — delivered on Path A only (see Path B caveat below)	Plausible client-rooted, but a documented non-client alternate cause exists
Other (unclassified)	bare ClosedChannelException (unchanged)	e.g. RestServiceException-triggered aborts, and anything else	No client-rooted signal at all

• ClientChannelCloseException and PossibleClientChannelCloseException (both new, ambry-utils) each extend java.nio.channels.ClosedChannelException directly - they are siblings, not a subtype relationship between them. Every existing catch clause keyed on ClosedChannelException continues to behave identically (backward compatible). Because they're siblings, an instanceof ClientChannelCloseException check for the high-confidence "sure" tier can never be accidentally satisfied by a "possible" tier exception, and vice versa.
• Utils.isPossibleClientTermination(...) now recognizes both new types, in addition to its existing message-suffix heuristic, so existing OSS-internal callers keep working unchanged and downstream gets equivalent detection paths (type check OR the util) for both tiers combined. Note for downstream consumers that need to distinguish "sure" from "possible": isPossibleClientTermination(...) is intentionally a single flat boolean across all tiers (by design, to preserve its existing OSS-internal callers) — it does not tell you which tier matched. Use a direct instanceof ClientChannelCloseException / instanceof PossibleClientChannelCloseException check if tier-level distinction is needed.
• NettyRequest.markClientTerminated() / closeDueToClientTermination(): tag+close helpers for the "sure" tier (unchanged from the original design).
• NettyRequest.markPossibleClientTermination() / closeDueToPossibleClientTermination() (new): the equivalent pair for the "possible" tier. markPossibleClientTermination() will not downgrade an already-set "sure" tag - if both were somehow triggered for the same request, "sure" always wins.
• NettyMessageProcessor.channelInactive() remains the only call site tagging the "sure" tier. Idle-timeout and exceptionCaught's IOException branch now tag the "possible" tier (mark-only, no explicit close call - the existing onRequestAborted(...) -> responseChannel.close(...) -> request.close() flow already reliably closes the request later, so this introduces no new close-ordering behavior). RestServiceException-triggered aborts and any other exception type are left entirely untagged (the "other" tier).

Typed exceptions now flow into both the readInto callback (Path A) and the response-completion path (Path B), for the two call sites where doing so is behavior-neutral. channelInactive() (sure tier) and idle-timeout (possible tier) now pass the same typed ClientChannelCloseException/PossibleClientChannelCloseException to onRequestAborted(...), not just to readInto's callback — a downstream consumer of either path can now instanceof-detect the tier consistently. This is provably behavior-neutral: Utils.isPossibleClientTermination(...) already recognized both new types unconditionally (exactly as the legacy Utils.convertToClientTerminationException(...) message-wrap it replaces at these two call sites always matched), so NettyResponseChannel#getErrorResponse's response status code (BAD_REQUEST) and clientEarlyTerminationCount metric are unchanged — see the new outbound-status assertions added to channelInactiveDeliversClientTerminationToReadIntoTest and idleTimeoutDeliversPossibleClientTerminationToReadIntoTest.

exceptionCaught's IOException branch is a deliberate exception to this: it passes the original cause object directly to onRequestAborted(...) (not a synthetic wrap), and that cause's message does not always match the legacy pattern Utils.isPossibleClientTermination(...) checks for. Wrapping it in the typed exception would make isPossibleClientTermination(...) unconditionally true there, which would be a real response-code change (500→400) for causes whose message doesn't already match — this would violate the non-negotiable "no response-code behavior change" constraint, so this call site is intentionally left passing the raw, untyped cause to Path B (Path A already gets the typed PossibleClientChannelCloseException via markPossibleClientTermination(), unaffected by this caveat).

A downstream consumer that already calls isPossibleClientTermination(...) on either path sees no behavior change. A downstream consumer doing a raw instanceof ClosedChannelException catch-all on either path will now observe the two new concrete subtypes for channelInactive/idle-timeout - intentional, and the whole point of the change - but NOT for exceptionCaught's IOException branch on the Path B/response side specifically (Path A is unaffected and always typed).

Required, coupled fix: NettyResponseChannel.completeRequest() reorder

This PR also reorders completeRequest() so it closes the request (closeRequest(), which flips request.isOpen() to false) before scheduling the network channel's close listener, instead of after.

This is not a drive-by refactor — it is a precondition for the correctness of the channelInactive() "sure" tagging above, and is bundled deliberately rather than split into a separate PR:

Previously, if the response writeFuture was already complete at the point the CLOSE listener was added, Netty's ChannelFuture.addListener() fires the listener synchronously/re-entrantly. That could close the network channel and trigger channelInactive() before closeRequest() had run and flipped isOpen() to false — meaning a pure server-initiated response completion could look, from channelInactive()'s perspective, indistinguishable from an actual client disconnect (both would observe request.isOpen() == true when channelInactive fires). Without this reorder, the single "sure"-tagged call site (channelInactive()) would not be safe to tag at all.

A standalone regression test (completeRequestClosesRequestBeforeReturningTest in NettyResponseChannelTest) asserts request.isOpen() == false immediately after completeRequest()/onResponseComplete() returns — independent of the ClientChannelCloseException feature — and its javadoc documents this coupling explicitly, so it's clear this isn't scope creep.

Why idle-timeout and exceptionCaught's IOException branch are "possible," not "sure"

Per the non-negotiable invariant that a termination may only be tagged high-confidence client-rooted, and that a false positive (tagging a real server/destination failure as client-rooted) is worse than under-tagging, these two call sites get the "possible" tier rather than "sure":

• Idle-timeout (userEventTriggered's ALL_IDLE branch): NettyRequest suspends reads (autoRead=false) under destination-write backpressure (nettyServerRequestBufferWatermark). While suspended, ALL_IDLE can fire purely because our own downstream write is stalled, not because the client is idle. This isn't just a narrow race: NettyRequest.writeContent() calls setAutoRead(true) unconditionally the instant the last client chunk (LastHttpContent) arrives, before the destination write for that final chunk is even issued — so for the entire window between last-chunk-received and response-sent, if the destination is slow to complete that final write, the socket is silent in both directions with autoRead==true for the whole window: a deterministic (not merely racy) false-positive shape. A future "sure" follow-up would need to gate on "no destination write currently in flight" (a last-transition timestamp), not point-in-time autoRead state, plus a recovery-race test.
• exceptionCaught's IOException branch: NettyRequest.writeContent() writes to the destination (router/store) and is wrapped in try { ... } catch (Exception e) { ...; throw e; }, anticipating that the destination AsyncWritableChannel contract can throw synchronously. Such an exception can propagate uncaught through addContent()/handleContent() (whose only catch is IllegalStateException) and reach exceptionCaught via Netty's own implicit uncaught-exception-in-channelRead routing (not any explicit fireExceptionCaught() call in Ambry's code) while request.isOpen()==true — i.e. a genuine destination-side failure could reach this branch. Rather than leave it entirely untagged, it is tagged as "possible" so downstream at least gets a plausibility signal, while remaining clearly distinguishable from the "sure" tier.

Both branches have positive tests (idleTimeoutDeliversPossibleClientTerminationToReadIntoTest, exceptionCaughtIOExceptionDeliversPossibleClientTerminationToReadIntoTest) asserting they deliver PossibleClientChannelCloseException and NOT ClientChannelCloseException. serverAbortDoesNotDeliverClientTerminationToReadIntoTest asserts a RestServiceException-triggered abort delivers neither type, proving the "other" tier is never conflated with either client tier.

ambry-utils dependency caveat (flagged for downstream, not resolved here)

ambry-api/build.gradle declares implementation project(':ambry-utils'), not api, so a consumer depending only on published ambry-api would not automatically get ambry-utils on its compile classpath transitively. AmbryLI already calls Utils.isPossibleClientTermination(...) today, which suggests an existing direct compile dependency on ambry-utils, but this should be confirmed by the AmbryLI team before they attempt to consume ClientChannelCloseException/PossibleClientChannelCloseException.

Version / publish caveat

AmbryLI currently consumes published com.github.ambry:ambry-* 0.5.177. This repo's build reports Building version 0.5.183 (via shipkit-auto-version) at the time of this PR. This change must merge and be published (version bump) before AmbryLI can consume ClientChannelCloseException/PossibleClientChannelCloseException — publishing is out of scope for this PR.

Testing Done

Targeted:

./gradlew :ambry-rest:test --tests "com.github.ambry.rest.NettyRequestTest" \
  --tests "com.github.ambry.rest.NettyMessageProcessorTest" \
  --tests "com.github.ambry.rest.NettyResponseChannelTest" \
  :ambry-utils:test --tests "com.github.ambry.utils.UtilsTest"

Result: all ambry-rest tests pass (20 + 14 + 23 = 57/57). ambry-utils UtilsTest: 29/29 ran; the new/modified clientTerminationWrapAndRecognizeTest passed. 3 pre-existing failures in testGetByteBufferInputStreamFromCrcStreamShareMemoryWithNettyByteBuf (a java.lang.reflect.InaccessibleObjectException from a JDK17 module-access restriction, unrelated to this change) were confirmed to fail identically on the pre-change baseline via git stash.

New/changed tests:

• NettyRequestTest#markClientTerminatedDeliversTypedExceptionTest: covers markClientTerminated+close (sure tier tagged), plain close (untagged), and closeDueToClientTermination (sure tier tagged).
• NettyRequestTest#markPossibleClientTerminationDeliversTypedExceptionTest (new): covers markPossibleClientTermination+close (possible tier tagged, and NOT instanceof ClientChannelCloseException), plus a regression case proving a prior "sure" tag is never downgraded to "possible" by a later markPossibleClientTermination() call on the same request.
• NettyMessageProcessorTest#channelInactiveDeliversClientTerminationToReadIntoTest: positive — client disconnect → readInto callback exception is instanceof ClientChannelCloseException; now also asserts that if an error response is observed on the outbound side before the network channel physically closes, its status is unchanged (BAD_REQUEST), proving the Path B typed-exception propagation for this call site is behavior-neutral.
• NettyMessageProcessorTest#idleTimeoutDeliversPossibleClientTerminationToReadIntoTest, #exceptionCaughtIOExceptionDeliversPossibleClientTerminationToReadIntoTest (renamed/updated): positive — both now assert instanceof PossibleClientChannelCloseException and NOT instanceof ClientChannelCloseException. The idle-timeout test additionally asserts the outbound error response status remains BAD_REQUEST, proving Path B propagation there is also behavior-neutral.
• NettyMessageProcessorTest#serverAbortDoesNotDeliverClientTerminationToReadIntoTest (strengthened): negative — a server-side RestServiceException abort is NOT tagged as either ClientChannelCloseException or PossibleClientChannelCloseException.
• NettyResponseChannelTest#completeRequestClosesRequestBeforeReturningTest: standalone regression test (independent of this feature) asserting request.isOpen()==false immediately after completeRequest()/onResponseComplete() returns; javadoc documents why this is required for channelInactive()'s exclusivity guarantee.
• UtilsTest#clientTerminationWrapAndRecognizeTest: extended to assert isPossibleClientTermination recognizes both ClientChannelCloseException and the new PossibleClientChannelCloseException, in addition to the pre-existing message-suffix cases.

Compatibility

• No public REST API or response-code behavior change.
• ClientChannelCloseException/PossibleClientChannelCloseException both extend ClosedChannelException: any existing catch (ClosedChannelException e) continues to match identically.
• Utils.isPossibleClientTermination(...)'s existing message-suffix detection paths are unmodified; the new type checks are additive.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 9.52381% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.20%. Comparing base (52ba813) to head (8165718).
⚠️ Report is 406 commits behind head on master.

Files with missing lines	Patch %	Lines
...a/com/github/ambry/rest/NettyMessageProcessor.java	0.00%	18 Missing ⚠️
.../main/java/com/github/ambry/rest/NettyRequest.java	18.18%	9 Missing ⚠️
...va/com/github/ambry/rest/NettyResponseChannel.java	0.00%	7 Missing ⚠️
...main/java/com/github/ambry/router/RouterUtils.java	0.00%	2 Missing ⚠️
...ls/src/main/java/com/github/ambry/utils/Utils.java	0.00%	2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (52ba813) and HEAD (8165718). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (52ba813)	HEAD (8165718)
	3	2

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #3280       +/-   ##
=============================================
- Coverage     64.24%   38.20%   -26.04%     
+ Complexity    10398     6498     -3900     
=============================================
  Files           840      940      +100     
  Lines         71755    80414     +8659     
  Branches       8611     9679     +1068     
=============================================
- Hits          46099    30726    -15373     
- Misses        23004    47206    +24202     
+ Partials       2652     2482      -170     

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nicolaslopezbravo]

channelException isn't a dedicated field for this classification — validateState() also writes a real RestServiceException into it (lines 608/612) when the content length doesn't match the header. This assignment is unconditional, so a genuine BadRequest can be silently replaced by the client-close tag.

Realistic trigger: an undersized body is only detected on LastHttpContent, so a truncated PUT followed by a client reset hits validateState first and then channelInactive. The 400 is lost and the request lands in the client-early-termination bucket instead.

Suggested change

	void markClientTerminated() {
	channelException = CLIENT_CHANNEL_CLOSE_EXCEPTION;
	}
	void markClientTerminated() {
	// Only overwrite the default sentinel. A real error already recorded here (e.g. the RestServiceException from
	// validateState) is the more specific cause and must win.
	if (channelException == CLOSED_CHANNEL_EXCEPTION) {
	channelException = CLIENT_CHANNEL_CLOSE_EXCEPTION;
	}
	}

Checking for the untouched sentinel rather than excluding one specific value keeps any real error that was already recorded, and stays correct if another writer to this field is added later.

[beijxu]

Thanks — I traced this end to end and I'd like to push back on the impact before changing the guard, because in the context of the goal here (separating sure-client vs possible-client vs server/unclassified connection closes) this guard is actually doing the right thing.

Two findings:

1. The 400 is not lost. When validateState() throws the BadRequest, it propagates channelRead0 → exceptionCaught, where cause is a RestServiceException (not IOException), so the markPossibleClientTermination branch is skipped and onRequestAborted((Exception) cause) runs. The client-facing error response is built from that passed exception via getErrorResponse(exception) — it never reads channelException. So the client still gets its 400 regardless of what later overwrites the field. The only thing channelException feeds is the readInto callback tier.

2. On the readInto tier, keeping ClientChannelCloseException is the correct classification, not a regression. In the scenario you describe the connection closed because the client reset (channelInactive with serviceUp==true). That is a genuine sure-client close, so tagging it sure is right. Applying the suggested == CLOSED_CHANNEL_EXCEPTION guard would instead preserve the RestServiceException, which RouterUtils.isSystemHealthError() treats as true (a server health error) — i.e. it would reclassify a client-caused close as a server problem, the opposite of this PR's goal. There's also no durability impact: an aborted PUT commits nothing.

Given that, I'd prefer to leave markClientTerminated() as-is. If we ever do want to protect a business error from being overwritten, the right predicate is !(channelException instanceof RestServiceException) (not identity against the sentinel), so a real client reset can still upgrade a prior "possible" tag to "sure" — but since the client response is unaffected and the sure tag is the correct tier here, I don't think it's warranted. Happy to reconsider if you see a consumer of the readInto-side exception that I'm missing.

[nicolaslopezbravo]

Same issue as markClientTerminated above: this guard only prevents downgrading the "sure" tag, but it still overwrites a RestServiceException written by validateState().

Suggested change

	if (channelException != CLIENT_CHANNEL_CLOSE_EXCEPTION) {
	if (channelException == CLOSED_CHANNEL_EXCEPTION) {

Checking against the default sentinel ("has anything meaningful been written here yet?") rather than excluding one specific value covers both cases, and stays correct if another writer to this field is added later.

Worth a test where channelException already holds a RestServiceException when a mark fires — the current suite doesn't cover that ordering.

[beijxu]

Agreed the guard is imprecise, but I checked reachability and I don't think a RestServiceException can realistically still be in channelException when this runs, so I'd rather not add code for a path that can't execute.

For validateState's RestServiceException to survive to a later markPossibleClientTermination() on the same open request:

• The exceptionCaught IOException path can't get there — after validateState throws, cause is a RestServiceException, so the instanceof IOException gate is false and markPossibleClientTermination() is never called on that path.
• That leaves only the idle-timeout path (userEventTriggered ALL_IDLE) firing inside the millisecond-scale error-response write window. Idle timeout is nettyServerIdleTimeSeconds of total silence in both directions, so landing it inside that window isn't realistically reachable.

Functionally it also has no effect on the tiering: "possible" is already the lowest live tier, and the != SURE guard already prevents the one downgrade that would matter (sure → possible). So the suggested == CLOSED_CHANNEL_EXCEPTION is strictly-correct hardening but with de-minimis reachability and no classification change. I'll leave it as-is unless you feel strongly; if we do change it I'd apply the same !(channelException instanceof RestServiceException) predicate as the sibling method for consistency rather than identity-against-sentinel.

[nicolaslopezbravo]

Minor, but this drops the useful part of the diagnosis. cause here carries the actual socket message ("Connection reset by peer", "Broken pipe"), and the readInto callback ends up with a cause-less, message-less singleton instead.

It's also asymmetric: onRequestAborted((Exception) cause) a few lines below still forwards the real exception to the response path, so whether the underlying reason is visible depends on which side you're reading. During an incident that's exactly the detail you want.

Suggest an overload that preserves it — ClosedChannelException has no cause constructor, so it needs initCause, which also means not using the shared singleton on this path:

void markPossibleClientTermination(Throwable cause) {
  if (channelException == CLOSED_CHANNEL_EXCEPTION) {
    PossibleClientChannelCloseException e = new PossibleClientChannelCloseException();
    e.initCause(cause);
    channelException = e;
  }
}

The no-arg version can stay as-is for the idle-timeout call site, which genuinely has no cause to attach.

[beijxu]

You're right that the asymmetry is real — the readInto callback gets the message-less singleton while onRequestAborted((Exception) cause) forwards the real socket exception — and your point that initCause needs a fresh instance (the singleton is shared/class-loaded) is correct.

That said, for the classification goal this is observability-only, so I'm inclined to leave it:

• Nothing on the callback side consumes the message/cause. RouterUtils.isSystemHealthError() and Utils.isPossibleClientTermination() both classify purely by instanceof, so the tier is already correct (possible) with or without the cause attached.
• The real cause isn't lost during an incident — it's still logged on the response path via onResponseComplete's log(exception), which receives the actual cause. Only the router-side log line is less specific.

So the enrichment is a genuine nicety but doesn't change how any close is tiered. If diagnosing these on the router side turns out to be painful in practice I'm happy to add the markPossibleClientTermination(Throwable cause) overload (fresh instance + initCause, no-arg kept for the idle path) as a follow-up — but I'd keep it out of this PR since it's not part of the sure/possible/server separation.