Skip to content

fix(ext/node): submit http2 RST_STREAM before flushing END_STREAM DATA frame on respondWithFD read#33633

Merged
bartlomieju merged 1 commit into
denoland:mainfrom
divybot:claude/test-http2-respond-file-fd-invalid
Apr 28, 2026
Merged

fix(ext/node): submit http2 RST_STREAM before flushing END_STREAM DATA frame on respondWithFD read#33633
bartlomieju merged 1 commit into
denoland:mainfrom
divybot:claude/test-http2-respond-file-fd-invalid

Conversation

@divybot

@divybot divybot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Enables test-http2-respond-file-fd-invalid in node_compat suite.

Test plan

  • cargo test --test node_compat -- test-http2-respond-file-fd-invalid

…A frame on respondWithFD read

Enables tests/node_compat/runner/suite/test/parallel/test-http2-respond-file-fd-invalid.js

Co-authored-by: Divy Srivastava <[email protected]>

@fibibot fibibot 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.

LGTM. Three coordinated fixes that together unblock the test:

1. Rust: submit RST_STREAM before flushing (Session::submit_rst_stream and flush_pending_rst_streams) — the previous order (send_pending_data first, then nghttp2_submit_rst_stream) lets nghttp2 close the stream and free it (no_closed_streams=1) once the queued END_STREAM DATA frame is flushed, after which the RST_STREAM is silently dropped. Verified against Node's src/node_http2.cc::SubmitRstStream which submits first then schedules the write — the new order matches Node, the old order's "matches Node.js" comment was the bug. The find_stream null-check stays in place, so submitting RST for an already-freed stream still short-circuits cleanly.

2. JS: pre-submit RST_STREAM in closeStream — when kForceRstStream is requested with a non-zero code on an open writable, submit RST first so nghttp2 prioritizes it over the END_STREAM DATA frame that stream.end() will queue. The polyfill's synchronous shutdown chain (handle.shutdown → END_STREAM frame → scheduleSendPendingon_stream_close → kHandle cleared) completes before finishCloseStream would otherwise reach submitRstStream, so the deferred path silently drops the RST_STREAM. The rstAlreadySubmitted flag correctly skips the regular finishCloseStream callback path so the RST isn't submitted twice. Conditions (!ending && !stream.pending && stream[kHandle] !== undefined) cover the "we have something to actually reset" case.

3. processRespondWithFD error pathself.destroy(err) replaced with closeStream(self, NGHTTP2_INTERNAL_ERROR, kForceRstStream); self.destroy(). Matches Node's respondWithFD: a read failure resets the stream with NGHTTP2_INTERNAL_ERROR on the wire rather than leaking the underlying fs error to the user. The test's server-side errorCheck expects exactly that — code: 'ERR_HTTP2_STREAM_ERROR', message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' (twice; both arms of the round trip).

Test enrollment alphabetically positioned correctly (respond-file-fd-errors < respond-file-fd-invalid < respond-file-fd-leak).

CI fully green: 132/133 checks pass, only wpt release linux-x86_64 still in flight (unrelated to this change). All test node_compat debug, test unit_node debug, and lint debug jobs clean.

@bartlomieju
bartlomieju merged commit 2004b34 into denoland:main Apr 28, 2026
136 checks passed
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.

3 participants