fix(quic): stop ~ClientQUIC aborting the process on teardown #8

Merged

catbot merged 3 commits from claude/issue-7 into master

2026-08-25 17:10:08 +00:00

catbot commented

2026-08-25 17:10:02 +00:00

Member

~ClientQUIC aborted the process on roughly half of teardowns. The diagnosis
in the issue was close but landed on the wrong mechanism, so the fix covers
both what was reported and what was actually crashing.

What was actually aborting

MsQuicConnectionClose was reachable twice for the same handle:

~ClientQUIC called it unconditionally, and
the SHUTDOWN_COMPLETE connection callback also called it whenever
AppCloseInProgress was clear — which it is for the entire window between
the destructor's ConnectionShutdown and its ConnectionClose.

The second call trips CXPLAT_TEL_ASSERT(!Connection->State.HandleClosed) at
the top of MsQuicConnectionClose (a live CXPLAT_FRE_ASSERT in this build),
which is the quic_bugcheck ← MsQuicConnectionClose ← ~ClientQUIC stack from
the report. The callback's self->connection = nullptr did not help — the
destructor had already loaded the handle.

impl->connection is now a claim token, taken under the mutex by
ClaimConnection(). Exactly one of the two wins and closes. The callback keeps
closing peer-dropped connections so they do not leak while their ClientQUIC
lives on.

This also explains the 50 ms-sleep evidence in the issue: sleeping before the
destructor lets the callback finish and null the handle first, so the
destructor re-reads nullptr and quietly does nothing.

The reported stream-ordering race, fixed too

QUICStream::Stop only initiates a shutdown whose async completion does the
actual StreamClose, so a connection could be closed with streams msquic still
considered open. ~ClientQUIC now waits for them, as suggested, via a
StreamRegistry incremented where the msquic handler is installed
(OpenStream / the peer-stream constructor) and decremented in
FinalizeClose.

The wait is bounded at 5 s — the decrements arrive on msquic worker threads, so
an unbounded wait would turn a dropped peer into a hung destructor. The count
lives in a shared_ptr<StreamRegistry> rather than behind the ClientQUIC* so
a stream finalising after the wait gave up decrements a live object instead of
a freed connection.

Instrumenting the destructor showed this path is real but rarer than the double
close: streams were still open at close time on roughly 1 run in 3 (server side,
where the app holds stream wrappers past the connection). On its own it is a
refcount leak rather than the abort.

Also in here

Handshake bound. HandshakeIdleTimeoutMs was never set, so it inherited
the 120 s idle timeout and the constructor's cv.wait parked for minutes on a
host that never answers. Bounded to 10 s, as the issue's lower-priority note
suggested — kept as its own commit.
Handshake-failure leak. Throwing from the constructor body means
~ClientQUIC never runs, so the connection and configuration handles both
leaked — and the connection callback holds the impl that was about to be
destroyed with the half-built object. The failure path now closes both.
Stop() reads the handle under the lock instead of racing the callback.

Testing

tests/ShouldSurviveConnectionChurn — 25 build-up/tear-down cycles with reader
threads parked inside RecieveSync, the shape a reconnect loop has. Nothing
else in the suite exercised this: every other QUIC test ends in
std::_Exit(0) and so never runs a ClientQUIC destructor.

against the unfixed implementation:  10/20 runs aborted
with this branch:                     0/70 runs aborted

The server side of the loop deliberately destroys the connection before
clearing its stream wrappers, which is what exercises the drain wait.

One pre-existing failure

crafter-build test is 18 passed, 1 failed. The failure is ShouldSend,
the live-interop test against cloudflare-quic.com:443, which this environment
cannot reach (curl https://cloudflare-quic.com/ times out; the host is not on
the egress allowlist). README already flags it: "The external-interop test
requires outbound UDP/443; if your network blocks it the test will fail."

Verified on master in a clean worktree, same box: 17 passed, 1 timed out —
ShouldSend (60005ms) timeout. On this branch it fails in 10 s with
QUIC handshake failed: 0x3e (QUIC_STATUS_CONNECTION_IDLE) instead of hanging
for a minute, which is the handshake bound above doing its job.

Every other test passes, and the wasm32-wasip1 build is green.

Resolves #7

🤖 Generated with Claude Code

