fix: stream blob bodies with per-chunk deadlines

[datadot]

What type of PR is this?

bug

Which issue does this PR fix: Fixes #4140

What does this PR do / Why do we need it:

http.Server's ReadTimeout and WriteTimeout (added in #3984) are deadlines on the whole request and response, set when it starts. A blob upload or download that runs longer than the timeout is therefore cut off mid-stream once it passes the deadline, even while data is still flowing. It shows up as a TCP i/o timeout (WriteDataFromReader in pkg/api/routes.go) and a truncated body for the client, and as an occasional failure in the GC-on-S3 stress job.

This resets the deadline per chunk via http.NewResponseController, so the timeout behaves as an idle deadline: a transfer that keeps making progress completes, a stalled one still times out. It covers the blob serve paths (WriteDataFromReader and the multipart-range path) and the blob receive paths (FullBlobUpload, PutBlobChunk, PutBlobChunkStreamed, FinishBlobUpload). A zero timeout leaves deadlines off, so readTimeout/writeTimeout set to 0 still disables them.

statusWriter (the session-logging middleware wrapper) now implements Unwrap, otherwise the ResponseController stops at the wrapper and the deadline calls return http.ErrNotSupported and do nothing.

If an issue # is not available please add repro steps and logs showing the issue:

failed to copy data into http response: write tcp ...: i/o timeout   (pkg/api/routes.go)

Triggered by uploading or downloading a blob that takes longer than writeTimeout/readTimeout (default 60s), e.g. a multi-GB layer over a slow link, or a blob read under load in the GC-on-S3 stress test.

Testing done on this change:

Extended TestWriteDataFromReader to cover streaming with a deadline set, the http.ErrNotSupported path (recorder without deadline support), and the zero-timeout case. Existing blob GET, multipart-range and upload handler tests pass. Built and linted with golangci-lint v2.12.2.

Automation added to e2e:

No new e2e. The existing GC-on-S3 stress job exercises the serve path under load and acts as a regression check.

Will this break upgrades or downgrades?

No.

Does this PR introduce any user-facing change?:

HTTP readTimeout/writeTimeout now act as idle timeouts during blob upload and download: a transfer that keeps making progress is no longer aborted once it exceeds the configured timeout. They still bound idle connections, and setting either to 0 disables them. Note this means writeTimeout/readTimeout no longer cap the total time of a blob transfer.

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

[Copilot]

Pull request overview

This PR changes blob upload/download streaming to use http.ResponseController to extend connection read/write deadlines during transfers, so the configured http.Server ReadTimeout/WriteTimeout behave like idle timeouts for long-running blob I/O rather than absolute end-to-end caps.

Changes:

• Add statusWriter.Unwrap() so http.ResponseController can reach the underlying ResponseWriter/connection through the session-logging wrapper.
• Update blob GET (including multipart ranges) and blob upload handlers to stream with per-transfer deadline extensions derived from readTimeout/writeTimeout.
• Extend TestWriteDataFromReader to cover deadline-support, non-support, and zero-timeout behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
pkg/api/session.go	Adds Unwrap() to the session logging ResponseWriter wrapper to enable http.ResponseController deadline support through middleware.
pkg/api/routes.go	Implements deadline-aware streaming helpers and wires them into blob serve/upload paths using configured HTTP timeouts.
pkg/api/routes_test.go	Adds unit tests validating streaming behavior with/without deadline support and when timeout is zero.
pkg/api/config/config.go	Updates timeout field comments to document idle-timeout behavior for blob transfers.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[datadot]

Rebased onto main (resolved the conflict with the webhook-events change in the blob upload handlers) and addressed the review feedback.

On the deadline granularity: switched the write path from a 10MiB io.CopyN loop to io.Copy through a per-Write deadline wrapper, mirroring the read side. io.Copy writes in ~32KiB chunks, so the deadline now refreshes roughly every 32KiB instead of every 10MiB, which keeps it an idle timeout across link speeds and small configured timeouts.

On io.ErrUnexpectedEOF: io.CopyN returns io.EOF, not io.ErrUnexpectedEOF, when the source ends before the requested count, so the previous loop returned cleanly on a short final chunk rather than logging an error. The io.Copy version returns nil on a normal EOF regardless, so that path no longer applies.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[codecov]

Codecov Report

❌ Patch coverage is 95.12195% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.68%. Comparing base (ab36dc1) to head (2ddf8a4).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/api/routes.go	94.87%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4149   +/-   ##
=======================================
  Coverage   91.67%   91.68%           
=======================================
  Files         203      203           
  Lines       29879    29904   +25     
=======================================
+ Hits        27391    27416   +25     
+ Misses       1604     1603    -1     
- Partials      884      885    +1     

☔ 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.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[andaaron]

As far as I remember the timeout is supposed to be a fix in case an attacker deliberately sends slow traffic to hold goroutines indefinitely. We're supposed to cut loose those clients as far as I understand.

[rchincha]

In writeMultipartRanges, use io.CopyN(newStreamDeadlineWriter(...), pipeReader, contentLength) instead of plain io.Copy

