fix: Cloudflare streaming response backpressure#1186
Conversation
🦋 Changeset detectedLatest commit: 961a03b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Hi thanks for the PR! LGTM but I'll defer to the Cloudflare folks. |
commit: |
There was a problem hiding this comment.
Pull request overview
This PR improves streaming behavior in the Cloudflare Node wrapper and the core request handler by respecting downstream backpressure to prevent unbounded buffering/memory growth, and by avoiding unnecessary ReadableStream.tee() when the streamed chunks don’t need to be retained.
Changes:
- Implement backpressure-aware bridging from Node
Writableto WebReadableStreamin the Cloudflare Node wrapper. - Update
openNextHandlerstreaming path to usepipeline(Readable.fromWeb(...), response)and skiptee()whenStreamCreator.retainChunks === false. - Add unit tests covering bounded streaming, cancellation/destruction behavior, and
tee()skipping/retention semantics.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/open-next/src/overrides/wrappers/cloudflare-node.ts | Adds backpressure-aware streaming bridge and producer teardown handling. |
| packages/open-next/src/core/requestHandler.ts | Uses pipeline for proper backpressure and conditionally skips ReadableStream.tee() based on retainChunks. |
| packages/open-next/src/types/open-next.ts | Documents retainChunks: false behavior and tee() avoidance. |
| packages/tests-unit/tests/overrides/wrappers/cloudflare-node.test.ts | Adds streaming/backpressure + cancellation/destruction unit coverage for the Cloudflare wrapper. |
| packages/tests-unit/tests/core/requestHandler.test.ts | Adds unit coverage for tee() skipping/retention and bounded streaming in openNextHandler. |
| .changeset/cloudflare-stream-backpressure.md | Changeset entry describing the patch behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
conico974
left a comment
conico974
left a comment
There was a problem hiding this comment.
Could you fix the tests as well please.
| for await (const chunk of bodyToConsume) { | ||
| response.write(chunk); | ||
| let bodyToConsume = routingResult.body; | ||
| if (options.streamCreator.retainChunks !== false) { |
There was a problem hiding this comment.
This is a breaking change, this PR cannot just be a patch.
It will also likely break some converters that relies on the body being readable
We could gate that under an env variable that we'll remove on a future major
There was a problem hiding this comment.
Thanks for taking the time to review.
I've reverted back to keeping a copy of the body to avoid breaking changes (would be nice to remove down the line as you mentioned in a future major) but not essential for this PR.
There was a problem hiding this comment.
Add more comment on your change please, this is quite hard to review and follow what's going on
There was a problem hiding this comment.
I've tried to better explain what's going on with 961a03b
Prevents excessive memory usage when streaming large responses. Previously, writes did not account for consumer backpressure, so a fast producer could buffer large amounts of data when the consumer was slow. This PR pauses pulling from the producer until the consumer is ready for more data.
Also skipsReadableStream.tee()when chunks do not need to be retained, avoiding an unused duplicate response body during direct streaming.