Respond with 408 status code if not all request body bytes were received

[rwalerow]

Why I did it?
In order to have a test which might confirm an issue
with an incompletely send request

Issue reported here: #4169

How I did it:
I prepared a new test case in NettyCatsRequestTimeoutTest as following:

set request timeout to 1s
declare 10000 bytes as request body size(content length)
send request with a tiny request body

Note: This suppose to partial fix for case where server should return 408 on partially sent body without client closing the connection

[flsh86]

LGTM with some minors

[adamw]

Is there a related issue?

[adamw]

are we using the explicit-false case anywhere? Maybe we should just use a () as a marker? So that we have two states: RequestBodyCompletedTracker.BodyComplete is either set, or not. Now we have three (true, false, unset), with overlapping semantics

[rwalerow]

I understand the incentive to use only double, not the triple values.
I would stay with a given boolean version though. If we would change this to Unit type and try to make it work we would be forced to use deprecated Attribute.remove() api

I've tested this with the java doc suggestion of set(null) instead but the fix stopped working at this point.

I personally prefer to pay a fee of additional non set state than to use deprecated api

[adamw]

But is it at all possible to first observe LastHttpContent and then a HttpRequest? Or would it be a bug?

[rwalerow]

It would be a bug since even headers generate HttpRequest
Even option should generate those before LastHttpContent

[adamw]

Automated Claude code review. 10 inline comments: 3 confirmed correctness issues, 1 likely ordering problem, plus smaller cleanups.

[adamw]

🤖 automated Claude review

The flag is per-channel, but the timeout is per-request. A keep-alive client can fully send request 1 (flag = true), pipeline request 2's headers — this line resets the flag to false, since the tracker sits before HttpStreamsServerHandler and sees them immediately — and then stall. Request 1's reader-idle then reads false and aborts a valid, fully-received request with a 400. With a chunked/streaming response for request 1 this is a new failure: before this PR the exchange completed.

[rwalerow]

I'm not sure multiple requests are allowed in http apart from websockets.

2 requests and than response starts?

[adamw]

🤖 automated Claude review

toMillis.toInt overflows for timeouts above ~24.8 days; a negative value disables IdleStateHandler entirely, silently dropping both the 503 and the new 400. The constructor accepts long, so .toInt can just be dropped. (Pre-existing in the writer slot; this change copies it into the reader slot.)

[rwalerow]

I don't think that's relevant for this PR, it also applies for other timeouts

[rwalerow]

Is there a related issue?

Yes, here #4169

I've adjusted description accordingly

[adamw]

🤖 automated Claude review — re-review after the latest changes.

Replies marked as addressed, where the code says otherwise:

• RequestBodyCompletedTracker.scala and NettyFutureRequestTimeoutTests.scala still fail scalafmt --test (scalafmt 3.11.5, repo config). Only NettyServerHandler.scala is clean. CI does not check formatting, so this won't be caught automatically.
• The log message still reads "partially send request with pause exceeded request timeout" — the grammar fix wasn't applied. Also if (e.state == vs if (e.state() == on adjacent lines.
• The requestTimeout scaladoc no longer says what happens when the timeout hits (503, or 400 for an incomplete body) — the PR's own behavior is now undocumented, also in doc/server/netty.md. And "includes receiving the request" is not accurate: the IdleStateHandler is installed only after the request head arrives, and reader-idle bounds pauses between reads, not total receive time — a body trickling in slowly never triggers it.
• IO.sleep(1.second) is still in the test; readAllBytes() already blocks until the server closes.

Open discussions:

