fix(response): defer stream metadata until first chunk#1511
Conversation
📝 WalkthroughWalkthrough
ChangesStream response priming
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/stream.test.ts (1)
86-96: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise multi-chunk body delivery.
Because the implementation tees and consumes one probe chunk, enqueue at least two chunks here. That verifies the first chunk is preserved and later chunks are not lost.
Suggested test adjustment
- controller.enqueue(new TextEncoder().encode("Hello World")); + controller.enqueue(new TextEncoder().encode("Hello ")); + controller.enqueue(new TextEncoder().encode("World"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/stream.test.ts` around lines 86 - 96, Update the stream test around the /stream fetch to enqueue at least two distinct body chunks before closing the controller, then assert that res.text() contains both chunks in order. Keep the existing status and header assertions unchanged so the test verifies the first probe chunk is preserved and subsequent delivery remains intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/response.ts`:
- Line 111: Update the affected preparation function to accept an options object
as its second parameter, moving the nested and primed flags into named
properties. Adjust all call sites, including recursive calls, to pass those
properties by name while preserving their existing behavior and defaults.
- Around line 212-220: Update primeStream so the intentionally detached
reader.cancel() promise has a rejection handler, while keeping cancellation
non-blocking and preserving the existing tee/probe flow. Handle the rejection
without changing the returned body branch behavior.
---
Nitpick comments:
In `@test/stream.test.ts`:
- Around line 86-96: Update the stream test around the /stream fetch to enqueue
at least two distinct body chunks before closing the controller, then assert
that res.text() contains both chunks in order. Keep the existing status and
header assertions unchanged so the test verifies the first probe chunk is
preserved and subsequent delivery remains intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 79a806d7-48a8-477d-a6fe-e713e32d964a
📒 Files selected for processing (2)
src/response.tstest/stream.test.ts
| event: H3Event, | ||
| config: H3Config, | ||
| nested?: boolean, | ||
| primed?: boolean, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use an options object for the preparation flags.
Line 111 adds another positional boolean to an already multi-argument function. Move nested and primed into an options object as the second parameter to avoid order-sensitive recursive calls and comply with the repository convention.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/response.ts` at line 111, Update the affected preparation function to
accept an options object as its second parameter, moving the nested and primed
flags into named properties. Adjust all call sites, including recursive calls,
to pass those properties by name while preserving their existing behavior and
defaults.
Source: Coding guidelines
| async function primeStream(stream: ReadableStream): Promise<BodyInit> { | ||
| const [probe, body] = stream.tee(); | ||
| const reader = probe.getReader(); | ||
| await reader.read(); | ||
| // Do not await: tee cancellation settles only once the response branch ends. | ||
| void reader.cancel(); | ||
| return body; | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'Files:'
git ls-files | rg '(^|/)src/response\.ts$' || true
echo
echo 'Relevant src/response.ts section:'
if [ -f src/response.ts ]; then
nl -ba src/response.ts | sed -n '180,240p'
fi
echo
echo 'Search primeStream usages:'
rg -n "primeStream|void reader\.cancel|reader\.cancel\(|ReadableStream|ReadableStreamDefaultReader" src || trueRepository: h3js/h3
Length of output: 244
🌐 Web query:
WHATWG Streams tee cancellation propagate cancellation reason from one branch to the other ReadableStreamDefaultReader cancel promise rejects
💡 Result:
In the WHATWG Streams API, teeing a ReadableStream creates two independent branches [1][2]. Cancellation of the original stream is only propagated to the underlying source once both branches have been canceled [1]. Specifically, the behavior regarding cancellation and promise resolution is as follows: 1. Cancellation Propagation: When you cancel one branch, it does not immediately trigger cancellation of the underlying stream [1]. The underlying stream is only canceled after both branches have been canceled [1]. At that point, a composite cancellation reason (typically an array containing the reasons from both branches) is propagated to the original stream's underlying source [1][3]. 2. ReadableStreamDefaultReader cancel Promise: The promise returned by a branch's reader.cancel method remains pending until that specific branch is considered "canceled" within the context of the tee [1]. Historically, there have been edge cases where these promises would not resolve if the other branch continued to read until completion (the end of the stream), though specification updates have addressed these synchronization issues to ensure proper resolution [4][5]. If you are experiencing a situation where a cancel promise is rejecting or failing to resolve, it is typically because the tee logic requires both branches to reach a finalized state (either canceled or fully consumed/closed) before the internal cancellation of the original stream is finalized and the promises are settled [1][4]. Individual reader cancellation promises are tied to the state of their respective branches and the overall synchronization state of the tee operation [1][3].
Citations:
- 1: https://streams.spec.whatwg.org/
- 2: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream/tee
- 3: nodejs/node@6ed3367155
- 4: Cancelling one branch of a tee'd stream never finishes if other branch reads all chunks whatwg/streams#1038
- 5: Fix cancelPromise in ReadableStreamTee being resolved twice whatwg/streams#1112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'Relevant src/response.ts section:'
python3 - <<'PY'
from pathlib import Path
p = Path('src/response.ts')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 180 <= i <= 240:
print(f'{i:4d}\t{line}')
PY
echo
echo 'Search primeStream usages:'
rg -n "primeStream|void reader\.cancel|reader\.cancel\(|ReadableStream|ReadableStreamDefaultReader" src || trueRepository: h3js/h3
Length of output: 3699
🌐 Web query:
Node.js ReadableStream cancel() promise rejects underlying source cancel rejection streams
💡 Result:
When you call the cancel method on a ReadableStream (or its reader), the behavior regarding the underlying source's cancel rejection is as follows: According to the Streams Standard, if the underlying source's cancel method returns a rejected promise (or throws an exception), the promise returned by the consumer's readableStream.cancel call will also reject with that same error [1][2]. However, regardless of whether the cancellation process succeeds or fails, the stream itself is transitioned to a "closed" state rather than an "errored" state [1][2]. This design choice reflects that once a consumer has signaled a loss of interest in a stream by calling cancel, a subsequent failure in the cancellation cleanup mechanism does not impact the consumer's view of the stream as having been successfully discarded [1][2]. In Node.js, this behavior is consistently implemented; even if the underlying source's cancel rejection occurs, the stream is put into a closed state, and the failure is communicated solely through the rejection of the promise returned by the cancel call [3].
Citations:
- 1: https://streams.spec.whatwg.org/
- 2: https://streams.spec.whatwg.org/review-drafts/2024-08/
- 3: ReadableStream.cancel doesn't conform to spec nodejs/node#41048
Handle probe-cancellation rejection.
reader.cancel() is intentionally detached, but it can reject when the underlying stream/source cancellation fails; tee() propagates that result. Attach a rejection handler while keeping cancellation non-blocking.
Suggested fix
- void reader.cancel();
+ void reader.cancel().catch(() => undefined);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function primeStream(stream: ReadableStream): Promise<BodyInit> { | |
| const [probe, body] = stream.tee(); | |
| const reader = probe.getReader(); | |
| await reader.read(); | |
| // Do not await: tee cancellation settles only once the response branch ends. | |
| void reader.cancel(); | |
| return body; | |
| } | |
| async function primeStream(stream: ReadableStream): Promise<BodyInit> { | |
| const [probe, body] = stream.tee(); | |
| const reader = probe.getReader(); | |
| await reader.read(); | |
| // Do not await: tee cancellation settles only once the response branch ends. | |
| void reader.cancel().catch(() => undefined); | |
| return body; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/response.ts` around lines 212 - 220, Update primeStream so the
intentionally detached reader.cancel() promise has a rejection handler, while
keeping cancellation non-blocking and preserving the existing tee/probe flow.
Handle the rejection without changing the returned body branch behavior.
Source: MCP tools
|
AI |
🔗 Linked issue
Fixes #1510
📚 Description
Wait for a returned
ReadableStreamto produce its first chunk before preparing the HTTP response. This restores the v1 behavior needed by async streaming renderers: status and headers set while producing the first chunk are included before the runtime commits the response.The stream is teed so the probed chunk remains available to the response consumer, while cancellation continues to propagate to the source. A matrix regression test covers both web and Node adapters.
🧪 Validation
pnpm vitest run test/stream.test.ts test/dispose.test.ts --typecheck.enabled=false(31 passed, 3 skipped)pnpm exec oxlint src/response.ts test/stream.test.tspnpm exec oxfmt --check src/response.ts test/stream.test.tspnpm typecheckpnpm test: 1,610 passed, 44 skipped; one environment-specific IP assertion failed (::ffff:127.0.0.1vs expected loopback forms), plus the three bundle-size budgets increased by ~306–320 raw bytes.Summary by CodeRabbit
Bug Fixes
Tests