fix(miner-server): stop racing a cancel-unsafe read against job sends

[Bortlesboat]

Summary

Stop polling the cancel-unsafe read_message as a tokio::select! branch in connection_handler, so a job broadcast can no longer discard a half-read frame and desynchronise a miner's stream.

This change:

• extracts the two directions into read_loop and write_loop
• runs them as independent long-lived futures under a single outer select!, so read_message is always awaited to completion
• makes connection_handler generic over AsyncRead/AsyncWrite so the loop can be driven by an in-memory duplex in tests
• adds a regression test that delivers a JobResult in two halves with job broadcasts in between

Root cause

node/src/miner_server.rs used read_message as a select! branch against job_rx.recv():

tokio::select! {
    // Prioritize reading to detect disconnection faster
    biased;

    msg_result = read_message(&mut recv) => {

read_message is two read_exact calls:

reader.read_exact(&mut len_buf).await?;
...
reader.read_exact(&mut buf).await?;

AsyncReadExt::read_exact is explicitly documented as cancel-unsafe: if it is used as a select! branch and another branch completes first, the buffer may have been partially filled and those bytes are lost. quinn::RecvStream implements AsyncRead, so the loss is from the stream itself and the bytes cannot be put back.

The failure sequence:

1. A miner is sending a JobResult; the length prefix (or part of the JSON body) has been consumed by read_exact, which then returns Pending because the rest is still in flight.
2. In the same select!, the node broadcasts a new job — which happens on every block-template rebuild.
3. job_rx.recv() completes, the read_message future is dropped, and the consumed bytes are gone.
4. The next iteration builds a fresh read_message that starts mid-frame. It either reads body bytes as a length prefix (Message size N exceeds maximum 1024, InvalidData) or hands serde_json garbage.
5. connection_handler returns Err, serve_authenticated_miner calls remove_miner, and the miner is disconnected.

The seal that was in flight is lost and logged as a read error rather than as a lost result, so from the operator's side it is indistinguishable from a miner that simply did not find anything. Under transaction load this becomes a repeated silent disconnect/reconnect cycle, most likely to fire exactly at new-block time.

biased; does not help — it only controls poll order. The read is polled first, returns Pending after consuming some bytes, and is still dropped when the job branch becomes ready.

Proposed solution

Split send and receive so the read is never in a select! with anything else, which is the second option suggested in #674.

read_loop and write_loop are now separate futures, raced by one outer select! instead of a select! re-entered on every iteration. A job broadcast is handled inside write_loop without completing it, so an in-progress read is only ever dropped when the connection itself is being torn down — at which point the partial frame is irrelevant.

This also removes the related hazard noted in the issue: previously a miner that stopped reading could park the handler inside write_message and stop servicing its own reads. The two directions now make progress independently.

Spawned tasks were deliberately avoided — two futures under one select! need no Send + 'static bounds, no JoinHandle plumbing and no abort-on-drop handling, and the first direction to finish still returns its result straight out of connection_handler exactly as before.

biased is kept so the read is polled first and a disconnect is still noticed promptly.

Impact

• A JobResult that arrives split across a job broadcast is now read intact instead of desynchronising the stream and costing the miner its seal.
• Fewer spurious miner disconnect/reconnect cycles under transaction load.
• A miner that stops reading can no longer stall its own result submission.
• No wire-protocol, API or configuration change. serve_authenticated_miner is unchanged and still calls remove_miner on return; the generic parameters are inferred at the existing call site.

Verification

• SKIP_WASM_BUILD=1 cargo test --locked -p quantus-node --bins: 62 passed
• SKIP_WASM_BUILD=1 cargo clippy --locked -p quantus-node --bins -- -D warnings: clean
• scripts/fmt.sh --all -- --check: clean

The new test job_broadcast_does_not_corrupt_a_partially_read_result drives a real connection_handler over tokio::io::duplex: it writes the first half of a framed JobResult, lets the handler park mid-frame, sends eight job broadcasts (draining them so only the read's cancel-safety is under test), then writes the remainder and asserts the result is forwarded intact and tagged with the miner id.

Confirmed to fail against the previous loop — the handler dies on the desynchronised frame and the result channel closes without ever yielding the seal:

panicked at node/src/miner_server.rs: result channel must stay open
test job_broadcast_does_not_corrupt_a_partially_read_result ... FAILED

Fixes #674

[n13]

Read through the diff and the test. Summary of what this fixes, in plain terms, for anyone landing here later:

The bug. The node reads each miner message with read_message, which reads a 4-byte length first and then the body. In the current code that read runs as a select! branch racing against "send this miner a new job". When the other branch wins, the in-progress read future is dropped, and the bytes it already consumed are gone. So if the node has read the 4-byte length of an incoming JobResult and the body hasn't arrived yet, and a job broadcast fires in that gap, the next read starts in the middle of the frame, interprets body bytes as a length, fails with Message size N exceeds maximum, and the node disconnects the miner. The seal it was sending is lost.

How likely. Rare. It needs the JobResult to arrive in two pieces (length prefix in one packet, body in a later one, e.g. after a lost/retransmitted packet, or when the miner's two write_alls get packetized separately) and a job broadcast to land in that window. On a LAN this is essentially never. Over the internet with some packet loss it can happen, and the timing is the worst possible: it fires exactly when a miner has found a block, and the failure eats that block and forces a reconnect.

The fix. Reading and writing become two independent loops under one outer select!, so a read is only ever dropped when the connection itself is going away. Broadcasts are handled inside the write loop without completing it. A side benefit: a miner that stops reading can no longer park the handler inside write_message and stall its own result submission.

Verdict. Diff is correct, contained to one file, no wire-protocol change, and the duplex test reproduces the corruption against the old loop. Not required for the latency work in #700 (that one doesn't depend on this), but it's cheap insurance against a failure mode that costs a found block. Approving.

[n13]

Bot approved automatically, didn't mean to

[n13]

Reviewer model: GPT 5.6 Sol

APPROVE — no blocking findings at 3c6eee942027092b50fadddc4644a16149a887a9.

The refactor fixes the cancellation bug without changing the wire protocol: read_loop keeps each read_message alive to completion while write_loop handles job broadcasts independently, and either direction still ends the connection when it returns. The regression test covers the important interleaving by pausing a framed JobResult mid-body, delivering and draining broadcasts, then verifying the intact result and server-assigned miner ID.

Validation:

• git diff --check: passed
• scripts/fmt.sh --all -- --check: passed
• SKIP_WASM_BUILD=1 cargo test --locked -p quantus-node --bins: 62 passed
• Split-frame regression test: 100/100 repeated runs passed
• SKIP_WASM_BUILD=1 cargo clippy --locked -p quantus-node --bins -- -D warnings: passed

No blocking findings found. GitHub reported no checks for this head at review time.