alktunnels/tasks/tunnels/fix-udp-framed-adapter.md

id, name, status, depends_on, scope, risk, impact, level, tags, completed

id	name	status	depends_on	scope	risk	impact	level	tags	completed
tunnels/fix-udp-framed-adapter	U-1 remediation: make UdpHalf the FRAMED adapter (ADR-003 at the local boundary)	completed	tunnels/review-impl	moderate	medium	component	implementation	local udp codec adr-003 review-remediation	2026-09-08

Description

Review 001 finding U-1 (docs/reviews/001-implementation-review.md): the local feature's UdpHalf (src/local/mod.rs) is NOT the framed adapter ADR-003 mandates at the substrate boundary: it is boundary-preserving (one datagram per poll_read/poll_write) but raw. The [len: u16 BE] codec exists only on the session side, so UDP-over-local tunnels:

1. deliver codec-framed bytes to real targets (the target sees [len]payload, not payload; executable-pinned in the review probes);
2. tear the tunnel down on an empty datagram (a raw zero-length datagram is Ok(()) with 0 bytes filled = EOF-shaped to tokio::io::copy, the F-2 violation end-to-end; the session-side codec exists precisely to prevent this);
3. split datagrams > ~8 KiB at the target (tokio::io::copy's DEFAULT_BUF_SIZE bounds each read; the raw adapter sends each chunk as its own datagram);
4. silently drop unframed target-initiated datagrams (the session codec parses a raw datagram's first two bytes as a length header).

The session-side tests pass because the harness's framed_udp_echo_dial frames symmetrically at the test boundary, a correct-shape stand-in for what connect_udp itself must do.

The fix

Apply ADR-003's placement: UdpHalf becomes the framed adapter at the substrate boundary, as seen from the pump (the channel side):

• poll_write is fed CHANNEL bytes (already [len: u16 BE][payload] frames from the session's codec) → de-frame (an incremental DatagramReader held on the write half) and send each decoded payload to the target as one raw datagram. len=0 decodes to a zero-payload datagram, i.e. a real empty datagram, NOT EOF.
• poll_read yields bytes destined for the CHANNEL (the session's codec decodes there) → frame (frame_datagram) each raw target datagram and hand the frame bytes to the pump.

Both halves of the association then speak the same framing on both faces; the target sees payload-only bytes, and an empty datagram crosses the pump as its 2-byte frame (unambiguous with EOF, the F-2 fix end-to-end).

Orientation caution (the review's §U-1 sketch had this inverted and was corrected in re-validation; read the review's §Re-validation before implementing): "frame on write, de-frame on read" describes the TARGET's perspective. UdpHalf's caller is the pump; the channel side is the framed side. Getting the orientation wrong double-frames the channel→target direction and leaves unframed target→channel traffic undecodable; the U-1 defects survive the wrong fix.

The recv scratch (U-3 interplay): the de-frame path needs a FULL 65535-byte recv (tokio's try_recv truncates silently, so a smaller scratch corrupts the framing before any size check runs). Any memory mitigation must keep the full-size read underneath; see U-3 below.

Verification gates (from the review)

1. A recording UDP echo server asserts the datagrams it receives are payload bytes only and an empty datagram round-trips (session recv_datagram yields Some(b""), channel alive).
2. A >8 KiB datagram round-trips as ONE datagram at the target.
3. An unframed target-initiated datagram resolves at the session.
4. The existing local_halves suite stays green, EXCEPT truncation_fails_loud_oq_tn_13, whose assertion surface may legitimately change: once framing composes, a >caller-buffer frame is a partial-drain read (or an equivalent typed failure) rather than the raw adapter's exact InvalidData at that call. The fail-loud posture (OQ-TN-13) must hold; the assertion surface may be updated to match. (Re-validation correction to the review's "stays green unchanged" wording.)
5. cargo test + --features local + wasm checks as usual; 3× repeat-run stable.

Riders (from the same review, same file)

• U-2 (bind posture, sharpened during review follow-up; read the whole bullet before implementing): the crate must NOT wildcard-bind implicitly. Two distinct cases:
  • connect_udp binds ("0.0.0.0", 0): this is a CLIENT socket (bind-ephemeral → connect(target); the kernel delivers only datagrams from the connected peer), so its internet-exposure profile is small, but the v4 hardcoded bind breaks v6-only targets at connect, and wildcard binds are a bad default regardless. Fix: resolve the target first, bind the UNSPECIFIED address of the MATCHING family (0.0.0.0:0 for v4 targets, [::]:0 for v6). Do NOT bind loopback (127.0.0.1:0); that would break every non-loopback target (a query to 8.8.8.8 can't route from a loopback-bound socket). No user-supplied bind address here unless a caller actually needs source-address selection; keep the signature unless that need exists.
  • bind_tcp (the listener half) already REQUIRES the caller to supply the address; that is the correct posture (the assembly layer decides loopback vs a public interface). Keep it; add a doc warning that a wildcard bind (0.0.0.0 / ::) is the caller's explicit choice and exposes the listener per the host's firewall.
  • Note: ::1 is loopback (the v6 127.0.0.1), NOT a wildcard, as the risky v6 address is ::. The rule is "no implicit wildcard binds", not "no v6 loopback".
• U-3: the per-tunnel 65535-byte recv scratch is correctness- load-bearing ON THE DE-FRAME PATH, because tokio's try_recv truncates SILENTLY, so a scratch smaller than the largest in-flight datagram would corrupt the framing before any size check could reject it (a truncated frame decodes as a shorter declared length, desynchronizing the framed stream). The review's "size to the MTU discipline (1400)" suggestion is therefore NOT safe as written (re-validation correction). Mitigate memory instead: lazy allocation (allocate on first recv), a shared/pooled scratch across halves, or justify keeping the eager 65535 allocation. An explicit oversize-rejection design may layer on top, but only with a full-size read underneath.

Acceptance Criteria

• UdpHalf frames on write and de-frames on read (ADR-003's placement; the pump stays raw)
• The 5 verification gates pass; suites green 3× in both configs
• U-2/U-3 riders resolved or explicitly deferred with rationale
• Review 001's U-1/U-2/U-3/N-1 sections get resolution notes

Resolution (2026-09-08)

All acceptance criteria met. Key implementation notes beyond the sketch (pinned in ADR-003's §Amendment):

• Orientation (the caution held): de-frame on write, frame on read; the channel side is the framed side.
• The write half accepts ALL offered bytes and queues completed datagrams for the socket: tokio's copy re-offers a FULL window unsatisfied until drained (its read_done path never tops up), so the "hold the partial tail, re-feed" shape deadlocked gate 2 (the 9000-byte test) and Ready(Ok(0)) on a partial-only feed tripped the copy's WriteZero abrupt-close. The DatagramReader's partial-frame state is bounded by construction (u16 cap, per review 001 N-2); backpressure is an accepted-byte high-water mark (128 KiB), and the socket drain pumps on write/flush/shutdown (copy polls flush after every window drain).
• A >caller-buffer frame stages across reads (length prefix first, remainder on the next read), which is gate 4's predicted assertion-surface change. The fail-loud probe is now the 65507-byte max datagram reassembling through 16 KiB reads (truncation_fails_loud_oq_tn_13, rewritten) plus the direct staging test (oversized_frame_stages_across_reads).
• U-2: connect_udp resolves the target and binds the unspecified address of the MATCHING family; bind_tcp gained the wildcard-exposure doc warning. Pinned by a real ::1 round-trip (connect_udp_binds_the_matching_family_v6_targets_dial).
• U-3: the 65535 scratch allocates lazily on first recv (the full-size read kept; the MTU-shrink idea confirmed unsafe).

Verification: 63 default / 76 local tests green ×3 repeat runs; clippy -D warnings (all-targets + wasm32), fmt, doc, publish dry-run, wasm check all clean.

References

• docs/reviews/001-implementation-review.md §U-1 (executable-pinned probe evidence), §U-2, §U-3, §N-1, and §Re-validation (2026-09-08: findings re-confirmed by fresh probes; the fix orientation corrected; gates 4 and U-3 adjusted as above)
• docs/architecture/decisions/003-codec-and-udp-framing.md (the framed-adapter mandate + F-2), docs/architecture/wire.md §Datagram substrate
• src/local/mod.rs (UdpHalf), src/wire.rs (the codec to compose)