ClientQUIC destructor closes the connection before its streams finish shutting down (intermittent abort) #7

New issue

Closed

opened 2026-08-25 03:06:12 +00:00 by jorijnvdgraaf · 0 comments

jorijnvdgraaf commented

2026-08-25 03:06:12 +00:00

Owner

Summary

~ClientQUIC calls MsQuicConnectionClose without waiting for its streams to
finish shutting down. When a connection is destroyed while stream shutdowns are
still in flight, msquic trips a bugcheck and aborts the process.

Reproduces on master (0827e1f) about 45% of runs with the test below.

Impact

Any client that tears a connection down and builds another in the same process
— a reconnect loop, most obviously — can abort instead of reconnecting. It does
not affect a connection that simply stays up, which is probably why it has gone
unnoticed: every QUIC test in this repo ends in std::_Exit(0) and so never
exercises the teardown path.

I hit this building a long-lived QUIC ingress client, where the reconnect-with-
backoff path destroys a ClientQUIC per attempt.

Reproducer

tests/ShouldSurviveConnectionChurn/main.cpp, registered with
cfg.AddTest("ShouldSurviveConnectionChurn").Dependencies({ &cfg });

import Crafter.Network;
import Crafter.Thread;
import std;
using namespace Crafter;

namespace {
    constexpr int cycles = 25;
    constexpr int streamsPerConnection = 3;
}

int main() {
    ThreadPool::Start();

    for (int i = 0; i < cycles; ++i) {
        const std::uint16_t port = static_cast<std::uint16_t>(19200 + i);
        const std::string alpn = "churn/1";

        QUICServerCredentials serverCreds;
        serverCreds.selfSigned = true;

        std::mutex serverMutex;
        std::vector<QUICStream> serverStreams;
        ClientQUIC* accepted = nullptr;

        auto listener = std::make_unique<ListenerQUIC>(
            port, alpn, serverCreds, [&](ClientQUIC* peer) {
                std::lock_guard lock(serverMutex);
                accepted = peer;
                for (int s = 0; s < streamsPerConnection; ++s) {
                    QUICStream stream = peer->OpenStream(/*unidirectional=*/true);
                    const char payload[] = "hello";
                    stream.SendSync(payload, sizeof(payload) - 1, /*finish=*/false);
                    serverStreams.push_back(std::move(stream));
                }
            });
        listener->ListenAsyncAsync();

        QUICClientCredentials clientCreds;
        clientCreds.insecureNoServerValidation = true;

        {
            ClientQUIC client(std::string("localhost"), port, alpn, clientCreds);

            // Each inbound stream gets a thread that stays blocked in
            // RecieveSync for the life of the connection -- the shape a
            // long-lived streaming client has. Teardown therefore happens
            // while readers are parked inside the receive path.
            std::mutex clientMutex;
            std::condition_variable clientCv;
            std::vector<std::thread> readers;
            int started = 0;

            client.OnStream([&](QUICStream stream) {
                auto shared = std::make_shared<QUICStream>(std::move(stream));
                std::thread reader([shared] {
                    try {
                        while (true) (void)shared->RecieveSync();
                    } catch (...) {
                        // Connection closed.
                    }
                });
                std::lock_guard lock(clientMutex);
                readers.push_back(std::move(reader));
                ++started;
                clientCv.notify_all();
            });

            {
                std::unique_lock lock(clientMutex);
                clientCv.wait_for(lock, std::chrono::seconds(5), [&] {
                    return started == streamsPerConnection;
                });
            }

            client.Stop();
            for (std::thread& reader : readers) {
                if (reader.joinable()) reader.join();
            }
            // `client` is destroyed here, once the readers have unwound.
        }

        {
            std::lock_guard lock(serverMutex);
            serverStreams.clear();
            if (accepted) accepted->Stop();
        }
        listener->Stop();
        listener.reset();
    }

    std::println("survived {} connection teardowns", cycles);
    return 0;
}

$ for i in $(seq 1 20); do ./ShouldSurviveConnectionChurn >/dev/null 2>&1 || n=$((n+1)); done
9/20 runs aborted

The blocked readers matter. An otherwise identical loop whose OnStream
handler reads once and returns passed 20/20 for me — the failure needs threads
parked inside RecieveSync when the connection goes away, which is the normal
shape for a streaming client.

Stack

#3  quic_bugcheck ()                       libmsquic.so.2
#4  MsQuicConnectionClose ()               libmsquic.so.2
#5  Crafter::ClientQUIC::~ClientQUIC ()
#6  main ()

Diagnosis

