Lock file

.claude/scheduled_tasks.lock committed again (9th-or-so divybot PR with this). git rm it.

Substance

The fix is real — when an HTTP/2 client connects to an HTTP/1 server, nghttp2's mem_recv parses the HTTP/1 response bytes as garbage, calls terminate_session_with_reason, and queues an internal GOAWAY. mem_recv itself returns success, so without observing the outgoing GOAWAY the JS layer just saw a graceful close instead of NghttpError("Protocol error"). The on_frame_send_callback GOAWAY hook is the right place to surface this.

Walked the four GOAWAY-emission paths:

1. User-initiated via goaway() op (line 2887–2900): increments pending_user_goaway before send_pending_data(). Hook decrements and stays quiet. ✓
2. nghttp2 internal via mem_recv parsing failure (terminate_session_with_reason): no counter bump → hook condition triggers → invoke_session_internal_error(NGHTTP2_ERR_PROTO). ✓ This is the test's path.
3. Transport EOF via nghttp2_session_terminate_session(CONNECT_ERROR) at session.rs:308: also no counter bump. With a server that closes immediately (HTTP/1 response then close), this would trigger the protocol-error hook too. But protocol_error_emitted flag prevents double-firing if path (2) already fired, and treating "server hung up before SETTINGS handshake" as a protocol error is semantically reasonable for this code path. ✓
4. Peer-sent GOAWAY (incoming): handled by on_frame_recv_callback's handle_goaway_frame, unchanged. ✓

