review 001: re-validation + remediation decomposition
- All findings re-validated (fresh executable probes for U-1 over real sockets — all four behaviors reproduced; fixed-in-review items confirmed in-tree; battery green 63/70). - U-1 amendment: the review's fix sketch was direction-inverted (frame-on-write describes the TARGET face; UdpHalf's caller is the pump, so poll_write DE-frames channel bytes and poll_read FRAMES target datagrams). Gate 4 and U-3 corrected: the truncation test's assertion surface may change; the 65535 recv scratch is correctness-load-bearing (tokio try_recv truncates silently). - Task fix-udp-framed-adapter updated to the corrected mechanics. - New task decide-take-framed-halves (N-3): the datagram take_halves API posture, to decide before the first external consumer. Verification: cargo test 63/70 both configs, clippy -D warnings (all-targets + wasm32), fmt, cargo doc --no-deps (0 warnings), publish dry-run, taskgraph validate (15 tasks, acyclic).
This commit is contained in:
1 parent
f278cf7f69
commit
aa7b5b7169
3 files changed
+226
-12
No files matched your search
@@ -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,
|
||||
|
||||
@@ -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`)
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user