fix(uploadstore): preserve default transport while bounding R2 headers - #183
Conversation
When http.DefaultTransport is not *http.Transport, defaultR2HTTPClient
returned &http.Client{} with no ResponseHeaderTimeout. A stalled R2
origin then hung Save, Delete, and Serve until the caller disconnected.
Always attach a Transport with ResponseHeaderTimeout. Leave Client.Timeout
at 0 so streaming stays intact.
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs maintainer review before merge. Reviewed August 30, 2026, 6:54 PM ET / 22:54 UTC. ClawSweeper reviewWhat this changesThe PR routes default R2 save, delete, and download requests through the configured default HTTP transport while adding a 30-second post-write response-header deadline and preserving explicit client overrides. Regression provenancePossible regression — probable (reviewed change; failure trace). No predecessor PR is attributed. Merge readinessKeep this PR open for normal merge review: it fixes the current main fallback that returns a bare client for wrapped default transports, and the revised transport addresses the prior wrapper-bypass finding without a new correctness defect found. Priority: P2 Review scores
Verification
How this fits togetherR2 upload storage sends object save, delete, and download requests through an HTTP client. The new transport sits between those operations and the process-wide HTTP transport, enforcing a bounded response-header wait before response bodies reach the upload handlers. flowchart LR
A[R2 save delete download] --> B[Default R2 HTTP client]
B --> C[Delegating R2 transport]
C --> D[Configured default HTTP transport]
D --> E[R2 origin]
E --> F[Headers or stalled response]
C --> G[Header deadline]
F --> G
G --> H[Response body or timeout]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Merge the delegating transport with its documented explicit-client escape hatch so wrapped default transports remain effective while hook-aware transports receive bounded header waits. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main returns a bare client for a wrapped default transport, and the supplied real-origin test scenario directly exercises the resulting unbounded header wait. Is this the best way to solve the issue? Yes. Delegating through the configured transport, timing only the post-write header phase, and retaining an unchanged explicit-client override is the narrowest solution to the verified fallback defect. AGENTS.md: found, but no applicable review policy affected this item. Codex review notes: model internal, reasoning high; reviewed against c4f8978d922c. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Delegate through the configured default transport and compose its HTTP trace hooks. Time only the complete-header wait after a successful request write, with synchronized retry/phase cleanup and one response-body lifetime owner. Keep explicitly supplied clients unchanged and total client timeout disabled. Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
steipete
changed the title
|
@clawsweeper re-review The configured default transport is now delegated to, preserving wrappers while enforcing the after-write complete-header bound. The PR body includes fail-before/pass-after real HTTP evidence, a progressing PUT exceeding30s, and race/lifecycle coverage. The prior wrapper-bypass finding and rank-up move are addressed; supplied HTTP clients remain unchanged. Head2522032521d5e6d3482636b9f5dff5ee15dcdc5f. |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
Maintainer review complete at 2522032. The documented default-transport contract is accepted: delegation preserves configured wrappers, and the precise after-write deadline requires the existing Go trace/cancellation lifecycle. Explicit clients keep their own policy. This addresses the remaining review risk without bypassing routing or adding a total timeout that would break progressing uploads. Independent source and receipt review covered the shared lifetime owner, retry/phase races, cleanup, client contracts, and the real network proof: incomplete headers stopped at 30.002 seconds; a 35.082-second PUT completed through the configured wrapper. Full exact-head CI and all desktop jobs are green. The earlier wrapper-bypass finding and rank-up move are addressed; the fresh review has no actionable finding. |
2 participants
R2 header waits are now bounded without bypassing a process's configured
http.DefaultTransport. The default client uses one delegating RoundTripper, replacing the concrete-transport clone/bare-client split and the submitted replacement transport. An explicitly suppliedR2Config.HTTPClientremains the complete, unchanged override; productionopenUploadStorageuses the default client.The 30-second header timer starts only after a successful complete request write, through composed
httptrace.WroteRequest. It stops when complete headers return, not at the first byte. RetryGetConndisarms the previous wait. A mutex, phase, and generation prevent late timer callbacks or hooks from cancelling a later phase. One child context and response wrapper release resources on error, EOF, or Close without cancelling a successful body at RoundTrip return.Client.Timeoutremains zero.Exact after-write timing relies on Go's trace/cancellation lifecycle and wrappers forwarding it. An opaque transport that ignores those hooks or cancellation cannot offer the same bound. Body-read idle limits remain in #182.
Proof
go test ./...,go vet ./..., and focusedgo test -racepassed. Isolated P0-scope review found no accepted actionable finding.The prior ClawSweeper finding and rank-up move are addressed: the configured wrapper stays in the request path and its invocation is proven. Synthetic local origins and credentials only; no production bucket was accessed. Thanks @SebTardif for the original report/fix; contributor history and credit are preserved.
Exact head
2522032521d5e6d3482636b9f5dff5ee15dcdc5f: Go, TypeScript, full Playwright, Docker and all three desktop platforms passed. The immutable test binary's real HTTP receipt measured incomplete-header cancellation at30.002s and successful progressingPUT at35.082s. Fresh ClawSweeper review found no new correctness defect and marked the PR ready for maintainer review.The remaining review caveat is the accepted, documented transport contract: an opaque wrapper that omits trace hooks or ignores cancellation cannot provide equivalent after-write timing. A fixed whole-request timer would break progressing PUTs, so no such fallback is added.