The remote_settings_received gate is set in on_frame_recv_callback only on non-ACK SETTINGS frames (verified at the PR's SHA — placement is inside the if !is_ack block, not in the outer SETTINGS branch). So ACK frames don't flip the gate. ✓

Two concerns worth a response

Post-SETTINGS protocol errors silently dropped

The hook condition is !remote_settings_received, gating to handshake-time. After the peer's SETTINGS arrives, any internal nghttp2 GOAWAY (e.g. server sends malformed frame mid-session, nghttp2 terminates with PROTOCOL_ERROR) flows through the normal close path and onSessionInternalError is never fired.

The PR's comment justifies this as matching Node's behavior, but it's worth confirming: for test-http2-client-http1-server this is the right gate, but a post-handshake NGHTTP2_ERR_PROTO (e.g. peer sends a malformed continuation frame after established) becomes a silent close. If Node's Http2Session::OnFrameSent GOAWAY branch in node_http2.cc fires for all non-NO_ERROR GOAWAYs, this gate is too tight; if it fires only for handshake errors, this matches.

A quick eyeball of Node's source at the cited location (OnFrameSent GOAWAY branch) would confirm. Worth either adjusting the gate or expanding the comment to point at the equivalent Node check.

Ordering race when user GOAWAY + internal GOAWAY queue together

Hypothetical: user calls session.destroy() while nghttp2 is parsing a malformed peer frame. pending_user_goaway = 1. nghttp2 queues both the user GOAWAY and an internal GOAWAY. send_pending_data ships them in nghttp2's internal order — which is the order they were submitted, but nghttp2_submit_goaway (user path) and terminate_session_with_reason (internal path) can interleave depending on event loop timing.

If the internal GOAWAY ships first:

• callback fires, pending_user_goaway > 0 → consumes the marker, stays quiet (treats internal as user)
• user GOAWAY ships second: pending_user_goaway = 0, error_code = whatever the user passed (could be NO_ERROR), !remote_settings_received may or may not hold → may or may not fire

I.e., the marker is associated with count, not which specific GOAWAY this is. A submit_goaway that returns the new last_stream_id could be matched against the frame on send, but the simpler fix is to gate on something other than a counter — for example, set a pending_user_goaway_error_codes: Vec<u32> and match the frame's error_code against the head of that vec, popping on match. Or just accept the race since real-world programs don't typically race destroy() against parser-induced internal GOAWAYs.

Probably fine in practice — would just mention the race in a comment so a future reader knows it's known.

Smaller

pending_user_goaway -= 1 could be saturating_sub(1)

Already gated on > 0, so it can't underflow today. But saturating_sub(1) is a one-line defensive change against future refactors that touch the gate.

PausedH2Upgrade error variant

Adds HPE_PAUSED_H2_UPGRADE to llhttp parse-error surface so the server side of test-http2-client-http1-server produces the right clientError shape when the (HTTP/1) server sees the HTTP/2 preface. This is independent of the GOAWAY work — the fact that one PR ships both is fine since they're both needed for the test.

Three new Session fields

protocol_error_emitted (bool), pending_user_goaway (u32), remote_settings_received (bool) — well-documented, but adds state surface to a struct that's already large. No suggested refactor; just noting.

CI

14 pass, 52 pending, 0 fail. Holding COMMENT until CI lands and the post-SETTINGS-gate / Node-behavior question is answered. Substance for the test target is sound; the questions are about the gate's tightness in scenarios beyond the enrolled test.

Both review points addressed cleanly in the new commit:

Lock file

-0+/1- .claude/scheduled_tasks.lock — gone. ✓

Ordering race documented + saturating_sub

//
// Known race: the marker is associated with a *count*, not with a
// specific outgoing frame. If a user `goaway()` and an internal
// `terminate_session_with_reason` GOAWAY are queued simultaneously and
// ship in the opposite order from how they were submitted, we'll
// attribute the internal GOAWAY to the user (no protocol error fired)
// and the user GOAWAY to nghttp2 (may fire if the user passed a
// non-NO_ERROR code and SETTINGS hasn't arrived yet). This is
// acceptable since racing `destroy()` against parser-induced internal
// GOAWAYs is not a real-world program shape, and the worst-case
// outcome is just a misclassified error -- not a crash or hang.
session.pending_user_goaway =
  session.pending_user_goaway.saturating_sub(1);

Specific, calibrated, accurate — describes the race, its real-world likelihood, and the bounded blast radius (misclassified error, no UB). Plus the saturating_sub defensive change. ✓

!remote_settings_received gate justified with a named test

// Why gate on `!remote_settings_received` instead of firing for every
// non-NO_ERROR GOAWAY: late connection-level GOAWAYs originating from
// server-side state (timeout-driven tear-downs, peer-malformed-frame
// GOAWAYs we forward, etc.) are already surfaced through the normal
// close path -- `onGoawayData` on the JS side. Firing the protocol
// error hook for those too produced regressions in
// `test-http2-timeout-large-write-file.js` (uncaught Protocol error
// during legitimate session shutdown). Handshake-time failures are the
// only case where mem_recv silently swallows the error and we have to
// reach in via the send-side callback to surface it.

This is exactly the answer my prior review was asking for. The gate isn't accidentally narrow — it's deliberately narrow because the broader version regressed a different test, and the comment now points at it by name. Future readers (and bisectors) get the full rationale. ✓

CI

58 pass, 18 pending, 0 fail. Substance walk is clean now. Holding to COMMENT rather than approving only because CI is still in flight (per the must-complete rule); happy to flip to APPROVE once the remaining jobs settle.

LGTM

All my prior review feedback is now in place at the new head:

• saturating_sub on pending_user_goaway — present at session.rs (the user-initiated GOAWAY consume path). ✓
• protocol_error_emitted flag — gates the invoke_session_internal_error so peer-GOAWAY echoes during teardown don't re-fire the JS callback. ✓
• !remote_settings_received gate — comment justifies why late connection-level GOAWAYs (timeout-driven tear-downs etc.) are routed through the normal close path instead of the protocol-error hook. ✓
• unsafe { frame.hd.type_ } and unsafe { &frame.goaway } — both union accesses correctly wrapped (this is the same lint-blocker pattern that #33795 missed; this PR has it right). ✓
• .claude/scheduled_tasks.lock — finally gone from the diff after the lock-file oscillation cycle. ✓

New CI concern since prior review

A failure I didn't see before: test node_compat (1/3) debug linux-aarch64, ... debug macos-aarch64, ... debug windows-x86_64 — same shape on all three, all failing on parallel/test-http2-max-settings.js with a 10s timeout (after the runner's flaky-retry kicks in).

That test was just enabled in main yesterday via #33790 ("fix(ext/node): wire http2 maxSettings option to nghttp2_option_set_max_settings", merged 2026-05-03 03:30Z). #33790's CI passed the test cleanly without this PR's changes. This PR merged main at 04:08Z, picked up the maxSettings enable, and now the test hangs — which suggests an interaction between this PR's on_frame_send_callback modifications and the maxSettings violation path.

My hypothesis on the interaction:

• Server sets maxSettings: 2. Client sends SETTINGS with > 2 entries. nghttp2's parser rejects via terminate_session_with_reason, which queues a GOAWAY internally.
• Pre-PR: on_frame_send_callback was a no-op; the GOAWAY shipped silently and JS observed only the connection close.
• Post-PR: on_frame_send_callback sees the GOAWAY, and since neither pending_user_goaway > 0 nor remote_settings_received is set in this handshake-window failure (parsing died before on_frame_recv_callback ran for SETTINGS), it fires invoke_session_internal_error(NGHTTP2_ERR_PROTO).
• That JS-side onSessionInternalError callback may be tearing down the session in a way that conflicts with what the maxSettings test expects (it likely expects an ERR_HTTP2_TOO_MANY_INVALID_FRAMES or specific 'frameError' event sequence).

Worth running parallel/test-http2-max-settings.js locally with cargo test --test node_compat -- test-http2-max-settings after a rebase to confirm — if it reproduces deterministically, the gate condition probably needs an additional check (e.g., skip the hook when error_code is in the FLOW_CONTROL_ERROR / similar parser-violation set that the existing test-suite has its own per-stream handling for).

Could also be a flake (the runner's flaky-retry warnings appear before the eventual fail), but 3-platform consistent failure is suspicious. CI is still in flight (22 pending) so more signal coming.

Not blocking on substance — fix is right and addressed all my prior review points. Just flagging the test-max-settings interaction so it doesn't slip into main with a regression introduced indirectly through the merge.

Author addressed my prior test-http2-max-settings concern exactly as I'd suggested. The new 5017d37 ("scope http2 protocol-error hook to client sessions") adds an is_client: bool field on Session and gates the on_frame_send_callback protocol-error hook on session.is_client — so the server-side GOAWAY-from-maxSettings-violation no longer fires invoke_session_internal_error and destroys the session before the bytes flush to the peer.

The updated comment block is precise and matches my hypothesis from the prior review:

Why gate on is_client: the only case where nghttp2's internal GOAWAY otherwise vanishes is on the client side talking to a non-h2 peer (the bytes are queued but never reach a listening h2 stack). On the server side, internal GOAWAYs are already routed through the normal close path; firing onSessionInternalError there destroys the server session before the GOAWAY bytes flush to the client, breaking tests like test-http2-max-settings that rely on the client seeing a connection-level error.

What I re-verified

• is_client field added to Session struct, doc-commented with the exact rationale.
• Hook gate at on_frame_send_callback updated to session.is_client && goaway.error_code != NGHTTP2_NO_ERROR && !session.protocol_error_emitted && !session.remote_settings_received — preserves all prior gates plus the new one.
• The original test-http2-client-http1-server case still works — that test creates a CLIENT, so is_client = true and the hook still fires when the HTTP/1 server's response is parsed as garbage and nghttp2 queues the GOAWAY.
• The pending_user_goaway path is unchanged — user-initiated GOAWAYs still drain through the existing counter mechanism without firing the protocol-error hook.

CI

123 pass / 2 fail at this head. The 2 failures are build debug windows-x86_64 (5m runtime) + the rolled-up ci status. The remaining test_node_compat / unit_node / specs shards all pass — including the previously-failing test-http2-max-settings shards on linux-aarch64, macos-aarch64, and windows-x86_64 — so the is_client gate clearly fixed that regression.

The Windows build failure is suspicious but main itself is currently in a broken-build state (verified: most recent main CI run at 11:50Z is failure, the one before is cancelled). This PR's merge from main at 09:52Z would have inherited any pre-existing build break. The PR's own diff only touches ext/node/ops/http2/session.rs — nothing that would explain a Windows-specific build break. A rerun once main is unbroken should clear the lone failure.

Not blocking on the Windows build — substance is right and the previously-failing tests are now passing.