[rchincha]

Also, possible loss of sendfile / optimized copy behavior on filesystem-backed blob downloads, at least for local storage based deployments.

[datadot]

On the security intent (@andaaron): the #3984 timeout is there to cut clients that hold a connection with slow traffic. A per-read/write deadline still cuts a client that stalls (no bytes within the window), which covers the usual slow-loris case, but a client that trickles just enough to keep resetting it would survive, so it does not bound total connection lifetime the way a fixed deadline does. If bounding the worst case is the goal, the options are roughly:

1. Idle deadline only: fixes the large-transfer failure and cuts stalled clients, but a steady trickle lives on.
2. Keep an absolute deadline, sized for real transfers (larger than 60s, or derived from blob size).
3. Hybrid: an idle deadline plus a generous absolute cap.

Which model would you like? I'll adapt accordingly.

On sendfile (@rchincha): the local driver returns an *os.File, so wrapping the ResponseWriter defeats response.ReadFrom and the zero-copy path. To keep sendfile, the deadline has to be set on the raw response inside an io.CopyN loop (per chunk) rather than via a wrapper. That preserves sendfile but makes the deadline granularity the chunk size instead of per write, which is the other side of the granularity point Copilot raised. A moderate chunk (around 1MiB) keeps sendfile efficient while staying tolerant of slow-but-real links. Uploads and multipart ranges read from the network and a pipe respectively, not an *os.File, so sendfile does not apply there and a wrapper is fine for them.

On the multipart copy (@rchincha): switching it back to io.CopyN(newStreamDeadlineWriter(...), pipeReader, contentLength) so the exact Content-Length invariant holds.

[rchincha]

| If bounding the worst case is the goal, the options are roughly:

Idle deadline only: fixes the large-transfer failure and cuts stalled clients, but a steady trickle lives on.
Keep an absolute deadline, sized for real transfers (larger than 60s, or derived from blob size).
Hybrid: an idle deadline plus a generous absolute cap.
Which model would you like? I'll adapt accordingly.

Unfortunately, none of these are great options!

[rchincha]

After thinking about this some more, this is sounding more like a keepalive mechanism for long running connections and re-purposing the read/write timeouts for this.

[datadot]

The keepalive framing fits the problem better than a request deadline. TCP keepalive works at the transport layer: it probes idle connections and drops peers that stop responding (dead or abandoned), while a connection that is actively transferring, even slowly, stays up. That is the distinction the absolute read/write deadline cannot make, since it fires on wall-clock regardless of whether bytes are still moving.

That would mean:

• On the blob upload and download handlers, clear the per-request read/write deadline (set it to zero via the ResponseController) so a large transfer is not capped by the server timeout. This also keeps sendfile, since the response is not wrapped.
• Rely on TCP keepalive to reap dead or abandoned connections. zot listens with a plain net.Listen, so keepalive is whatever Go defaults to; we can make it explicit and tune the period via net.ListenConfig.
• Leave the 60s read/write timeout in place for the non-streaming endpoints (manifests, API), where it does its original job.

The case this does not cover is a client that stays responsive but deliberately trickles bytes to hold a goroutine. Keepalive will not drop it (it answers the probes), and no per-request deadline stops it without also cutting legitimate slow transfers, which is the bug being fixed. The options for that case, none of them free:

• Minimum-throughput floor: track bytes/sec over a window on the streaming read/write and cut below a floor. The only option that targets the attack directly, since a real slow link still sustains some rate while a 1-byte-per-window trickle does not. Cost is a small rate-tracking reader/writer and choosing the floor.
• Concurrency caps: bound max connections globally (netutil.LimitListener) and/or per IP, so trickle clients cannot exhaust goroutines. This caps the blast radius rather than stopping a single slow client, and would be new, since the current tollbooth limiter is request-rate, not concurrent-connection.
• Absolute request cap: a hard ceiling as in fix(security): enhance timeout configurations and body size limits fo… #3984, which bounds the worst case but cuts legitimate large transfers unless set generously.

I'd probably go for keepalive for dead connections plus a concurrency cap for blast radius, adding the throughput floor only if we want to actively cut trickle clients.

If that direction works I can rework the PR around it: clear the deadline on the streaming handlers, make keepalive explicit on the listener, and drop the deadline wrappers. The trickle case is the bit worth agreeing on first, since it's a genuine tradeoff.

[datadot]

Rebased onto latest main and resolved the conflict in pkg/api/routes.go (the blob-upload path was refactored upstream), so this is current and conflict-free again.

@rchincha, friendly nudge on this one. Going off your keepalive framing, the direction I sketched above was: clear the per-request read/write deadline on the streaming blob handlers, make TCP keepalive explicit/tunable on the listener via net.ListenConfig, and keep the 60s read/write timeout for the non-streaming endpoints. The one open call is the trickle-client case, since keepalive alone won't reap a responsive-but-trickling connection. My lean is keepalive plus a concurrency cap for blast radius, with a minimum-throughput floor only if we want to actively cut trickle clients.

If that direction works for you I'll rework the PR around it. Just need your read on the trickle tradeoff before I do.