• 400/503 double write: only a test assertion was added; the two if branches are still independent and the IdleStateHandler is not removed after writing an error. The test passes on timing (reader task is scheduled first, close usually wins). If the client also stopped reading (full TCP send buffer — exactly this attack), the 400's flush stays pending and the writer task writes a 503 behind it. An if/else on body completeness plus removing the handler (or checking e.isFirst) fixes this.
• HTTP/1.1 pipelining is allowed (RFC 9112 §9.3.2), so the channel-scoped flag vs per-request timer conflation is real: request 2's head flips the flag while request 1's timer is armed, and a fully-received request 1 can get a 400.
• toMillis.toInt: the PR rewrites this exact line, and the TimeUnit constructor takes longs — the fix is deleting .toInt. A 30-day timeout goes negative and silently disables both timers.
• The open question about message order: HttpObjectDecoder always emits HttpRequest before body parts, and FullHttpRequest matches the LastHttpContent case first, so LastHttpContent-before-HttpRequest cannot happen.

New findings:

1. The PR doesn't fix the main scenario of [BUG] Server should return 400 Bad request if not all body bytes received #4169. The issue is a client that disconnects mid-body: channelInactive → HandlerPublisher calls onComplete unconditionally → the truncated body reaches the codec as complete. No idle event fires there, so that path is unchanged; only the stall-until-timeout case gets a 400. The underlying fix is failing the body publisher with onError when the channel closes before LastHttpContent.
2. Status code: RFC 9110 defines 408 Request Timeout for "didn't receive the complete request in time". 400 signals a malformed request and suppresses retries, which is backwards for the flaky-network use case, and mixes these into malformed-request metrics.
3. Users with a custom initPipeline silently lose the 400 behavior and can't opt in (the tracker is private[netty]), while the reader-idle timer in runRoute is armed unconditionally. The mechanism is split across two places, with the ordering constraint only in a comment.
4. The new test can hang the suite instead of failing: readAllBytes() has no setSoTimeout, so a regression blocks until the global timeout. Also only the Future backend is covered.
5. Simpler design: a var field on the tracker handler (looked up via the pipeline) replaces the AttributeKey machinery — same event-loop thread, no public key/BodyComplete surface, no null→Option fallback. A named handler could also be removed after a WS upgrade; currently it inspects every WS frame forever.

[rwalerow]

• RequestBodyCompletedTracker.scala and NettyFutureRequestTimeoutTests.scala still fail scalafmt --test (scalafmt 3.11.5, repo config). Only NettyServerHandler.scala is clean. CI does not check formatting, so this won't be caught automatically.

Adjusted, I've fixed faulty setting in my intellij

• The log message still reads "partially send request with pause exceeded request timeout" — the grammar fix wasn't applied. Also if (e.state == vs if (e.state() == on adjacent lines.

I've rephrased the log message

• The requestTimeout scaladoc no longer says what happens when the timeout hits (503, or 400 for an incomplete body) — the PR's own behavior is now undocumented, also in doc/server/netty.md. And "includes receiving the request" is not accurate: the IdleStateHandler is installed only after the request head arrives, and reader-idle bounds pauses between reads, not total receive time — a body trickling in slowly never triggers it.

Reintroduced

• IO.sleep(1.second) is still in the test; readAllBytes() already blocks until the server closes.

I've introduced socket closing as a resource code

Open discussions:

Those were left open on purpose

1. The PR doesn't fix the main scenario of [BUG] Server should return 400 Bad request if not all body bytes received #4169. The issue is a client that disconnects mid-body: channelInactive → HandlerPublisher calls onComplete unconditionally → the truncated body reaches the codec as complete. No idle event fires there, so that path is unchanged; only the stall-until-timeout case gets a 400. The underlying fix is failing the body publisher with onError when the channel closes before LastHttpContent.

This will spawn a new PR

2. Status code: RFC 9110 defines 408 Request Timeout for "didn't receive the complete request in time". 400 signals a malformed request and suppresses retries, which is backwards for the flaky-network use case, and mixes these into malformed-request metrics.

Changed return status

3. Users with a custom initPipeline silently lose the 400 behavior and can't opt in (the tracker is private[netty]), while the reader-idle timer in runRoute is armed unconditionally. The mechanism is split across two places, with the ordering constraint only in a comment.

I've removed private[netty] so it would be available everywhere

