Skip to content

[BUGFIX] Drain deduplicated upload request bodies - #161

Merged
hferentschik merged 2 commits into
mainfrom
dd/fix/drain-deduplicated-upload-body-20260921
Sep 22, 2026
Merged

hferentschik merged 2 commits into
mainfrom
dd/fix/drain-deduplicated-upload-body-20260921

Conversation

@marchmallow

@marchmallow marchmallow commented Sep 22, 2026 •

Copy link
Copy Markdown
Member

Motivation & Context

  • Goal: Large deduplicated PUT requests are fully consumed before HTTP 200 is written, avoiding Go's HTTP/1.1 unread-body connection-close path.
  • Root cause: The v2 deduplication path returned before consuming the PUT body. At 256 KiB of unread body, Go's HTTP/1.1 server switches to Connection: close; after its 500 ms reset-avoidance wait, the final close can prevent an intermediary from delivering the already-produced response headers to the client.
  • Why now: Repeated multi-megabyte uploads reliably exposed this behavior in a proxied environment.
  • Defense in depth: Request-body reads are now bounded to the already-validated Content-Length, and limit errors are mapped consistently across the deduplication and physical-write paths.
  • Follow-up: After release, validate repeated large deduplicated uploads through a representative proxy path. Separately configure/document an appropriate request-body read timeout and add stalled-upload coverage.
Observable behavior Before After
Repeated PUT for an existing large blob The handler can return HTTP 200 with an unread body, causing Go to close the HTTP/1.1 connection for sufficiently large bodies. The handler consumes and validates the complete declared body before returning HTTP 200.
Body exceeds its validated declared length No explicit read bound on the handler-provided reader. http.MaxBytesReader stops after observing Content-Length + 1 bytes and the handler returns HTTP 413 when the error remains recoverable.
Clean EOF below the declared length on the deduplication path The body is not read. The handler returns HTTP 400 instead of acknowledging an incomplete request.
Premature network EOF or another body/storage read failure The deduplication path does not observe it. The handler attempts to return HTTP 500.
Valid new-blob PUT The server consumes the body and returns HTTP 201. Successful behavior is unchanged; the supplied storage reader is now bounded to the validated declaration.
HTTP schema and successful response payloads Existing v2 contract. Unchanged.

Interface Changes

  • Modified: Existing PUT /v2/blobs/put deduplication behavior now requires the server to consume the declared request body before returning HTTP 200.
  • Malformed direct/custom bodies that exceed the validated declaration are rejected with HTTP 413 when the limit error is preserved.
  • A clean short EOF on the deduplication path is rejected with HTTP 400; premature network EOF and other read failures retain HTTP 500 handling.
  • No endpoint, request schema, response schema, or successful status code changes.
  • No caller changes are required.

Why use a local http.MaxBytesReader?

The handler already rejects Content-Length values above its configured maximum. The validated declaration is therefore a tighter and more appropriate runtime read limit than the full service maximum.

The wrapper is kept in a local variable and is not assigned back to r.Body. This preserves Go 1.25's concrete request-body types so net/http can retain its special early-response behavior when validation fails before the body is consumed, including requests using Expect: 100-continue.

The deduplication path drains the bounded reader and checks the copied byte count before writing HTTP 200. The physical-write path passes the same bounded reader through the checksum tee to the selected storage driver.

Why does this also change the GCS storage driver?

On a deduplication hit, the handler itself reads the bounded body and can classify *http.MaxBytesError directly. On a physical write, the storage driver consumes the supplied reader, so any limit error is returned through that driver's error chain.

The GCS driver previously formatted its io.Copy failure with %v:

fmt.Errorf("io.Copy: %v", err)

That preserved only the error text and discarded the underlying error identity. Consequently, the handler's errors.As check could not recover *http.MaxBytesError, and an oversized physical write through GCS would be reported as HTTP 500 instead of HTTP 413.

The GCS wrapper now uses %w:

fmt.Errorf("io.Copy: %w", err)

This does not change the upload outcome: GCS still stops and returns an error. It only preserves the cause so the handler can classify body-limit failures consistently across storage backends. A small copy helper makes this behavior testable without credentials or a live GCS bucket.

Validation

  • A direct reproduction against the unmodified Go server found that a 262,143-byte deduplicated body returns HTTP 200 without closing the connection, while 262,144 bytes returns HTTP 200 with Connection: close.
  • Multi-megabyte deduplicated requests returned after approximately 502 ms, matching Go's 500 ms reset-avoidance delay and the observed client failure timing.
  • Tests verify that a large deduplicated body is fully consumed, including EOF observation, before HTTP 200 is committed.
  • Boundary and failure tests cover exact length, zero length, clean short EOF, io.ErrUnexpectedEOF, unrelated reader failures, and an overrun that reads exactly one byte beyond the declaration before returning HTTP 413.
  • Physical-write tests verify that valid writes still return HTTP 201 and wrapped body-limit errors return HTTP 413.
  • A credential-free GCS regression verifies that body-limit and unrelated read errors remain discoverable through its wrapper.
  • Raw HTTP/1.1 tests verify that early validation errors are returned without waiting for an unsent body, both with and without Expect: 100-continue.
  • The focused handler, GCS, and non-container server tests pass under Go 1.25.6. Lint, copyright, license, GitHub Actions, DDCI Task Sourcing, and mergegate also pass.
  • Not yet verified: End-to-end behavior through an intermediary with this fix. This will be verified after a release is available using repeated large deduplicated uploads.

Risks

  • A slow duplicate upload now keeps the handler occupied while it reads the remaining body. A byte limit does not bound elapsed read time. A configured request-body read timeout bounds this wait, but the public executable and embedding example do not currently configure one.
  • If draining fails, the server attempts to return an error instead of the previous HTTP 200; depending on the transport failure, the client may receive no response. The existing blob remains unchanged and no stored data is corrupted.
  • Normal HTTP/1.1 and HTTP/2 framing can reject malformed bodies before the handler's wrapper observes them. The synthetic overrun coverage is defense in depth for direct/custom readers, not a promise that every malformed wire request reaches the handler-level HTTP 413 path.
  • Valid duplicate uploads perform the additional body read. Valid new uploads already consume the complete body.

@marchmallow
marchmallow marked this pull request as ready for review September 22, 2026 08:11
@marchmallow marchmallow changed the title Drain deduplicated upload request bodies [ATLAS] Drain deduplicated upload request bodies Sep 22, 2026
@marchmallow marchmallow changed the title [ATLAS] Drain deduplicated upload request bodies [BUGFIX] Drain deduplicated upload request bodies Sep 22, 2026
@DataDog DataDog deleted a comment from datadog-ddstaging Bot Sep 22, 2026
Comment thread server/handler/v2/handler.go Outdated
@marchmallow
marchmallow force-pushed the dd/fix/drain-deduplicated-upload-body-20260921 branch from ae41152 to 603a588 Compare September 22, 2026 11:31
@marchmallow

Copy link
Copy Markdown
Member Author

@hferentschik concern on checking size addressed but a bit more complicated than 1 line of code changes, agent rationale for it added to the PR description under "Why use a local http.MaxBytesReader?"

@hferentschik
hferentschik merged commit 603a588 into main Sep 22, 2026
3 checks passed
@hferentschik
hferentschik deleted the dd/fix/drain-deduplicated-upload-body-20260921 branch September 22, 2026 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants