fix(uploadstore): bound R2 Serve body read

[SebTardif]

What Problem This Solves

Fixes an issue where users opening an R2-backed upload (image preview, file download, range seek) would see the request hang forever when Cloudflare R2 accepted the GET, returned headers, and then stopped sending body bytes. The outbound client only bounds the wait for response headers. After headers arrive, ServeHTTP copies the body with Client.Timeout left at 0, so a stalled origin never unblocks the download handler.

This is the R2 serve path used by GET /api/uploads/:upload_id, not the browser fetch timeout from #168 and not a change to the streaming-safe overall client timeout.

Why This Change Was Made

ServeHTTP now wraps the R2 response body with an idle read deadline (30s). Each Read that makes no progress in that window closes the body and returns r2 serve: body read stalled. Bytes that keep arriving reset the idle timer, so large or slow-but-live objects still stream.

http.Client.Timeout stays 0, matching TestR2ConfigValidation and the #168 contract. The empty DefaultTransport fallback is left unchanged.

User Impact

A hung R2 object body fails the download after 30s of idle instead of pinning the upload handler until the user disconnects. Successful and progressing streams are unchanged.

Evidence

Before the patch, ServeHTTP stayed in io.Copy after the origin flushed headers and sent no body. A 15s process timeout still had the copy blocked:

panic: timed out after 15s
io.Copy(...)
github.com/openclaw/clickclack/apps/api/internal/uploadstore.(*R2).ServeHTTP
FAIL

After the patch, the same stalling origin against production NewR2 (default 30s idle, Client.Timeout still 0) returns instead of hanging:

$ go run ./apps/api/cmd/r2stallproof
origin_headers=flushed
elapsed_ms=30054
err=r2 serve: body read stalled

Client.Timeout is still 0 (Timeout != 0 would fail TestR2ConfigValidation). A 50ms idle on the same Serve path returns r2 serve: body read stalled in 0.10s.

Real behavior proof

• Behavior or issue addressed: R2 ServeHTTP hung forever when the origin sent response headers and then stalled the object body, because Client.Timeout is 0 and only ResponseHeaderTimeout was set.

• Real environment tested: macOS Darwin 25.6.0 arm64, go1.27.0, branch fix/f001-r2-body-deadline at the patched tree /tmp/oc-pr-clickclack-F001.

• Exact steps or command run after this patch:

  cd /tmp/oc-pr-clickclack-F001
  go run ./apps/api/cmd/r2stallproof

  The program built NewR2 against a local origin that flushed a 200 with Content-Length: 100 and then sent no bytes.

• Evidence after fix: terminal output from the patched tree:

  $ go run ./apps/api/cmd/r2stallproof
  origin_headers=flushed
  elapsed_ms=30054
  err=r2 serve: body read stalled

• Observed result after fix: After headers flushed, ServeHTTP returned r2 serve: body read stalled at 30054ms (the 30s idle bound). It did not remain blocked on the body copy. Client.Timeout remains 0.

• What was not tested: Live Cloudflare R2 credentials and a production bucket GET. Save/Delete paths are unchanged.

Related

• fix(web): abort hung API and slash-command fetches #168 kept R2 Client.Timeout == 0 for streaming; this PR bounds the Serve body without changing that contract
• Introduced with R2 serve in faacf69 (2026-05-17) and header-only bounding in f7a10e9 (2026-05-17)

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs changes before merge. Reviewed August 29, 2026, 2:59 PM ET / 18:59 UTC.

ClawSweeper review

What this changes

The PR adds a 30-second idle deadline around R2 object-body reads so R2-backed upload downloads do not wait indefinitely after response headers arrive.

Merge readiness

⚠️ Needs maintainer review before merge - 3 items remain

Keep open: the idle timeout bounds the stalled R2 read, but it now returns an error after the upload response has already started, causing the API handler to append a JSON error to a partial file response.

Priority: P2
Reviewed head: 3459131e309bcdf1a64429dd4de7dc48494db7d0

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The stalled-read recovery is demonstrated, but the post-header error path can still produce an invalid download response.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (terminal): The PR body provides a patched-tree macOS terminal trace of production NewR2 against a local HTTP origin that flushes headers then stalls, observing return after the 30-second idle bound; the remaining gap is endpoint response framing rather than proof of stalled-read recovery.
Patch quality	🦐 gold shrimp (3/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The PR body provides a patched-tree macOS terminal trace of production NewR2 against a local HTTP origin that flushes headers then stalls, observing return after the 30-second idle bound; the remaining gap is endpoint response framing rather than proof of stalled-read recovery.
Evidence reviewed	5 items	Current main leaves body reads unbounded: The fetched main revision calls io.Copy directly on the R2 response body after writing the upload status, without a body-read deadline. Introduced timeout returns a post-header error: The PR wraps the response body before ServeHTTP writes the successful response; the wrapper closes a stalled body and returns errServeBodyStalled from io.Copy. API error writer runs after serving errors: getUpload handles every upload-store error with writeError, while writeError calls writeJSON; for a stalled stream this happens after R2 ServeHTTP has already called WriteHeader and may have copied bytes.
Findings	1 actionable finding	[P2] Avoid appending JSON errors to a started upload response
Security	None	None.

How this fits together

ClickClack’s upload store retrieves R2-backed files for the authenticated upload endpoint. The endpoint starts the client response before streaming the R2 body, and later read errors return through the API handler.

flowchart LR
  A[Authenticated upload request] --> B[Upload API handler]
  B --> C[R2 upload store]
  C --> D[R2 response body]
  D --> E[Stream bytes to client]
  D --> F[Idle-read timeout]
  F --> G[Post-header error handling]

Loading

Before merge

• Avoid appending JSON errors to a started upload response (P2) - The new wrapper turns a stalled body into an error after ServeHTTP has already written the R2 status and may have copied bytes. getUpload then calls writeError, which serializes JSON to that same response; clients can receive partial file data followed by API error bytes under the original successful status. Track response commitment or return a stream-termination outcome that the endpoint does not serialize.
• Resolve merge risk (P1) - An existing R2 deployment that stalls after a partial response can receive a successful file status with appended JSON error bytes, producing a corrupt or ambiguous download rather than a clean failed stream.
• Complete next step (P2) - Repair the introduced post-header error handling before merge; the narrow endpoint and regression test are clear.

Findings

• [P2] Avoid appending JSON errors to a started upload response — apps/api/internal/uploadstore/r2.go:192

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Changed surface	2 files; production +60/-17, tests +62/-0	The patch adds one response-body wrapper and one direct store-level stalled-read test.

Merge-risk options

Maintainer options:

1. Handle post-header stream errors (recommended)
   Prevent the upload handler from serializing an HTTP error after the object response has already started, then cover a partially streamed stalled body.
2. Accept truncated-download behavior
   Land the idle bound while accepting that clients may receive a partial response when R2 stalls after headers.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Preserve the established upload response once headers are committed; add an end-to-end stalled-body regression test that proves no JSON error bytes are appended.

Technical review

Best possible solution:

Preserve a clean terminated download after a post-header R2 read failure and add an endpoint-level regression test for a partially delivered stalled object.

Do we have a high-confidence way to reproduce the issue?

Yes: current-main source shows the unbounded post-header io.Copy path, and the submitted trace exercises the corresponding stalled-body scenario against NewR2.

Is this the best way to solve the issue?

No: bounding the read is appropriate, but the endpoint must not write an HTTP error after file headers and bytes have begun.

Full review comments:

• [P2] Avoid appending JSON errors to a started upload response — apps/api/internal/uploadstore/r2.go:192
  The new wrapper turns a stalled body into an error after ServeHTTP has already written the R2 status and may have copied bytes. getUpload then calls writeError, which serializes JSON to that same response; clients can receive partial file data followed by API error bytes under the original successful status. Track response commitment or return a stream-termination outcome that the endpoint does not serialize.
  Confidence: 0.95

Overall correctness: patch is incorrect
Overall confidence: 0.93

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning high; reviewed against 486fd23545af.

Labels

Label changes:

• add P2: This is a bounded reliability change to R2-backed upload downloads.
• add merge-risk: 🚨 compatibility: The timeout can return an error after a successful file response has been committed to existing clients.
• add proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides a patched-tree macOS terminal trace of production NewR2 against a local HTTP origin that flushes headers then stalls, observing return after the 30-second idle bound; the remaining gap is endpoint response framing rather than proof of stalled-read recovery.
• add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
• add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body provides a patched-tree macOS terminal trace of production NewR2 against a local HTTP origin that flushes headers then stalls, observing return after the 30-second idle bound; the remaining gap is endpoint response framing rather than proof of stalled-read recovery.

Label justifications:

• P2: This is a bounded reliability change to R2-backed upload downloads.
• merge-risk: 🚨 compatibility: The timeout can return an error after a successful file response has been committed to existing clients.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body provides a patched-tree macOS terminal trace of production NewR2 against a local HTTP origin that flushes headers then stalls, observing return after the 30-second idle bound; the remaining gap is endpoint response framing rather than proof of stalled-read recovery.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides a patched-tree macOS terminal trace of production NewR2 against a local HTTP origin that flushes headers then stalls, observing return after the 30-second idle bound; the remaining gap is endpoint response framing rather than proof of stalled-read recovery.

Evidence

Acceptance criteria:

• [P1] go test ./apps/api/internal/uploadstore.
• [P1] go test ./apps/api/internal/httpapi.

What I checked:

• Current main leaves body reads unbounded: The fetched main revision calls io.Copy directly on the R2 response body after writing the upload status, without a body-read deadline. (apps/api/internal/uploadstore/r2.go:209, 486fd23545af)
• Introduced timeout returns a post-header error: The PR wraps the response body before ServeHTTP writes the successful response; the wrapper closes a stalled body and returns errServeBodyStalled from io.Copy. (apps/api/internal/uploadstore/r2.go:192, 3459131e309b)
• API error writer runs after serving errors: getUpload handles every upload-store error with writeError, while writeError calls writeJSON; for a stalled stream this happens after R2 ServeHTTP has already called WriteHeader and may have copied bytes. (apps/api/internal/httpapi/features.go:525, 3459131e309b)
• R2 behavior history: The current ServeHTTP lines trace to the R2 feature history, with the original storage implementation and outbound-header timeout work authored by Peter Steinberger and the current body carried through a later refactor by Shakker. (apps/api/internal/uploadstore/r2.go:175, 711f893befdd)
• Submitted real behavior proof: The PR body includes a patched-tree macOS terminal trace using production NewR2 against a local origin that flushes headers then stalls; it reports err=r2 serve: body read stalled after about 30 seconds. (3459131e309b)

Likely related people:

• Peter Steinberger: Introduced Cloudflare R2 storage and the subsequent outbound upload timeout behavior in the R2 implementation history. (role: introduced R2 storage and header-timeout behavior; confidence: high; commits: faacf69ae08b, f7a10e989706; files: apps/api/internal/uploadstore/r2.go)
• Shakker: Current-main blame attributes the surviving R2 ServeHTTP body to this refactor-era commit. (role: current serve-path history contributor; confidence: medium; commits: 711f893befdd; files: apps/api/internal/uploadstore/r2.go)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Add an authenticated endpoint-level test showing a stalled R2 response cannot append JSON error bytes after upload streaming begins.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.