4. The new test can hang the suite instead of failing: readAllBytes() has no setSoTimeout, so a regression blocks until the global timeout. Also only the Future backend is covered.

I've introduced socket closing as a resource code

5. Simpler design: a var field on the tracker handler (looked up via the pipeline) replaces the AttributeKey machinery — same event-loop thread, no public key/BodyComplete surface, no null→Option fallback. A named handler could also be removed after a WS upgrade; currently it inspects every WS frame forever.

I've introduced this approach

[adamw]

🤖 automated Claude review — re-review of a2775e2.

Everything else from the previous round looks addressed: scalafmt is clean now (checked with scalafmt 3.11.5 on all 4 files), the unified writeErrorThenClose, the boolean field instead of the AttributeKey, the case _ => true fallback for custom pipelines, the tracker gated on requestTimeout and removed on WS upgrade, 408 instead of 400, and the test cleanups.

Main finding: the 408 only happens when the whole partial request arrives in one read

I built the branch and probed it with requestTimeout = 500ms, Content-Length: 10000 and 4 bytes of body:

what the client does	response
head + 4 body bytes in one write	408
head, 200ms pause, 4 body bytes, then stall	503

IdleStateHandler is installed in runRoute, i.e. when the head arrives. Any read after that updates lastReadTime, so the reader task reschedules itself and the writer task fires first at the deadline, giving a 503. The new test passes only because it sends everything in one segment. A real stalled upload (headers, then part of the body, then nothing) still gets a 503.

Suggested fix, which is also simpler than what's in the PR: drop the reader-idle timer (go back to IdleStateHandler(0, requestTimeout, 0, ...)) and let the flag pick the status inside the existing writer-idle branch:

if (e.state() == IdleState.WRITER_IDLE) {
  if (wasRequestBodyFullyReceived(ctx)) {
    logger.error(...)
    writeErrorThenClose(ctx, HttpResponseStatus.SERVICE_UNAVAILABLE)
  } else {
    logger.debug(...)
    writeErrorThenClose(ctx, HttpResponseStatus.REQUEST_TIMEOUT)
  }
}

I applied exactly this to the branch and re-ran the probe: both stalled-upload shapes give 408, and a complete request with a 2s handler still gives 503. As a bonus, with a single timer the "408 followed by a 503 on the same connection" problem goes away on its own, without having to remove the handler.

The test should then write the head and the body fragment separately — that is the case that is currently broken.

Other points

