Skip to content

fix(node, fastify): answer HTTP/2 HEAD requests - #117

Merged
dinwwwh merged 6 commits into
mainfrom
claude/gifted-darwin-vczbsc
Sep 29, 2026
Merged

dinwwwh merged 6 commits into
mainfrom
claude/gifted-darwin-vczbsc

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Sep 29, 2026

Copy link
Copy Markdown
Member

Summary

This PR fixes improper stream body handling when responding to HTTP/2 HEAD requests. Previously, stream bodies were not being properly released when sent in response to HEAD requests, causing resource leaks. The fix ensures that stream bodies are destroyed when the response connection closes, particularly for HTTP/2 where the underlying stream may end before the response object.

Key Changes

  • Stream cleanup on connection close: Modified both @standard-server/node and @standard-server/fastify response handlers to listen for close events on the underlying connection stream (for HTTP/2) rather than just the response object, ensuring proper cleanup when the stream ends.

  • HTTP/2 stream detection: Updated code to detect HTTP/2 responses by checking for the stream property and using that for event listeners instead of the response object directly, since HTTP/2 streams may end before the response object reports it.

  • Improved abort signal handling: Refactored toAbortSignal in signal.ts to properly handle HTTP/2 HEAD requests where the stream is already ended when the handler runs. The signal now correctly detects when the stream is destroyed and aborts appropriately.

  • Simplified response writability check: Streamlined canWriteToNodeResponse to focus on the essential checks (headersSent, writableEnded, and destroyed) rather than checking multiple stream state properties, making it more reliable for both HTTP/1 and HTTP/2.

  • Comprehensive test coverage: Added extensive tests for:

    • HTTP/2 HEAD requests with stream bodies (both auto-exposed and explicit routes)
    • Stream cleanup on client-initiated stream resets with various error codes
    • Abort signal behavior on HTTP/2 HEAD responses
    • Response writability checks for HTTP/2 HEAD requests

Implementation Details

  • The fix uses the 'stream' in res pattern to detect HTTP/2 responses and access the underlying stream for event listeners
  • Stream bodies are explicitly destroyed in close handlers to ensure cleanup even when Fastify's automatic cleanup doesn't occur
  • Tests verify both successful cleanup and proper error propagation when streams are reset by clients

https://claude.ai/code/session_019TvbqaqZZZK5rU6p35jamj

Node ends the writable side of an http2 HEAD stream before the request
is handled, but its headers can still be sent. `canWriteToNodeResponse`
took that ended stream as a finished response, so HEAD requests got no
response at all (the fastify auto HEAD route fell back to a bare 200)
and their abort signal started out aborted.

- canWriteToNodeResponse: for http2, only require the stream to stay
  open and the response itself not to have ended
- sendStandardResponse (node): send only the headers on an http2 HEAD
  stream, and release a stream body instead of piping it, which stalled
  without ever closing `res`
- sendStandardResponse (fastify): hand fastify an empty stream in place
  of a stream body it would pipe into the ended stream and stall on
- toAbortSignal: listen to the http2 stream's `close`, since Node skips
  the response's `close` for a HEAD stream closing before `res.end()`

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019TvbqaqZZZK5rU6p35jamj
A response can still be sent while its headers aren't sent, it hasn't
ended, and its connection is open. `destroyed` alone tells the last one
(Node sets it on http1 responses and http2 streams when the client goes
away), and `writableEnded` covers `writableFinished`, which on http2
mirrors the stream Node ends up front for HEAD requests.

toAbortSignal now follows one rule: abort when the connection closes
before the response ends.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019TvbqaqZZZK5rU6p35jamj
- sendStandardResponse (node): handle an http2 HEAD response in the
  existing no-body branch, releasing a stream body there, and keep the
  pipe branch unchanged
- canWriteToNodeResponse: a single expression
- drop comments the http2 HEAD tests already cover

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019TvbqaqZZZK5rU6p35jamj
… of checking HEAD

