Keep buffered data deliverable when no receive is pending - #6180
Keep buffered data deliverable when no receive is pending#6180Masahiro Kozuka (masa-koz) wants to merge 1 commit into
Conversation
Why this is here rather than upstream This is microsoft#6180, opened on 2026-07-28 and still awaiting review. A project running on this branch has since hit what looks like this bug, so the fix is being carried here rather than waiting on upstream. If and when the upstream PR lands, pulling upstream into this branch will conflict on src/core/stream_recv.c and on the test. The two sides should be identical, so resolving it means keeping either one — that is for whoever does the pull. This note is the reason the duplicate exists. Description A stream can stop receiving while data is already sitting in its receive buffer: QUIC_STREAM_EVENT_RECEIVE is never indicated for it, and StreamReceiveSetEnabled does not bring it back. Cause QuicStreamReceiveComplete decided whether anything was left to deliver like this: if (Stream->RecvPendingLength == 0 || QuicRecvBufferDrain(&Stream->RecvBuffer, BufferLength)) { Stream->Flags.ReceiveDataPending = FALSE; // No more pending data to deliver. } RecvPendingLength == 0 means "no receive is currently pending". It says nothing about whether the receive buffer still holds undelivered data. Only QuicRecvBufferDrain answers that, and the || short-circuits past it. ReceiveDataPending gates every subsequent indication: QuicStreamRecvQueueFlush (stream_recv.c:132) queues a flush only if ReceiveEnabled && ReceiveDataPending QuicStreamRecvFlush (stream_recv.c:896) returns immediately if it is clear QuicStreamReceiveComplete (stream_recv.c:1161) asks for a re-flush only if it is set So once it is cleared while data remains, that data is never indicated. StreamReceiveSetEnabled does not help — it ends up in QuicStreamRecvQueueFlush, whose condition is not met. Only newly arriving data sets the flag again (stream_recv.c:581), which never happens if the peer has finished sending, or is blocked by flow control precisely because the application never consumed what it was not told about. A completion processed with no receive pending is not an error case. docs/api/StreamReceiveComplete.md says: Duplicate StreamReceiveComplete calls are ignored silently if no QUIC_STREAM_EVENT_RECEIVE is pending when the call is processed, but they could race with a new QUIC_STREAM_EVENT_RECEIVE event and complete it It is documented as harmless, and it also arises without any application mistake: in multi-receive mode an asynchronous completion can be absorbed by a flush that is already in progress (stream_recv.c:1017), leaving the queued completion operation to run with nothing outstanding. Fix Ask the two questions separately. When no receive is pending there is no buffer space to reclaim, so the receive buffer is consulted directly for whether anything is still waiting: if (Stream->RecvPendingLength == 0) { if (!QuicRecvBufferHasUnreadData(&Stream->RecvBuffer)) { Stream->Flags.ReceiveDataPending = FALSE; } } else if (QuicRecvBufferDrain(&Stream->RecvBuffer, BufferLength)) { Stream->Flags.ReceiveDataPending = FALSE; } Simply dropping the short-circuit and always calling QuicRecvBufferDrain does not work: the FIN-only indication path (QuicStreamRecvFlush with no data available) reaches the completion with a receive buffer that has no written ranges, and QuicRecvBufferDrain asserts on that (recv_buffer.c:1064). QuicRecvBufferHasUnreadData handles an empty buffer safely (recv_buffer.c:375-377) and returns FALSE there, so the FIN-only case still clears the flag exactly as before. The behaviour change is confined to the case where a completion is processed with no receive pending and the buffer still holds unread data: previously the remaining data became undeliverable, now it stays deliverable. Testing Misc.StreamReceiveCompleteWithNoPendingReceive, registered for both user and kernel mode. Using the default receive mode: 100 bytes are sent; the server returns QUIC_STATUS_PENDING from the receive event. StreamReceiveComplete(50) accepts half, which pauses receives and leaves 50 bytes buffered with no receive pending. StreamReceiveComplete(0) is the case under test — a completion seen with nothing outstanding. StreamReceiveSetEnabled must produce the remaining half, at offset 50 with length 50. Results on this branch: Without the fix the test fails: the second receive event never arrives. With the fix it passes. *Receive*:*Recv*:*Stream*:*Data*:*Abort* passes in full: 2179 tests. No compiler warnings. Step 3 needs the completion in step 2 to have been processed first, and there is no API-visible signal for that, so the test sleeps between the two calls. If the sleep were ever too short the two completions would merge into one and the test would still pass — it loses coverage rather than becoming flaky. The cherry-pick applied cleanly; stream_recv.c carries no local changes in this area. Documentation No documentation change. docs/api/StreamReceiveComplete.md already states the behaviour this implements.
…ixes Summary Bumps the seera-msquic submodule 910edff -> 3397280, which merges seera-main into the submodule's masa-koz/qmux-01 branch. That brings the same three fixes proposed for main — #73, #69 and #68 — plus one commit that exists only on this line, db1883ed, for the QMUX early data buffer. No API or FFI surface changed, so nothing on the Rust side needed updating. Keep buffered data deliverable when no receive is pending (#73, 143f0af) QuicStreamReceiveComplete cleared ReceiveDataPending whenever no receive was pending, without asking whether the receive buffer still held undelivered data — the || short-circuited past QuicRecvBufferDrain, the only thing that answers that. Once cleared with data still buffered, that data was undeliverable for good: the flag gates QuicStreamRecvQueueFlush, QuicStreamRecvFlush and the re-flush at the end of QuicStreamReceiveComplete alike, and only newly arriving data sets it again. The two questions are now asked separately, consulting QuicRecvBufferHasUnreadData when no receive is pending; that helper returns FALSE safely on a buffer with no written ranges (recv_buffer.c:371), which QuicRecvBufferDrain asserts against (recv_buffer.c:1064) and the FIN-only indication path would otherwise hit. This is the one to care about here. msquic-async never completes a receive inline: handle_event_receive returns QUIC_STATUS_PENDING and the completion goes out later from read_complete, when the application drops the StreamRecvBuffer. That is precisely the shape that reaches QuicStreamReceiveComplete with nothing pending, and the symptom on this side would be a Stream read that never resolves while data sits in the buffer. The change is carried from the still-unreviewed microsoft/msquic#6180. Arm the path validation timer when a validation starts (#69, 0c9c96a4) QuicConnOpenNewPath and QuicPathIDAssignCids both set PathValidationStartTime without arming the timer that gives up on a validation, and the other callers of QuicConnPathValidationTimerUpdate need either a packet on the path or the timer to have already fired. A path whose peer never answered its PATH_CHALLENGE was therefore never abandoned. Both now call it. On this side that is a path added with Connection::add_path() that goes unanswered. Fix the QTIP failures from bindings with no remote address (#68, 7893d73e) The raw datapath reads a socket with no remote address as a server listener and rejects a non-wildcard local address for one with QUIC_STATUS_INVALID_STATE (datapath_raw_win.c:149). An unconnected socket is exactly that combination, and under QTIP there is no falling back to an OS socket. QuicConnStart and QuicConnOpenNewPath now reject the pairing up front, and QuicConnAddBoundAddress binds the wildcard while keeping the caller's port when QTIP is on. This only concerns callers enabling QTIP in Settings alongside Connection::set_unconnected_socket() or Connection::add_bound_addr(). Record the QMUX early data buffer's allocated length (db1883ed, this line only) QuicPacketBuilderQMuxFinalize grew the early data buffer without recording the new size, so EarlyDataBufferAllocLength stayed at whatever it was — zero, for a client. Every later record recomputed the requirement from zero and reallocated, copying everything already buffered each time. Correct but wasteful, and it also left WinKernel's /analyze without a size to check the copy against, which failed that build under /WX. The assignment is added, the record length held in a local, and the two bounds the copy depends on asserted as well as assumed. Only reachable through Connection::new_qmux(). Test plan cargo test -p msquic-async --lib --no-default-features --features tokio,msquic-seera — 42 passed, i.e. the suite actually built against the bumped submodule Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ixes (#83) Summary Bumps the seera-msquic submodule 77a0f86 → 143f0af (three commits). No API or FFI surface changed, so nothing on the Rust side needed updating; all three are runtime fixes, one of which lands squarely on how this crate reads streams. Keep buffered data deliverable when no receive is pending (#73) QuicStreamReceiveComplete decided whether anything was left to deliver with if (Stream->RecvPendingLength == 0 || QuicRecvBufferDrain(&Stream->RecvBuffer, BufferLength)) { Stream->Flags.ReceiveDataPending = FALSE; } RecvPendingLength == 0 means "no receive is currently pending" and says nothing about whether the buffer still holds undelivered data — only QuicRecvBufferDrain answers that, and the || short-circuits past it. Clearing ReceiveDataPending with data still buffered makes that data undeliverable for good: the flag gates QuicStreamRecvQueueFlush, QuicStreamRecvFlush and the re-flush at the end of QuicStreamReceiveComplete alike, and only newly arriving data sets it again. StreamReceiveSetEnabled does not help either, because it goes through QuicStreamRecvQueueFlush, whose condition is not met. The two questions are now asked separately, consulting QuicRecvBufferHasUnreadData when no receive is pending. That helper returns FALSE safely on a buffer with no written ranges (recv_buffer.c:371), which is why it is used rather than simply dropping the short-circuit: QuicRecvBufferDrain asserts on exactly that state (recv_buffer.c:1064), and the FIN-only indication path reaches the completion with it. This is the one to care about here. msquic-async never completes a receive inline — handle_event_receive returns QUIC_STATUS_PENDING and the completion goes out later from read_complete, when the application drops the StreamRecvBuffer. That is precisely the shape that reaches QuicStreamReceiveComplete with nothing pending, and the symptom on this side would be a Stream read that never resolves while data sits in the buffer. The change is carried from the still-unreviewed microsoft/msquic#6180; the submodule commit notes that pulling upstream later will conflict on stream_recv.c with identical content on both sides. Arm the path validation timer when a validation starts (#69) QuicConnOpenNewPath and QuicPathIDAssignCids both set PathValidationStartTime without arming the timer that gives up on a validation. The only other callers of QuicConnPathValidationTimerUpdate are the receive path, which needs a packet on the path in question, and the tail of the timer handler, which needs the timer to have fired once — so a path whose peer never answers its PATH_CHALLENGE was never abandoned. Both now call it. On this side that is a path added with Connection::add_path() that goes unanswered. Fix the QTIP failures from bindings with no remote address (#68) The raw datapath reads a socket with no remote address as a server listener and rejects a non-wildcard local address for one with QUIC_STATUS_INVALID_STATE (datapath_raw_win.c:149). An unconnected socket is exactly that combination, and under QTIP there is no falling back to an OS socket. QuicConnStart and QuicConnOpenNewPath now reject the pairing up front instead of spending a thousand rebind attempts on it, and QuicConnAddBoundAddress binds the wildcard while keeping the caller's port when QTIP is on. This only concerns callers enabling QTIP in Settings alongside Connection::set_unconnected_socket() or Connection::add_bound_addr(). Note that set_unconnected_socket()'s rustdoc lists the parameter's requirements and does not yet mention this new incompatibility — worth a follow-up if QTIP is in scope for users of this crate. Test plan cargo test -p msquic-async --lib --no-default-features --features tokio,msquic-seera — 42 passed, i.e. the suite actually built and ran against the bumped submodule cargo test -p msquic-async --lib — 39 passed 🤖 Generated with Claude Code
Picks up 1c3f3dc..069682e, which bumps the nested seera-msquic to 3397280 and with it microsoft/msquic#6180. That fix is the one this branch was chasing. A stream stopped receiving while data still sat in its receive buffer: QuicStreamReceiveComplete cleared ReceiveDataPending whenever no receive was pending, without asking whether anything was left to deliver, and once cleared with data buffered that data was undeliverable for good. msquic-async never completes a receive inline — it returns QUIC_STATUS_PENDING and completes later, when the application drops the StreamRecvBuffer — which is exactly the shape that arrives with nothing pending, so this side is where it shows. It showed as a camera-client whose picture froze while its connection carried on receiving at half a megabyte a second, acknowledging everything, reporting no loss and no congestion, and never erroring. On the camera the same event read as push_one blocked on flow control, because the peer had stopped consuming a stream it would never consume again. A second viewer appeared to trigger it and did not: it only made the window wider, by putting two of these processes on one machine. In one run the first viewer froze ten seconds before the second one's leg existed. The bump carries two more, both of which touch paths this uses: the path validation timer is now armed when a validation starts, so an unanswered add_path is eventually abandoned rather than left forever; and the raw datapath no longer rejects a binding that has a local address and no remote one, which is what every relay leg here is. Evidence From a device, with a client whose picture had frozen: recv_bytes on the client climbing ~500 KB/s to the last sample, recv_dropped=0 video connection stats when="tick" stops — that sampler runs in the receive loop's select!, so a read that never finishes takes it with it heartbeat (a separate task) keeps ticking for another minute errors, closes, timeouts none So the connection was receiving and acknowledging while the read parked on one stream never resolved. On the camera, push_one was blocked on flow control with send_lost=0 and send_congestion=0 — not congestion, a peer that had stopped consuming. Verification cargo build -p camera-server -p camera-client against the new submodule — no API or FFI change, nothing on the Rust side needed updating cargo build -p seera-msquic — builds The fix itself is native and is exercised on Windows, where the failure was seen; that re-test is what confirms it. Independent of the open stack Touches only the submodule pointer, so it does not depend on #79-#82 and they do not depend on it. 🤖 Generated with Claude Code
The client sampled its connection from inside the receive loop, which reads one stream to completion at a time — so a read that never finished took the sampling down with it, and the log went quiet at exactly the moment the numbers mattered. The server side already samples from its own task for this reason, and says so at push_frames. Sampling moves to a task, and it also names the condition when it sees it: a connection receiving bytes while no frame comes out means a stream's buffered data is not reaching the application and the read parked on it cannot finish. Why this is worth having That is the msquic stream-receive bug fixed by microsoft/msquic#6180 and picked up in #83. Working it out from the outside took three rounds of logs across two machines, and the thing that finally identified it was noticing that one sampler had gone quiet while another kept ticking. A single line saying so would have been the first round. The bug is fixed, so this is not chasing a live failure. It is that the signature was legible only by accident, and the next transport-level stall will not be this one. Verification cargo test -p camera-core — all passing cargo clippy -p camera-core introduces no new warnings cargo fmt on the touched file only 🤖 Generated with Claude Code
Guillaume Hetier (guhetier)
left a comment
There was a problem hiding this comment.
Thanks for the PR.
It looks largely good, but I'll need to find a bit of time to properly review the details - the receive operation synchronization is not simple.
| // case under test: a completion seen with no receive pending. The API | ||
| // contract is that it is ignored silently. | ||
| // | ||
| CxPlatSleep(500); |
There was a problem hiding this comment.
Let's try to avoid the sleep and be event based instead.
Sleep makes tests slow and unreliable. It might be tricky in this case though.
There was a problem hiding this comment.
Guillaume Hetier (@guhetier) All three sleeps are gone, and the test now runs in 23ms instead of ~1.1s. The waiting here is not incidental, so instead of sleeping I used a normal priority GetParam as a barrier — it is queued behind the completion and blocks until the queue reaches it. This isn't a new trick in this file — QuicTestOperationPriority already relies on the same property.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6180 +/- ##
==========================================
- Coverage 85.96% 84.80% -1.17%
==========================================
Files 60 60
Lines 18974 18978 +4
==========================================
- Hits 16311 16094 -217
- Misses 2663 2884 +221 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A stream can stop receiving while data is already sitting in its receive buffer: QUIC_STREAM_EVENT_RECEIVE is never indicated for it, and StreamReceiveSetEnabled does not bring it back. QuicStreamReceiveComplete treated "no receive is currently pending" as "nothing is left to deliver". Those are different facts - only the receive buffer knows the second one, and the short-circuit skipped asking it. The flag it cleared, ReceiveDataPending, gates every later indication: QuicStreamRecvQueueFlush queues a flush only when it is set, QuicStreamRecvFlush returns immediately when it is clear, and QuicStreamReceiveComplete asks for a re-flush only when it is set. Once cleared with data still buffered, that data is never indicated, and StreamReceiveSetEnabled cannot recover it because it goes through QuicStreamRecvQueueFlush. Only newly arriving data sets the flag again, which never happens when the peer has finished sending or is blocked by flow control because the application never consumed what it was not told about. A completion processed with no receive pending is not an error case. The documentation for StreamReceiveComplete states that such calls are ignored silently, and they also occur without any application mistake when an asynchronous completion is absorbed by a flush already in progress. Ask the two questions separately: when no receive is pending there is no buffer space to reclaim, so consult the receive buffer directly for whether anything is still waiting. Dropping the short-circuit entirely does not work, because the FIN-only indication path reaches the completion with a receive buffer that has no written ranges, which QuicRecvBufferDrain asserts on. QuicRecvBufferHasUnreadData handles that safely and returns FALSE, so the FIN-only case still clears the flag as before. Covered by QuicTestStreamReceiveCompleteWithNoPendingReceive, which accepts half of an indicated receive, issues a completion with nothing outstanding, and requires the remaining half to still be delivered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e6a39d3 to
bcfc592
Compare
Description
A stream can stop receiving while data is already sitting in its receive buffer:
QUIC_STREAM_EVENT_RECEIVEis never indicated for it, andStreamReceiveSetEnableddoes not bring it back.Cause
QuicStreamReceiveCompletedecides whether anything is left to deliver like this:RecvPendingLength == 0means "no receive is currently pending". It says nothing about whether the receive buffer still holds undelivered data. OnlyQuicRecvBufferDrainanswers that, and the||short-circuits past it.ReceiveDataPendinggates every subsequent indication:QuicStreamRecvQueueFlush(stream_recv.c:132) queues a flush only ifReceiveEnabled && ReceiveDataPendingQuicStreamRecvFlush(stream_recv.c:896) returns immediately if it is clearQuicStreamReceiveComplete(stream_recv.c:1161) asks for a re-flush only if it is setSo once it is cleared while data remains, that data is never indicated.
StreamReceiveSetEnableddoes not help — it ends up inQuicStreamRecvQueueFlush, whose condition is not met. Only newly arriving data sets the flag again (stream_recv.c:581), which never happens if the peer has finished sending, or is blocked by flow control precisely because the application never consumed what it was not told about.A completion processed with no receive pending is not an error case.
docs/api/StreamReceiveComplete.mdsays:It is documented as harmless, and it also arises without any application mistake: in multi-receive mode an asynchronous completion can be absorbed by a flush that is already in progress (
stream_recv.c:1017), leaving the queued completion operation to run with nothing outstanding.Fix
Ask the two questions separately. When no receive is pending there is no buffer space to reclaim, so the receive buffer is consulted directly for whether anything is still waiting:
Simply dropping the short-circuit and always calling
QuicRecvBufferDraindoes not work: the FIN-only indication path (QuicStreamRecvFlushwith no data available) reaches the completion with a receive buffer that has no written ranges, andQuicRecvBufferDrainasserts on that (recv_buffer.c:1064).QuicRecvBufferHasUnreadDatahandles an empty buffer safely (recv_buffer.c:375-377), and returnsFALSEthere, so the FIN-only case still clears the flag exactly as before.The behaviour change is confined to the case where a completion is processed with no receive pending and the buffer still holds unread data: previously the remaining data became undeliverable, now it stays deliverable.
Testing
New test
QuicTestStreamReceiveCompleteWithNoPendingReceive(Misc.StreamReceiveCompleteWithNoPendingReceive), registered for both user and kernel mode. Using the default receive mode:QUIC_STATUS_PENDINGfrom the receive event.StreamReceiveComplete(50)accepts half, which pauses receives and leaves 50 bytes buffered with no receive pending.StreamReceiveComplete(0)is the case under test — a completion seen with nothing outstanding.StreamReceiveSetEnabledmust produce the remaining half, at offset 50 with length 50.Results:
*Receive*:*Recv*:*Stream*:*Abort*:*Data*:*Basic*:*Handshake*:*Shutdown*:*Mtu*passes in full: 2950 tests, no failures. That includesMisc.StreamAbortRecvFinRace, which is the test that catches theQuicRecvBufferDrainassert described above.Step 3 needs the completion in step 2 to have been processed first, and there is no API-visible signal for that, so the test sleeps between the two calls. If the sleep were ever too short the two completions would merge into one and the test would still pass — it loses coverage rather than becoming flaky.
Documentation
No documentation change.
docs/api/StreamReceiveComplete.mdalready states the behaviour this change implements.