review: phase-gate review 001 — spec conformance of the v1 implementation
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.
This commit is contained in:
@@ -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<HashMap>` 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<Result<Establishment,
|
||||
EstablishmentError>>`; `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)
|
||||
Reference in New Issue
Block a user