diff --git a/docs/architecture/decisions/007-direct-open-op.md b/docs/architecture/decisions/007-direct-open-op.md index 86f686a..5907493 100644 --- a/docs/architecture/decisions/007-direct-open-op.md +++ b/docs/architecture/decisions/007-direct-open-op.md @@ -161,6 +161,25 @@ sentence (also ADR-006 amendment). are breaking. Additive evolution: a new substrate is a new `substrate` value + a new target key (never a format change); new fields inside a target object must be optional-with-default. +- **`SubstrateAddr`'s untagged leniency is schema-gated** (review 002 + N-4): untagged serde variants cannot `deny_unknown_fields`, so a + cross-shape target (e.g. `{path, host}`) parses at the serde layer — + the SCHEMA's `additionalProperties: false` per branch is the real + shape-vs-substrate gate. One caution for future registry variants: a + schema-less registry would leave the establisher's serde backstop as + the only gate, and it keys `render_target` off the sibling + `substrate` — a cross-shape target that sneaked past would render + *something* (`/p` for the unix case) rather than fail; the serde + backstop renders the sibling-keyed target, so cross-shape garbage is + the schema's absence, not a serde failure. If this crate is ever + consumed against a schema-less registry variant, pin an ADR line + there before the first such consumer. +- **`SubstrateAddr` must not grow overlapping untagged variants.** If + a future variant's shapes overlap (e.g. a `{host, port, flowlabel}` + form), untagged disambiguation starts ordering (first-match wins + becomes load-bearing and silent) — switch to an internally-tagged + or untagged-with-content form at that point, deliberately (an ADR + line), never by accident. Pre-consumer and additive until then. - **The BAST question is moot** — params is JSON inside the channels open op; the data plane is unchanged (raw pass-through for tcp/unix, the `[len: u16 BE]` codec for udp — ADR-003). diff --git a/docs/architecture/decisions/008-listen-metadata-forwarded-op.md b/docs/architecture/decisions/008-listen-metadata-forwarded-op.md index 32595fc..a13092c 100644 --- a/docs/architecture/decisions/008-listen-metadata-forwarded-op.md +++ b/docs/architecture/decisions/008-listen-metadata-forwarded-op.md @@ -81,7 +81,15 @@ accept-first shape (which stays — see §5): and runs the assembly-provided accept loop concurrently — each accepted connection produces a forwarded channel (§3); the channel's own `BiStream` stays idle and its EOF (the opener dropping the - listener session) cancels the loop and tears the listener down. + listener session) cancels the loop and tears the listener down. The + idle-read check is deliberately WIDER than a bare EOF comparison + (review 002 N-2): the watch treats ANY read as teardown — `Ok(0)` + (the EOF sentinel), an error, **and** any non-empty read (a peer + that writes to the idle listen channel is misbehaving; the channel + is not a data plane — fail closed). An implementer reading only + "its EOF cancels the loop" and checking `Ok(0)` alone would hang on + a write-happy peer; the shipped posture is the any-byte one, + pinned by the write-happy-peer test. ### 2. The `bound` reply field (upstream ask) diff --git a/src/producer.rs b/src/producer.rs index 13e9436..c7b159b 100644 --- a/src/producer.rs +++ b/src/producer.rs @@ -589,7 +589,11 @@ pub struct BoundListener { /// non-zero-seeded `AtomicU32` counter held in the establisher's /// closure state — fresh per open, unique per producer, wraps /// acceptable (a wrap collision requires 2^32 concurrent opens on one -/// producer). The counter seeds at 1: `fetch_add` returns the +/// producer — and that bound is loose: the token is +/// per-producer-CONNECTION and each open rides one connection, so the +/// alkcall 256-channels-per-connection cap makes a live-collision +/// window unreachable by a wide margin, review 002 N-5). The counter +/// seeds at 1: `fetch_add` returns the /// PREVIOUS value, so the first mint is 1 and the minted stream is /// 1, 2, 3, … — token 0 (an illegal wire value; the forwarded params' /// `listen` pin rejects it) is never minted, and a u32 wrap that lands @@ -988,6 +992,16 @@ pub type ForwardedHandler = /// keeps the R-02 accepted-flag telemetry truthful), and hand the /// parsed params + the stream to the injected [`ForwardedHandler`]. /// +/// **Parse-before-accept ordering (the R-02 telemetry note):** the +/// params parse happens BEFORE `conn.accept_bi()`, so for a +/// malformed-params open the R-02 accepted-flag telemetry reads +/// `false` — by design. A malformed-params open IS a birth-teardown +/// shape (the exact case the telemetry's birth-teardown hint exists +/// for); accepting first would hold a stream open for a params set +/// the producer already got wrong. The hint firing here is correct, +/// not noise — future alkcall reviewers: do not flag the flag +/// (review 002 N-3). +/// /// **Pump responsibility split (the honest-shape decision):** the /// injected handler receives the channel `BiStream` and owns the pump /// — `pump_bidi(forwarded_bidi, local_read, local_write)` — because diff --git a/tasks/tunnels/review-002-recorder.md b/tasks/tunnels/review-002-recorder.md index 9279cfb..4eefe2b 100644 --- a/tasks/tunnels/review-002-recorder.md +++ b/tasks/tunnels/review-002-recorder.md @@ -1,7 +1,7 @@ --- id: tunnels/review-002-recorder name: "N-1..N-7 — record the review's no-action notes in the code/ADR trail (N-4's ADR line + N-3's telemetry note)" -status: pending +status: completed depends_on: [] scope: narrow risk: trivial @@ -111,4 +111,69 @@ G-1's four duplicated closures into one `params_to_input` helper ## Notes -> Agent fills during implementation. \ No newline at end of file +> Agent fills during implementation. + +## Implementation Notes (2026-09-20) — the review's no-action dispositions, recorded + +The two durable-trail items landed as specified; the no-action notes' +dispositions follow (this section is the tracker the task asked for). + +### Landed + +- **N-3**: the parse-before-accept telemetry sentence added to + `make_forwarded_op_handler`'s doc comment + (`src/producer.rs`) — "the accepted-flag telemetry reads false here + by design; a malformed-params open IS a birth-teardown shape; do + not flag the flag (review 002 N-3)". +- **N-4**: both ADR-007 Consequences lines added — (1) the + `SubstrateAddr` untagged-leniency-is-schema-gated paragraph (the + sibling-keyed `render_target` backstop; "cross-shape garbage is the + schema's absence, not a serde failure"; the schema-less-registry + ADR-line-if-ever requirement) and (2) the overlapping-variants + caution (untagged disambiguation starts ordering → switch to an + internally-tagged form, deliberately, never by accident). +- **N-2 (ADR-008 §1 widening — APPLIED, not declined)**: §1's + accept-dispatcher bullet now states the any-byte idle-read posture + (`Ok(0)` / error / any non-empty read all tear down; fail closed; + the write-happy-peer test pins it) and names the `Ok(0)`-only + hang-on-write-happy-peer trap the narrower reading would produce. + Note: the doc-sync task and this task both touched ADR files but + never ADR-008, so no coordination conflict materialized. +- **N-5 (optionally tighten — done)**: `mint_token`'s doc comment now + records the review's cap analysis (the token is + per-producer-connection; the 256-channels-per-connection cap makes + the live-collision window unreachable by a wide margin). + +### No-action dispositions (unchanged from the review) + +- **N-1**: no action. `parse_listen_reply`'s positive-`listen` + requirement matches the forwarded params' `minimum: 1`; JSON-null- + as-absent is the standard lenient reading; the `deny_unknown_fields` + warning in `TunnelListenReply`'s doc comment is doing real work — + kept as-is. +- **N-2's code**: no action (fail-closed any-byte watch is correct as + shipped; only the ADR sentence was widened, above). +- **N-6**: no action. Sleep-then-assert is the established house + pattern (3x-repeat stable historically); if CI ever flakes on the + forwarded-arrival tests, the fix is a bounded poll loop on + `channel_ids()` extended to the arrivals — recorded here, not + implemented. +- **N-7**: no action. `test_bind()`'s bare `Arc` listener is + fine; the downcast-miss probe + (`accept_pair_loop_downcast_miss_fails_loud`) covers the failure + mode. +- **G-1/G-3 (the unreachable-by-construction arms)**: left uncovered, + per the review's recommendations — `open`/`open_direct`/ + `TunnelListener::open` (+ `open_reverse_channel`'s twin) + params-serialize closures (serde_json cannot fail on these types) + and `join`'s pump-`Err` arm (unreachable through the public API; the + only path in is a panicking pump, out of contract). This section is + the "unreachable error arm" record the coverage appendix asked for; + the optional `params_to_input` helper collapse was skipped + (cosmetic; the review says fine to skip). + +### Verification + +- `cargo fmt --check`; `cargo clippy --all-targets -D warnings` + (native + wasm32); `cargo test` (all suites + doc-tests); + `cargo doc --no-deps` — all green. \ No newline at end of file