fix(quic): close stream handles whose async StreamStart lost the teardown race #10

Merged

catbot merged 1 commit from claude/issue-9 into master

2026-08-25 23:19:15 +00:00

catbot commented

2026-08-25 23:19:04 +00:00

Member

Resolves #9.

What was wrong

StreamStart is asynchronous. MsQuicStreamStart returns QUIC_STATUS_PENDING
and queues the start onto the connection, so it can still fail later — and it
fails with QUIC_STATUS_INVALID_STATE whenever the connection is shut down
before the queued start runs. Passed QUIC_STREAM_START_FLAG_NONE, msquic leaves
such a stream neither started nor shut down:

Exit:
    if (!IsRemoteStream) {
        QuicStreamIndicateStartComplete(Stream, Status);
        if (QUIC_FAILED(Status) &&
            (Flags & QUIC_STREAM_START_FLAG_SHUTDOWN_ON_FAIL)) {   // <- we didn't pass it
            QuicStreamShutdown(Stream, ABORT | IMMEDIATE, 0);
        }
    }

SHUTDOWN_COMPLETE therefore never arrives, and the stream close was driven
entirely off that event — so the handle stayed open, ~ClientQUIC's bounded
drain expired, the connection was closed with a stream msquic still considered
open, and the registration's rundown at exit() waited forever for the handle.
That is both reported symptoms from one cause, as #9 guessed.

Instrumenting every StreamOpen/StreamClose/ConnectionClose over the
reproducer confirms it: each failing run leaks exactly one locally-opened stream
handle, one per ConnCloseDtorTIMEOUT, correlating 1:1 with a START_COMPLETE
carrying 0x1 (INVALID_STATE).

run 1 rc=0   startfail=0 timeouts=0
run 2 rc=124 startfail=2 timeouts=2
run 3 rc=124 startfail=1 timeouts=0
run 4 rc=0   startfail=0 timeouts=0
run 5 rc=124 startfail=1 timeouts=1

What changed

OpenStream passes QUIC_STREAM_START_FLAG_SHUTDOWN_ON_FAIL. A start that
loses the race is now followed by SHUTDOWN_COMPLETE like any other stream, so
its handle is closed. This is the fix for the hang.
QUICStream::Stop also aborts the receive direction. GRACEFUL closes only
the send side, and msquic rejects it combined with any other flag, so it needs a
second call. Without it a bidirectional stream whose peer keeps its send side
open never reaches SHUTDOWN_COMPLETE either — the same leak by a different
route, and one that does not need a race to hit.
The msquic stream handle is refcounted. Holders are the msquic callback (one
ref, released once SHUTDOWN_COMPLETE has been delivered) and every app thread
inside an msquic call on the handle; whoever lets go last closes it.
StreamSend / StreamShutdown / GetParam previously ran against a raw handle
that a worker thread could close in between — StreamClose is "equivalent to
free", so that is a use-after-free, and it is the most likely source of the
quic_bugcheck ← QuicOperationDequeue abort. A refcount rather than an
exclusion lock because MsQuicGetParam blocks on the connection's worker, so
that worker must never be the thread waiting for app calls to drain.
~ClientQUIC drops undispatched peer streams before closing the connection
rather than with the rest of Impl afterwards, so the drain actually covers them.

On the question #9 raised — the 5s drain bound is left alone. It expiring was a
symptom, not the cause; with the above the count always reaches zero, and the
bound stays as a backstop against wedging a destructor outright. That reasoning
is written down next to the constant.

Testing

tests/ShouldSurviveConnectionChurn takes #9's reproducer shape: a client-opened
bidirectional control stream with a thread sending across the teardown, on top of
the existing parked-reader churn loop. It also no longer ends in std::_Exit —
returning from main runs the static MsQuicRuntime destructor, and its
RegistrationClose is exactly where a leaked handle shows up, so exiting early
skipped half the point.

	before	after
`ShouldSurviveConnectionChurn`	5 ok / 10 hung of 15	90 ok / 0 hung / 0 aborted of 90

Re-instrumented under the fix, all 25 cycles balance and the drain never expires:

