cluster: nodes never dial peers they discover, never retry failed dials, and never drop dead peers

[fabracht]

The cluster requires a full peer mesh — broadcast() is a single hop over directly-connected peers with no relaying (cluster/quic_transport.rs:342), and a targeted send() to a non-peer fails with NodeNotFound and is discarded. Nothing in the crate routes or relays.

Three gaps make that requirement hard to satisfy:

1. No discovery dial. A node learns that members exist (partition map, voter gossip, handle_node_alive) but never dials them. connect_to_peer has exactly one caller: a startup loop over --peers (cluster_agent/broker.rs:488). Heartbeats carry no address (node_id, bitmaps, voters only), so a node cannot dial a peer it was not configured with.
2. No dial retry. A peer that is down at startup logs a warning and is never retried.
3. Dead peers are never removed. The only mutations to the peers map are two inserts (quic_transport.rs:258, :504); the receiver task simply returns on disconnect. A dead link still counts as connected, so the detection added in warn when the cluster peer mesh is incomplete and fix the seed-node docs #139 misses it, and a restarted node is not re-dialled by the nodes that started before it.

Measured on a live 3-node hub-and-spoke cluster (nodes 2 and 3 each peered only to node 1 — previously the documented setup): cross-node pub/sub between nodes 2 and 3 fails in both directions, and a read on node 3 cannot see data created on node 2. Both are deterministic across repeated runs; mqdb dev test --all reports 4/6 pub/sub and 5/6 DB CRUD on that topology.

Fix direction: propagate node addresses in the gossip, dial discovered members, retry failed dials, and remove peers on disconnect using a per-connection token so that a reconnecting peer's fresh connection is not dropped by the old connection's cleanup.

Every mqdb dev start-cluster topology forms a complete graph, so no existing test covers this. #139 added detection and corrected the documentation; this issue is the root cause.

[fabracht]

