cluster: a timed-out send leaves a partial frame and desynchronizes the QUIC stream

[fabracht]

A send that times out leaves a partial frame on the wire and permanently desynchronizes the stream, which is then reused for every subsequent send to that peer.

send_to_peer writes a 4-byte big-endian length prefix followed by the payload, wrapping each write in a 5 second timeout (cluster/quic_transport.rs:296-316, SEND_TIMEOUT_MS = 5000 at :18):

tokio::time::timeout(timeout, stream.write_all(&len_prefix)).await ...
tokio::time::timeout(timeout, stream.write_all(&payload)).await ...

quinn::SendStream::write_all is a loop, and every completed iteration has already committed its bytes to the connection's send buffer (quinn 0.11.11, src/send_stream.rs:72-78):

pub async fn write_all(&mut self, mut buf: &[u8]) -> Result<(), WriteError> {
    while !buf.is_empty() {
        let written = self.write(buf).await?;
        buf = &buf[written..];
    }
    Ok(())
}

tokio::time::timeout cancels that future between iterations, so a timed-out send transmits a prefix of the frame and abandons the rest. The stream is not closed and the entry is not removed, so the next send writes a fresh frame directly after the truncated one.

The reader is strictly length-prefixed (quic_transport.rs:524-542): it does read_exact for 4 bytes, treats them as a length, and read_exacts that many bytes. After a truncation it reads payload bytes as a length. Either the bogus length exceeds MAX_MESSAGE_SIZE (10 MiB, :26) and the receiver logs "message too large" and breaks — killing the peer link — or the length is plausible and the connection silently mis-frames from then on, handing parse_message garbage.

Two things make this worse than it first looks:

• The timeout fires exactly under congestion, which is when the cluster is least able to absorb a broken link.
• Callers discard the error (broadcast logs a warning at :350-352 and continues; send_tick_output does let _ = ...), so nothing reacts to the failed send and nothing repairs or closes the stream.

The underlying problem is that write_all is not cancellation-safe for framed output. A timeout can only be applied safely to a whole frame if failure also tears down the stream. Options:

1. Give each peer a writer task fed by a bounded channel. Backpressure then shows up as a failed try_send on an intact stream instead of a cancelled write on a corrupt one, and the task's exit is an unambiguous per-peer death signal (useful for cluster: nodes never dial peers they discover, never retry failed dials, and never drop dead peers #140).
2. Keep the timeout but treat expiry as fatal for that connection: close the stream and drop the entry so the next send re-establishes rather than appending to a truncated frame.
3. Write the prefix and payload as a single buffer, which narrows but does not close the window, since write_all can still be cancelled mid-buffer.

Option 1 is preferred; it also gives #140 the death signal and generation ownership it needs.

Found while modelling the peer map for #140. Not covered by any test: crates/mqdb-cluster/tests/ has no QUIC transport test, and the unit tests in quic_transport.rs cover only serialization.

[fabracht]

Implementation brief

Recording the analysis behind the chosen approach so it does not have to be rediscovered. Line numbers are against 3c94ea5.

Mechanism (verified)

quinn::SendStream::write_all is a loop and each completed iteration has already committed its bytes (send_stream.rs:72-78). send_to_peer wraps it in tokio::time::timeout (quic_transport.rs:308,313), which cancels between iterations, so a timed-out send leaves a partial frame. The reader is strictly length-prefixed read_exact (:524-542), the stream is not closed, the peer entry is not removed, and callers discard the error — so the next send appends a fresh frame after a truncated one and the stream never resynchronizes.

Chosen approach: a per-peer writer task behind a bounded channel

Preferred over the alternatives because it makes backpressure visible as a dropped message on an intact stream, instead of a cancelled write on a corrupted one. It also gives the transport an unambiguous per-peer death signal (the task exits), which #140 needs.

Constraints found while investigating — please do not rediscover these

• Do not wrap a framed write_all in a timeout again. A timeout is only safe if expiry also tears the stream down; otherwise it is precisely this bug.
• send_to_peer holds peers.read() across both 5s timeouts (:299-316). Any peer-map mutation on the failure path is therefore drop-read-then-acquire-write, a window of up to 2 × SEND_TIMEOUT_MS (10s). Moving the write into a task takes the write off the map-read path entirely, which is most of the benefit.
• peer.send_stream.lock().await (:304) has no timeout. The timeouts cover only the writes, so the dominant stall mode — contention on that mutex — produces no error at all. Any design that keys off send failure inherits this blind spot; a writer task owning the stream removes the mutex.
• broadcast is serial (:347-353), so per-peer stalls add up. With two congested peers, one broadcast can exceed the 15s heartbeat_timeout_ms (transport.rs:561) while heartbeats themselves go out via broadcast (node_controller/mod.rs:751-752,779), escalating transient congestion into Raft-visible node death. Non-blocking try_send per peer removes that coupling.
• There is no backoff anywhere in this transport. If anything retries, copy the existing precedent in inflight_store.rs:77-85 (exponential, 1 << attempts.min(10), capped at 300s).
• Do not add peer removal or re-dial here. Nothing removes entries today and connect_to_peer has exactly one caller (startup, no retry). That is cluster: nodes never dial peers they discover, never retry failed dials, and never drop dead peers #140, and its design is contested — keep this change to framing safety.

Testability — read this before writing the test

A reproduction driven through send_to_peer is not currently possible, which is why the attempt in #144 ended up exercising raw quinn rather than MQDB. receiver_task always drains and drops on a full inbox (try_send, :544), so a peer never exerts enough backpressure to force a deterministic 5s send timeout; forcing one means relying on wall-clock throughput, which is fragile.

The writer task fixes this, and that is a reason to prefer it: a bounded channel gives a deterministic full-queue condition. So the fix should land together with an MQDB-level test that fills the channel and asserts the message is dropped while the stream stays framed — i.e. a subsequent send is still received intact. That test will fail against today's code and pass after the fix, which is what #144 could not do.

Acceptance criteria

1. A frame is either written whole or the stream is not reused.
2. Backpressure drops a message; it never truncates one.
3. A slow or dead peer cannot block broadcast for other peers.
4. An MQDB-level test (not a raw-quinn one) covers 1 and 2.
5. No new #[allow]; no peer-map removal (that is cluster: nodes never dial peers they discover, never retry failed dials, and never drop dead peers #140).

[fabracht]

Fixed by #145 (per-peer writer task behind a bounded channel; a frame is written whole or the stream is torn down). Squash-merged as 5ef8767.