fix(cf-workers): size aws-chunked streamed PUTs so the egress leg has a Content-Length - #140
Draft
alukach wants to merge 1 commit into
Draft
fix(cf-workers): size aws-chunked streamed PUTs so the egress leg has a Content-Length#140alukach wants to merge 1 commit into
alukach wants to merge 1 commit into
Conversation
… a Content-Length `fixed_body_length` returned `None` for any `content-encoding: aws-chunked` body, so those PUTs took the raw `init.set_body(stream)` branch. Per that branch's own doc comment, a bare `ReadableStream` makes the Workers runtime send the subrequest with `Transfer-Encoding: chunked` and drop `Content-Length` — leaving the outbound HTTP leg unsized. The exemption was justified by "S3 sizes those from `x-amz-decoded-content-length`", which sizes only the de-chunked payload, not the transfer. On data.source.coop this correlates with ~0.6% of aws-chunked PUTs dying as a Cloudflare-minted 520 on the worker's egress leg, with S3 never producing a response at all. `Content-Length` is the encoded byte count (chunk framing and trailer included), which is exactly what goes on the wire, so it sizes the leg without reframing the body. It is forwarded unsigned (see `build_streaming_forward`), so changing the transfer framing cannot invalidate the SigV4 signature. Moves the helper to `multistore::backend::streamed_put_body_length` so it can be unit tested: `crates/cf-workers` is not in the workspace `default-members`, so a `#[cfg(test)]` module there is never compiled by `cargo test`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B3x2KzUtojpwPvDKmSgvht
|
Claude finished @alukach's task in 1m 38s —— View job ✅ No blocking issues — safe to merge. Reviewed the diff ( Correctness
Tests
Simplify (ponytail): none — the new function is a minimal header lookup, the move to Branch: 💰 Estimated review cost: $0.44 · 1m37s · 26 turns |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Candidate fix for intermittent 520s on streamed uploads through the Workers runtime. See source-cooperative/data.source.coop#206 for the production evidence.
What I'm changing
WorkerBackend::forwardstreams PUT bodies through without buffering them into WASM memory. A bareReadableStreambody makes the Workers runtime send the subrequest withTransfer-Encoding: chunkedand dropContent-Length, so the branch wraps the stream in aFixedLengthStreamto force a realContent-Length— except thatfixed_body_length()returnedNonefor anycontent-encoding: aws-chunkedbody, exempting exactly the shape AWS SDKs and the CLI send by default.The exemption was justified in-comment by "aws-chunked bodies are sized by S3 from
x-amz-decoded-content-length". That is true for the object: S3 uses it to size the payload it reconstructs after de-chunking. It says nothing about the HTTP leg, which was left unsized.Content-Lengthis the correct size for an aws-chunked body too. It counts the bytes actually placed on the wire — chunk framing and trailer included — e.g.10094598encoded vs10094463decoded, a 135-byte gap of framing plus the CRC32 trailer. Sizing the leg does not reframe the body, so the chunk framing still reaches S3 untouched. It is forwarded unsigned (build_streaming_forward: "the transfer framing is the runtime's to manage, so signing it risks a mismatch"), so changing the framing cannot invalidate the SigV4 signature.Why this is a candidate and not a confirmed fix
On
data.source.coop, ~0.6% of aws-chunked PUTs (25/4,059 in a 15-minute production sample) fail as a Cloudflare-minted 520 on the worker's own egress leg —server: cloudflare, nox-amz-*headers, a 16-byteerror code: 520body, dead in 34–701 ms. S3 never produces an HTTP response at all.That rate is the problem for this change's own theory. The failures are Poisson (CV of inter-failure gaps = 0.99) and independent of body size (failed p50 6.56 MB vs succeeded 6.76 MB) and of colo (0.43% vs 0.82% across the only two in the sample). A structurally identical request failing 0.6% of the time is a race, not a protocol defect — if an unsized chunked leg were simply rejected, the rate would be ~100%, not 0.6%. S3 evidently accepts this wire shape thousands of times per window.
So the missing
Content-Lengthcan be at most a predisposing condition, never the trigger. The surviving form of the argument is second-order: a request with a knownContent-Lengthis replayable, whereas a chunked body streaming from a live inbound socket is not — so a transient egress event (a stale pooled keep-alive connection, a reset) that would otherwise be retried invisibly instead surfaces as an empty response. Pooled-connection races are Poisson, size-independent, and kill you just after headers, which fits. That is a plausible mechanism, not a demonstrated one, and the 520 is minted inside Cloudflare's egress so S3's side of the connection is not observable from the worker.This PR may therefore be a no-op for the bug. #141 (stacked on this branch) is the experiment that tells you: it captures the
pipe_torejection, and a 520 arriving with no rejection is connection-side evidence for this change, while one arriving with a rejection means the fault is body-side and this change is irrelevant. Landing this without that instrumentation risks a green soak that nobody can attribute.Independent of whether it fixes the 520s, the sizing itself is a correctness improvement: an unsized leg is a strictly worse thing to hand an origin than a sized one, at no cost.
How I did it
crates/core/src/backend/mod.rs— newpub fn streamed_put_body_length(&HeaderMap) -> Option<u64>: the old helper minus the aws-chunked early return, soContent-Lengthnow sizes both body shapes. ReturnsNoneonly when there is no usableContent-Length. Doc comment records why the encoded length is the right number and why this does not disturb the framing.crates/cf-workers/src/backend.rs— drops the localfixed_body_length, calls the core helper, and rewrites the stale rationale comment on the PUT branch.crates/cf-workersis absent from the workspacedefault-members, socargo testnever compiles it and a#[cfg(test)]module there would be dead (the existing one incors.rsnever runs). The function is pure header inspection with nowasmdependencies, so hosting it incrates/core— next to the existing freepub fn create_builder— is what makes it testable at all. No visibility widening was needed elsewhere.docs/change: the sizing behaviour is not documented there (docs/reference/operations.mdcovers aws-chunked only for conditional-write precondition forwarding).Cargo.lockdeliberately left untouched; it carries an unrelated stale0.7.1→0.7.2version correction.Test plan
Bug-first, per
CLAUDE.md. The three tests were added against the unfixed helper andaws_chunked_put_is_sized_by_encoded_content_lengthfailed as intended (left: None, right: Some(10094598)) before the early return was removed — so the test demonstrably detects the bug rather than merely passing.cargo test— 161 core + all suites pass, 0 failurescargo fmtcargo clippy --all-targets— clean (one pre-existingtoo_many_argumentswarning, unrelated)cargo check -p multistore-cf-workers --target wasm32-unknown-unknownNot covered here, and needed before this can be called confirmed:
Content-Lengthand noTransfer-Encoding: chunked, and that the bytes arrive byte-identical with framing and CRC32 trailer intact. This needs a workerd/miniflare harness, which does not exist in this repo.(1-0.006)^100≈ 55%. Roughly 1,000 consecutive clean multi-MB aws-chunked PUTs puts the odds of a broken fix still looking clean near 0.2%.🤖 Generated with Claude Code
https://claude.ai/code/session_01B3x2KzUtojpwPvDKmSgvht