alktunnels/tasks/tunnels/review-002-m1-substrate-fallback.md

id, name, status, depends_on, scope, risk, impact, level, tags

id	name	status	depends_on	scope	risk	impact	level	tags
tunnels/review-002-m1-substrate-fallback	M-1 remediation — the accept dispatcher's silent substrate fallback (stamp the plan, fail loud)	completed		narrow	low	component	implementation	producer bind-first review-002 review-remediation

Description

Review 002 finding M-1 (docs/reviews/002-graduation-implementation-review.md §M-1): the bind-first accept dispatcher's substrate extraction (src/producer.rs:766-768, inside make_bindfirst_accept_handler) silently falls back to Substrate::Tcp when the second serde_json::from_value::<TunnelParams> parse fails:

let substrate = serde_json::from_value::<TunnelParams>(input.clone())
    .map(|p| p.substrate)
    .unwrap_or(Substrate::Tcp);

In the wired path the fallback is unreachable (the registry schema gate and the establisher's parse_params both validate first), but the shape itself is wrong: a future drift would silently stamp 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's U-1 was about), degrading silently instead of loudly.

The fix (decide deliberately, then implement)

The review offers two shapes; the second is recommended (the review calls it strictly better, and the pre-consumer window is open — BindFirstPlan has no external consumer yet):

1. Log-loud-and-return on parse failure (teardown-at-birth — the same posture as every other early return in the file). Smaller, but keeps the second parse and the fallback branch.
2. 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 establisher already parsed the params successfully; the substrate is per-open-static; the plan is already the per-open carrier. BindFirstPlan gains a substrate: Substrate field (a plan-payload change — cheap now, pre-consumer); the dispatcher reads it from the plan it already downcast.

If shape 2: the establisher's construction site (bindfirst_establisher) carries the parsed substrate into the plan; the per-dispatcher token-discipline test continues to pass; check the Debug impl for BindFirstPlan still compiles (opaque listener field) and the doc comment on the new field states why it rides the plan (the dispatcher must not re-parse wire input).

Tests

• Keep the existing suite green (the two-open token-discipline test pins per-dispatcher stamping from its own plan — it now also pins the substrate's origin implicitly).
• A failure-path test: with shape 2 there is no failure branch left in the dispatcher (assert the removed fallback is gone — e.g. the forwarded opens for a UDP bind-first listen carry substrate: udp from the plan, not a re-parse). With shape 1: a malformed-input accept teardown test (loud return, channel reaps, no phantom channel).

Acceptance Criteria

• The silent unwrap_or(Substrate::Tcp) fallback is gone from make_bindfirst_accept_handler
• The chosen shape documented in this task's Notes (plan-stamp vs log-and-return, with the rationale)
• A test pins the forwarded-op substrate source (plan-stamped, or the loud teardown on parse failure)
• cargo test green; clippy --all-targets -D warnings clean; fmt clean; wasm32 check + clippy clean

References

• docs/reviews/002-graduation-implementation-review.md §M-1 (the finding, both fix shapes, and why not fixed inline)
• src/producer.rs — BindFirstPlan (~line 97), the establisher, and make_bindfirst_accept_handler (~line 755)
• tasks/tunnels/bindfirst-producer.md (the original implementation task + Notes on the plan's shape)

Notes

Agent fills during implementation.

Chosen shape: 2 — stamp the plan (the review's recommendation). Rationale: one parse (the establisher's parse_params — which rejects malformed input with handler_error before any channel exists), one source of truth; the substrate is per-open-static and the plan is already the per-open carrier; the fallback branch is not just guarded against, it is REMOVED — a future drift cannot degrade silently because there is no fallback to degrade into. Pre-consumer window used (BindFirstPlan has no external consumer yet — a plan-payload change is cheap).

Implemented:

• BindFirstPlan gains substrate: Substrate (doc comment states why it rides the plan — the dispatcher must not re-parse wire input; the removed fallback's data-plane mismatch risk is cited). The Debug impl stamps it (the opaque listener field stays).
• bindfirst_establisher stamps the parsed params.substrate into the plan at construction time.
• make_bindfirst_accept_handler: the second serde_json::from_value::<TunnelParams> parse and the unwrap_or(Substrate::Tcp) fallback are gone; the dispatcher reads plan.substrate from the plan it already downcast. The handler's input param is now unused (_input).

Tests:

• Existing suite kept green — the two-open token-discipline test (token_discipline_two_opens_distinct_tokens) still pins per-dispatcher stamping from its own plan.
• bindfirst_plan_downcasts_through_channel_plan (unit): the plan's substrate rides the downcast (Substrate::Udp constructed → recovered).
• bindfirst_establisher_flow_end_to_end (unit): the establisher stamps the parsed substrate into the plan (Tcp input → plan.substrate == Tcp).
• forwarded_opens_carry_plan_stamped_substrate (integration, the M-1 pin): a UDP bind-first listen over the wire — the forwarded open the dispatcher makes carries substrate: udp (the plan's stamp), observed through a substrate-recording forwarded-op stub on the opener side. The removed fallback would have stamped Tcp; the assert is vec![Substrate::Udp].

Docs synced: docs/architecture/producer.md (§3 — the plan carries {listener, token, substrate}; §The Forwarded Op item 1 — the substrate also comes from the plan, never re-parsed), docs/architecture/decisions/008-listen-metadata-forwarded-op.md (§1 — the plan's substrate stamp + the removed fallback, review 002 M-1 referenced).

Verification (all green): cargo test (145 tests across the lib + 10 integration binaries), cargo test --all-features (153), cargo clippy --all-targets -- -D warnings, cargo clippy --all-targets --all-features -- -D warnings, cargo fmt --check, cargo doc --no-deps, cargo check --target wasm32-unknown-unknown + cargo clippy --target wasm32-unknown-unknown -- -D warnings.