run 1 rc=0 timeouts=0 opens=175 closes=175 LEAKED=[]
...  (12 runs, identical)

Full suite: 19 passed, 1 failed. The failure is ShouldSend, which dials
cloudflare-quic.com:443; outbound UDP is blocked in this environment
(echo > /dev/udp/… → Operation not permitted), and it fails identically with
these changes stashed on master. Everything else, including all the QUIC,
HTTP/3 and WebTransport tests, was run 5 more times to check the Stop() change
did not make anything flaky — 10/10 each time.

🤖 Generated with Claude Code

Resolves #9. ## What was wrong `StreamStart` is asynchronous. `MsQuicStreamStart` returns `QUIC_STATUS_PENDING` and queues the start onto the connection, so it can still fail *later* — and it fails with `QUIC_STATUS_INVALID_STATE` whenever the connection is shut down before the queued start runs. Passed `QUIC_STREAM_START_FLAG_NONE`, msquic leaves such a stream neither started nor shut down: ```c Exit: if (!IsRemoteStream) { QuicStreamIndicateStartComplete(Stream, Status); if (QUIC_FAILED(Status) && (Flags & QUIC_STREAM_START_FLAG_SHUTDOWN_ON_FAIL)) { // <- we didn't pass it QuicStreamShutdown(Stream, ABORT | IMMEDIATE, 0); } } ``` `SHUTDOWN_COMPLETE` therefore never arrives, and the stream close was driven entirely off that event — so the handle stayed open, `~ClientQUIC`'s bounded drain expired, the connection was closed with a stream msquic still considered open, and the registration's rundown at `exit()` waited forever for the handle. That is both reported symptoms from one cause, as #9 guessed. Instrumenting every `StreamOpen`/`StreamClose`/`ConnectionClose` over the reproducer confirms it: each failing run leaks exactly one locally-opened stream handle, one per `ConnCloseDtorTIMEOUT`, correlating 1:1 with a `START_COMPLETE` carrying `0x1` (`INVALID_STATE`). ``` run 1 rc=0 startfail=0 timeouts=0 run 2 rc=124 startfail=2 timeouts=2 run 3 rc=124 startfail=1 timeouts=0 run 4 rc=0 startfail=0 timeouts=0 run 5 rc=124 startfail=1 timeouts=1 ``` ## What changed - **`OpenStream` passes `QUIC_STREAM_START_FLAG_SHUTDOWN_ON_FAIL`.** A start that loses the race is now followed by `SHUTDOWN_COMPLETE` like any other stream, so its handle is closed. This is the fix for the hang. - **`QUICStream::Stop` also aborts the receive direction.** `GRACEFUL` closes only the send side, and msquic rejects it combined with any other flag, so it needs a second call. Without it a bidirectional stream whose peer keeps its send side open never reaches `SHUTDOWN_COMPLETE` either — the same leak by a different route, and one that does not need a race to hit. - **The msquic stream handle is refcounted.** Holders are the msquic callback (one ref, released once `SHUTDOWN_COMPLETE` has been delivered) and every app thread inside an msquic call on the handle; whoever lets go last closes it. `StreamSend` / `StreamShutdown` / `GetParam` previously ran against a raw handle that a worker thread could close in between — `StreamClose` is "equivalent to free", so that is a use-after-free, and it is the most likely source of the `quic_bugcheck ← QuicOperationDequeue` abort. A refcount rather than an exclusion lock because `MsQuicGetParam` blocks on the connection's worker, so that worker must never be the thread waiting for app calls to drain. - **`~ClientQUIC` drops undispatched peer streams before closing the connection** rather than with the rest of `Impl` afterwards, so the drain actually covers them. On the question #9 raised — the 5s drain bound is left alone. It expiring was a symptom, not the cause; with the above the count always reaches zero, and the bound stays as a backstop against wedging a destructor outright. That reasoning is written down next to the constant. ## Testing `tests/ShouldSurviveConnectionChurn` takes #9's reproducer shape: a client-opened bidirectional control stream with a thread sending across the teardown, on top of the existing parked-reader churn loop. It also no longer ends in `std::_Exit` — returning from `main` runs the static `MsQuicRuntime` destructor, and its `RegistrationClose` is exactly where a leaked handle shows up, so exiting early skipped half the point. | | before | after | |---|---|---| | `ShouldSurviveConnectionChurn` | 5 ok / 10 hung of 15 | 90 ok / 0 hung / 0 aborted of 90 | Re-instrumented under the fix, all 25 cycles balance and the drain never expires: ``` run 1 rc=0 timeouts=0 opens=175 closes=175 LEAKED=[] ... (12 runs, identical) ``` Full suite: **19 passed, 1 failed**. The failure is `ShouldSend`, which dials `cloudflare-quic.com:443`; outbound UDP is blocked in this environment (`echo > /dev/udp/…` → `Operation not permitted`), and it fails identically with these changes stashed on `master`. Everything else, including all the QUIC, HTTP/3 and WebTransport tests, was run 5 more times to check the `Stop()` change did not make anything flaky — 10/10 each time. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

