Skip to content

Commit 97dfbe7

Browse files
Richard Wheelerclaude
andcommitted
net.quic: prune dead_request_streams once the peer's stream is drained
fail_request_stream() marked a failed stream dead forever with no pruning, unlike the two sibling maps (request_streams/request_decoders) it already prunes in the same function -- unbounded on a long-lived pooled HTTP/3 connection. Flagged by JalonSolov's review on PR #28129. Adds prune_terminal_dead_streams(), called once per drive() cycle, which drops an entry once QuicConn.stream_recv_status confirms the underlying stream's receive side has itself reached a terminal state (reset or fin received) -- at that point RFC 9000 guarantees no further frames for that ID can ever arrive, so the bookkeeping is no longer needed. Entries for streams the peer hasn't finished draining yet are left in place, since it may still legally keep sending (this module has no STOP_SENDING API yet). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent eef1920 commit 97dfbe7

2 files changed

Lines changed: 85 additions & 6 deletions

File tree

vlib/net/quic/h3_conn.v

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,7 @@ fn (mut h H3Conn) drive(qc_result PollResult) !H3PollResult {
262262
h.open_own_streams_if_ready()!
263263
h.drain_known_peer_streams(mut result)!
264264
}
265+
h.prune_terminal_dead_streams()
265266
return result
266267
}
267268

@@ -761,16 +762,53 @@ fn (mut h H3Conn) fail_request_stream(stream_id u64, error_code u64, reason stri
761762
// number of requests ever opened on a long-lived pooled connection, not
762763
// the number currently in flight -- see the identical pruning (and this
763764
// same reasoning) in finalize_request_stream_if_done's success path.
764-
// dead_request_streams is deliberately left alone: it is a tiny,
765-
// permanent bool marker (unlike these two, which hold buffered decoder
766-
// state), and drain_known_peer_streams/retry_blocked_sections both
767-
// still check membership in it as a defense-in-depth guard even though,
768-
// after this deletion, `stream_id` no longer appears in request_
769-
// streams.keys() for drain_known_peer_streams to iterate at all.
765+
// dead_request_streams itself is NOT deleted here (unlike these two):
766+
// the peer may still legally send more STREAM frames for `stream_id`
767+
// after this call (this module has no per-stream RST_STREAM/
768+
// STOP_SENDING send API yet, see this function's own doc comment), and
769+
// drain_known_peer_streams/retry_blocked_sections/finalize_request_
770+
// stream_if_done all rely on membership here to keep ignoring those --
771+
// deleting it now would let a same-poll-cycle re-entry into
772+
// finalize_request_stream_if_done (from dispatch_request_stream_frames
773+
// falling through after this call, e.g. via decode_or_queue_headers)
774+
// double-fail this same stream. prune_terminal_dead_streams() removes
775+
// the entry once the QUIC layer confirms no further frames can ever
776+
// arrive for it, bounding this map instead of leaving it permanent.
770777
h.request_streams.delete(stream_id)
771778
h.request_decoders.delete(stream_id)
772779
}
773780