QUICStream::Stop only initiates a graceful shutdown — the comment says so
directly (implementations/Crafter.Network-ClientQUIC.cpp:197-205):

void QUICStream::Stop() {
    if (!handle) return;
    // Only INITIATE a graceful shutdown here; the async SHUTDOWN_COMPLETE
    // callback performs the actual StreamClose + ref release.

The real StreamClose happens later, in FinalizeClose
(Crafter.Network-ClientQUIC.cpp:91-97), driven by the terminal
QUIC_STREAM_EVENT_SHUTDOWN_COMPLETE callback.

~ClientQUIC (Crafter.Network-ClientQUIC.cpp:553-559) does not wait for any
of that:

ClientQUIC::~ClientQUIC() {
    if (!impl) return;
    if (impl->connection) {
        Runtime().api->ConnectionShutdown(impl->connection, QUIC_CONNECTION_SHUTDOWN_FLAG_NONE, 0);
        Runtime().api->ConnectionClose(impl->connection);

So ConnectionClose can run while streams belonging to that connection are
still open on the msquic side.

Supporting evidence: inserting a 50 ms sleep immediately before the connection
is destroyed took a variant of this from 7/15 to 2/15 failures. It narrows the
window without closing it, which is what you would expect if the race is
"streams have not reached SHUTDOWN_COMPLETE yet".

Suggested fix

Have ClientQUIC::Impl track outstanding streams — increment where a
QUICStream is created (ClientQUIC::OpenStream, and the peer-initiated path
in the connection callback), decrement in FinalizeClose — and have
~ClientQUIC wait on a condition variable, bounded, for that count to reach
zero before ConnectionShutdown / ConnectionClose.

A bounded wait matters: the decrement arrives on an msquic worker thread, so an
unbounded wait would turn a dropped peer into a hung destructor.

Environment

Crafter.Network master @ 0827e1f
msquic main, built by Crafter.Build into the external cache
clang 22.1.8, libc++, x86_64-pc-linux-gnu
Arch Linux, kernel 7.1.6

ClientQUIC's constructor waits on impl->cv.wait(...) with no timeout
(Crafter.Network-ClientQUIC.cpp:524), and QUIC_SETTINGS.HandshakeIdleTimeoutMs
is never set. Dialling a host that does not answer therefore blocks for minutes
rather than failing. Setting HandshakeIdleTimeoutMs alongside the existing
IdleTimeoutMs in OpenClientConfiguration bounds it. Happy to split this into
its own issue if you would rather keep them separate.

## Summary `~ClientQUIC` calls `MsQuicConnectionClose` without waiting for its streams to finish shutting down. When a connection is destroyed while stream shutdowns are still in flight, msquic trips a bugcheck and aborts the process. Reproduces on `master` (0827e1f) about **45% of runs** with the test below. ## Impact Any client that tears a connection down and builds another in the same process — a reconnect loop, most obviously — can abort instead of reconnecting. It does not affect a connection that simply stays up, which is probably why it has gone unnoticed: every QUIC test in this repo ends in `std::_Exit(0)` and so never exercises the teardown path. I hit this building a long-lived QUIC ingress client, where the reconnect-with- backoff path destroys a `ClientQUIC` per attempt. ## Reproducer `tests/ShouldSurviveConnectionChurn/main.cpp`, registered with `cfg.AddTest("ShouldSurviveConnectionChurn").Dependencies({ &cfg });` ```cpp import Crafter.Network; import Crafter.Thread; import std; using namespace Crafter; namespace { constexpr int cycles = 25; constexpr int streamsPerConnection = 3; } int main() { ThreadPool::Start(); for (int i = 0; i < cycles; ++i) { const std::uint16_t port = static_cast<std::uint16_t>(19200 + i); const std::string alpn = "churn/1"; QUICServerCredentials serverCreds; serverCreds.selfSigned = true; std::mutex serverMutex; std::vector<QUICStream> serverStreams; ClientQUIC* accepted = nullptr; auto listener = std::make_unique<ListenerQUIC>( port, alpn, serverCreds, [&](ClientQUIC* peer) { std::lock_guard lock(serverMutex); accepted = peer; for (int s = 0; s < streamsPerConnection; ++s) { QUICStream stream = peer->OpenStream(/*unidirectional=*/true); const char payload[] = "hello"; stream.SendSync(payload, sizeof(payload) - 1, /*finish=*/false); serverStreams.push_back(std::move(stream)); } }); listener->ListenAsyncAsync(); QUICClientCredentials clientCreds; clientCreds.insecureNoServerValidation = true; { ClientQUIC client(std::string("localhost"), port, alpn, clientCreds); // Each inbound stream gets a thread that stays blocked in // RecieveSync for the life of the connection -- the shape a // long-lived streaming client has. Teardown therefore happens // while readers are parked inside the receive path. std::mutex clientMutex; std::condition_variable clientCv; std::vector<std::thread> readers; int started = 0; client.OnStream([&](QUICStream stream) { auto shared = std::make_shared<QUICStream>(std::move(stream)); std::thread reader([shared] { try { while (true) (void)shared->RecieveSync(); } catch (...) { // Connection closed. } }); std::lock_guard lock(clientMutex); readers.push_back(std::move(reader)); ++started; clientCv.notify_all(); }); { std::unique_lock lock(clientMutex); clientCv.wait_for(lock, std::chrono::seconds(5), [&] { return started == streamsPerConnection; }); } client.Stop(); for (std::thread& reader : readers) { if (reader.joinable()) reader.join(); } // `client` is destroyed here, once the readers have unwound. } { std::lock_guard lock(serverMutex); serverStreams.clear(); if (accepted) accepted->Stop(); } listener->Stop(); listener.reset(); } std::println("survived {} connection teardowns", cycles); return 0; } ``` ``` $ for i in $(seq 1 20); do ./ShouldSurviveConnectionChurn >/dev/null 2>&1 || n=$((n+1)); done 9/20 runs aborted ``` The blocked readers matter. An otherwise identical loop whose `OnStream` handler reads once and returns passed 20/20 for me — the failure needs threads parked inside `RecieveSync` when the connection goes away, which is the normal shape for a streaming client. ## Stack ``` #3 quic_bugcheck () libmsquic.so.2 #4 MsQuicConnectionClose () libmsquic.so.2 #5 Crafter::ClientQUIC::~ClientQUIC () #6 main () ``` ## Diagnosis `QUICStream::Stop` only *initiates* a graceful shutdown — the comment says so directly (`implementations/Crafter.Network-ClientQUIC.cpp:197-205`): ```cpp void QUICStream::Stop() { if (!handle) return; // Only INITIATE a graceful shutdown here; the async SHUTDOWN_COMPLETE // callback performs the actual StreamClose + ref release. ``` The real `StreamClose` happens later, in `FinalizeClose` (`Crafter.Network-ClientQUIC.cpp:91-97`), driven by the terminal `QUIC_STREAM_EVENT_SHUTDOWN_COMPLETE` callback. `~ClientQUIC` (`Crafter.Network-ClientQUIC.cpp:553-559`) does not wait for any of that: ```cpp ClientQUIC::~ClientQUIC() { if (!impl) return; if (impl->connection) { Runtime().api->ConnectionShutdown(impl->connection, QUIC_CONNECTION_SHUTDOWN_FLAG_NONE, 0); Runtime().api->ConnectionClose(impl->connection); ``` So `ConnectionClose` can run while streams belonging to that connection are still open on the msquic side. Supporting evidence: inserting a 50 ms sleep immediately before the connection is destroyed took a variant of this from 7/15 to 2/15 failures. It narrows the window without closing it, which is what you would expect if the race is "streams have not reached SHUTDOWN_COMPLETE yet". ## Suggested fix Have `ClientQUIC::Impl` track outstanding streams — increment where a `QUICStream` is created (`ClientQUIC::OpenStream`, and the peer-initiated path in the connection callback), decrement in `FinalizeClose` — and have `~ClientQUIC` wait on a condition variable, bounded, for that count to reach zero before `ConnectionShutdown` / `ConnectionClose`. A bounded wait matters: the decrement arrives on an msquic worker thread, so an unbounded wait would turn a dropped peer into a hung destructor. ## Environment - Crafter.Network `master` @ 0827e1f - msquic `main`, built by Crafter.Build into the external cache - clang 22.1.8, libc++, `x86_64-pc-linux-gnu` - Arch Linux, kernel 7.1.6 ## Related, lower priority `ClientQUIC`'s constructor waits on `impl->cv.wait(...)` with no timeout (`Crafter.Network-ClientQUIC.cpp:524`), and `QUIC_SETTINGS.HandshakeIdleTimeoutMs` is never set. Dialling a host that does not answer therefore blocks for minutes rather than failing. Setting `HandshakeIdleTimeoutMs` alongside the existing `IdleTimeoutMs` in `OpenClientConfiguration` bounds it. Happy to split this into its own issue if you would rather keep them separate.

jorijnvdgraaf added the

claude:ready

label

2026-08-25 16:29:27 +00:00