catbot added 1 commit

2026-08-25 23:19:04 +00:00

fix(quic): close stream handles whose async StreamStart lost the teardown race ffc118f825

A client that sends on a stream while its connection is being torn down —
the shape any long-lived client with periodic acks or keepalives has —
would hang at exit inside MsQuicRegistrationClose, and occasionally
bugcheck on an msquic worker thread instead.

Root cause: StreamStart is asynchronous. It returns QUIC_STATUS_PENDING
and queues the start onto the connection, so it can still fail later, and
it fails with QUIC_STATUS_INVALID_STATE whenever the connection is shut
down before the queued start runs. Passed QUIC_STREAM_START_FLAG_NONE,
msquic leaves such a stream neither started nor shut down, so
SHUTDOWN_COMPLETE never arrives. The stream close is driven entirely off
that event, so the handle stays open: ~ClientQUIC's bounded drain expires,
the connection is closed with a stream msquic still considers open, and
the registration's rundown at exit() waits forever for the handle.

Instrumenting the handle lifecycle over the reproducer shows one leaked
locally-opened stream per failing run, correlating 1:1 with a
START_COMPLETE carrying 0x1 (INVALID_STATE).

Fixes:

  - OpenStream passes QUIC_STREAM_START_FLAG_SHUTDOWN_ON_FAIL, so a start
    that loses the race is followed by SHUTDOWN_COMPLETE like any other
    stream and its handle is closed.

  - QUICStream::Stop also aborts the receive direction. GRACEFUL closes
    only the send side and msquic rejects it combined with any other flag,
    so a bidirectional stream whose peer keeps its send side open never
    reached SHUTDOWN_COMPLETE either.

  - The msquic stream handle is now refcounted, held by the callback plus
    every app thread inside an msquic call on it, and closed by whoever
    lets go last. StreamSend/StreamShutdown/GetParam could previously run
    against a handle a worker thread had just closed — a use-after-free
    msquic reports as a bugcheck. A refcount rather than an exclusion lock
    because GetParam blocks on the connection's worker, which must never
    be the thread waiting.

  - ~ClientQUIC drops undispatched peer streams before closing the
    connection rather than with the rest of Impl afterwards, so the drain
    covers them.

The reproducer (tests/ShouldSurviveConnectionChurn) grows a client-opened
bidirectional control stream with a thread sending across the teardown,
and no longer ends in std::_Exit — returning from main runs the static
MsQuicRuntime destructor, and its RegistrationClose is where a leaked
handle shows up. Before: 5 of 15 runs clean, 10 hung. After: 90 of 90
clean, with every opened handle observed closed and the drain never
expiring.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

catbot merged commit 05f7652770 into master

2026-08-25 23:19:15 +00:00

catbot deleted branch claude/issue-9

2026-08-25 23:19:15 +00:00

catbot referenced this pull request from a commit

2026-08-25 23:19:16 +00:00

Merge pull request 'fix(quic): close stream handles whose async StreamStart lost the teardown race' (#10) from claude/issue-9 into master