fix(ext/node): submit http2 RST_STREAM before flushing END_STREAM DATA frame on respondWithFD read#33633
Conversation
…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
left a comment
There was a problem hiding this comment.
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 → scheduleSendPending → on_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 path — self.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.
Summary
Enables
test-http2-respond-file-fd-invalidin node_compat suite.Test plan
cargo test --test node_compat -- test-http2-respond-file-fd-invalid