`~ClientQUIC` aborted the process on roughly half of teardowns. The diagnosis in the issue was close but landed on the wrong mechanism, so the fix covers both what was reported and what was actually crashing. ## What was actually aborting `MsQuicConnectionClose` was reachable **twice for the same handle**: - `~ClientQUIC` called it unconditionally, and - the `SHUTDOWN_COMPLETE` connection callback also called it whenever `AppCloseInProgress` was clear — which it is for the entire window between the destructor's `ConnectionShutdown` and its `ConnectionClose`. The second call trips `CXPLAT_TEL_ASSERT(!Connection->State.HandleClosed)` at the top of `MsQuicConnectionClose` (a live `CXPLAT_FRE_ASSERT` in this build), which is the `quic_bugcheck ← MsQuicConnectionClose ← ~ClientQUIC` stack from the report. The callback's `self->connection = nullptr` did not help — the destructor had already loaded the handle. `impl->connection` is now a claim token, taken under the mutex by `ClaimConnection()`. Exactly one of the two wins and closes. The callback keeps closing peer-dropped connections so they do not leak while their `ClientQUIC` lives on. This also explains the 50 ms-sleep evidence in the issue: sleeping before the destructor lets the callback finish and null the handle first, so the destructor re-reads `nullptr` and quietly does nothing. ## The reported stream-ordering race, fixed too `QUICStream::Stop` only initiates a shutdown whose async completion does the actual `StreamClose`, so a connection could be closed with streams msquic still considered open. `~ClientQUIC` now waits for them, as suggested, via a `StreamRegistry` incremented where the msquic handler is installed (`OpenStream` / the peer-stream constructor) and decremented in `FinalizeClose`. The wait is bounded at 5 s — the decrements arrive on msquic worker threads, so an unbounded wait would turn a dropped peer into a hung destructor. The count lives in a `shared_ptr<StreamRegistry>` rather than behind the `ClientQUIC*` so a stream finalising *after* the wait gave up decrements a live object instead of a freed connection. Instrumenting the destructor showed this path is real but rarer than the double close: streams were still open at close time on roughly 1 run in 3 (server side, where the app holds stream wrappers past the connection). On its own it is a refcount leak rather than the abort. ## Also in here - **Handshake bound.** `HandshakeIdleTimeoutMs` was never set, so it inherited the 120 s idle timeout and the constructor's `cv.wait` parked for minutes on a host that never answers. Bounded to 10 s, as the issue's lower-priority note suggested — kept as its own commit. - **Handshake-failure leak.** Throwing from the constructor body means `~ClientQUIC` never runs, so the connection and configuration handles both leaked — and the connection callback holds the `impl` that was about to be destroyed with the half-built object. The failure path now closes both. - **`Stop()`** reads the handle under the lock instead of racing the callback. ## Testing `tests/ShouldSurviveConnectionChurn` — 25 build-up/tear-down cycles with reader threads parked inside `RecieveSync`, the shape a reconnect loop has. Nothing else in the suite exercised this: every other QUIC test ends in `std::_Exit(0)` and so never runs a `ClientQUIC` destructor. ``` against the unfixed implementation: 10/20 runs aborted with this branch: 0/70 runs aborted ``` The server side of the loop deliberately destroys the connection *before* clearing its stream wrappers, which is what exercises the drain wait. ## One pre-existing failure `crafter-build test` is **18 passed, 1 failed**. The failure is `ShouldSend`, the live-interop test against `cloudflare-quic.com:443`, which this environment cannot reach (`curl https://cloudflare-quic.com/` times out; the host is not on the egress allowlist). README already flags it: *"The external-interop test requires outbound UDP/443; if your network blocks it the test will fail."* Verified on `master` in a clean worktree, same box: `17 passed, 1 timed out` — `ShouldSend (60005ms) timeout`. On this branch it fails in 10 s with `QUIC handshake failed: 0x3e` (`QUIC_STATUS_CONNECTION_IDLE`) instead of hanging for a minute, which is the handshake bound above doing its job. Every other test passes, and the wasm32-wasip1 build is green. Resolves #7 🤖 Generated with [Claude Code](https://claude.com/claude-code)

catbot added 3 commits

2026-08-25 17:10:02 +00:00

test(quic): cover connection teardown 25e8c84794

Every other QUIC test ends in std::_Exit(0), so nothing in the suite ever
ran a ~ClientQUIC. This adds 25 build-up/tear-down cycles with reader
threads parked inside RecieveSync -- the shape a reconnect loop has --
which aborts on ~10/20 runs against the current implementation.

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

fix(quic): stop ~ClientQUIC racing the callback to close the connection 42a34de244

MsQuicConnectionClose was reachable twice for one handle: ~ClientQUIC
called it unconditionally, and the SHUTDOWN_COMPLETE callback also called
it whenever AppCloseInProgress was clear -- which it is for the whole
window between the destructor's ConnectionShutdown and its ConnectionClose.
The second call trips CXPLAT_TEL_ASSERT(!Connection->State.HandleClosed)
and aborts the process. Clearing impl->connection from the callback did not
help: the destructor had already loaded it.

`connection` is now a claim token, taken under the mutex, and only whoever
wins the claim closes. The callback keeps closing peer-dropped connections
so they do not leak while their ClientQUIC lives on.

Separately, the destructor now waits (bounded, 5s) for the connection's
streams to reach StreamClose before closing it. QUICStream::Stop only
initiates a shutdown whose async completion does the close, so a
connection could otherwise be closed with streams msquic still considers
open. The count lives in a shared StreamRegistry rather than behind the
ClientQUIC*, so a stream finalising after the wait gave up decrements a
live object.

A failed handshake also closes its connection and configuration now.
Throwing from the constructor body means ~ClientQUIC never runs, so both
handles leaked -- and the connection callback holds the impl that was
about to be destroyed with the half-built object.

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

fix(quic): bound the client handshake f14b85694f

QUIC_SETTINGS never set HandshakeIdleTimeoutMs, so it inherited the
120s idle timeout. The constructor blocks until msquic reports the
connection either up or shut down, which meant dialling a host that does
not answer parked the caller for minutes. Bound it to 10s.

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

catbot merged commit e8e9c0d180 into master

2026-08-25 17:10:08 +00:00

catbot deleted branch claude/issue-7

2026-08-25 17:10:08 +00:00

catbot referenced this pull request from a commit

2026-08-25 17:10:08 +00:00

Merge pull request 'fix(quic): stop ~ClientQUIC aborting the process on teardown' (#8) from claude/issue-7 into master