Skip to content

Fix passing incomplete body to parsers on connection abruptly closed - #5573

Merged
adamw merged 19 commits into
masterfrom
fix/pass-down-incomplete-body-on-connection-loss
Oct 9, 2026
Merged

adamw merged 19 commits into
masterfrom
fix/pass-down-incomplete-body-on-connection-loss

Conversation

@rwalerow

@rwalerow rwalerow commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

This PR fixes the last part of #4169

When client send partially declared body and than connection closes for any reason
Server reports issue and clears already read request and proceeds with a regular operation.

@adamw

adamw commented Oct 6, 2026

Copy link
Copy Markdown
Member

This is an automated review.

The fix works for the Netty Future and Sync servers: a truncated body no longer reaches the codec. But #4169 is not fully fixed yet:

  1. Cats and ZIO Netty servers are not covered. They don't use SimpleSubscriber. NettyCatsRequestBody.publisherToBytes and NettyZioRequestBody.publisherToBytes read the body via Fs2StreamCompatible.fromPublisher / ZioStreamCompatible.fromPublisher, which don't check the length, so a truncated body still goes to the codec. The repro in Chunked transmission lasts longer than timeout #4214 used the Cats server. Maybe the check should live in a place shared by all backends.
  2. Zero body bytes received → request hangs. If the connection closes after the headers but before any body bytes, buffers is empty, onComplete hits case _ => (), and the promise is never completed. This should also fail when contentLength.exists(_ > 0).
  3. File bodies: FileWriterSubscriber has no length check either, so a truncated upload is passed on as a complete file.

Minor:

  • In onComplete, the second case's guard contentLength.forall(_ == totalLength) is always true once the first case didn't match; length > 0 is enough.
  • The new test covers only the Future server. It also waits a fixed 500ms and then checks the logic wasn't called, so it could pass even without the fix if the logic just hasn't run yet. Tests for Cats, ZIO and Sync would help.

@rwalerow
rwalerow force-pushed the fix/pass-down-incomplete-body-on-connection-loss branch 2 times, most recently from e39fd75 to 2c942a2 Compare October 8, 2026 13:22
@rwalerow
rwalerow force-pushed the fix/pass-down-incomplete-body-on-connection-loss branch from 2c942a2 to edbf85b Compare October 8, 2026 13:22
@adamw

adamw commented Oct 8, 2026

Copy link
Copy Markdown
Member

This is an automated review (follow-up, at edbf85bca).

All points from the previous review are addressed. Checking the length in SubscribeTrackingStreamedHttpRequest covers every backend (Future, Cats, ZIO, Sync) and every body type (bytes/string, file, multipart, stream). Zero body bytes no longer hangs, and copying buffers in CopyingPublisher avoids leaks when the stream fails. The ZIO path is fine without it: zio-interop-reactivestreams emits queued chunks before failing, so they still get released.

Remaining, minor:

  1. No end-to-end test for the reported case: a truncated string/JSON body must not reach the codec or the server logic. The Future test from the first version was removed. Right now only the subscriber unit test and the Cats streaming test exist. One test in the shared server tests (send a partial body with a large Content-Length, check the logic never runs) would cover all backends.
  2. Sync multipart now copies each chunk to an array and wraps it back into DefaultHttpContent, which is one more copy than before. Probably fine, just noting it.

rwalerow and others added 4 commits October 9, 2026 13:28
…ncomplete body tests

Sync multipart passes Netty chunks through ox again, releasing the ones left
unconsumed when the flow ends early (ox drops buffered elements on errors).

The incomplete body test waits for the request to be rejected instead of a fixed
sleep, checks the failure's cause where it is observable, and covers multipart.
- document CopyingPublisher, IncompleteRequestBodyException, and that truncated
  chunked requests aren't detected
- unit tests: drop a duplicate case, move the no Content-Length case to a
  SimpleSubscriber test, fail on a timeout instead of hanging
- drop an unused socket helper in the fs2 interrupted test
It now also validates the body's length, not only tracks subscriptions.
adamw added 3 commits October 9, 2026 15:03
…ests

Points to softwaremill/ox#528, which would make the tracker unnecessary.
The other Netty servers cancel request processing when the connection closes, so these tests could not fail there.
…bled

With interruption enabled (the default), request processing is cancelled before a truncated body is decoded. The tests now wait until request processing starts, as Sync skips requests whose connection closed before that.
@adamw
adamw merged commit 077e008 into master Oct 9, 2026
22 checks passed
@adamw
adamw deleted the fix/pass-down-incomplete-body-on-connection-loss branch October 9, 2026 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants