diff --git a/docs/reviews/001-implementation-review.md b/docs/reviews/001-implementation-review.md index e469703..4462272 100644 --- a/docs/reviews/001-implementation-review.md +++ b/docs/reviews/001-implementation-review.md @@ -17,7 +17,9 @@ including its regression test. The trivial minors (N-4, N-5, N-7, N-8, N-9, N-10) are fixed with the same commit — the review's "trivial batch" remediation ordering, applied inline. The forward `-L`/TCP/unix paths and the wire surface itself (the one-way doors) -are conformant as-is. +are conformant as-is. **All findings re-validated 2026-09-08 in a +follow-up session (fresh executable probes; U-1's fix sketch corrected +— see §Re-validation).** Findings use prefix `U` (units — remediation-work findings) and `N` (notes/trivia). This is the first review in this crate's series @@ -615,6 +617,96 @@ U-1 remediation docs. in the next phase or file an OQ. (N-2 was in this list and was retracted during the review — see its section.) +# Re-validation (2026-09-08, follow-up session, tree `f278cf7`) + +Every finding re-validated before remediation decomposition. Method: +full re-read of the touched sources (`src/local/mod.rs`, +`src/producer.rs`, `src/consumer.rs`, `src/wire.rs`, `src/lib.rs`, +`tests/harness.rs`, `tests/local_halves.rs`, +`tests/producer_listen.rs`) + fresh executable probes over real +127.0.0.1 UDP sockets (temporary test file, deleted after the run — +independent of the original review's probe file) + the verification +battery re-run. + +- **U-1 [major] — CONFIRMED by fresh probes.** All four behaviors + reproduced independently: (1) `send_datagram(b"query")` → the real + recording target received `00 05 71 75 65 72 79` (the codec frame); + (2) an empty datagram from the target → session `recv_datagram` + resolved `Ok(None)` (EOF collapse — F-2 violated end-to-end); + (3) a 9000-byte datagram arrived at the target as two datagrams + (8192 + 810); (4) an unframed `HELLO` from the target never + resolved at the session (2s timeout — parked mid-frame). The + harness-stand-in explanation also confirmed: `framed_udp_echo_dial` + (tests/harness.rs) frames at the test boundary, and + `local_halves.rs`'s byte-for-byte echo server passes the round-trip + while the target still sees framed bytes. +- **U-2, U-3, N-1 — CONFIRMED by inspection** (bind at + `src/local/mod.rs:116`; 65535 scratch at :164; the FRAMED-adapter + doc claims in the module header, `connect_udp`, and `UdpHalf`). +- **C-1 — CONFIRMED FIXED.** The `Notified`-before-check shape is in + `src/producer.rs` `AcceptQueue::pop` with the contract pinned in the + doc comment; the 64-round multi-thread regression test rides in + `tests/producer_listen.rs` + (`pop_wakeup_survives_push_landing_in_the_registration_window`). + Test totals are +1 over the review's counts in both configs (63 + default / 70 with `local`) — that regression test. +- **Fixed-in-review minors (N-4, N-5, N-7, N-8, N-9, N-10, N-11) — + CONFIRMED in-tree** (payload serialization in `open_reverse_channel`; + the module-doc pointer; root re-exports incl. `TunnelParams`; + README.md present; the harness comment; the `pump_against` framing + note; `cargo doc --no-deps` warning-free; `cargo publish --dry-run + --allow-dirty` completes). +- **N-2 retraction and N-6 non-finding — CONFIRMED** (decoder state + bounded by construction: `acc ≤ declared ≤ 65535`, `buf` drains + into in-flight frames, < 2 bytes idle; no `Timeout` variant is + correct). +- **Battery green**: 63/70 tests, clippy `-D warnings` (all-targets + + wasm32), fmt, doc, publish dry-run. + +### U-1 amendment: the fix sketch in §U-1 is direction-inverted + +The §U-1 "The fix (small)" sketch reads "apply `frame_datagram` in +`poll_write` … de-frame in `poll_read`" — written from the *target's* +perspective. `UdpHalf`'s pump face is the CHANNEL side, which is the +framed side (the session's codec; ADR-003; probe 1 proves the framed +bytes arrive at the adapter). The correct mechanics: + +- `poll_write` is fed CHANNEL bytes (already `[len]payload` from the + session) → it must **de-frame** (`DatagramReader`-style decode) and + send each decoded payload as one raw datagram to the target. +- `poll_read` yields bytes destined for the CHANNEL (the session + decodes there) → it must **frame** (`frame_datagram`) each raw + target datagram and hand the frame bytes to the pump. + +The sketch's version would double-frame the channel→target direction +and strip framing the target needs in the target→channel direction — +probes 1 and 4's defects would survive it. The five verification +gates are unaffected (they assert end-to-end behavior); ADR-003's +consequence paragraph already pins the right orientation ("`len=0` +yields a 2-byte wire frame" — on the channel side, where the pump's +copy runs). + +Two knock-on corrections for the remediation: + +1. **Gate 4's "stays green unchanged" is too strong** for + `truncation_fails_loud_oq_tn_13` (tests/local_halves.rs): the raw + adapter's `InvalidData` shape changes once framing composes — a + >caller-buffer frame becomes a partial-drain read (or an + equivalent typed failure) rather than that exact error at that + exact call. The fail-loud *posture* must hold; the assertion + surface may legitimately need updating. Gate 4's intent (no + regression in the other `local_halves` tests) stands. +2. **U-3's "size the scratch to 1400" suggestion is unsafe as + written**: `UdpSocket::try_recv` truncates SILENTLY at the buffer + size — a scratch smaller than the largest datagram would corrupt + the framing (a truncated frame decodes as a shorter declared + length, desynchronizing the stream) before any size check could + reject it. The 65535 scratch is correctness-load-bearing on the + de-frame path; mitigate memory (lazy allocation, shared scratch, + or a pool) instead of shrinking it. The MTU-scratch idea survives + only as an explicit oversize-rejection design with a full-size + read underneath it. + # References - The spec set under review: `docs/architecture/` (wire.md, diff --git a/tasks/tunnels/decide-take-framed-halves.md b/tasks/tunnels/decide-take-framed-halves.md new file mode 100644 index 0000000..4a49341 --- /dev/null +++ b/tasks/tunnels/decide-take-framed-halves.md @@ -0,0 +1,81 @@ +--- +id: tunnels/decide-take-framed-halves +name: "N-3 decision — take_halves on datagram sessions hands RAW FRAMED bytes; decide the API posture before the first external consumer" +status: pending +depends_on: [tunnels/review-impl] +scope: single +risk: low +impact: component +level: research +tags: [api, udp, codec, adr-005, api-stability, review-remediation] +--- + +## Description + +Review 001 finding N-3 +(`docs/reviews/001-implementation-review.md`): a datagram session +exposes `send_datagram`/`recv_datagram` (codec applied) until +`take_halves` — then the caller owns RAW halves whose wire bytes carry +the codec framing (`[len: u16 BE][payload]`, documented on +`take_halves`). The doc covers it, but it is a footgun asymmetry: the +same session flips from "codec applied" to "codec is yours" depending +on which accessor was called, and nothing in the type system warns +about it. + +This is an API-posture question, not a bug: ADR-005's surface becomes +ABI-stable at the first external consumer, so the decision must land +before then. The options on the table: + +1. **Keep as-is + docs** (today's posture): raw framed halves are + power-user territory; the doc note carries it. +2. **Add a convenience `take_framed_halves`** (the review's sketch): a + `DatagramReader`-wrapped pair that keeps the codec so the halves + API matches the datagram methods' framing posture. Additive to the + raw `take_halves`, which stays. +3. **Re-shape `take_halves` for datagram sessions** (breaking, before + the window closes): e.g. a datagram session's `take_halves` returns + framed-preserving halves by default, raw access via a distinct + explicit method — removes the footgun entirely, at the cost of + diverging `take_halves` semantics per substrate. + +The decision should weigh: the pump_against shape (a UDP session +pumped against an accepted local half needs the FRAMED adapter on the +accepted side — the same framing-responsibility seam N-11's doc note +covers), what an actual consumer (alkcall's assembly layer, a +DNS-proxy shape) reaches for first, and whether a `DatagramReader` + +write-frame wrapper pair is small enough to just ship (option 2 is a +thin compose of existing public pieces — `src/wire.rs` already +exposes both codec halves of the job). + +## Work + +- Re-read review 001 §N-3 + ADR-005 (the API-stability window); + confirm no external consumer has pinned the `take_halves` import + style yet (if one has, the window is closed — record that and + choose the additive path (2) or docs-only (1)). +- Write the decision as an ADR amendment (ADR-005) or a new ADR if + the posture adds a public method; file the OQ only if the decision + defers again. + +## Verification + +- `taskgraph` graph stays acyclic; if an ADR lands, `docs/ + architecture/` and the crate docs stay in sync (`cargo doc + --no-deps` clean); if a method lands, the full battery + wasm + checks pass. + +## Acceptance Criteria + +- [ ] A written decision (ADR amendment or task-note resolution) with + the chosen option and rationale, before the first external + consumer +- [ ] If the decision adds surface: tests + battery green; review 001 + §N-3 gets a resolution note + +## References + +- `docs/reviews/001-implementation-review.md` §N-3 (the finding) and + §N-11 (the adjacent pump_against framing seam) +- `docs/architecture/decisions/005-consumer-session-owns-teardown.md` + (the API-stability window), `src/consumer.rs` (`take_halves`, + `TakenHalves`), `src/wire.rs` (`frame_datagram`, `DatagramReader`) \ No newline at end of file diff --git a/tasks/tunnels/fix-udp-framed-adapter.md b/tasks/tunnels/fix-udp-framed-adapter.md index 9d1dda2..7b0a8fd 100644 --- a/tasks/tunnels/fix-udp-framed-adapter.md +++ b/tasks/tunnels/fix-udp-framed-adapter.md @@ -38,12 +38,35 @@ correct-shape stand-in for what `connect_udp` itself must do. ### The fix -Apply ADR-003's placement: `UdpHalf::poll_write` frames the caller's -buffer (`frame_datagram`) and sends one datagram per framed frame; -`poll_read` de-frames (feed `DatagramReader`, emit the next payload). -Both halves of the association then speak the same framing; `len=0` -becomes a 2-byte frame (unambiguous with EOF — the F-2 fix -end-to-end). +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 — 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 — 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) @@ -52,8 +75,14 @@ end-to-end). (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 unchanged (its - session-level round-trips are framing-symmetric today). +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. @@ -82,8 +111,18 @@ end-to-end). - Note: `::1` is loopback (the v6 `127.0.0.1`), NOT a wildcard — the risky v6 address is `::`. The rule is "no implicit wildcard binds", not "no v6 loopback". -- U-3: size the recv scratch to the codec's MTU discipline instead of - a per-tunnel 65535-byte allocation (or justify the bound). +- U-3: the per-tunnel 65535-byte recv scratch is correctness- + load-bearing ON THE DE-FRAME PATH — 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 @@ -96,7 +135,9 @@ end-to-end). ## References - `docs/reviews/001-implementation-review.md` §U-1 (executable-pinned - probe evidence), §U-2, §U-3, §N-1 + 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