Modelled the connection lifecycle in TLA+ before implementing: specs/ClusterPeerMap.tla (PR #142). The model changed the intended fix, so recording the result here.

The spec models peers: HashMap<NodeId, PeerConnection> as it actually behaves: two insert sites under the same key, so an insert replaces and drops the displaced connection's send stream, while each detached receiver task outlives the replacement. Two independent reviewers checked the model against quic_transport.rs and the quinn source; RecvStream::drop calling stop() and SendStream::drop calling finish() were verified directly.

Results, each from an exhausted state space, with removal triggered by the receiver task ending (the only death signal the code has):

• today (no removal) — InvNoDeadLink violated: a dead peer stays in the map forever. Because nothing re-dials, it never recovers, and direct_peers() keeps reporting it as linked, so the mesh warning added in warn when the cluster peer mesh is incomplete and fix the seed-node docs #139 goes silent exactly in the case it exists to report.
• remove by key on receiver exit — InvNoLiveDrop violated two ways: it removes a link whose send side still works, and on a reconnect the old connection's cleanup deletes the newer entry. Not a rare interleaving: on a mesh startup both nodes dial each other, the second insert drops the loser's send stream, and the resulting FIN ends the old receiver strictly after the newer insert.
• guarded compare-and-remove — InvNoLiveDrop still violated. The guard asks whether the slot still holds my generation, not whether the link is dead, so it also removes a connection whose send stream is healthy.

Conclusion: receiver exit is the wrong removal trigger. A receiver ends whenever the remote drops its send stream, which it does on every replacement, while our send path to that peer may be perfectly usable. Removal should be driven by send failure or an explicit liveness probe, guarded by a per-connection id, and paired with a re-dial.

Use a monotonic counter for that id, not quinn's Connection::stable_id — it is a heap pointer that a later connection can reuse, which would reintroduce ABA in precisely a compare-and-remove.

Two limits of the model, recorded in the spec header so they are not mistaken for guarantees: no async implementation can satisfy InvNoDeadLink, since a break and its cleanup are separate events separated by unbounded time; and guarded removal without a re-dial converts a stale entry into a permanently absent one, which the safety invariants cannot distinguish from a real fix. Separating those needs a Redial action and a liveness property, which the spec does not yet have.

[fabracht]

Correction to my previous comment — please do not implement the recommendation in it.

A multi-reviewer audit of the model found errors in it, including one in a claim I said I had verified. Retracting the prescription and keeping only what holds up.

What was wrong

1. "Receiver exit is the wrong removal trigger" — not established. The model excused the transient window for one invariant ("no async implementation can satisfy InvNoDeadLink, since a break and its cleanup are separate events") while treating the same kind of transient as disqualifying for the other. Applied evenhandedly, the guarded configuration's violation is a handshake-duration transient that the event causing it repairs: our receiver ends because the remote is mid-dial, and the remote's dial then re-creates the entry. That sentence was prose in the spec header, not checker output.

2. "RecvStream::drop calls stop() on our send stream" — wrong on the paths that matter, and I claimed to have verified it. Drop early-returns when all_data_read is set (quinn 0.11.11 src/recv_stream.rs:511-523), and the FIN path sets exactly that flag (ReadStatus::Finished → all_data_read = true, :387). So on FIN-after-replacement — the path the argument was built on — stop() is never called.

3. "The only death signal the code has" — wrong. broadcast already observes per-peer send failure and discards it (quic_transport.rs:350-352).

4. The published configurations could not reproduce the table in the spec header: TLC halts at the first violation, so the naive and guarded configs never evaluated the invariant the header reports for them.

What still holds, verified independently against the code by several reviewers:

• Two insert sites, both keyed by NodeId, so an insert replaces and the displaced PeerConnection is dropped; SendStream::drop finishes the stream (send_stream.rs:344-364).
• Receiver tasks are detached and are not stopped by a replacement; the connection survives it because RecvStream holds its own ConnectionRef.
• Nothing ever removes an entry, and connect_to_peer has exactly one caller, in the startup loop, with no retry.
• A stale entry therefore silences the mesh warning added in warn when the cluster peer mesh is incomplete and fix the seed-node docs #139, since direct_peers() reports map keys.
• Removing by key when a receiver task ends is wrong — it drops links whose send side still works, and on a reconnect the old connection's cleanup can delete the newer entry. This is the one design conclusion worth keeping.
• If any compare-and-remove is implemented, guard it with a monotonic counter, not Connection::stable_id, which is a reusable heap pointer (connection.rs:917-919).

Suggested reshaping (a proposal, not a verified conclusion):

1. Fix the reported symptom cheaply. The blast radius of a stale entry is one log line and one status field, so consider intersecting unlinked_from with heartbeat.alive_nodes() — the heartbeat already has hysteresis and a 15s timeout against the 60s mesh check — rather than changing connection lifecycle to make a warning accurate.
2. Fix cluster: a timed-out send leaves a partial frame and desynchronizes the QUIC stream #143 first. A per-peer writer task removes the framing corruption and hands this issue an unambiguous death signal and a home for the generation counter.
3. Only then consider removal, and note re-dial is not implementable as described: the transport keeps no address book, connect_to_peer's address argument is discarded, and an accept-only peer's remote address is an ephemeral client port, so learning a dialable address needs a wire-format change. Removal without working re-dial converts a stale entry into a permanently absent one.

The spec (PR #142) is being closed rather than corrected; a single-node, single-peer safety model cannot express the cross-node coupling and liveness questions this issue actually turns on.

[fabracht]

One more gap for this issue, split out from #141 / PR #151.

After #151, SubscriptionCache::reconcile repairs the local TopicIndex/WildcardStore from the replicated subscription record, so a node that holds the client's session partition recovers from a missed subscribe broadcast (live 5-node E2E: 6/6 delivered vs 0/6 on main).

A node that holds no record for the client has nothing to repair from. If it misses the one-hop TopicSubscriptionBroadcast/WildcardBroadcast (unlinked peer, down at the time, dropped under backpressure), publishes made on that node never reach the subscriber. The only other way the entry arrives is a partition snapshot, if the node happens to hold the topic's own partition. specs/ClusterSubReconcile_nonholder.cfg records this: NonHolderIndexed is violated even with the fixed reconcile.

It belongs here because it is the same root cause: subscription state reaches non-holders only by a single-hop broadcast with no retry or resync. Possible directions, not yet modelled or verified: resync the subscription index when a peer link is (re)established, or route publishes through the topic's partition owners instead of relying on every node having a full index.

Not yet checked live: whether a restarted node's index is also incomplete for clients in partitions it doesn't hold. Recovery rebuilds the index only from locally held records, which suggests it is, but I haven't confirmed it by running it.