review 002: the graduation implementation — spec conformance + coverage appendix
This commit is contained in:
glm-5.3-flash committed
1 parent
474844e563
commit
46d3a87b24
1 file changed
+674
@@ -0,0 +1,674 @@
|
||||
# Review 002 — The Graduation Implementation (direct op + bind-first + forwarded op)
|
||||
|
||||
## Status
|
||||
|
||||
**Conformant with findings.** Six sessions reviewed
|
||||
(`52b6252..474844e` — the taskgraph fix + the six graduation feature
|
||||
commits): the direct op end to end, the bind-first establisher + accept
|
||||
dispatcher, the forwarded op's serving side, the local bind-first
|
||||
halves, and `TunnelListener`. Wire surface, scope separation, typed
|
||||
errors, teardown ownership, and the R-02/ADR-050 contracts all conform
|
||||
(verified against the alkcall 0.8.0 source, not just its docs). No
|
||||
[critical] findings. **One [minor] implementation gap (M-1) and a
|
||||
cluster of [major]-labeled doc/spec-sync items (D-1..D-4) that are
|
||||
doc-only in code impact** — the code is the wire-stable side and is
|
||||
correct; the *docs* lag it. Nothing blocks the graduation consumers.
|
||||
|
||||
Findings use prefix `M` (minor code), `D` (doc/spec sync), `N` (notes).
|
||||
This is the second review in this crate's series (numbering follows the
|
||||
alkcall house pattern — each review numbers independently).
|
||||
|
||||
## Scope
|
||||
|
||||
The six sessions' work at tree `474844e` reviewed against ADR-007,
|
||||
ADR-008 (as amended), the updated wire.md/consumer.md/producer.md, the
|
||||
AGENTS.md conventions, and — load-bearing — the alkcall 0.8.0 source
|
||||
(`/workspace/@alkdev/alkcall/src/channels/{operations,pump,manager,mux,source,client}.rs`,
|
||||
`src/registry/registration.rs`), cross-checking the six contracts this
|
||||
implementation leans on.
|
||||
|
||||
```
|
||||
Verified against: alktunnels 474844e; alkcall 0.8.0 (source-read)
|
||||
Reading list: src/{lib,params,producer,consumer,error,wire}.rs,
|
||||
src/local/mod.rs, tests/{harness,producer_bindfirst,producer_direct_op,
|
||||
producer_forwarded,consumer_listener,local_bindfirst}.rs,
|
||||
docs/architecture/*.md incl. decisions/007, 008
|
||||
Executed: cargo test (132 default / 157 --features local, doc-tests 2/2),
|
||||
clippy --all-targets -D warnings (native + wasm32), fmt --check,
|
||||
cargo doc --no-deps (0 warnings), cargo check + clippy on
|
||||
wasm32-unknown-unknown — all green, matching the commit messages' claims;
|
||||
cargo-llvm-cov 0.8.4 line/region coverage, default (95.63%) +
|
||||
--features local (93.66%) — see the coverage appendix
|
||||
Source-read (alkcall 0.8.0): pump_bidi shutdown-on-completion,
|
||||
teardown_channel drop-sender EOF, mux implicit-EOF sentinel,
|
||||
yield-once accept_bi + accepted-flag telemetry,
|
||||
Establishment::with_reply_field + reserved-key guard,
|
||||
open_channel_with_reply shape, registry input_schema gate
|
||||
(compiled at registration, runs after ACL, INVALID_INPUT on violation)
|
||||
```
|
||||
|
||||
## Severity legend
|
||||
|
||||
Same scale as review 001 / the alkcall reviews:
|
||||
|
||||
- **[critical]** — a decided spec invariant violated so a promised
|
||||
capability is unreachable end-to-end; or data corruption.
|
||||
- **[major]** — a core protocol path cannot serve a decided behavior;
|
||||
or a wire/ABI promise is not delivered by the code.
|
||||
- **[minor]** — drift, doc/spec inconsistency, or a missing
|
||||
convenience with no correctness impact.
|
||||
|
||||
---
|
||||
|
||||
# Checklist verdicts
|
||||
|
||||
1. **Wire conformance (ADR-007/008 + wire.md) — PASS.** The direct op's
|
||||
spec (`tunnel_direct_spec`) and the forwarded op's spec
|
||||
(`tunnel_forwarded_spec`) match wire.md §The Direct Op / §The
|
||||
Forwarded Op: op ids, `Sub` type, `alk/tunnel` ALPN marker, the
|
||||
per-substrate `if`/`then` target/peer sub-schemas with
|
||||
`additionalProperties: false`, and the three independent scopes
|
||||
(`tunnel:open`/`tunnel:direct`/`tunnel:forwarded`) — each proven
|
||||
never-implied by wire-level tests in both directions
|
||||
(`producer_direct_op.rs` `tunnel_open_scope_does_not_imply_*`,
|
||||
`producer_forwarded.rs::forwarded_scope_gate_is_independent_grant`).
|
||||
The `listen` token's positivity is enforced at three layers
|
||||
(schema `minimum: 1`, the `positive_u32` serde pin, the reply
|
||||
projection's `> 0` filter) with the mint never yielding 0
|
||||
(`mint_token` seeded at 1, wrap-skip pinned by test). The reply
|
||||
projection's asymmetry is deliberate and tested both ways:
|
||||
`parse_listen_reply` is the loud typed viewer (malformed →
|
||||
`MalformedReply`), `listen_reply_fields` the lenient session-surface
|
||||
projection (malformed → both-`None` + loud log) — the doc comments
|
||||
on both correctly state which to use when, and the additive posture
|
||||
(unknown reply fields tolerated) is pinned by test.
|
||||
|
||||
2. **Producer conformance — PASS.** The bind-first establisher is the
|
||||
awaited bounded establishment phase (registry gate → mint → bind →
|
||||
reply fields → `Establishment::new(plan)`), the plan is
|
||||
typed-opaque (`BindFirstPlan` downcast test), and the accept
|
||||
dispatcher is a faithful ADR-008 §1 implementation: `accept_bi`
|
||||
FIRST (the R-02 accepted-flag telemetry — verified in upstream
|
||||
`operations.rs:954-1000`, the wrapper's birth-teardown hint keys on
|
||||
it), the idle-stream EOF watch races the accept loop in `select!`,
|
||||
per-accept forwarded opens are isolated (drop the accept, keep
|
||||
serving — pinned by two tests), loop-critical accept errors end the
|
||||
loop, `drop(plan)` tears the listener down, and the handler's
|
||||
`JoinHandle` awaits every per-accept pump (R-02 — no forwarded pump
|
||||
outlives its listener). The forwarded-op serving handler parses
|
||||
params (deny-unknown + positive-listen serde pin) and accepts the
|
||||
`BiStream`, awaiting the injected handler's `JoinHandle` inline —
|
||||
a malformed open tears down at birth with no phantom channel
|
||||
(pinned: `forwarded_malformed_params_tear_down_loud`).
|
||||
`register_openable` (no establisher) for the forwarded op matches
|
||||
ADR-008 §3's no-establishment shape.
|
||||
|
||||
3. **Consumer conformance — PASS.** `TunnelSession::open_direct` is the
|
||||
separate constructor ADR-007 §5 pins (not a params enum — two
|
||||
scopes, the caller states which it holds); the session type, data
|
||||
planes, and teardown API are identical to `open`. `TunnelListener`
|
||||
holds the write half IDLE (never exposed, never pumped), projects
|
||||
the reply fields via the lenient path, and owns teardown
|
||||
(`close` = graceful `shutdown` → EOF sentinel → reap; `Drop` =
|
||||
implicit-EOF chunk → reap — the mux's implicit-EOF path is real,
|
||||
verified in upstream `mux.rs:67-82`). No `Clone` compile-asserted
|
||||
for both session types. `adopt` gains nothing — pinned by test at
|
||||
both the standalone and through-the-real-forwarded-registration
|
||||
levels.
|
||||
|
||||
4. **Conventions sweep — PASS with notes.** No `unwrap`/`expect`/
|
||||
panics outside tests (the `unreachable!` arms in
|
||||
`take_halves`/`pump_against` are unreachable by the documented
|
||||
data-plane-XOR-pump invariant and are the pre-existing v1 shape).
|
||||
No substrate types outside `src/local/` (grep-verified). No
|
||||
comments beyond doc comments (the inline `//` density in
|
||||
`producer.rs`/`local/mod.rs` is high but they are documenting
|
||||
ADR-pinned contracts, which AGENTS.md convention 1's exception
|
||||
covers — correctness constraints that would otherwise be missed).
|
||||
The wasm-clean invariant holds (`cargo check + clippy` on
|
||||
wasm32-unknown-unknown pass; `tokio` features unchanged; no new
|
||||
non-local deps). Feature-gate verification: both `cargo test`
|
||||
(default) and `cargo test --features local` pass.
|
||||
|
||||
5. **Conformance to the amended ADR-008 — PASS.** Amendment 1's
|
||||
producer-minted token is implemented exactly as pinned: minted at
|
||||
establisher time (non-zero-seeded `AtomicU32`, per-open fresh,
|
||||
per-producer unique, wrap-skip of the illegal 0), carried twice
|
||||
(plan + `listen` reply field), stamped per-dispatcher from its own
|
||||
plan (pinned by the two-open token-discipline test), and consumed
|
||||
by the opener-side correlation (assembly-side bookkeeping — the
|
||||
crate provides the op + params, not the wait state machine).
|
||||
|
||||
---
|
||||
|
||||
# Findings
|
||||
|
||||
## M-1 [minor] — The accept dispatcher's substrate fallback is silent and wrong-shaped
|
||||
|
||||
`src/producer.rs:766-768`
|
||||
(`make_bindfirst_accept_handler`):
|
||||
|
||||
```rust
|
||||
let substrate = serde_json::from_value::<TunnelParams>(input.clone())
|
||||
.map(|p| p.substrate)
|
||||
.unwrap_or(Substrate::Tcp);
|
||||
```
|
||||
|
||||
The listen open's input params have already passed the registry's
|
||||
schema gate (enum-validated) **and** the establisher's
|
||||
`parse_params` (which rejects unparseable input with
|
||||
`handler_error`), so in the wired path the fallback is unreachable.
|
||||
But the fallback shape itself is the problem: if the parse ever fails
|
||||
(a registry whose schema gate is looser, an assembly variant, future
|
||||
drift), the dispatcher silently stamps `Tcp` into every forwarded
|
||||
open — for a UDP listen tunnel that is a raw-vs-framed data-plane
|
||||
mismatch on the *opener* side, the exact invariant class review 001
|
||||
U-1 was about, and it degrades silently instead of loudly.
|
||||
|
||||
The same silent-fallback shape exists in no other handler: the
|
||||
forwarded-op handler (`make_forwarded_op_handler`,
|
||||
`src/producer.rs:993-1002`) parses and **returns** on failure
|
||||
(teardown-at-birth, the correct loud shape); the pump handler
|
||||
downcasts the plan and returns on failure.
|
||||
|
||||
**Suggested fix (small, mechanical):** on parse failure, log loud and
|
||||
return (teardown-at-birth — the channel reaps, the far listener dies
|
||||
with the plan, the same posture as every other early-return in this
|
||||
file). Alternatively, since the establisher already parsed the params
|
||||
successfully, stamp the substrate into `BindFirstPlan` at establisher
|
||||
time and drop the second parse entirely — one parse, one source of
|
||||
truth, no fallback branch at all. The second shape is strictly better
|
||||
(the plan is already the per-open carrier; the substrate is
|
||||
per-open-static) but changes a public-ish struct.
|
||||
|
||||
**Why not fixed inline:** the choice between "log-and-return" and
|
||||
"stamp the plan" is a small API-shape decision (`BindFirstPlan`
|
||||
gains a field — a plan-payload change, cheap now, pre-consumer) that
|
||||
the next session should make deliberately, and both need a
|
||||
failure-path test.
|
||||
|
||||
## D-1 [major-as-labeled, doc-only] — wire.md omits the `port` bounds the shipped schema enforces
|
||||
|
||||
The shipped direct-op and forwarded-op schemas pin
|
||||
`"port": {"minimum": 0, "maximum": 65535}` plus
|
||||
`"additionalProperties": false` on the target/peer objects
|
||||
(`src/params.rs:208-222`, `453-467`), and `listen` with
|
||||
`"minimum": 1` (`src/params.rs:442`). wire.md (the normative WHAT)
|
||||
says only `port: <u16>` and "integer > 0" in prose — it does not
|
||||
record the bounds as schema properties, nor the
|
||||
`additionalProperties: false` gate that makes cross-shape targets
|
||||
fail the registry's `INVALID_INPUT` class *before* the establisher
|
||||
(the behavior `malformed_target_is_schema_class_and_leaves_no_channel`
|
||||
pins on the wire). wire.md is the wire-stable contract doc: the
|
||||
schema details are part of the one-way door (ADR-007 §"Wire-stable":
|
||||
"the target sub-schema" is listed as wire-stable). The BAST doc
|
||||
(`bast.md`) covers only the data-plane codec, not the params schemas,
|
||||
so there is no second source recording these bounds.
|
||||
|
||||
**Impact:** none at runtime (the code is the gate); the risk is a
|
||||
future consumer implementing from wire.md alone and accepting a
|
||||
shape the reference producer rejects (or vice versa). Sync the doc
|
||||
before the first consumer (alksocks) ships.
|
||||
|
||||
## D-2 [major-as-labeled, doc-only] — wire.md's `if`/`then` structure differs textually from ADR-007's `oneOf`
|
||||
|
||||
ADR-007 §2 says "the target sub-schema is per-substrate (`oneOf` on
|
||||
the sibling `substrate` value, validated at establisher time — see
|
||||
Consequences)"; the shipped schema (and wire.md §The Forwarded Op's
|
||||
structure, and the unit tests pinning it) use **`allOf` of two
|
||||
`if`/`then` branches** instead. `oneOf` of full-target-schemas would
|
||||
actually be *wrong* here (a `unix`-substrate open would need to fail
|
||||
the `tcp` branch — plain `oneOf` on sibling-conditioned subschemas is
|
||||
awkward; `if`/`then` composes correctly with the base
|
||||
`properties`/`required`). The code is right; ADR-007's one-word
|
||||
schema-structure reference is stale. One-line ADR erratum (or an
|
||||
amendment note), plus ADR-008 §3's "validated at establisher time"
|
||||
(ADR-007 §2's parenthetical) vs the shipped registry-side gate: the
|
||||
establisher-time sentence in ADR-007 §2 line 82 ("validated at
|
||||
establisher time") contradicts both the shipped design and the
|
||||
wire.md text ("validated at establisher time" appears in
|
||||
wire.md §The Direct Op but the test proves the registry gate fires
|
||||
first — `malformed_target_is_schema_class_and_leaves_no_channel`
|
||||
asserts the establisher's dial never runs). The ADR/wire.md sentence
|
||||
should read "validated registry-side (the schema gate), before the
|
||||
establisher; re-checked by serde at establisher time as the
|
||||
backstop" — which is exactly what `src/params.rs`'s doc comments
|
||||
already say.
|
||||
|
||||
## D-3 [minor] — `jsonschema_valid` in `src/params.rs` tests is a stub, not a validator
|
||||
|
||||
`src/params.rs:628-637`: `jsonschema_valid` parses the value with
|
||||
serde and pattern-matches the substrate string — it validates the
|
||||
serde layer, not the JSON Schema. Both call-sites'
|
||||
names (`schema_rejects_unknown_substrate_value`,
|
||||
`direct_target_schema_gates_shape_per_substrate` — the latter
|
||||
correctly does *structural* schema assertions instead and is honest
|
||||
about it) overpromise for the former: `schema_rejects_unknown_
|
||||
substrate_value` does not execute the spec's `input_schema` at all.
|
||||
The registry-side gate IS exercised end-to-end by the integration
|
||||
tests (`schema_rejects_unknown_substrate_loudly`,
|
||||
`malformed_target_is_schema_class…`, the forwarded `listen: 0` gate),
|
||||
so coverage is not lost — but the unit test's name promises more
|
||||
than it delivers. Either rename it (e.g.
|
||||
`serde_rejects_unknown_substrate_value`) or drop it in favor of the
|
||||
integration probes. alkcall already compiles the real validators at
|
||||
registration; a `jsonschema` dev-dependency purely for this unit
|
||||
test is not warranted.
|
||||
|
||||
## D-4 [minor] — CHANGELOG "Unreleased" preamble is stale
|
||||
|
||||
`CHANGELOG.md`'s Unreleased section still opens with "implementation
|
||||
is decomposed in `tasks/tunnels/` and lands in the next release…
|
||||
the only behavior-affecting change so far (no wire or API change at
|
||||
this level yet)" — written before the six implementation commits.
|
||||
The section below it correctly lists all six features. The preamble
|
||||
contradicts the section it introduces. Trim the preamble to a
|
||||
one-liner (or drop it) before release.
|
||||
|
||||
## N-1 [note] — `parse_listen_reply`'s malformed-`listen` message vs the serde pin's semantics
|
||||
|
||||
`parse_listen_reply` requires `listen` to be "a positive u32" and
|
||||
rejects 0 (`src/params.rs:329-339`) — matching the forwarded params'
|
||||
wire contract (`minimum: 1`). Consistent and intentional, but note
|
||||
the asymmetry with `bound`: a `bound: null` and a `listen: null` are
|
||||
both treated as absent (`None | Some(Value::Null) => None`) — fine,
|
||||
JSON-null-as-absent is the standard lenient reading, and the loud
|
||||
path still fires for present-but-malformed shapes. No action; noting
|
||||
the deliberate shapes for the record since `TunnelListenReply`
|
||||
carries `deny_unknown_fields` while the *parse function* tolerates
|
||||
unknown fields — the struct-level attribute is only load-bearing if
|
||||
someone deserializes the struct directly from a full reply, which the
|
||||
doc comment explicitly warns against ("Parse the full reply with
|
||||
`parse_listen_reply`, never with `deny_unknown_fields`
|
||||
deserialization of the whole reply"). The warning is doing real
|
||||
work; keep both.
|
||||
|
||||
## N-2 [note] — The idle-stream EOF watch treats ANY byte as teardown
|
||||
|
||||
`make_bindfirst_accept_handler`'s `eof_watch` (`src/producer.rs:781-794`)
|
||||
triggers teardown on `Ok(0)`, `Err(_)`, **and** `Ok(1..)` — a
|
||||
misbehaving opener that writes to the idle listen channel also tears
|
||||
the listener down. The doc comment owns this ("a peer that writes is
|
||||
misbehaving; treat any non-empty read as an EOF trigger too"), and it
|
||||
is the safe direction (fail closed — the channel is not a data
|
||||
plane). Verified upstream that a `MpscSendStream` drop without
|
||||
`shutdown` produces the implicit-EOF chunk (the `Ok(0)` path) and a
|
||||
`shutdown` produces the sentinel — both arrive as `Ok(0)` at the far
|
||||
reader, so both `TunnelListener::close` (explicit shutdown) and
|
||||
`Drop` (implicit EOF) trigger the far teardown. Correct. No action;
|
||||
recorded because a reviewer reading only ADR-008 §1 ("its EOF
|
||||
cancels the loop") might implement the narrower `Ok(0)`-only check
|
||||
and hang on a write-happy peer — the code is right, the ADR sentence
|
||||
is thinner than the implementation.
|
||||
|
||||
## N-3 [note] — `make_forwarded_op_handler` parses params BEFORE `accept_bi`
|
||||
|
||||
`src/producer.rs:993-1006`: the malformed-params early return happens
|
||||
before `conn.accept_bi()`, so the R-02 accepted-flag telemetry reads
|
||||
`false` for a malformed-params open (the wrapper logs the
|
||||
birth-teardown hint). This is the deliberate reading of R-02 — the
|
||||
flag is telemetry for the "handler exited without accepting" *hint*,
|
||||
and a malformed-params open IS a birth-teardown shape — but the
|
||||
ordering means the hint fires for a handled-rejection case. Cosmetic
|
||||
telemetry noise at worst (a `tracing::debug!`); the test suite pins
|
||||
the teardown behavior, which is what matters. The alternative (accept
|
||||
first, then parse) would hold a stream open for a params set the
|
||||
producer already got wrong — the current order is the better one.
|
||||
Documented here so a future alkcall reviewer doesn't flag the flag.
|
||||
|
||||
## N-4 [note] — `SubstrateAddr` untagged leniency is schema-gated, and that's the right call (but it is a latent sharp edge)
|
||||
|
||||
The untagged `SubstrateAddr` accepts extra fields (untagged variants
|
||||
cannot `deny_unknown_fields`), so a cross-shape target like
|
||||
`{"path": "/p", "host": "h"}` parses as the `Path` variant at serde
|
||||
level. Both the direct-op and forwarded-op tests pin this asymmetry
|
||||
with the same rationale: the SCHEMA is the shape-vs-substrate gate
|
||||
(`additionalProperties: false` per branch). That holds for the
|
||||
registry-dispatched path. Two things to keep in view:
|
||||
|
||||
- The establisher-level backstop (the serde parse inside
|
||||
`direct_establisher`) keys `render_target` off the sibling
|
||||
`substrate` — a cross-shape target that sneaked past a looser
|
||||
registry would render *something* (`/p` for the unix case) rather
|
||||
than fail. Acceptable (garbage-in handled by the schema's absence),
|
||||
but worth an ADR-line if a future consumer runs a schema-less
|
||||
registry variant.
|
||||
- If `SubstrateAddr` grows a variant whose shapes overlap
|
||||
(e.g. a future `{host, port, flowlabel}`), untagged disambiguation
|
||||
starts ordering — at that point switch to
|
||||
`#[serde(untagged)]`-with-content or an internally-tagged form.
|
||||
Pre-consumer, additive; just don't let it happen by accident.
|
||||
|
||||
## N-5 [note] — The token-mint discipline is sound; the concurrent-mint shape is untestable at this layer and fine
|
||||
|
||||
`mint_token` is a relaxed-ordering `fetch_add` loop skipping 0. The
|
||||
"2^32 concurrent opens" wrap-collision math in the doc comment is
|
||||
right (and the collision consequence — an opener correlating a
|
||||
forwarded open to the wrong listener — requires the same producer to
|
||||
have 2^32 *live* listeners simultaneously, impossible under the
|
||||
256-channel per-connection cap; the token is per-producer-connection,
|
||||
and each open rides one connection). Worth noting the cap makes the
|
||||
wrap analysis even more favorable than the comment states. No action.
|
||||
|
||||
## N-6 [note] — Test-only race window in `bindfirst_open_surfaces_reply_fields_and_teardown_cascades`
|
||||
|
||||
`tests/consumer_listener.rs:276` (and several sibling tests) settle
|
||||
the forwarded open with `tokio::time::sleep(300ms)` then poll
|
||||
`channel_ids()`. Sleep-then-assert is the established house pattern
|
||||
in this suite (the harness has no push-based settle surface), but
|
||||
these are the tests most likely to flake on a loaded CI box (the
|
||||
forwarded open traverses: producer accept → call plane → opener
|
||||
dispatch → registry → wrapper → handler spawn). The suite has been
|
||||
3x-repeat stable historically (review 001's methodology); if CI ever
|
||||
shows a flake here, the fix is a bounded poll loop on
|
||||
`channel_ids()` (the teardown assertions already use that shape —
|
||||
extend it to the arrivals). No action now.
|
||||
|
||||
## N-7 [note] — `test_bind()`'s listener is a bare `Arc<u32>`
|
||||
|
||||
The test bind closures in `producer_bindfirst.rs` /
|
||||
`consumer_listener.rs` put `Arc::new(4242u32)` in the plan's opaque
|
||||
listener slot. Fine for tests that feed accepts via side channels —
|
||||
and the mismatch probe (`accept_pair_loop_downcast_miss_fails_loud`)
|
||||
proves the real dispatch path fails loud on exactly this shape —
|
||||
but if a test ever wired `local_bindfirst_accept_loop()` against
|
||||
`test_bind()`, the failure would surface as the generic
|
||||
`handler_error`, not a test bug message. The tests consistently use
|
||||
the queue loops instead. No action; the downcast-miss test covers
|
||||
the failure mode.
|
||||
|
||||
---
|
||||
|
||||
# What was checked and found conformant (highlights)
|
||||
|
||||
- **The token path end-to-end** (Amendment 1): mint (seed-1, wrap-skip)
|
||||
→ reply field (reserved-key guard upstream keeps `channel_id`
|
||||
wrapper-owned — verified in `operations.rs:927-946`) → lenient
|
||||
projection → forwarded-param keying → opener-side correlation →
|
||||
per-dispatcher own-plan stamping (two-open test) → hub-relay
|
||||
pass-through (params untouched, reply fields per-hop-truthful —
|
||||
upstream ADR-051 machinery, source-verified in the relay docs).
|
||||
- **The teardown cascades**: listener drop → implicit-EOF chunk
|
||||
(upstream `mux.rs` implicit-EOF path verified) → far dispatcher's
|
||||
idle-read fires → loop breaks → `drop(plan)` (listener dies) →
|
||||
pumps awaited → handler task exits → wrapper's handler-exit
|
||||
teardown reaps the serving-side entry. Both directions (close's
|
||||
explicit `shutdown` sentinel and Drop's implicit path) tested; the
|
||||
leak-free guarantee asserted via `channel_ids()` on both managers.
|
||||
- **Per-accept isolation**: forwarded-open failure (FORBIDDEN /
|
||||
NOT_FOUND / correlation miss) drops only that accept; the loop
|
||||
serves the next (two tests pin queue-drain + no-stray-channels).
|
||||
- **Scope separation** proven structurally at the wire level in all
|
||||
three pairwise directions, at both the raw-call and typed-session
|
||||
layers.
|
||||
- **CF-005/CF-006 identity surfaces**: the establisher witnesses the
|
||||
END CALLER's identity on base/direct/bind-first paths (witness
|
||||
wrapper for bind-first, inline for the others); the forwarded op's
|
||||
opener-side gate sees the producer's per-call identity (the
|
||||
upstream per-call overlay — tested via the scope gate).
|
||||
- **Schema-gate ordering**: ACL → input_schema → establisher →
|
||||
handler, verified in upstream `registration.rs` (all three dispatch
|
||||
entry points) + pinned by the tests that assert the establisher's
|
||||
dial never runs on a malformed target.
|
||||
- **Doc-test parity**: both `compile_fail` asserts (no-Clone) live
|
||||
and green.
|
||||
|
||||
---
|
||||
|
||||
# Remediation
|
||||
|
||||
Ordered smallest-first; nothing here blocks alksocks composition.
|
||||
|
||||
1. **M-1** — the accept dispatcher's substrate fallback (log-and-return,
|
||||
or stamp the substrate into `BindFirstPlan`). Small; needs a
|
||||
failure-path test.
|
||||
2. **D-3, D-4** — rename/strengthen the unit-test helper; trim the
|
||||
CHANGELOG preamble. Trivial.
|
||||
3. **D-1, D-2** — sync wire.md's schema bounds + ADR-007's
|
||||
oneOf→if/then wording + the "establisher-time" →
|
||||
"registry-side-gate, serde-backstop" sentence. Doc-only but
|
||||
wire-contract-adjacent: do it before the first consumer ships.
|
||||
|
||||
---
|
||||
|
||||
# Coverage review (appendix — `cargo-llvm-cov` 0.8.4, line/region mode)
|
||||
|
||||
Measured with `cargo llvm-cov` (default) and `cargo llvm-cov
|
||||
--features local`; branch-level instrumentation needs a nightly
|
||||
toolchain (`-Z coverage-options=branch`), so these are line/region
|
||||
numbers — branch coverage is analyzed manually below where it matters.
|
||||
|
||||
## Headline numbers
|
||||
|
||||
| Configuration | Line coverage | Note |
|
||||
|---|---|---|
|
||||
| default | **95.63%** (74 missed of 1692) | the wire/protocol surface |
|
||||
| `--features local` | **93.66%** (133 missed of 2098) | adds `src/local/mod.rs` (84.0% lines) |
|
||||
|
||||
By file (default / `local`): `error.rs` 100/100, `params.rs`
|
||||
100/100, `wire.rs` 96.0/96.0, `producer.rs` 93.8/94.7,
|
||||
`consumer.rs` 90.5/90.5, `local/mod.rs` —/84.0.
|
||||
|
||||
The numbers are healthy for this suite's design (the integration tests
|
||||
drive the real paths over the harness duplex and — with `local` — real
|
||||
sockets); what remains uncovered is overwhelmingly **defensive
|
||||
early-return arms that no wired path can reach** plus a handful of
|
||||
genuine, cheap-to-close gaps. Classification follows.
|
||||
|
||||
## Genuine gaps (worth closing)
|
||||
|
||||
### G-1 — `TunnelSession::open`/`open_direct`/`TunnelListener::open` params-serialize error closures
|
||||
`src/consumer.rs:94-97, 134-137, 487-490` (and
|
||||
`open_reverse_channel`'s twin at `src/consumer.rs:593-594`). All four
|
||||
are `serde_json::to_value(..).map_err(..)` arms over types with no
|
||||
non-string keys or fallible serializers — unreachable in practice, and
|
||||
the established house shape for this conversion. Closing these needs a
|
||||
test-only serializer poison (`serde_json` can't fail on these types
|
||||
without `try_collect` tricks). **Recommendation: leave uncovered; do
|
||||
not chase.** If the suite ever grows a shared "unreachable error arm"
|
||||
prose note, these four are its members. (Alternative: collapse the four
|
||||
duplicated arms into one `fn params_to_input<T: Serialize>(..)` helper
|
||||
in `consumer.rs` — then only ONE closure exists and the gap shrinks by
|
||||
3. Cosmetic; fine to skip.)
|
||||
|
||||
### G-2 — `TunnelListener::close`'s `shutdown` arm never observed at the far end
|
||||
`src/consumer.rs:529-531`: `close()` runs
|
||||
`write.shutdown().await` (the explicit EOF sentinel) then reaps — but
|
||||
no test currently observes the FAR accept dispatcher's idle-read
|
||||
firing from a `close()` (the graceful path). The Drop path (implicit
|
||||
EOF → far teardown) IS observed end-to-end
|
||||
(`bindfirst_open_surfaces_reply_fields_and_teardown_cascades`), and
|
||||
the close path is pinned at the reap level
|
||||
(`close_and_drop_reap_the_listen_channel`) — but the sentinel's
|
||||
far-side effect for `close()` specifically is asserted nowhere: the
|
||||
"the pushed accept was never consumed" assertion runs only in the
|
||||
Drop-shaped tests. **Recommendation: one small test** — open against a
|
||||
bind-first producer, push an accept, `listener.close().await`, then
|
||||
poll that the far accept loop is gone (the same queue-len assertion
|
||||
the Drop tests use). This is the only gap that touches a *promised*
|
||||
behavior (ADR-008 §1's teardown contract, graceful half) with no
|
||||
direct observation. Cheap: mirror
|
||||
`bindfirst_full_flow…`'s harness with a `close()` instead of a drop.
|
||||
|
||||
### G-3 — `TunnelSession::join`'s pump-`Err` arm (`src/consumer.rs:368-373`)
|
||||
The `JoinHandle` error path (pump aborted → `JoinError` → warn + `(0,
|
||||
0)`) never fires in tests: `close()`/`Drop` abort the pump and reap
|
||||
without joining; `join()` is only ever exercised on live pumps. A
|
||||
one-line test: open a session, `pump_against`, `close().await` (aborts
|
||||
the pump), then… `join` consumes self — the shape needs a
|
||||
`pump_against` session dropped into a spawn that is aborted before
|
||||
`join`. Practical shape: `let s = session.pump_against(dup).await;`
|
||||
take the pump out via `s.join()` racing a `s.abort()`-equivalent —
|
||||
there is no public abort on the session, so the test should close the
|
||||
channel out-of-band (`manager.teardown_channel`) while `join()` is
|
||||
pending: the pump's copies error/EOF, `pump.await` still resolves Ok —
|
||||
actually the JoinError arm fires only on abort/cancel, which the
|
||||
session type never does before `join`. **Recommendation: leave the arm
|
||||
as-is (defensive, log-only) but note it in the review as
|
||||
unreachable-by-public-API** — the only way to hit it is a pump
|
||||
panicking, which is out of contract. Alternatively add a
|
||||
`#[cfg(test)]`-only unit test driving `join` on a session whose pump
|
||||
handle was manually aborted via a test seam — not worth the surface
|
||||
pollution on a warn-log arm.
|
||||
|
||||
### G-4 — `local/mod.rs` `UnixListenerHalves` accept-first loop never driven (591-609) + `TcpListenerHalves::accept_pair` downcast path in the *queue*-shaped tests
|
||||
`UnixListenerHalves::accept_loop` (src/local/mod.rs:591-607) has zero
|
||||
coverage: no test constructs it (the unix dial test hand-rolls its own
|
||||
echo listener; the bind-first unix test uses `accept_pair`). It is the
|
||||
unix parity twin of the covered `TcpListenerHalves::accept_loop`
|
||||
(hit at line 107-125) and its body is 15 lines of the same shape.
|
||||
**Recommendation: one small unix listen-producer test** (mirror
|
||||
`producer_listen.rs::accept_loop_stops_feeding_after_close_and_maps_the_drop`
|
||||
with `bind_unix` + a temp path) — closes the entire 593-607 range and
|
||||
exercises `UnixListenerHalves` beyond the bind-first path. Low value
|
||||
(copy-shaped), but it is the largest single uncovered block in the
|
||||
crate.
|
||||
|
||||
### G-5 — The unnamed-peer error arms (unix)
|
||||
`src/local/mod.rs:626-633` (the `accept_pair` unnamed-peer
|
||||
`handler_error`) and `src/local/mod.rs:578-583` (the
|
||||
`local_addr` unnamed-bind error) never fire: the unix bind-first test
|
||||
binds a named client socket deliberately (the parity case), and no
|
||||
test binds an abstract/autobind unix listener. Both are
|
||||
fail-loud guards the ADR pins (ADR-008 §3 — "a producer that cannot
|
||||
determine the peer does not serve a bind-first listener over that
|
||||
substrate"). **Recommendation: one compact test** — bind an abstract
|
||||
unix listener (Linux: a leading-`\0` path via socket2, or
|
||||
`UnixListener::bind` on an unnamed autobind socket), call
|
||||
`local_addr()` (expect the "no filesystem path" `handler_error`), and
|
||||
drive one autobind-client accept through `accept_pair` expecting the
|
||||
unnamed-peer error. Cheap, deterministic, and it pins the two ADR
|
||||
sentences the doc comments promise.
|
||||
|
||||
### G-6 — UDP bind-family resolve edge (`src/local/mod.rs:180-181`)
|
||||
`connect_udp`'s `no addresses` arm (the resolver returning an empty
|
||||
list). `connect_udp_unresolvable_target_maps_to_dial_failed` covers
|
||||
the `to_socket_addrs` Err arm; the empty-success arm needs a
|
||||
resolvable-but-zero-address name, which is not deterministically
|
||||
producible with std resolution. **Recommendation: leave uncovered**
|
||||
(documented as the std resolver's unreachable-in-practice arm) — or
|
||||
refactor `connect_udp` to take the resolution behind a small
|
||||
`ResolveFn` seam so the empty case is unit-testable. The seam is the
|
||||
better long-term shape (it also removes the blocking
|
||||
`to_socket_addrs` call from the async context — see N-8 below), so if
|
||||
this gap is addressed, do it via the seam.
|
||||
|
||||
### G-7 — `UdpHalf` `poll_read`/`poll_flush` readiness-error arms
|
||||
(`src/local/mod.rs:369-377, 395-396, 422-423, 427, 520-521`): the
|
||||
`poll_recv_ready`/`poll_send_ready` `Ready(Err)` arms and the
|
||||
high-water `Pending`-after-drain arm. The veth sender-block test
|
||||
(`udp_half_write_side_blocks_stashes_and_drains_on_resolve`) covers
|
||||
the stash/queue/resolve spine (lines 505-528 mostly hit; 519-521's
|
||||
`poll_send_ready` Pending arm never resolves Pending inside the test
|
||||
window because the veth's EAGAIN pattern resolves readiness between
|
||||
polls). These arms are one-poll-window states — deterministic capture
|
||||
needs a mocked readiness source, not a socket. **Recommendation:
|
||||
leave; the veth test already pins the observable behavior (stash →
|
||||
retry → deliver), and the arms are unreachable differently per-kernel
|
||||
(host-notes in the test document this).**
|
||||
|
||||
## Deliberately uncovered (defensive; no action)
|
||||
|
||||
- **Handler early-return arms** (`src/producer.rs:446-455, 759-764,
|
||||
1004-1005`): the `accept_bi`-failed / wrong-plan-payload /
|
||||
`try_unwrap`-failed telemetry returns. A wired open always has a
|
||||
plan of the right type; these fire only on a protocol bug or a
|
||||
mismatched registration (assembly error, not wire state). Covered in
|
||||
spirit by the unit-level downcast tests. To hit them, a test would
|
||||
need a handler registered against a plan the establisher never
|
||||
produced — an upstream-invariant violation, not a code path.
|
||||
- **`warn!`/`error!` telemetry bodies** (`src/producer.rs:825, 833,
|
||||
838-849`; `src/consumer.rs:368-373`): per-accept forwarded-open
|
||||
failure logging (the FORBIDDEN isolation test hits the *drop*
|
||||
branch's effects but the tracing body's region shows missed because
|
||||
the macro expands past the branch) and the loop-critical accept
|
||||
error arm — wait: **the loop-critical arm IS behaviorally covered**
|
||||
(`accept-loop-ended` via the queue_loop's closed-listener Err in
|
||||
the teardown-cascade tests); the missed lines 838-849 are the
|
||||
second `tracing::warn!` expansion inside that arm. Logging regions
|
||||
in macro expansion show as missed lines while the branch executes.
|
||||
Verified: `listen_channel_eof_cancels_loop_and_tears_listener_down`
|
||||
drives a loop-critical accept error. No action.
|
||||
- **`unreachable!` arms** (`src/wire.rs:113`,
|
||||
`src/consumer.rs:248, 328, 333`): guarded by invariants
|
||||
(`current` set before the `else` branch; data-plane XOR pump; the
|
||||
spent-session double-take the type makes impossible). Standard
|
||||
posture; the `debug_assert`-shaped gaps review 001 already passed.
|
||||
- **`wire.rs:184`** — the `datagram_split_across_awkward_chunks_reassembles`
|
||||
test's failure panic (the test always passes the reassembly).
|
||||
- **`Default for DatagramReader`** (`src/wire.rs:82-84`): derive-shaped
|
||||
boilerplate; all tests construct via `new()`. Trivial to close if
|
||||
wanted (`let _: DatagramReader = Default::default();` in an existing
|
||||
codec test) — otherwise delete the impl in favor of `#[derive(Default)]`.
|
||||
- **`src/params.rs` 99.3%** — the two "missed regions" are the
|
||||
`serde_json::from_value` error closures inside
|
||||
`parse_listen_reply`'s bound/listen arms; the `listen_reply_rejects_malformed_shapes`
|
||||
unit test exercises both paths (the macro-expansion region
|
||||
accounting again). Line-level: 100%. No gap.
|
||||
- **`Debug` impls** (`src/producer.rs:66, 109`): `TargetHandle`'s and
|
||||
`BindFirstPlan`'s manual `fmt`. Never invoked in tests (no
|
||||
`{:?}` on those types). One-line fix if desired: `assert_ne!` two
|
||||
plans in the token-discipline unit test (forces `Debug`), or drop
|
||||
the manual impls for `#[derive(Debug)]` with the opaque fields
|
||||
skipped (`#[derive(Debug)]` on `Arc<dyn Any…>` prints the pointer —
|
||||
the manual impl exists to keep the listener opaque). Keep the impls;
|
||||
optionally add a `debug_fmt_smoke` unit test.
|
||||
- **`witness_establisher`'s `None` arm** (`src/producer.rs:917`): the
|
||||
registration wrapper's `None => establisher` branch — every test
|
||||
registers with `Some(witness)`. One test registering
|
||||
`register_tunnel_bindfirst_openable` with `identity_witness: None`
|
||||
would cover it; trivial.
|
||||
|
||||
## Branch-coverage notes (manual, no nightly needed here)
|
||||
|
||||
The instrumented run is line/region only (stable; branch mode requires
|
||||
`-Z coverage-options=branch`). The manually-reviewed decision points
|
||||
and their test-pinned states:
|
||||
|
||||
- `AcceptQueue::pop`'s three arms — all covered (`producer_listen.rs`:
|
||||
empty-then-push, closed-and-empty, the lost-wakeup window test).
|
||||
- `UdpHalf::poll_read`'s staged-frame / fresh-recv / WouldBlock / error
|
||||
arms — all covered except the `poll_recv_ready` Err/Pending arms
|
||||
(G-7).
|
||||
- `UdpHalf::poll_write`'s high-water arm — covered by the veth test
|
||||
(Pending observed); the inner `Poll::Ready(Err)` return (422-423)
|
||||
uncovered (G-7's class).
|
||||
- `pump_queue_to_socket`'s WouldBlock/ENOBUFS stash arm — covered
|
||||
(veth test); the `poll_send_ready` triple (519-521) uncovered except
|
||||
via the flush-driven path (G-7).
|
||||
- `parse_listen_reply`'s malformed arms — covered (unit test);
|
||||
line-level 100%.
|
||||
- The idle-read `Ok(_)` arm (`src/producer.rs:792`) — same body as the
|
||||
`Ok(0) | Err(_)` arm; llvm-cov counts the second match arm's
|
||||
region separately. The write-happy-peer teardown shape is
|
||||
documented (N-2 in the main review) but untested — a test would
|
||||
write a byte to the idle channel and assert far teardown. Same
|
||||
harness shape as the EOF tests; cheap to add with G-2 if desired
|
||||
(it pins the "any non-empty read tears down" posture).
|
||||
|
||||
## Verdict
|
||||
|
||||
No coverage *gap* hides a correctness risk in the shipped paths: the
|
||||
uncovered set is defensive arms + two doc-promised fail-loud behaviors
|
||||
(G-5) + one ADR-contract half (G-2). The suite's integration-first
|
||||
design (real sockets with `local`, real forwarded-openable
|
||||
registrations, real EOF sentinels) is why the uncovered residue is
|
||||
this small. Recommended additions, in order of value: **G-2, G-5, G-4,
|
||||
G-7's `poll_send_ready` Err arm (skip — kernel-dependent), G-6 via the
|
||||
ResolveFn seam (optional), G-3 (document-only)**. Combined with M-1's
|
||||
failure-path test, that is roughly one session's worth of test work.
|
||||
|
||||
## References
|
||||
|
||||
- The six commits: `0789ab6` (direct op), `d586a5d` (forwarded params
|
||||
+ plan type), `7d23a89` (bind-first establisher + accept
|
||||
dispatcher), `3fb26d9` (forwarded serving + reply projection),
|
||||
`514e0c1` (local bind-first halves), `474844e` (`TunnelListener`)
|
||||
- ADR-007, ADR-008 (as amended), wire.md, consumer.md, producer.md
|
||||
- alkcall 0.8.0 surfaces this work rides: `open_channel_with_reply`,
|
||||
`Establishment::with_reply_field` + the reserved-key guard,
|
||||
flavor-form op ids (ADR-047 amendment 3), `pump_bidi` (ADR-050),
|
||||
the registry `input_schema` gate, the R-02 accepted-flag telemetry
|
||||
+ birth-teardown hint
|
||||
- Review 001 (the prior review; U-1's framed-adapter fix is
|
||||
re-verified here by the direct-op UDP round-trip test)
|
||||
Reference in new issue
Block a user