Drop the http2 HEAD checks from sendStandardResponse: a HEAD handler is
expected to return no body. A stream body is instead released when the
connection closes (the underlying stream on http2, whose close Node
still emits for HEAD), so a stream body sent to a HEAD request no longer
runs forever or leaves the promise pending. This also stops fastify's
auto-exposed HEAD route from draining an endless body forever.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019TvbqaqZZZK5rU6p35jamj
On http2, Node never forwards stream errors to the response object, so
`res.once('error', ...)` never fired and a stream that failed (a client
reset with an error code, or a body written to a HEAD response) still
resolved sendStandardResponse. Listen for `error` where `close` is
already listened for: the underlying stream on http2. A plain client
cancel is not a stream error in Node, so it still resolves.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019TvbqaqZZZK5rU6p35jamj
Add a HEAD request to the data transfer method table and a HEAD variant
of the "never aborted" signal test, run against every adapter. Both fail
on node-http2 and node-http2-secure before the http2 HEAD fix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019TvbqaqZZZK5rU6p35jamj
@pkg-pr-new

pkg-pr-new Bot commented Sep 29, 2026

Copy link
Copy Markdown
@standard-server/aws-lambda

npm i https://pkg.pr.new/@standard-server/aws-lambda@117

@standard-server/core

npm i https://pkg.pr.new/@standard-server/core@117

@standard-server/fastify

npm i https://pkg.pr.new/@standard-server/fastify@117

@standard-server/fetch

npm i https://pkg.pr.new/@standard-server/fetch@117

@standard-server/node

npm i https://pkg.pr.new/@standard-server/node@117

@standard-server/peer

npm i https://pkg.pr.new/@standard-server/peer@117

@standard-server/shared

npm i https://pkg.pr.new/@standard-server/shared@117

commit: 1b4167d

@dinwwwh dinwwwh changed the title Fix stream body handling for HTTP/2 HEAD requests fix(node, fastify): answer HTTP/2 HEAD requests Sep 29, 2026
@codspeed

codspeed Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 26 untouched benchmarks
⏩ 108 skipped benchmarks1


Comparing claude/gifted-darwin-vczbsc (1b4167d) with main (c6342d0)

Open in CodSpeed

Footnotes

  1. 108 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@codecov

codecov Bot commented Sep 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No blocking issues. One optional consistency nit inline, plus a stale test comment noted below.

Reviewed changes

  • HTTP/2 HEAD writability — canWriteToNodeResponse now treats an already-ended underlying stream (the HTTP/2 HEAD case) as writable, keying off res.writableEnded rather than the underlying stream's write state, and drops the old _canWriteToStream helper.
  • Connection-scoped lifecycle — the Node and Fastify response adapters attach error/close to the underlying Http2Stream when present ('stream' in res), and release a stream body once that connection closes, so an endlessly-piping body no longer leaks on HEAD.
  • Abort signal rework — toAbortSignal resolves the connection stream once, invokes onClose immediately when it is already destroyed, and decides the abort on !res.writableEnded, routing the response error through the shared handler.
  • Test coverage — new HTTP/2 HEAD / client-reset / abort tests in the Node and Fastify packages, plus HEAD cases in the e2e data-transfer and signal-and-cancel suites. I ran the four package test files (61 tests) and both e2e suites (558 tests) plus pnpm run type:check — all green.

ℹ️ Nitpicks

  • packages/node/src/utils.test.ts:620 — the comment "what sendStandardResponse settles on" still describes res.once('close'), but this PR moved that settle point to res.stream close. Switching the listener (and comment) to res.stream would keep the test mirroring production.
  • packages/fastify/src/response.ts:40 — see inline note on error parity with the Node adapter.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

Comment thread packages/fastify/src/response.ts
@dinwwwh
dinwwwh merged commit f414f41 into main Sep 29, 2026
11 checks passed
dinwwwh pushed a commit that referenced this pull request Sep 29, 2026
Main answers http2 HEAD requests on its own (#117), so take its
`canWriteToNodeResponse` and drop the duplicate check and test from this
branch.

With the bodiless branch, an http2 HEAD response with a stream body now
ends without piping instead of rejecting with ERR_STREAM_WRITE_AFTER_END,
so its test expects the send to resolve.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RyZFAU35Wz3M57DNoiPxjb
dinwwwh pushed a commit that referenced this pull request Sep 29, 2026
main (#117) already destroys the body on the connection's `close`, which
also covers HTTP/2, so the extra `reply.raw` listener was redundant. Keep
its explanation on main's handler, drop the HEAD test main already has,
and keep the onSend replacement test on the shared createEndlessBody.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BwTBYhcHBjq26pbGTu4mRR
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.

2 participants