Skip to content

fix(response): defer stream metadata until first chunk#1511

Closed
MFA-G wants to merge 1 commit into
h3js:mainfrom
MFA-G:fix/defer-stream-response
Closed

fix(response): defer stream metadata until first chunk#1511
MFA-G wants to merge 1 commit into
h3js:mainfrom
MFA-G:fix/defer-stream-response

Conversation

@MFA-G

@MFA-G MFA-G commented Jul 24, 2026

Copy link
Copy Markdown

🔗 Linked issue

Fixes #1510

📚 Description

Wait for a returned ReadableStream to 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.ts
  • pnpm exec oxfmt --check src/response.ts test/stream.test.ts
  • pnpm typecheck
  • Full pnpm test: 1,610 passed, 44 skipped; one environment-specific IP assertion failed (::ffff:127.0.0.1 vs expected loopback forms), plus the three bundle-size budgets increased by ~306–320 raw bytes.

Summary by CodeRabbit

  • Bug Fixes

    • Improved streaming responses so status codes and headers are finalized after the first content chunk is ready.
    • Ensures clients receive accurate response metadata alongside streamed content.
  • Tests

    • Added coverage verifying delayed stream initialization, response metadata, and streamed body content.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

prepareResponse now primes un-primed ReadableStream responses by reading their first chunk before committing response state. A new test verifies delayed status and header updates alongside complete stream body delivery.

Changes

Stream response priming

Layer / File(s) Summary
ReadableStream priming flow
src/response.ts
prepareResponse tracks priming state, tees un-primed streams, reads the first chunk through primeStream, and recursively prepares the remaining stream.
Delayed metadata validation
test/stream.test.ts
A Web stream test verifies status 201, the x-stream-header: ready header, and the Hello World body after metadata is set during stream startup.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: pi0, pi0

Poem

A rabbit watched the first chunk flow,
Then headers bloomed in status glow.
The stream hopped on, the body read,
“Hello World!” the bunny said.
Tee and probe, then safely run—
Response metadata now joins the fun.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main fix: deferring stream response metadata until the first chunk.
Linked Issues check ✅ Passed The change restores first-chunk response preparation so status and headers set during stream generation are committed as required by issue #1510.
Out of Scope Changes check ✅ Passed The changes stay focused on stream response timing and regression coverage, with no obvious unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
test/stream.test.ts (1)

86-96: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Exercise 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

📥 Commits

Reviewing files that changed from the base of the PR and between dd19737 and beed645.

📒 Files selected for processing (2)
  • src/response.ts
  • test/stream.test.ts

Comment thread src/response.ts
event: H3Event,
config: H3Config,
nested?: boolean,
primed?: boolean,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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

Comment thread src/response.ts
Comment on lines +212 to +220
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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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 || true

Repository: 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:


🏁 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 || true

Repository: 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:


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.

Suggested change
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

@pi0

pi0 commented Jul 24, 2026

Copy link
Copy Markdown
Member

AI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[v2] Regression: Response sended before first stream payload

2 participants