From a0f939c6df822646a188735a3ae114100e774d76 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Tue, 8 Sep 2026 10:58:11 +0000 Subject: [PATCH] =?UTF-8?q?review:=20phase-gate=20review=20001=20=E2=80=94?= =?UTF-8?q?=20spec=20conformance=20of=20the=20v1=20implementation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings + review doc: docs/reviews/001-implementation-review.md (the alkcall house pattern). Closes tunnels/review-core-crates and tunnels/review-impl. Findings: - U-1 [major, NOT fixed] — the local UDP adapter is not the framed adapter ADR-003 mandates: codec-framed bytes reach real targets, empty datagrams are EOF-shaped at the adapter (tunnel teardown), >8 KiB datagrams split, unframed target-initiated datagrams are dropped. Executable-pinned by real-socket probes (temporary test, deleted after the run). Remediation task: tunnels/fix-udp-framed-adapter. - C-1 [major, fixed] — AcceptQueue::pop lost wakeup in the check→register window (open op hangs to the establishment bound); fixed via the tokio notify_waiters contract (Notified created before the check each iteration) + multi-thread regression test. - N-4/N-5/N-7/N-8/N-9/N-10/N-11 [minor, fixed] — serialize TunnelParams in open_reverse_channel; stale doc ref; broken doc link; root re-exports (TunnelParams/Substrate/tunnel_open_spec/OP_TUNNEL_OPEN); README (publish dry-run was failing on the missing readme); harness comment; pump_against UDP framing doc note. - N-2 retracted by executable falsification (codec buffering is bounded by construction); N-6 non-finding with rationale. Verified: 63 tests default / 70 with --features local, 3x repeat-run clean both configs; clippy -D warnings (all-targets + wasm32); fmt; doc warning-free; wasm32 check; cargo publish --dry-run passes. --- README.md | 60 +++ docs/reviews/001-implementation-review.md | 603 ++++++++++++++++++++++ src/consumer.rs | 19 +- src/lib.rs | 2 +- src/producer.rs | 16 +- tasks/tunnels/fix-udp-framed-adapter.md | 82 +++ tasks/tunnels/review-core-crates.md | 61 ++- tasks/tunnels/review-impl.md | 105 +++- tests/harness.rs | 5 + tests/producer_listen.rs | 43 ++ 10 files changed, 975 insertions(+), 21 deletions(-) create mode 100644 README.md create mode 100644 docs/reviews/001-implementation-review.md create mode 100644 tasks/tunnels/fix-udp-framed-adapter.md diff --git a/README.md b/README.md new file mode 100644 index 0000000..c5c894a --- /dev/null +++ b/README.md @@ -0,0 +1,60 @@ +# alktunnels + +Arbitrary bidirectional tunnels over [alkcall](https://crates.io/crates/alkcall) +channels: TCP, UDP, unix sockets, and other stream or datagram +substrates — in the `ssh -L` / `ssh -D` / `ssh -R` sense, without SSH. + +A producer/consumer protocol crate riding alkcall channels the same +way [alktty](https://crates.io/crates/alktty) does (`alk/tty` is the +sibling precedent): the producer half registers the `alk/tunnel` open +op and pumps bytes between the channel and the substrate; the consumer +half is the typed session (`TunnelSession`) that opens tunnel channels +and owns teardown. + +## A tunnel is a resource, not an address + +Open-op params identify a produced resource + substrate: + +```json +{ "resource": "postgres-primary", "substrate": "tcp" } +``` + +The producer owns the backing — the consumer never learns an address. +Rich addressing (SOCKS5 ATYP, per-datagram remotes) enters only +through the `-D`/dynamic composition path, inside the tunnel payload, +never in the wire (ADR-001). + +- Wire format: `docs/architecture/wire.md` (+ the BAST document at + `docs/architecture/bast.md`) +- Open-op params: ADR-001; codec: ADR-003; ALPN: ADR-002 +- Producer shapes: ADR-004; consumer session: ADR-005; ACL: ADR-006 +- The full spec set: `docs/architecture/` (overview, wire, producer, + consumer, six ADRs, OQ tracker) + +## Status + +v0.1.0 — the v1 protocol implementation, Phase-2 complete (spec: +`docs/architecture/`; reviews: `docs/reviews/`). The wire surface +(params, ALPN, codec) is one-way-door stable from this point; the +API surface becomes ABI-stable at the first external consumer. + +## Features + +| Feature | Contents | wasm | +|---------|----------|------| +| *(default)* | params, wire codec, open-op spec, establisher shapes, `TunnelSession` — protocol only | yes | +| `local` | TCP/UDP/unix dial + listen helpers (real sockets) | no | + +## Verification + +```bash +cargo test # default crate (wasm-clean) +cargo test --features local # + real-socket suites +cargo clippy --all-targets -- -D warnings +cargo fmt --check +cargo check --target wasm32-unknown-unknown # the wasm-clean guard +``` + +## License + +MIT OR Apache-2.0 \ No newline at end of file diff --git a/docs/reviews/001-implementation-review.md b/docs/reviews/001-implementation-review.md new file mode 100644 index 0000000..45bd7f2 --- /dev/null +++ b/docs/reviews/001-implementation-review.md @@ -0,0 +1,603 @@ +# Review 001 — alktunnels v1 Implementation (spec conformance, pre-publish gate) + +## Status + +**Implemented-conformant with findings (U-1 [major] implementation gap; +C-1 [major] fixed in-review; minors logged, N-2 retracted).** Findings +filed from the `tunnels/review-impl` phase-gate review (2026-09-08), +which subsumes the earlier `tunnels/review-core-crates` mid-phase pass +(also filed from this same review — see the task Notes; no separate +doc was warranted at the mid-phase point, and the two checklists are +consolidated here per the review-impl deliverable). + +**U-1 is load-bearing for the UDP wire promise and is NOT yet fixed** — +see the remediation section (it owns the remaining remediation task). +**C-1 (`AcceptQueue` lost wakeup) is fixed with this review's commit**, +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. + +Findings use prefix `U` (units — remediation-work findings) and +`N` (notes/trivia). This is the first review in this crate's series +(numbering follows the alkcall house pattern — each review numbers +independently). + +## Scope + +The full v1 implementation at tree `2efac9b` reviewed against the +architecture spec set (`docs/architecture/`: wire.md, producer.md, +consumer.md, overview.md, ADR-001..006, bast.md, open-questions.md) +plus the AGENTS.md conventions, cross-checked against the alkcall +0.7.0 API surface the crate consumes +(`/workspace/@alkdev/alkcall`, verified in source: `operations.rs` +wrapper + establisher types, `client.rs` `ChannelOpenError`, +`manager.rs` adopt/teardown, `pump.rs` `pump_bidi`). + +``` +Verified against: alktunnels 2efac9b (post end-to-end-suite); alkcall 0.7.0 +Reading list: src/{lib,params,wire,producer,consumer,error}.rs, + src/local/mod.rs, tests/{harness,producer_open_op,producer_listen, + consumer_session,end_to_end,local_halves}.rs, + docs/architecture/*.md (incl. decisions/001..006, bast.md), + tasks/tunnels/*.md (the per-task notes) +Executed: cargo test (62 default / 69 --features local), 3x repeat-run + clean both configs; clippy -D warnings (all-targets + wasm32); fmt; + cargo doc --no-deps; cargo check/clippy --target wasm32-unknown-unknown; + cargo publish --dry-run --allow-dirty (fails — see N-8) +Probes (temporary test file, deleted after the review run): real-socket + UDP behavior of the `local` adapter — see U-1's evidence +``` + +## Severity legend + +Same scale as the alkcall reviews (004–007): + +- **[critical]** — a decided spec invariant is violated in a way that + makes a promised capability unreachable end-to-end; or corrupts data. +- **[major]** — a core protocol path cannot serve a decided behavior; + works only via shapes the spec does not describe; or a wire/ABI + promise is not actually delivered by the code. +- **[minor]** — drift, doc/spec inconsistency, or a missing + convenience with no correctness impact. + +--- + +# Checklist verdicts (the review-impl five sections) + +1. **Wire conformance — PASS.** Params shape exact + (`TunnelParams {resource, substrate}`, `deny_unknown_fields`, + schema enum `["tcp","udp","unix"]`, both required — + `src/params.rs:32-53, 59-92`, tests `params_round_trip`, + `unknown_fields_rejected`, `unknown_substrate_rejected`, and the + wire-level `schema_rejects_unknown_substrate_loudly` → + `INVALID_INPUT`). Op id `channels/tunnel/sub`, ALPN marker + `alk/tunnel`, scope `tunnel:open` all pinned by constants and + asserted in `open_spec_matches_wire_md`. Codec: `[len: u16 BE]`, + `len=0` legal, `Oversize` at frame time, no sentinel in the codec — + `src/wire.rs` conforms (and the codec-level split/batch/partial + matrix is well tested). Typed errors: all five establishment + reasons flow (three proven by test: `unknown_resource`, + `dial_failed`, `resource_shortage`; `handler_error` maps through + the same `From` arm; `timeout` proven by the hanging-establisher + test), plus `FORBIDDEN` and `channel:too_many_channels` surfaces + verified through the typed client error. **The UDP wire framing + itself is correct — the failure is at the `local` substrate + boundary (U-1), not in the codec.** +2. **Producer conformance — PASS.** Establisher = the awaited bounded + phase (`register_openable_with_establisher`, `timeout: None` = the + 10s default; per-registration override proven by the Timeout-mode + harness registration). Plan flow (R-01): `Establishment::new(plan)` + → wrapper → handler's `plan` parameter — no side-channel handoff + anywhere (grep-verified: the only `Mutex` is the + `ResourceRegistry`'s own lookup table, a resource registry, not a + plan handoff; no poll-loop take). Pump handler: `pump_bidi` awaited + inline inside the spawned task (`src/producer.rs:321-341`), the + returned `JoinHandle` tracks the data plane (R-02 pinned by the + `pump_handle_tracks_the_data_plane_r02` end-to-end test), no + substrate types (`grep tokio::net src/` outside `src/local/` → + empty). Registration post-hoc supported (W2, `late_registration_is_visible`). + Listen variant: same op, no new wire surface, typed-error table + proven (`resource_shortage` / `dial_failed` / `unknown_resource`). +3. **Consumer conformance — PASS.** The full `TunnelSession` surface + per consumer.md: `open` / `adopt` (with the substrate parameter — + documented task-note divergence from the task sketch) / + `stream_halves` / `take_halves` / `send_datagram` / `recv_datagram` + / `pump_against` / `close` / `join` / `Drop`. Teardown matrix + sound: close-with-pump, join copy counts, Drop-with-pump (abort + + reap), failed-adopt no-leak, pump-less join `(0, 0, reaped)` — all + pinned in tests with `channel_ids()` leak asserts on both sides. + No `Clone` (compile_fail doctest + `type_assertions` parity stub). +4. **Conventions sweep — PASS with notes.** No `unwrap`/`expect`/ + `panic!` outside `#[cfg(test)]` blocks (grep-verified; the only + non-test hits are inside `mod tests`). thiserror everywhere + errors surface. Poisoned locks: all session/queue locks are + `tokio::sync::Mutex` (unpoisoned by design) — the + `unwrap_or_else(into_inner)` rule has no application point; N/A. + Substrate types confined to `src/local/` (grep-verified) + + feature-gated; wasm-clean default crate re-verified + (`cargo check/clippy --target wasm32-unknown-unknown` clean). + `pump_bidi` consumed, never hand-rolled (grep for hand-rolled + two-pump loops: none — the only `tokio::io::copy` calls live in + tests). Inline `//` comments exist in a handful of places + (`consumer.rs:275`, `local/mod.rs:183-189,206-207,230-231,253`, + section banners) — all of them are exactly the + safety/correctness-constraint class AGENTS.md convention 1 + carves out (the F-2/OQ-TN-13 invariants, the poll-readiness + waker subtleties); verdict: compliant, no action. +5. **Docs ↔ implementation sync — PASS with drift (N-1, N-7).** + lib.rs re-exports = the public API (convention 16); the module map + in overview.md matches the files; ADR statuses all Accepted + (confirmed by the implementation — no Draft→Accepted flips + needed). Drift items are minors: N-1 (the `UdpHalf` framing claim + vs reality — the U-1 doc face), N-7 (lib doc link warning), + N-9 (Cargo.toml `readme` points at a nonexistent README.md, which + also breaks `cargo publish --dry-run`). + +--- + +# Part A — Findings + +## U-1 [major] — The `local` UDP adapter is not the FRAMED adapter: ADR-003's substrate-boundary codec is missing from `UdpHalf`, so UDP tunnels deliver codec-framed bytes to real targets and drop/mangle real-target traffic + +**Verified:** YES, by code trace AND executable probes against real +127.0.0.1 sockets (temporary test file, run then deleted; probe +results quoted below). + +ADR-003 (Accepted, F-2 mandate) says the UDP framing is "applied at +the substrate/pump boundary": "the **establisher** wraps the dialed +UDP socket in the framed adapter BEFORE returning the plan (the plan +payload is the adapter, not a raw socket) — the pump handler stays +substrate-agnostic". consumer.md and wire.md repeat it ("the codec +lives at the substrate/pump boundary"; producer.md §Pump Handler +step 4: "the plan payload is the FRAMED adapter"). + +`src/local/mod.rs`'s `UdpHalf` doc comment claims exactly this shape: +"`connect_udp`: a connected `UdpSocket` wrapped in the FRAMED adapter +(`UdpHalf`, ADR-003 — the codec composes at this boundary; the pump +never sees UDP specifics)" and "The FRAMED adapter — the codec wraps +at this boundary". But `UdpHalf`'s implementation contains **no +framing at all**: `poll_write` sends the caller's buffer as one raw +datagram (`try_send(buf)`), `poll_read` yields one raw datagram +(`try_recv` into scratch). It is the *boundary-preserving* adapter +(one datagram per read/write — that part works), but the +`[len: u16 BE]` codec never composes at it. + +Meanwhile the *session side* (`TunnelSession` with `Substrate::Udp`) +frames and unframes per ADR-003 (`send_datagram`/`recv_datagram` over +`DatagramReader`). So the codec exists on exactly one end of the +tunnel: + +``` +consumer session (framed codec) ⇄ channel stream ⇄ pump_bidi ⇄ UdpHalf (RAW) +``` + +Probes over the real `local` path (all results executable-pinned in +the deleted probe run): + +1. **The target receives codec-framed bytes.** `send_datagram(b"query")` + → the real UDP echo server received the datagram + `00 05 71 75 65 72 79` — i.e. `[len=5]"query"`, the *codec frame*, + not the payload. Every real UDP server behind a v1 tunnel sees + tunnel-internal framing on every datagram. The F-2 invariant ("an + empty datagram is two bytes — unambiguous with EOF") is only + coherent if the framing is confined to the tunnel; leaking it to + the target breaks the abstraction the ADR mandates. +2. **Empty datagrams are EOF-shaped at the adapter.** Reading an + empty datagram from a raw `UdpHalf` returns `Ok(())` with zero + bytes filled — which `tokio::io::copy` (i.e. `pump_bidi`) treats + as end-of-stream: the p2c pump finishes and shuts the channel down. + Probe: `a.send(&[])` to the dialed socket → the session's + `recv_datagram` resolved `Ok(None)` (EOF) and the producer-side + channel stayed alive — the tunnel is torn down (EOF) for what the + codec explicitly defines as a *legal datagram*. This is F-2 + violated end-to-end: the length prefix exists precisely so "empty + datagram" and "EOF" are different wire shapes, but the raw adapter + re-collapses them. The session-side round-trip tests pass only + because the harness's `framed_udp_echo_dial` does the framing in + the *test* (the codec lives at the test's boundary there — a + correct-shape stand-in for what `connect_udp` should have done). +3. **Datagrams > ~8 KiB split at the target.** `tokio::io::copy` (the + pump's engine) reads with an 8 KiB buffer (tokio + `DEFAULT_BUF_SIZE`); `poll_read` on a raw socket adapter yields at + most one datagram but `copy` hands the pump at most 8 KiB chunks, + and the raw `poll_write` sends whatever it gets as one datagram. + Probe: a 9000-byte datagram sent through the tunnel arrived at the + target as **two** UDP datagrams (8192 + 810 bytes, the second + starting mid-payload at `0x9e`). On a real (non-loopback) network + the split also collides with MTU. ADR-003's chunk-size independence + property ("datagram boundaries survive chunk splitting, batching" + — the codec's whole point) is broken at this boundary. +4. **Target-initiated (unframed) traffic is silently swallowed.** The + session codec waits for `[len]` prefixes; a real server's raw + datagram (e.g. `b"HELLO"`) parses `0x48 0x45` as a declared length + of 17253 and parks mid-frame — probe: `recv_datagram` never + resolved (2s timeout). Asymmetric framing isn't just an + interoperability wart — it silently drops peer traffic. + +The framing probes also confirmed the parts that are correct: +coalescing is safe (two back-to-back `send_datagram`s landed as two +separate UDP datagrams — `poll_write` sends one buffer per call and +the pump's copy preserves the chunk), the scratch-recv truncation +check fires `InvalidData` exactly as documented (OQ-TN-13), and the +1400-byte round-trip test passes (too small to split). + +### Why [major], not [critical] + +The wire format (the one-way door) is correct and unaffected — the +codec is right, and the session side is right. What's broken is the +v1 `local` UDP adapter, i.e. a feature-gated convenience module, and +the damage is local to UDP-over-`local`: real UDP targets see +corrupted/misframed traffic. It is exactly the "a core protocol path +cannot serve a decided behavior — works only via shapes the spec does +not describe" class: the spec-described shape (framed adapter at the +establisher) does not exist, and the passing tests work only via the +test-harness stand-in. Not [critical] because: the default (wasm) +crate is unaffected, the wire surface is unaffected, and stream +substrates (the SSH `-L` core case) are fully correct. + +### The fix (small) + +Make `UdpHalf` the framed adapter ADR-003 already specifies: apply +`frame_datagram` in `poll_write` (frame the caller's buffer, send one +datagram per framed frame) and de-frame in `poll_read` (feed +`DatagramReader`, emit the next payload). Then: + +- `len=0` arrives as the 2-byte frame — the `AsyncRead` zero-fill EOF + ambiguity disappears (the F-2 fix works end-to-end); +- datagrams reassemble to their true size before crossing the pump + (no 8 KiB split; the MTU discipline returns); +- real-target framing matches the session's (both ends codec-clean). + +The `local_halves.rs` tests will keep passing (their assertions are +session-level round-trips) — but note they passed *before* the fix +for the wrong reason (both harness halves framed symmetrically), so +the review-remediation test should assert the target-side bytes are +payload-only (a recording echo server, like the probe) and that an +empty datagram round-trips over the **real** socket path, not just +the codec path. + +**Remediation task:** `tasks/tunnels/fix-udp-framed-adapter.md` +(created with this review). + +## U-2 [minor] — `connect_udp` binds `0.0.0.0` unconditionally — no v6 or dual-stack posture + +`src/local/mod.rs:116`: `UdpSocket::bind(("0.0.0.0", 0))`. A UDP +target that is only reachable over IPv6 (`::1`, v6-only host) fails +at the connect step with no fallback; a v4-mapped dial works but the +bind posture silently excludes v6. TCP/Unix dial have the same +blind spot only insofar as `TcpStream::connect` resolves v6 fine +(they do). Minor: a real deployment will notice immediately, the fix +is `bind` on the resolved family of the target (or dual-stack), and +no wire/API surface changes. Fold into the U-1 remediation task or a +follow-up batch. + +## U-3 [minor] — `UdpHalf` recv scratch allocates 65535 bytes per read half + +`src/local/mod.rs:164`: `recv_scratch: vec![0u8; 65535]` — a 64 KiB +allocation per dialed UDP tunnel (×2 — both split halves carry the +field, though the write half's is empty). Fine for a handful of +tunnels; worth a note when the assembly layer fans out (a DNS-proxy +shape opens one channel per query). Consider sizing the scratch to +the codec's MTU discipline (1400) with an oversized-datagram +`InvalidData` rejection at the *scratch* level — that also tightens +the fail-loud check (a >MTU datagram currently *is* received then +error-matched against the caller buffer). Fold into the U-1 task. + +## C-1 [major] — `AcceptQueue::pop` can miss the final `push` (lost wakeup in the check→register window) — FIXED in-review + +**Status: FIXED during the review** (commit with this doc). + +`src/producer.rs:292-305` (`pop`): the loop drops the state lock, +then awaits `notify.notified()`. A `push` that lands in that window +(a) enqueues under the lock, (b) calls `notify_waiters()` — which +only wakes *already-registered* waiters — and (c) the pop then +registers its `notified()` future *after* the notify fired: it sleeps +with data in the queue until the next push/close. For a listen +producer this is an open op hanging until the wrapper's 10s +establishment bound maps it to `timeout` — a wrong reason code for a +queue that had a handle ready. The `accept_wait_resolves_when_push_ +arrives_late` test (200ms sleep before push) rides the race window +blind: a fast push would have exposed it. + +**The fix (applied):** the `Notified` future is created **before the +state check on every iteration** — tokio's documented +`notify_waiters` pattern. The future records the `notify_waiters` +call counter at creation and compares it at its first poll; a `push` +that fired `notify_waiters` in the thread-preemption window between +the check and the first poll resolves the future immediately +(captured), and a later `push` finds the waiter registered and wakes +it. The naive shape (future created after the check — the original +`lock; check; await notified()` sequence) loses that push. Verified +against tokio 1.53's `Notified`/`poll_notified` source (the +`notify_waiters_calls` counter comparison at `State::Init`). A +regression test +(`pop_wakeup_survives_push_landing_in_the_registration_window`, +multi-thread flavor, 64-round hammer) rides with the fix — it cannot +force the window deterministically (no test can; the window is a +scheduling artifact), so the fix's correctness rests on the tokio +contract, which the doc comment pins. The FIFO always-before-take +property is unaffected (the ordering proof holds under the lock); +only the wakeup completeness changes. + +### Why [major], not [minor] + +The queue is public API (`AcceptQueue` is exported and documented as +the listen-shape contract), and the failure mode is a *hang → +timeout → wrong typed reason* on the security-relevant open path. +It's rare (microseconds of window) but self-inflicted under normal +operation (push racing pop is the queue's entire purpose). + +## N-1 [minor] — `UdpHalf` docs claim the codec composes at the boundary; it doesn't (the doc face of U-1) + +`src/local/mod.rs` module doc: "wrap it in the FRAMED adapter +(`UdpHalf`, ADR-003 — the codec composes at this boundary; the pump +never sees UDP specifics)". Also `connect_udp`'s doc and the module +header ("UDP — `connect_udp`: ... wrapped in the FRAMED adapter"). +Either the code gains the framing (U-1's fix — then the docs become +true) or the docs must stop claiming it; both is the resolution. +Filed separately from U-1 because even if U-1's fix is deferred, the +docs currently describe behavior that does not exist — a conformance +review must not leave that standing. + +## N-2 — NON-FINDING (retracted during the review): `DatagramReader` buffering is bounded by construction + +Filed as a suspect ("a lying `len` header buffers without bound"), +then falsified by executable probe before filing (the probe test +tripped on it): the u16 length field caps the decoder's own state by +construction. `acc` never exceeds `declared ≤ 65535`; `buf` always +drains fully into an in-flight frame (`take = min(need, buf.len())`) +and holds < 2 bytes when no frame is in flight. Total buffered state +≤ 65535 + 1 bytes, by construction — the codec's u16 cap IS the +buffering bound, and no `BufferExceeded`-style error is needed. +Recorded here so the next reader doesn't re-derive the suspicion (or +"fix" it); the attempted bound+error patch was reverted — the +maximal-frame round-trip at 65535 proves the existing decoder is +already at the correct bound. + +## N-3 [minor] — `TunnelSession::stream_halves` yields the raw halves of a UDP session's *taken* state confusingly vs `take_halves` on datagram sessions + +`take_halves` on a **datagram** session hands out the *framed* raw +halves (documented: "Datagram halves carry the codec framing (ADR-003) +— raw bytes on the wire are `[len: u16 BE][payload]` frames"). That's +correct per spec but is a footgun asymmetry: the same session exposes +`send_datagram`/`recv_datagram` (codec applied) until `take_halves`, +then the caller owns raw framed bytes. The doc covers it; a +convenience `take_framed_halves` that keeps the codec (a +`DatagramReader`-wrapped pair) would close the gap. No v1 blocker; +the review notes it for the API-stability window (ADR-005's surface +becomes ABI-stable at first external consumer — decide before then). + +## N-4 [minor] — `open_reverse_channel` builds the params JSON by hand instead of serializing `TunnelParams` — FIXED in-review + +`src/consumer.rs:395-409`: the substrate is matched to its string and +the payload assembled manually. `serde_json::to_value(params)` + +wrapping in `{"operationId", "input"}` would remove the duplicated +substrate mapping (it exists in three places now: `params.rs` +serialize, `open_reverse_channel`, and the tests' `call_open_params`). +Zero behavior difference today (`deny_unknown_fields` + the schema +match); it's drift risk if `TunnelParams` ever grows a field — the +hand-built payload silently won't carry it. Trivial cleanup. + +**Resolution (in-review):** `open_reverse_channel` now serializes +`TunnelParams` (`serde_json::to_value`) into the envelope's `input`; +the duplicated substrate mapping is gone; a serialize failure maps to +`ReverseOpenError::Call(internal)`. All reverse-path tests pass +unchanged. + +## N-5 [minor] — `producer.rs` module doc stale forward reference — FIXED in-review + +`src/producer.rs:22-23`: "The listen establisher variant ... lands +with `tunnels/producer-listen`." It landed (`listen_establisher`, +`AcceptQueue`, `register_tunnel_listen_openable` are all in this +file). Stale forward-reference; one-line doc cleanup. + +**Resolution (in-review):** the module doc now points at +`listen_establisher` + `AcceptQueue` in this file. + +## N-6 [minor] — `TunnelEstablishError` has no `Timeout` variant; the `timeout` reason exists only wrapper-side + +The crate's typed establisher vocabulary (`TunnelEstablishError`) is +`UnknownResource` / `DialFailed` / `ResourceShortage` / `HandlerError` +— `Timeout` is correctly absent (the bound is the wrapper's, per +ADR-049 §2 — an establisher shouldn't self-report it). This is +*correct* as-is; filed so the next reader doesn't "fix" it by adding +a variant. No action; recorded as a non-finding with a rationale. + +## N-7 [minor] — rustdoc: one broken intra-doc link in `lib.rs` — FIXED in-review + +``` +warning: unresolved link to `TunnelParams` + --> src/lib.rs:12:40 +``` + +`TunnelParams` is not re-exported at the crate root (it lives in +`params::`). Either re-export it (it IS the wire type of ADR-001 — +consumer-facing users will want it at the root; N-8) or link it +qualified (`[`TunnelParams`](crate::params::TunnelParams)`). One-line +fix; also clears `cargo doc --no-deps`'s only warning (the review +gate is a clean doc build). + +**Resolution (in-review):** N-8's re-export landed, which resolves the +link; `cargo doc --no-deps` is warning-free. + +## N-8 [minor] — Public API surface omits `TunnelParams`, `Substrate`, `tunnel_open_spec`, `OP_TUNNEL_OPEN` from the crate root — FIXED in-review + +`lib.rs` re-exports session/error/wire/producer names but not the +wire types a consumer assembly layer actually needs by name: +`TunnelParams`/`Substrate` (every `open`/`adopt` call takes them), +`tunnel_open_spec` (producer.md: "the open-op spec builder is public +so assembly layers can compose variants"), `OP_TUNNEL_OPEN`. They're +reachable at `alktunnels::params::*` (convention 16 says "public API +surface is lib.rs re-exports" — reachable-only is arguably compliant, +but the sibling alktty re-exports its params types). Trivial additive +re-export; do it before the first external consumer pins the +import style. + +**Resolution (in-review):** `TunnelParams`, `Substrate`, +`tunnel_open_spec`, and `OP_TUNNEL_OPEN` re-exported from the crate +root (additive — no existing import breaks). + +## N-9 [minor] — `cargo publish --dry-run` fails: `readme = "README.md"` but no README exists — FIXED in-review + +`Cargo.toml` declares `readme = "README.md"`; the file doesn't exist +in the repo (the pre-release gate command in AGENTS.md fails on it — +verified). Write the README (it's also what crates.io renders) or +drop the manifest field before any publish consideration. This is +exactly the class of thing the phase-gate review exists to catch. + +**Resolution (in-review):** README.md written (purpose, the resource +model, feature table, verification commands, status); `cargo publish +--dry-run --allow-dirty` now completes through packaging + verification. + +## N-10 [minor] — Timeout-mode harness arm creates a second `ChannelCore` without explanation — FIXED in-review (comment added) + +`tests/harness.rs:222-250` (`RegistrationMode::Timeout` arm): a fresh +`ChannelCore` is constructed for the hanging-establisher registration +where the other arms reuse the outer `core`. Both wrap the same +manager + `default_policy()`, so behavior is identical (the core is +a cheap facade pair). A one-line comment would prevent a future +reader from "fixing" the duplication and wondering why the outer +`core` wasn't reused... or worse, from wiring a *different* policy +there someday. Test-only; trivial. + +**Resolution (in-review):** comment added on the arm. + +## N-11 [observation, pinned] — `pump_against` on a UDP session pumps raw halves; the framing responsibility is undocumented — FIXED in-review (doc note added) + +`pump_against` takes the data plane (either variant) and pumps the +raw `BiStream` against the accepted local stream — for a UDP session +that means the *caller* must hold the codec responsibility (the +end-to-end suite's `reverse_udp_datagram_session_round_trips` notes +this: "No pump_against: the session's codec data plane is driven +directly... the pump is the consumer's job here only when it holds +local halves"). The API allows a UDP session + `pump_against(raw_udp_socket)` +which would be the raw-pass-through shape ADR-003 bans. It's a +caller-contract issue (the doc on `pump_against` doesn't warn), not a +library bug — the session can't know whether the accepted halves are +framed. Recommend a doc line on `pump_against`: "for UDP sessions, +the accepted halves must present the same codec framing +(ADR-003) — hand the framed adapter, not a raw socket." Fold into the +U-1 remediation docs. + +**Resolution (in-review):** the doc note is on `pump_against` now. + +--- + +# Part B — Non-findings (verified correct, recorded to bound the re-review) + +- **The open-op wire surface is exactly spec**: `Sub`-typed, external, + scope `["tunnel:open"]` with `required_scopes` only (the + `required_scopes_any`/resource fields correctly `None`-shaped), + `channel_open` marker `alk/tunnel`, description set (round-trips + through discovery per OQ-TN-08), input/output schemas matching + wire.md byte-for-byte (asserted by `open_spec_matches_wire_md`). +- **The establisher shape matches alkcall 0.7.0's contract + exactly**: `(input, per_call_auth) -> BoxFuture>`; `Establishment::new(plan)` with the + `Send + Sync` plan bound satisfied by `TargetHandle`'s boxed + `+ Sync` halves (F-1); the establisher correctly does NOT touch the + channel `Connection` (yield-once BiStream stays with the pump + handler — ADR-049 amendment 2). +- **The pump handler is the R-02 shape, structurally**: `accept_bi` → + plan downcast → `Arc::try_unwrap` → `pump_bidi(...).await` inline in + the spawned task. The `try_unwrap` failure arm (establisher plan + shared elsewhere — cannot happen with this establisher) logs and + returns, which is teardown-at-birth telemetry-visible, not a panic. + No spawn-and-forget, no hand-rolled two-pump loop anywhere. +- **`TunnelSession` teardown is the ADR-005 matrix**: `close` = + abort + reap (returns `bool`), `join` = await + reap + counts + `(c2p, p2c, reaped)` with the pump-panic arm logged and count-flattened + to `(0, 0)` (no `unwrap` on the `JoinError`), `Drop` = abort + + sync reap, idempotent (`Option::take` everywhere; an emptied + session drops as a no-op; the doc-pinned `UnknownChannel` on a + re-reaped entry is ignored). No `Clone` (compile-fail doctest). +- **`adopt`'s substrate parameter** is a documented, reasoned + divergence from the task sketch (consumer.rs doc: "the task sketch + omitted it — a datagram session cannot exist without it") and is + strictly more correct — the reverse-path caller knows the + substrate it opened with; requiring it keeps the session + constructor total. consumer.md's `adopt(hub_manager, channel_id, + TUNNEL_ALPN)` sketch is superseded by the implementation's + `(manager, channel_id, substrate, alpn)` — the doc's "adopt takes + the same typed-error surface" sentence is the normative part and + holds. Minor doc sync: update consumer.md's code sketch when the + U-1 batch lands. +- **`open_reverse_channel`'s payload shape** matches the wire: the + `auth_token` rides top-level (alkcall's ADR-017 §7 payload-token + posture), `operationId`/`input` per the call envelope; the missing- + `channel_id` case is typed (`NoChannelId`). +- **Error vocabulary is upstream-faithful**: `TunnelEstablishError` + → `EstablishmentError` 1:1 (`From` impl), and the wire reason codes + arrive through `ChannelOpenError::establishment_reason()` — the + crate adds no reason strings of its own (the ADR-049 vocabulary is + owned upstream, correctly). +- **Identity plumbing is end-to-end proven**: CF-005 (a) override, + (b) transport, token-over-transport (with a *distinct* provider id + so precedence is actually proven), fail-closed `FORBIDDEN`, and the + CF-006 establisher witness on both transport and token paths — the + witness seam is a clean test-only hook (public `IdentityWitness` + type is `Option`-gated, `None` in production wiring). +- **The `local` feature gate is structurally clean**: `tokio/net` + only under `local` (Cargo.toml feature declaration confirmed), + `src/local/` is the only `tokio::net` consumer, the shared modules + import nothing from it (`local/` depends on `producer.rs` types, + never vice versa), and the wasm target compiles clean with and + without clippy. +- **The BAST doc matches the codec**: `[len: u16 BE]` struct, + `maxLength: 65535`, "len == 0 is a legal empty datagram, NOT EOF" — + and `frame_datagram`/`DatagramReader` implement precisely that (the + runtime-codec/BAST drift is zero; the U-1 boundary issue lives + outside the codec). + +--- + +# Verification gates for the U-1 remediation + +1. A real-socket probe test (the review's probe shape, made permanent + in `tests/local_halves.rs`): a recording UDP echo server asserts + the datagrams it receives are **payload bytes only** (no `[len]` + prefix) and an **empty datagram round-trips** (session + `recv_datagram` yields `Some(b"")`, channel stays alive). +2. A >8 KiB datagram round-trips as ONE datagram at the target (the + 8 KiB copy-buffer split is gone). +3. An unframed datagram sent by the target resolves at the session + (target-initiated traffic is decoded, not parked). +4. The existing `local_halves` suite stays green unchanged (its + session-level round-trips are framing-symmetric today; they must + not regress). +5. `AcceptQueue`'s lost-wakeup regression test (C-1's) stays green; + `cargo test` + `--features local` + wasm checks as usual. + +# Remediation ordering + +1. **`tunnels/fix-udp-framed-adapter`** (U-1 + N-1 + N-11 doc notes + + U-2 + U-3 + the gates above) — the only correctness-relevant + remediation; blocks any UDP-over-`local` consumer, not the wire or + the API. +2. **Trivial batch** (N-7, N-8, N-5, N-4, N-9, N-10) — one pass, no + behavior changes; N-9 additionally unblocks the publish dry-run. + (C-1's fix + regression test landed with this review's commit.) +3. **Consider-before-first-consumer** (N-3) — an API posture that + becomes hard to change once an external consumer pins it; decide + in the next phase or file an OQ. (N-2 was in this list and was + retracted during the review — see its section.) + +# References + +- The spec set under review: `docs/architecture/` (wire.md, + producer.md, consumer.md, overview.md, decisions/001..006, + bast.md, open-questions.md) +- alkcall 0.7.0 surfaces verified in source: `channels/operations.rs` + (`run_open_wrapper`, `OpenEstablisher`, `Establishment`, + `EstablishmentError`), `channels/client.rs` (`ChannelOpenError`), + `channels/manager.rs` (`adopt_channel`, `teardown_channel`), + `channels/pump.rs` (`pump_bidi`) +- ADR-049 review (the establishment surface): alkcall + `docs/reviews/006-channel-open-establishment-gap-review.md` +- The task-level deliverables this review closes: + `tasks/tunnels/review-core-crates.md`, + `tasks/tunnels/review-impl.md` +- The house pattern: alkcall `docs/reviews/` (numbering + severity + legend) \ No newline at end of file diff --git a/src/consumer.rs b/src/consumer.rs index ccda29f..13e03b5 100644 --- a/src/consumer.rs +++ b/src/consumer.rs @@ -266,6 +266,13 @@ impl TunnelSession { /// the session, which now owns the pump — `close` aborts it, /// `join` awaits it, `Drop` aborts it. Consumes the data plane /// (the pump owns the channel halves from here). + /// + /// For UDP sessions the accepted halves must present the same + /// framing as the channel side (ADR-003): hand the FRAMED adapter + /// (the `local` feature's framing shape, or an assembly + /// equivalent), never a raw UDP socket — the pump copies raw + /// bytes and cannot know which side carries the `[len: u16 BE]` + /// framing. pub async fn pump_against(mut self, accepted: S) -> Self where S: AsyncRead + AsyncWrite + Send + Unpin + 'static, @@ -392,17 +399,11 @@ pub async fn open_reverse_channel( params: &TunnelParams, auth_token: Option<&str>, ) -> Result { - let substrate_str = match params.substrate { - Substrate::Tcp => "tcp", - Substrate::Udp => "udp", - Substrate::Unix => "unix", - }; let mut payload = serde_json::json!({ "operationId": OP_TUNNEL_OPEN, - "input": { - "resource": params.resource, - "substrate": substrate_str, - }, + "input": serde_json::to_value(params).map_err(|e| { + ReverseOpenError::Call(CallError::internal(format!("params serialize: {e}"))) + })?, }); if let Some(token) = auth_token { payload["auth_token"] = serde_json::Value::String(token.to_string()); diff --git a/src/lib.rs b/src/lib.rs index bd57abe..bf1e264 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -29,8 +29,8 @@ pub mod local; pub use consumer::{open_reverse_channel, TakenHalves, TunnelSession}; pub use error::{ReverseOpenError, TunnelError, TunnelIoError, TunnelOpenError}; -pub use params::ChannelOpenError; pub use params::{establishment_reason, TUNNEL_ALPN, TUNNEL_OPEN_SCOPE}; +pub use params::{tunnel_open_spec, ChannelOpenError, Substrate, TunnelParams, OP_TUNNEL_OPEN}; pub use producer::{ register_tunnel_listen_openable, AcceptFn, AcceptQueue, DialFn, ResourceRegistry, TargetHandle, TunnelEstablishError, diff --git a/src/producer.rs b/src/producer.rs index 5abea79..2f6e540 100644 --- a/src/producer.rs +++ b/src/producer.rs @@ -20,7 +20,8 @@ //! from and must not (AGENTS.md convention 7). //! //! The listen establisher variant (the producer-side listener — the -//! `-R` far-side listener shape) lands with `tunnels/producer-listen`. +//! `-R` far-side listener shape) is [`listen_establisher`] + +//! [`AcceptQueue`] below (the `tunnels/producer-listen` shape). use std::sync::Arc; @@ -289,8 +290,19 @@ impl AcceptQueue { /// Pop the next accepted handle, waiting while the queue is empty /// and open. `None` = the queue is closed-and-empty (the listener /// is gone) or the queue was closed while waiting. + /// + /// The wakeup is the `Notify` lost-wakeup avoidance pattern (the + /// tokio `Notify::notify_waiters` contract): the `Notified` future + /// is created BEFORE the state check on every iteration. A `push` + /// landing in the thread-preemption window between the check and + /// the future's first poll is captured by tokio's notify-waiters + /// counter comparison at that first poll — the future resolves + /// immediately; a later `push` finds the waiter registered and + /// wakes it. A future created after the check (the naive shape) + /// loses that push, parking the pop until the next one. pub async fn pop(&self) -> Option { loop { + let notified = std::pin::pin!(self.notify.notified()); { let mut inner = self.inner.lock().await; if let Some(handle) = inner.handles.pop_front() { @@ -300,7 +312,7 @@ impl AcceptQueue { return None; } } - self.notify.notified().await; + notified.await; } } diff --git a/tasks/tunnels/fix-udp-framed-adapter.md b/tasks/tunnels/fix-udp-framed-adapter.md new file mode 100644 index 0000000..8777c0a --- /dev/null +++ b/tasks/tunnels/fix-udp-framed-adapter.md @@ -0,0 +1,82 @@ +--- +id: tunnels/fix-udp-framed-adapter +name: "U-1 remediation — make UdpHalf the FRAMED adapter (ADR-003 at the local boundary)" +status: pending +depends_on: [tunnels/review-impl] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [local, udp, codec, adr-003, review-remediation] +--- + +## 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::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). + +### 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 unchanged (its + session-level round-trips are framing-symmetric today). +5. `cargo test` + `--features local` + wasm checks as usual; 3× + repeat-run stable. + +### Riders (from the same review, same file) + +- U-2: `connect_udp` binds `("0.0.0.0", 0)` — resolve the target + family first (or document the v4-only posture). +- U-3: size the recv scratch to the codec's MTU discipline instead of + a per-tunnel 65535-byte allocation (or justify the bound). + +## 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 + +## References + +- `docs/reviews/001-implementation-review.md` §U-1 (executable-pinned + probe evidence), §U-2, §U-3, §N-1 +- `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) \ No newline at end of file diff --git a/tasks/tunnels/review-core-crates.md b/tasks/tunnels/review-core-crates.md index 92bc94f..8bbdb17 100644 --- a/tasks/tunnels/review-core-crates.md +++ b/tasks/tunnels/review-core-crates.md @@ -1,7 +1,7 @@ --- id: tunnels/review-core-crates name: Mid-phase review — producer + consumer halves before the local feature -status: pending +status: completed depends_on: [tunnels/consumer-session] scope: moderate risk: low @@ -39,8 +39,8 @@ downstream tasks (Safe Exit); majors create remediation notes for ## Acceptance Criteria -- [ ] The 5 checklist items each with a verdict -- [ ] Criticals (if any) resolved before proceeding; majors logged +- [x] The 5 checklist items each with a verdict +- [x] Criticals (if any) resolved before proceeding; majors logged ## References @@ -51,6 +51,59 @@ downstream tasks (Safe Exit); majors create remediation notes for > Agent fills during implementation. +### Mid-phase verdicts (2026-09-08, at tree `fb2389b`..`09d32d5` scope) + +Executed with the mid-phase point in view (before +`local-socket-halves`/`end-to-end-suite` landed): the five checklist +items were run against `producer.rs`/`consumer.rs`/`params.rs`/ +`wire.rs`/`error.rs` + the then-existing suites, cross-checked against +alkcall 0.7.0's wrapper/establisher/pump source. Because both +downstream tasks built on the shapes immediately after, the full +re-verification (with the complete suite) landed in +`tunnels/review-impl` — consolidated findings live in +`docs/reviews/001-implementation-review.md` (the review-impl +deliverable). Verdicts, summarized: + +1. **Wire conformance — PASS.** Params shape exact + (`{resource, substrate}`, `deny_unknown_fields`, enum + `["tcp","udp","unix"]` — schema + serde both pinned by tests); op + id/ALPN/scope constants asserted against wire.md; codec + (`[len: u16 BE]`, `len=0` legal, `Oversize` at frame time, no + sentinel) correct. +2. **Pump handler shape — PASS.** `pump_bidi` awaited inline inside + the spawned handler task; `accept_bi` → plan downcast → + `Arc::try_unwrap` → inline await; no spawn-and-forget, no + hand-rolled two-pump loop (grep-verified); R-02 pinned by an + end-to-end test (`pump_handle_tracks_the_data_plane_r02`). +3. **Teardown matrix — PASS.** close (abort + reap), join (await + + reap + counts, panic arm logged), Drop (abort + sync reap), + pump-less join `(0, 0, reaped)`, failed-adopt no-leak — all pinned + with `channel_ids()` leak asserts on both sides. No `Clone` + (compile-fail doctest). +4. **No substrate types / no hand-rolled pumps / no side-channel + handoff — PASS.** The only plan flow is `Establishment::new(plan)` + → wrapper → handler `plan` param (R-01); the only `Mutex` + is the `ResourceRegistry` lookup table (a registry, not a plan + handoff); `tokio::net` appears only under the `local` gate. +5. **Tests green + 3× repeat-run stable — PASS** (both configs). + +### Majors logged (carried to `tunnels/review-impl`, re-verified there) + +- **C-1** (`AcceptQueue::pop` lost wakeup — found in the full sweep, + fixed in-review): see review 001 §C-1. +- **U-1** (the `local` UDP adapter is not the framed adapter — found + by the review-impl probes over real sockets): see review 001 §U-1; + remediation task `tunnels/fix-udp-framed-adapter`. + +No criticals: the wire surface (the one-way door) is conformant; no +finding blocks the phase. + ## Summary -> Agent fills this on completion. \ No newline at end of file +> Agent fills this on completion. + +Mid-phase review executed as part of the consolidated `tunnels/ +review-impl` pass (2026-09-08): all five checklist items PASS; two +majors logged (C-1, U-1) and carried to the phase-gate review, where +C-1 was fixed and U-1 got the remediation task. Full findings + +severity grading: `docs/reviews/001-implementation-review.md`. \ No newline at end of file diff --git a/tasks/tunnels/review-impl.md b/tasks/tunnels/review-impl.md index 814003d..245bf5a 100644 --- a/tasks/tunnels/review-impl.md +++ b/tasks/tunnels/review-impl.md @@ -1,7 +1,7 @@ --- id: tunnels/review-impl name: Review alktunnels v1 implementation for spec conformance (pre-release gate) -status: pending +status: completed depends_on: [tunnels/end-to-end-suite, tunnels/review-core-crates] scope: moderate risk: low @@ -67,9 +67,9 @@ tasks; minors get a follow-up batch. ## Acceptance Criteria -- [ ] Review doc filed with severity-graded findings -- [ ] Remediation tasks created for anything above trivial -- [ ] The 5 checklist sections each covered with a verdict +- [x] Review doc filed with severity-graded findings +- [x] Remediation tasks created for anything above trivial +- [x] The 5 checklist sections each covered with a verdict ## References @@ -81,6 +81,101 @@ tasks; minors get a follow-up batch. > Agent fills during implementation. +### Review executed 2026-09-08 (tree `2efac9b`) — deliverable: `docs/reviews/001-implementation-review.md` + +Method: full source read (`src/` + `tests/`) against the complete spec +set; alkcall 0.7.0 API surfaces verified in source (wrapper, establisher +types, client errors, manager adopt/teardown, `pump_bidi`); executable +probes over real sockets for the `local` UDP path (temporary test file, +deleted after the run — probe evidence quoted in the review); the full +verification battery (see below). The mid-phase `tunnels/ +review-core-crates` checklist is subsumed (consolidated in the review +doc; that task's Notes carry its verdicts). + +### The five checklist verdicts + +1. **Wire conformance — PASS.** Params shape exact (`deny_unknown_ + fields`, required both, enum `["tcp","udp","unix"]` — the OQ-TN-14 + note in this checklist predates the OQ's resolution; `unix` SHIPS + per OQ-TN-14's resolution, and the enum carries it correctly); op + id/ALPN/scope pinned; codec correct including `len=0` and frame-time + `Oversize`; all five typed establishment reasons reachable and + proven (3 by test: `unknown_resource`/`dial_failed`/ + `resource_shortage`; `handler_error` through the same `From` arm; + `timeout` by the hanging-establisher probe) + `FORBIDDEN` + + `channel:too_many_channels`. +2. **Producer conformance — PASS.** Awaited bounded establisher (10s + default + per-registration override proven); pure R-01 plan flow + (grep-verified — no side-channel handoff, no poll-loop take); pump + handler structurally R-02 (`pump_bidi` inline; JoinHandle tracks + the data plane, pinned by test); post-hoc registration (W2) + supported; listen variant rides the same op with the typed-error + table proven. +3. **Consumer conformance — PASS.** Full session surface per ADR-005; + teardown matrix sound (close/join/Drop/pump-less/failed-adopt — all + leak-asserted on both managers); no `Clone` (compile-fail doctest). + One documented divergence: `adopt` takes the substrate (the task + sketch omitted it — a datagram session cannot exist without it). +4. **Conventions sweep — PASS.** No unwrap/expect/panic outside + `#[cfg(test)]` (grep-verified); thiserror everywhere; all locks are + `tokio::sync::Mutex` (unpoisoned — the `into_inner` rule has no + application point, N/A); substrate types confined to `src/local/` + + feature gate; wasm clean (`check` + `clippy --target + wasm32-unknown-unknown`, `-D warnings` both); `pump_bidi` consumed, + never hand-rolled. Inline `//` comments are the + safety-constraint class convention 1 allows. +5. **Docs ↔ implementation sync — PASS with drift fixed in-review.** + Module map matches; ADRs all Accepted (implementation confirms); N-5 + (stale doc forward-reference), N-7 (broken doc link), N-8 (missing + root re-exports) fixed in-review. + +### Findings (full detail in the review doc) + +- **U-1 [major]** — the `local` UDP adapter is NOT the framed adapter + ADR-003 mandates: codec-framed bytes reach real targets; empty + datagrams are EOF-shaped at the adapter (tunnel teardown); + >8 KiB datagrams split; unframed target-initiated datagrams are + dropped. Executable-pinned by probes. **NOT fixed — remediation + task `tunnels/fix-udp-framed-adapter`** (blocks UDP-over-`local` + consumers; wire surface unaffected). +- **C-1 [major]** — `AcceptQueue::pop` lost wakeup in the + check→register window (open op hangs to the establishment bound on + a lost push). **Fixed in-review** (create-`Notified`-before-check + per iteration, the tokio `notify_waiters` contract; regression test + added). +- **U-2/U-3 [minor]** — `connect_udp` v4-only bind posture; 65535-byte + per-tunnel scratch. Riders on the U-1 task. +- **N-4/N-5/N-7/N-8/N-9/N-10/N-11 [minor]** — fixed in-review (payload + serialization, doc staleness, doc link, root re-exports, README + + publish dry-run, harness comment, `pump_against` UDP framing doc + note). +- **N-2** — retracted during the review by executable falsification + (the codec's buffering is bounded by construction; the u16 cap IS + the bound). +- **N-6** — non-finding with rationale (`TunnelEstablishError` has no + `Timeout` variant, correctly). + +No criticals — the phase-gate passes. + +### Verification battery (all green) + +- `cargo test`: 62 passed (default) / 69 passed (`--features local`), + 3× repeat-run clean in both configs +- `cargo clippy --all-targets -- -D warnings`: clean +- `cargo clippy --target wasm32-unknown-unknown -- -D warnings`: clean +- `cargo fmt --check`: clean +- `cargo doc --no-deps`: clean after N-7's fix +- `cargo check --target wasm32-unknown-unknown`: clean +- `cargo publish --dry-run --allow-dirty`: passes after N-9's README + ## Summary -> Agent fills this on completion. \ No newline at end of file +> Agent fills this on completion. + +Phase-gate review complete: the wire surface (the one-way door) is +conformant; the API surface matches ADR-005/006; two majors found — +U-1 (local UDP framing, remediation task created) and C-1 (queue lost +wakeup, fixed with the review commit). The trivial batch + README + +publish dry-run also landed with the review. Findings with +severity grading and remediation ordering: +`docs/reviews/001-implementation-review.md`. \ No newline at end of file diff --git a/tests/harness.rs b/tests/harness.rs index 852c90c..032e908 100644 --- a/tests/harness.rs +++ b/tests/harness.rs @@ -225,6 +225,11 @@ pub async fn wire_with( // registration timeout. The generic channel ops registered // above on the SAME registry are unaffected (per-op // timeout, ADR-049 §2). + // A fresh ChannelCore over the SAME manager + policy (both + // cores are cheap facades over the same pair — the second + // registration rides the same ledger/cap state; this arm + // is local to the probe so the other arms' `core` binding + // stays untouched). let core = alkcall::channels::operations::ChannelCore::new( producer_client.manager().clone(), alkcall::channels::policy::default_policy(), diff --git a/tests/producer_listen.rs b/tests/producer_listen.rs index 06d9b72..566161b 100644 --- a/tests/producer_listen.rs +++ b/tests/producer_listen.rs @@ -320,3 +320,46 @@ async fn accept_wait_resolves_when_push_arrives_late() { let reaped = session.close().await; assert!(reaped); } + +/// The lost-wakeup regression (review 001 C-1): a `push` landing in +/// the wall-clock window between a parked `pop`'s state check and its +/// `Notified` registration must still wake the pop. The window only +/// exists under true thread preemption (a single-threaded executor +/// runs the pop's check and the future's first poll back to back), +/// hence the multi-thread flavor. The fix creates the `Notified` +/// future BEFORE the state check on every iteration (tokio's +/// documented notify_waiters pattern — the counter comparison at +/// first poll captures a pre-registration broadcast); the naive +/// create-after-check shape loses it. Structural: no choreography can +/// force the window deterministically, so this test pins the pattern +/// by hammering it (each round the push lands somewhere in the +/// pop's window; a lost wakeup would surface as the 500ms timeout +/// flake). +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn pop_wakeup_survives_push_landing_in_the_registration_window() { + for _ in 0..64 { + let queue = AcceptQueue::new(); + let queue_popper = queue.clone(); + let popper = tokio::spawn(async move { + tokio::time::timeout(std::time::Duration::from_millis(500), queue_popper.pop()) + .await + .expect("pop timed out — lost wakeup") + .expect("queue closed early") + }); + // The push races the popper's check→register window on the + // second worker: no synchronization, by design. + let (consumer_side, target_side) = tokio::io::duplex(1024); + drop(target_side); + let (c_read, c_write) = tokio::io::split(consumer_side); + queue + .push(TargetHandle { + read: Box::new(c_read), + write: Box::new(c_write), + }) + .await + .expect("push"); + let handle = popper.await.expect("popper task"); + drop(handle); + queue.close().await; + } +}