Skip to content

Don't touch a QUIC connection's state after it is gone - #182

Merged
jagerman merged 3 commits into
session-foundation:devfrom
mpretty-cyro:fix/quic-response-null-conn
Oct 7, 2026
Merged

jagerman merged 3 commits into
session-foundation:devfrom
mpretty-cyro:fix/quic-response-null-conn

Conversation

@mpretty-cyro

Copy link
Copy Markdown
Collaborator

Two fixes to QuicTransport's per-connection bookkeeping, both about state outliving a connection. They're separate commits so the second can be dropped on its own.

1. Don't dereference a connection that is already gone when a response arrives

The response handler in _send_on_connection looks up the request's connection to return the stream to the pool, or close it:

auto conn = _endpoint->get_conn(conn_id);
if (conn && conn->get_streams_available() <= ACTIVE_STREAM_PRUNE_LIMIT)
    _available_stream_ids[conn_id].insert(stream_id);
else if (auto stream = conn->get_stream<oxen::quic::BTRequestStream>(stream_id))  // conn may be null
    stream->close();

When the connection has already closed, conn is null, so the first test fails and the else if dereferences null. The code predates the libquic bump, but libquic 1.11 now always fires a request's callback, including from ~sent_request as the connection is torn down (oxen-libquic 005631f / 13d4f40). That makes "response arrives, connection gone" an ordinary teardown path, e.g. on suspend, on a node dropping us, or on network loss. With the connection gone there is nothing to return to the pool or close, and any ids pooled for it are dead, so those are dropped too.

2. Clear only the closed connection's state when a connection fails

The connection-closed callback ignored the connection it was given, and _fail_connection cleaned up by pubkey: it extracted _active_connection_ids[pubkey], then erased the stream ids of whichever connection that pubkey currently mapped to. A newer connection to the same node can be established before an old one's close arrives. The send path already handles "connection no longer exists" by dropping the mapping and failing the request, which the caller then retries. When the old close lands after that:

  • it drops the new connection's mapping and pooled stream ids, so the next request opens yet another connection;
  • the old connection's stream ids are never erased.

The closed callback now passes conn.reference_id(), and _fail_connection erases that connection's stream ids. It removes the pubkey's mapping only if the mapping still points at that connection. Callers without a connection (the connect exception paths) keep the old behaviour.

Not changed, and worth a look separately: the rest of _fail_connection (failing pending verification callbacks, failure listeners, node-failure reporting on handshake timeout) still acts per pubkey. So a stale close for an old connection can still fail callbacks that belong to a newer attempt on the same node.

Tested: both commits compile in Session iOS's Debug_Compile_LibSession build (Xcode 27; networking enabled) on top of dev + #181, and clang-format-19 is clean. The quic transport unit tests were not run; this was not built with tests on locally.

mpretty-cyro and others added 3 commits October 7, 2026 10:54
On a normal close, libquic fails a stream's pending requests from
close_all_streams() while the connection still exists; it only fails
them from ~sent_request when a stream outlives its connection.
@jagerman

jagerman commented Oct 7, 2026

Copy link
Copy Markdown
Member

Minor comment clarification pushed; otherwise LGTM.

@jagerman

jagerman commented Oct 7, 2026

Copy link
Copy Markdown
Member

Also worth noting: both issues fixed here were possible before 1.11; I think 1.11 just made them a little more likely.

@jagerman
jagerman merged commit d21126b into session-foundation:dev Oct 7, 2026
16 of 26 checks passed
@jagerman

jagerman commented Oct 7, 2026

Copy link
Copy Markdown
Member

(Merged with CI failing: the failures are all from a temporary fmt 1.11 transition causing issues with spdlog in debian-sid that should go away in a couple of days).

@mpretty-cyro
mpretty-cyro deleted the fix/quic-response-null-conn branch October 7, 2026 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants