feat(cf-workers): log the streamed PUT body pipe's rejection - #141
Draft
alukach wants to merge 1 commit into
Draft
feat(cf-workers): log the streamed PUT body pipe's rejection#141alukach wants to merge 1 commit into
alukach wants to merge 1 commit into
Conversation
`WorkerBackend::forward` dropped the promise from `stream.pipe_to(&transform.writable())`. The reasoning held — a pipe failure also errors `readable`, so the awaited outbound fetch fails anyway — but the fetch only reports *that* it failed, never why the body stopped, which leaves no way to tell a body-side failure from a connection-side one. That distinction is what the intermittent 520s on streamed PUTs turn on (source-cooperative/data.source.coop#206): the failures are Poisson and size-independent, so they are a per-request race, and the two candidate races sit on opposite sides of this promise. If the pipe rejects on a failing request the body is at fault; if it stays healthy while the fetch still returns 520, the connection is. The pipe is still not awaited inline — that would deadlock, since nothing drains `readable` until the fetch runs. Only the rejection is captured, so backpressure and streaming behaviour are unchanged. 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 41s —— View job Review
✅ No blocking issues — safe to merge.
Simplify (ponytail) Nothing to flag — the diff reuses the existing 💰 Estimated review cost: $0.38 · 1m41s · 18 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.
Diagnostic for source-cooperative/data.source.coop#206. This changes no behaviour; it exists to discriminate between two competing explanations that current logging cannot separate.
What I'm changing
WorkerBackend::forwarddropped the promise returned bystream.pipe_to(&transform.writable()). The existing reasoning is sound as far as it goes — a pipe failure also errorsreadable, so the awaited outbound fetch fails regardless. But the fetch only reports that it failed, never why the body stopped, and that is exactly the missing bit.Production evidence on
data.source.coop: ~0.6% of streamed PUTs die as a Cloudflare-minted 520 on the worker's egress leg, with S3 never answering. The rate is Poisson (CV of inter-failure gaps = 0.99) and independent of body size (failed p50 6.56 MB vs succeeded 6.76 MB), which makes it a per-request race rather than a protocol defect — a structural defect would fail ~100% of the time, not 0.6%.The two candidate races sit on opposite sides of this promise:
Right now we cannot tell which, so we cannot tell whether #140 is a fix or a no-op.
Why this is stacked on #140 rather than independent
On
mainthe pipe exists only in theSome(len)(FixedLengthStream) branch.fixed_body_length()returnsNoneforaws-chunked, so those requests takeinit.set_body(stream)and never touch the pipe at all — and every failure in the 4,059-PUT production sample wasaws-chunked.So on
mainthis handler instruments a path the dominant failure mode never takes, and would log nothing. #140 is what routesaws-chunkedthrough the transform, which is what makes this promise observable for the requests that actually fail. Landing this on its own would produce silence and be misread as evidence.A useful corollary: it also means the dropped pipe cannot be the cause of the
aws-chunked520s as they occur onmaintoday — there is no pipe on that path. The pipe-race theory only ever applied to the plain-Content-Lengthshape.How I did it
crates/cf-workers/src/backend.rs— bind the pipe promise, clonerequest.request_idfor correlation, andspawn_locala task that logs awarnwith theJsValueerror if it rejects. The pipe is deliberately still not awaited inline: nothing drainsreadableuntil the fetch below runs, so awaiting it there would deadlock. Only the rejection is captured, so backpressure and streaming are unchanged.Side effect worth expecting: the unattributed
TypeError: Can't read from request stream after responding with an exceptionalready visible in Workers logs should now arrive as an attributedstreamed PUT body pipe failedline carrying arequest_id, instead of a bare unhandled rejection.Test plan
cargo check -p multistore-cf-workers --target wasm32-unknown-unknowncargo fmt,cargo clippy --all-targets,cargo checkcrates/cf-workersis not in the workspacedefault-members). Verification is reading production logs after deploy.How to read the result once deployed: filter Workers logs for
streamed PUT body pipe failedand join to the existingserver error responseline onrequest_id. A 520 with a matching pipe rejection is body-side; a 520 with none is connection-side.🤖 Generated with Claude Code
https://claude.ai/code/session_01B3x2KzUtojpwPvDKmSgvht