• The first line of the requestTimeout scaladoc ("the maximum duration to wait for the request to be received in full") does not match the code, and contradicts the 408 bullet below it. The timeout bounds inactivity, not total receive time: a body arriving one byte at a time never trips it.
• toMillis.toInt overflows for timeouts over ~24.8 days, and a negative value disables the timer with no error. The constructor takes a long, so dropping .toInt is enough. The PR rewrites this line anyway.
• RequestBodyCompletedTracker is public now, but it lives in .internal and nothing tells users with a custom initPipeline that they should add it, or that it has to go before HttpStreamsServerHandler. Either mention it in the initPipeline scaladoc, or make it private[netty] again.
• bodyFullyReceived is a plain var read from userEventTriggered. It is correct because both run on the channel's event loop, but that is worth one line of comment.
• import NettyResponseContent._ replaces the explicit import list — unrelated to this PR, and less explicit.
• Leftover from the last round: if (e.state == next to if (e.state() ==.

[adamw]

🤖 automated Claude review — final review of 55c3eac.

The design is now right: one writer-idle timer, with the status picked by the body-completion flag. I checked out the branch and ran it:

• both new 408 tests pass, including the "head, then body fragment in a separate write, then stall" case that returned 503 on a2775e2
• the 503 path still works (properly update metrics when a request times out)
• nettyServer scalafmtCheck is clean, main and test

Everything actionable from the previous rounds is addressed: .toInt dropped, unified writeErrorThenClose, debug log for the client-side case, a boolean field instead of the AttributeKey, missing tracker falls back to 503, tracker gated on requestTimeout and removed on WS upgrade, 408 instead of 400, scaladoc and netty.md describe what the code does, and the test cleanups. The "408 followed by a 503" problem went away on its own with the single timer.

Nothing blocking. Four things to decide on before merge.

1. Test duplication

The two new tests are ~45 near-identical lines each, differing only in whether the body fragment goes out in the same write as the head. Extract one helper that takes the write steps.

Worth knowing while doing that: the two cases may not actually differ. The decoder emits HttpRequest then HttpContent as separate messages either way, and with no pause between the two writes they usually end up in one TCP segment anyway. Since the reader-idle timer is gone, both tests now reach the tracker the same way. Either drop one, or put a short sleep between the writes so the second one is genuinely a separate read.

2. The flag is per connection, the timeout is per request

Known and accepted, but worth one sentence in the tracker scaladoc so the next reader doesn't assume otherwise: on a keep-alive connection the headers of request N+1 can be decoded while request N is still being handled, which flips the flag to false, so a slow handler on a fully received request N is then reported as 408. Only a wrong status code on a connection that is closing anyway.

3. Writer-idle can fire more than once

IdleStateHandler re-fires WRITER_IDLE every requestTimeout until a write completes, so a client that stalls and also stops reading gets one more error response queued per period. Pre-existing (the 503 had it too), and handleRequestTimeout could remove the handler after writing. Optional.

4. Docs

Correct, but long for what they say. Concretely:

• netty.md — revert the bullet at the top to * request timeout plus a pointer; it repeats the section below it. In the section, cut the explanatory tails ("the endpoint's logic is too slow", "then either stalled or kept sending too slowly", the Content-Length: 10000 example) so each bullet is one clause.
• The requestTimeout scaladoc reads better as prose than as a bullet list, roughly: "The maximum duration between receiving the request headers and producing a response; it therefore also bounds how long the client has to send the body. If exceeded, an empty response with Connection: close is sent and the connection is closed — 503 if the request had been fully received, 408 if the body was still incomplete. Ignored in Web Sockets (after a handshake is established). Make sure it's lower than idleTimeout."

The same explanation currently appears in five places (netty.md, the requestTimeout scaladoc, the initPipeline scaladoc, the defaultInitPipeline comment, the tracker scaladoc). Once is enough.

Smaller things

• RequestBodyCompletionTracker.channelRead: the case order is load-bearing — FullHttpRequest is both an HttpRequest and a LastHttpContent, so LastHttpContent has to come first. One comment, otherwise a future tidy-up silently sticks the flag at false.
• NettyServerHandler still has idleTimeout.toMillis.toInt for the idle-timeout handler; same overflow that was just fixed for the request timeout.
• Nothing pins the per-request reset of the flag. A test that sends a complete request and then a stalled one on the same socket would catch a regression where the flag is only ever set once.
• Optional, and it is the shape suggested earlier: installing the tracker from NettyServerHandler.initHandler (which already does addFirst(new IdleStateHandler(...))) with addAfter(ServerCodecHandlerName, ...) would drop the gating in defaultInitPipeline, the companion-object lookup, the null fallback, and the custom-pipeline caveat from the docs — and custom pipelines would get 408 too. initWsPipeline already requires the codec to be registered under that name, so the dependency is not new. The trade is that a custom pipeline without that name would fail loudly instead of quietly falling back to 503.

Unrelated, pre-existing

Requests with Expect: 100-continue are broken on master today, and the new doc sentence ("The request timeout starts when the request headers are received") does not hold for them. The branch in channelRead0 writes 100 Continue and returns without running the route or arming any timeout; that write also makes HttpStreamsServerHandler close the connection. I checked against master: the client gets HTTP/1.1 100 Continue, connection: close, then EOF, and never a real response. HttpStreamsServerHandler handles 100-continue itself, so deleting the branch is likely the fix. Separate issue, not this PR — but the doc sentence needs a caveat if it stays as is.