781+
// prune_terminal_dead_streams drops any dead_request_streams entry whose
782+
// underlying QUIC receive side has itself reached a terminal state
783+
// (reset_recvd/reset_read or size_known/data_recvd/data_read): once that
784+
// has happened, the transport guarantees no further STREAM frames for that
785+
// ID can ever arrive (RFC 9000 -- a stream ID is never reused within a
786+
// connection), so this layer no longer needs to remember it was locally
787+
// failed. Runs once per drive() cycle, strictly after drain_known_peer_
788+
// streams/retry_blocked_sections have both finished processing this
789+
// cycle's frames -- never mid-dispatch, so it cannot retire an ID out from
790+
// under fail_request_stream's own same-poll-cycle reentrancy guard in
791+
// finalize_request_stream_if_done. Bounds dead_request_streams to the
792+
// number of failed streams the peer hasn't finished draining yet, instead
793+
// of the total ever failed over the connection's lifetime (see fail_
794+
// request_stream's own doc comment for why the entry can't just be
795+
// deleted immediately on failure). If the QUIC layer has no record of the
796+
// ID at all (stream_recv_status returns none), the entry is left alone
797+
// rather than guessed at -- a rare, harmless conservative miss, not a
798+
// correctness risk.
799+
fn (mut h H3Conn) prune_terminal_dead_streams() {
800+
mut terminal := []u64{}
801+
for stream_id, _ in h.dead_request_streams {
802+
status := h.qc.stream_recv_status(stream_id) or { continue }
803+
if status.state == .reset_received || status.state == .fin_received {
804+
terminal << stream_id
805+
}
806+
}
807+
for stream_id in terminal {
808+
h.dead_request_streams.delete(stream_id)
809+
}
810+
}
811+
774812
// has_blocked_section_for reports whether any queued blocked field
775813
// section still belongs to `stream_id`.
776814
fn (h &H3Conn) has_blocked_section_for(stream_id u64) bool {

vlib/net/quic/h3_conn_test.v

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -613,6 +613,47 @@ fn test_h3_conn_prunes_request_stream_state_on_failure_too() {
613613
assert stream_id in h.dead_request_streams
614614
}
615615

616+
// test_h3_conn_prunes_dead_request_stream_once_peer_side_fully_terminal is a
617+
// regression test for the third sibling of the SAME unbounded-growth bug
618+
// class the two tests above already cover: dead_request_streams itself was
619+
// never pruned (a repo-review miss caught externally, PR #28129 review
620+
// comment 2026-08-21 -- see code-review-misses.md), unlike request_streams/
621+
// request_decoders right next to it, on which fail_request_stream already
622+
// prunes above. Reproduces the growth first (the entry must still be
623+
// present immediately after failure, since the peer may legally keep
624+
// sending on that stream ID -- fail_request_stream's own doc comment),
625+
// then drives the underlying QUIC stream to ITS OWN terminal receive state
626+
// (an empty FIN-carrying continuation frame here; a RESET_STREAM would
627+
// work identically) and asserts the entry is gone on the next poll(), once
628+
// the transport itself guarantees no further frames can ever arrive.
629+
fn test_h3_conn_prunes_dead_request_stream_once_peer_side_fully_terminal() {
630+
mut c, mut h, _, now := h3_test_conn()!
631+
defer {
632+
c.handshake.free()
633+
}
634+
stream_id := h.open_request_stream()!
635+
h.poll(none, now)!
636+
assert stream_id in h.request_streams
637+
638+
// Same framing error as the sibling test above: fails the stream at the
639+
// H3 layer while leaving the underlying QUIC stream open (no FIN yet).
640+
data_frame := encode_data_frame([u8(1), 2, 3])!
641+
req_stream_frame := encode_stream_frame(stream_id, 0, data_frame, false, true)!
642+
req_datagram := build_fake_one_rtt_packet(c.scid, 0, req_stream_frame, read_keys(mut c), false)!
643+
fail_result := h.poll(req_datagram.bytes, now)!
644+
assert fail_result.events.any(it.kind == .request_error)
645+
assert stream_id in h.dead_request_streams, 'must still be tracked immediately after failure -- the peer may legally keep sending on this ID'
646+
647+
// The peer now finishes sending on the same stream -- its receive side
648+
// reaches a terminal QUIC-layer state, so no further frames for this ID
649+
// can ever legally arrive.
650+
fin_frame := encode_stream_frame(stream_id, u64(data_frame.len), []u8{}, true, true)!
651+
fin_datagram := build_fake_one_rtt_packet(c.scid, 1, fin_frame, read_keys(mut c), false)!
652+
h.poll(fin_datagram.bytes, now)!
653+
654+
assert stream_id !in h.dead_request_streams, 'dead_request_streams must not grow unboundedly for the lifetime of a long-lived pooled connection -- once the QUIC layer confirms the peer can never send more on this stream ID, this bookkeeping entry must be pruned too'
655+
}
656+
616657
// test_h3_conn_blocked_headers_retry_after_delayed_encoder_instruction is
617658
// the standout new-integration-behavior case (no direct Phase 10/11 test
618659
// precedent): a HEADERS frame referencing a dynamic-table entry arrives

0 commit comments

Comments
 (0)