fix(coord): wall-clock deadline for a partial Noise frame (#127)

[iliabaranov]

Closes #127.

Problem

coord_recv() bounded a partial length-prefixed read by a retry count (300). Every retry first blocks for the socket's SO_RCVTIMEO — 2 s in long-poll, 60 s during the MapResponse fetch — so a stalled-but-open peer could hold the coord task for up to 10 min and starve the 120 s control watchdog, which only runs between reads. Abandoning also left errno == EAGAIN, which noise_recv/poll_map_update read as "retry later": the misaligned stream was resumed.

Fix (ml_coord.c, 36+/10−)

• COORD_PARTIAL_FRAME_DEADLINE_MS = 10000: armed by the first consumed byte, not extended by progress (a trickle is a stall). 10 s = the connect-phase SO_RCVTIMEO, ≥ 4 long-poll socket timeouts, 1/12 of the watchdog.
• Expiry → errno = ETIMEDOUT (callers reconnect); orderly EOF mid-frame → ECONNRESET.
• noise_recv's own 300-count payload loop gets the same budget.
• Log lines distinguish a per-read socket timeout from the overall deadline.

Verification

Host test (gcc -std=c11 -Wall -Wextra -Werror -fsanitize=address,undefined) extracts the real loop bodies and drives them with a scripted recv() and fake clock: 88 checks, 0 failures — header stall → ETIMEDOUT at 10 s; slow-but-progressing payload → success; trickle past deadline → fail; EOF ×4; EAGAIN interleave; attribution matrix. Negative control on the unpatched loop: 605 s / 302 recvs / errno=EAGAIN for a 1-byte header stall. Cross-compiles for both roles; running on four bench remotes in the current soak.

Residual: worst-case block is deadline + one SO_RCVTIMEO (70 s in the 60 s fetch phase — left alone deliberately).

[graphify-labs]

Graphify reviewed this change.

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).

---

Graphify review — findings

Replaces coord_recv's 300-retry count with a 10 s wall-clock deadline (COORD_PARTIAL_FRAME_DEADLINE_MS) armed by the first consumed byte, so a stalled-but-open peer can no longer pin the coord task for up to 10 minutes across long SO_RCVTIMEO reads and starve the control watchdog. On deadline expiry mid-frame it fails with errno = ETIMEDOUT (and maps orderly EOF to ECONNRESET) so callers reconnect rather than misreading it as EAGAIN and resuming a misaligned Noise stream. Applies the same wall-clock budget to noise_recv's payload-completion loop, converting a post-header EAGAIN into ETIMEDOUT so a partly-read frame forces a reconnect instead of an unretryable resume.

No blocking issues surfaced. 7 lower-confidence candidates did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 27 functions depend on the 22 functions this change touches.

Health — this change adds coupling hotspots:

• new: noise_send() — 7 callers, 3 callees
• new: do_fetch_peers() — 1 callers, 13 callees
• new: noise_recv() — 4 callers, 3 callees
• new: do_register() — 1 callers, 11 callees
• new: self_heal_rehome() — 1 callers, 11 callees
• new: do_send_endpoint_update() — 1 callers, 7 callees
• new: do_noise_handshake() — 1 callers, 6 callees
• new: do_start_long_poll() — 1 callers, 6 callees
• …and 6 more — each is listed as a finding

Verification — 27 functions in the blast radius were not formally verified this run (proofs are advisory here).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

• verification_scope: 27 function(s) in the blast radius were not formally verified this run

· 14 more finding(s) on lines outside this diff (see the check run).

[claude]

I reviewed this PR and didn't find any bugs. Because it reworks partial-frame retry/timeout handling on the control-plane socket that also feeds the control watchdog, a human look would still be worthwhile.

What was reviewed:

• coord_recv's deadline arm/check ordering (armed after first byte, checked before each subsequent read) and its interaction with per-read SO_RCVTIMEO retries.
• errno handling: n==0 (EOF) now forces ECONNRESET, deadline expiry forces ETIMEDOUT, distinguishing both from a retryable EAGAIN — traced into poll_map_update, which still only treats EAGAIN/EWOULDBLOCK as "no data yet" and now correctly falls through on the new codes.
• noise_recv's outer payload retry loop vs. coord_recv's own internal retry — confirmed the outer loop only re-invokes coord_recv (restarting at buffer offset 0) when zero bytes were consumed in the failed call, so no partial payload bytes are silently discarded/overwritten.

Extended reasoning...

Overview

The change is confined to components/microlink/src/ml_coord.c and reworks two related retry loops in the Tailscale-style control-plane client's coordination socket code: coord_recv (raw partial-frame reads) and noise_recv's payload retry loop. It replaces a fixed retry-count backoff with a wall-clock deadline (COORD_PARTIAL_FRAME_DEADLINE_MS = 10000), and tightens errno semantics so an orderly EOF becomes ECONNRESET, deadline expiry becomes ETIMEDOUT, and only a genuine EAGAIN/EWOULDBLOCK from recv() itself is treated as retryable.

Security risks

No injection, auth, or crypto-boundary changes here — the Noise handshake/encrypt/decrypt calls themselves are untouched; this only changes how many bytes are read off the socket and when the read loop gives up. The main risk class is protocol/stream desynchronization (resuming a misaligned Noise frame stream after a bogus retry), which is exactly what this PR targets. I traced the errno propagation into poll_map_update and confirmed the new ETIMEDOUT/ECONNRESET codes are treated as hard errors (triggering reconnect) rather than misread as "retry later," and confirmed noise_recv's outer retry only restarts coord_recv from scratch when zero bytes were actually consumed by the failed call, so there's no silent data loss/corruption path.

Level of scrutiny

This warrants more than a rubber-stamp: it touches timing-sensitive retry/deadline arithmetic and errno-based control flow that feeds a control-plane watchdog, in a repo whose broader context is a robot's protective-stop/safety system. The logic is subtle enough (armed-once deadlines, per-read timeouts nested inside overall deadlines, interaction between two retry loops in two functions) that a second human pass is valuable even though my own trace did not turn up a defect.

Other factors

The PR description is candid about a known residual: worst-case block can be deadline + one SO_RCVTIMEO (up to ~70s during the 60s MapResponse fetch phase), left in deliberately as a large improvement over the prior ~10 minute worst case. That's a reasonable tradeoff but is a judgment call a human maintainer should sign off on. The PR also references a host-side test harness (88 checks) exercising the extracted loop logic under a fake clock/scripted recv(), though that harness isn't part of this diff to independently verify.

[iliabaranov]

Superseded by #156 (consolidated remote-connectivity PR, same commits cherry-picked linearly; all review findings from this thread are addressed there). Closing.