# Review 007 — Establishment Follow-Ups (from the alktunnels UDP POC) ## Status Resolved — all three units landed in alkcall 0.6.0 (2026-09-07): Unit 1 `dc4ad2b` (R-01 + R-02's doc notes), Unit 2 `8f122b0` (R-02's telemetry), Unit 3 `9c6fec1` (R-03 + ADR-050). Two deviations from the sketches below, both recorded in the ADRs: (1) R-01's plan is **typed-opaque** (`ChannelPlan = Arc`), not `Option` — the sketch could not satisfy this review's own verification gate, because the payloads establishers hand off are live handles (dialed sockets, TTY handles) with no JSON representation; (2) R-03's helper returns `(u64, u64)`, not `io::Result<(u64, u64)>` — both pumps swallow copy errors by the ADR-078 contract (mid-stream error = abrupt close), so an `Err` state would be dead code. Original findings below, retained as filed (verified against tree `36e74cd`, 0.5.0). Open — findings filed from the alktunnels UDP POC pass (`alktunnels-udp-poc`, 2026-09-06; summary at `/workspace/@alkdev/alktunnels/docs/research/poc-summary.md`). This review exists to prevent a second fix→publish→update-dependents cycle: every finding below is either (a) guaranteed to be needed by alktunnels Phase 1 implementation, or (b) a documented contract gap that will bite the next `OpenHandler` author the way it bit the POC. Nothing here is speculative — each finding was reached by writing working code against 0.5.0 and finding the shape insufficient. Findings continue the review numbering with prefix `R` (001–006 used P/C/R/A/B/C/D/F/G/E — each review numbers independently). > **Post-remediation note (2026-09-07):** the "Open —" paragraph > above is the original filing state, retained for the record. All > units landed; see Status above and ADR-049 amendment 2 + ADR-050. ## Scope The ADR-049 establishment surface (`OpenEstablisher`, `Establishment`, `EstablishmentError`, `register_openable_with_establisher`) and the `OpenHandler` contract, reviewed from the alktunnels UDP POC's consumer/producer implementations. Everything verified against source at tree `36e74cd` (0.5.0). Cross-references: alkcall review 006 (E-01..E-04), alkcall ADR-049, alktunnels `poc-summary.md` (§Issues Surfaced), alktty's channels establisher (`make_tty_establisher`). ``` Verified against: alkcall 36e74cd (0.5.0) Reading list: src/channels/operations.rs (OpenEstablisher, Establishment, run_open_wrapper, teardown_failed_channel), src/channels/client.rs (ChannelOpenError::establishment_reason), src/channels/manager.rs (teardown_channel, adopt_channel), docs/architecture/decisions/ 047 + 049, and the consumers: alktty src/channels.rs (make_tty_ establisher / make_tty_open_handler), alkhttp src/websocket/ upgrade.rs (OpenableAlpn ferry), alktunnels-udp-poc src/producer.rs ``` ## Severity legend Same scale as reviews 004–006. --- # Part A — The findings ## R-01 [major] — `Establishment` is payloadless, but the channel plan is exactly what establishers need to hand to the pump handler **Verified:** YES — by building against the API. The ADR-049 §1 text itself anticipates this: *"Reserved for a channel plan — today the wrapper consults only success/failure, so `()` carries no data"* (`src/channels/operations.rs:341-345`, the empty `pub struct Establishment {}`). ### The mechanism The establisher is pre-data-plane: it cannot see the channel's yield-once `BiStream` (ADR-049 amendment), so anything it establishes — a dialed socket, an allocated handle — must cross to the pump handler through a side channel the ALPN crate invents. The alktunnels POC's workaround is a resource-keyed `Mutex>` + a poll-loop `take` (`producer.rs::HandleHandoff`) that works only because the wrapper guarantees establisher-before-handler ordering. The costs: 1. **Concurrent same-resource opens race the slot.** Two opens of the same resource: the second establisher's `deliver` overwrites the first handler's not-yet-taken handle. The POC documents this as a simplification; a real crate cannot. 2. **alktty hit the same wall and documented it as a limitation.** `alktty/src/channels.rs:253-257`: the establisher cannot carry the allocated `TtyHandle` across, so *backend allocation* stays post-open in the pump handler (`allocate_failed` remains an in-band negotiation error frame — ADR-010) — exactly the phantom-channel-ish shape ADR-049 exists to eliminate, still alive one layer down. TTY's establisher can only validate/lookup/ ownership-check; the one thing that can actually fail with a runtime error (allocate) is unreachable from the typed error path. 3. **Every ALPN crate pays the side-channel tax again.** Handoffs, slots, poll loops or oneshots — per crate, per resource key, all with the same concurrency caveat. ### The ask Give `Establishment` its payload — the channel plan ADR-047 §3 originally described ("return a 'channel plan'... the wrapper consults its result"). Minimal shape: ```rust pub struct Establishment { /// ALPN-defined plan data — the dialed handle, an allocation /// ticket, whatever the pump phase needs. Opaque to alkcall. pub plan: Option, } ``` or, keeping it typed at the boundary: ```rust pub struct Establishment { pub plan: Option } ``` with the wrapper passing `establishment.plan` (or `Null`) to the `OpenHandler`'s `input` — e.g. as a well-known key the pump handler reads, or as a third callback parameter. The wire surface is **unchanged** (the plan is process-local: establisher → wrapper → handler in the same process on the producing side; nothing crosses the transport that isn't already the open op's input). Consumers' `Ok(Establishment {})` construction sites (alktty has two; alkhttp's ferry has none) break mechanically at 0.6 — `Establishment::new(plan)` / `Establishment::default()` make the migration one-liners. **The break is the point of doing this now:** 0.5.0 published yesterday with `Establishment {}` documented as reserved. The next consumer (alktunnels) needs the payload in Phase 1 — implementing Phase 1 without it means shipping the POC's side-channel handoff into the real crate, with its same-resource race, and then the payload lands later anyway as *another* breaking release. Filling the reserved field now is the cheap moment; the alternative is paying the breaking change twice. ### Severity [major] — the capability gap is structural for any establisher whose backend produces a handle (tunnels: dial; TTY: allocate), not a polish item. Not [critical] because workarounds exist (the POC proves one), but every workaround carries the same-resource race or forces failure classes back into per-crate in-band frames — the exact cost ADR-049 was written to remove. --- ## R-02 [minor] — The `OpenHandler` `JoinHandle` lifetime contract is undocumented and load-bearing (found empirically by the POC) The POC's first pump implementation returned a wrapper task that spawned the pump as a nested fire-and-forget task. The result: **every tunnel connected then instantly EOF'd with zero bytes** — the wrapper awaited the (already-complete) handler task, tore the channel down at birth, and both pumps saw immediate EOF. Everything upstream (establisher, open reply, channel routing) looked healthy; the bug was purely in the handler's return-value semantics. The contract, as implemented: the wrapper awaits the returned `JoinHandle` and *that completion is the teardown trigger* (`run_open_wrapper`'s spawned task: `let _ = raw_task.await;` → `teardown_channel(id)` → drop the demux sender → EOF to the handler's read half). Therefore: - **The returned `JoinHandle` must track the data-plane lifetime.** A handler that returns before its pumps finish tears the channel down at birth. The pump must be awaited *inline* inside the handler task (`let _ = pump_halves(...).await;`), not spawned-and-forgotten. - Half-open semantics fall out of this correctly (one pump finishing shuts down the opposite sink per ADR-078; the handler's task completes when both pumps finish — which is when teardown *should* happen). The contract is right; only its documentation is missing. The current type docs (`operations.rs:325-339`) say the handle is "recorded for teardown (abort on `channel/close` / connection drop)" — abort semantics — but never say **early return = teardown-at-birth**. alktty got this right by accident of shape (`drive_session_pre_negotiated` awaited inline in its single spawned task — `channels.rs:355-395`); the POC got it wrong the natural way. The next consumer will write the wrong shape too, because the natural reading of "spawn your protocol and return the `JoinHandle`" is a task-spawner, not a lifecycle promise. **The ask:** doc note on `OpenHandler` and `ChannelCore::register_openable*` — "the returned `JoinHandle` must track the data-plane lifetime: the wrapper awaits it and tears the channel down on completion; return a task that runs the handler to completion, never a spawner that exits early" — plus the ADR-049 amendment (§1 or §6) recording the semantics. Optional hardening (not required): a `debug!` in `run_open_wrapper` when the awaited handler exits without the channel's `BiStream` having been accepted (a telemetry hint for the birth-teardown pattern; the accept is observable in-process). Doc-only; no break. --- ## R-03 [minor, optional unit] — The two-pump helper's convergence test is satisfied; extracting it now removes the last reason for a later sweep alknet ADR-078 deferred helper extraction until a second two-pump consumer existed and the shapes converged. The POC supplies both halves of that test: - **Producer side:** the alktunnels POC's `pump_halves` — two `tokio::io::copy` pumps over split channel-vs-substrate halves, each pump shutting down the opposite sink on completion, joined. - **Consumer side:** `TunnelSession::take_halves` + the assembly layer's copy — the same shape modulo channel side. The shapes converged. The helper (`pump_bidi` or similar) is a candidate for **this same 0.6 sweep** — additive (a new pub fn in `channels` or `core`), zero breaking change, and it pins the ADR-078 contract in one place instead of three. The natural signature follows the POC: ```rust pub async fn pump_bidi(a: A, b: B) -> io::Result<(u64, u64)> where A: AsyncRead + AsyncWriteExt + Send + Unpin, B: AsyncRead + AsyncWriteExt + Send + Unpin, ``` (join'd two-pump with shutdown-on-completion; returns the copy counts for observability). alktty's three-pump session does not fit it (the exit future is a third signal) — that is fine; the helper serves the two-pump shape, TTY stays as-is. **The ask:** decide in this sweep — either extract in 0.6 (additive; recommended, since alktunnels Phase 1 will implement the pattern anyway and an upstream helper makes the third consumer free), or record the explicit decision to keep it per-crate. Do not leave it half-decided: an un-extracted helper is not itself a break, but finding this out during Phase 1 would be the same fix→publish→update treadmill for a purely additive change. --- # Part B — Verified non-issues (bounded so the next review doesn't re-check) - **The reverse-flow (`-R`) story needs no upstream change.** A hub proxy opening a channel *toward* a worker is the worker serving its own open op on the connect side — supported by `ChannelClient::from_connection_with_serving` (ADR-022 §2 both-sides semantics) + `ChannelOperations::register_on` on the serving registry + the wrapper allocating on the serving side (ADR-047 §5 "the side that holds the `ChannelManager` allocates" — both sides do, with odd/even split per ADR-047 §5). Verified by trace; no API gap. alktunnels' reverse-flow POC (OQ-TN-10 #2) will exercise this end-to-end, but no upstream mechanism is missing. - **The establisher receives registry-validated input** — `invoke_streaming`'s `input_schema` check runs before the wrapper (`registration.rs:397`); the establisher's `input.clone()` (`operations.rs:718`) is post-schema. Confirmed; no gap. - **Typed establishment errors are complete on the wire** — `establishment_error_to_call_error` carries both `reason` and `message` in `details` (`operations.rs:646-653`); the client-side `establishment_reason()` branches on it (`client.rs:75-83`); alktty's open spec declares the matching `ErrorDefinition` (ADR-016). The vocabulary needs no extension for tunnels (`dial_failed` / `unknown_resource` / `resource_shortage` / `handler_error` / `timeout` cover every tunnel establisher failure class — verified by the POC's three typed-error tests). - **The early-arrival park cap (64)** — reviewed in 006 E-04; the POC treated it as a sizing constraint, not a defect. Unchanged. - **`EstablishmentError`'s reason set is sufficient** — the POC's establisher used three of four reasons; `handler_error` covers params-parse failures (the TTY pattern). No new reason needed. --- # Remediation plan ## Unit 1 — `Establishment` carries the channel plan (R-01) 1. ADR-049 amendment (or ADR-050-adjacent amendment): `Establishment` gains `plan: Option` (ALPN-opaque); the wrapper threads `plan` into the pump handler (third `OpenHandler` parameter, or merged into `input` under a reserved key — decide in the ADR; the separate-parameter shape avoids colliding with the input schema). 2. Bump to 0.6.0 (breaks `Ok(Establishment {})` construction sites — alktty, two sites; mechanical `Establishment::default()` or `Establishment { plan: None }`). 3. Migration: alktty can then move backend `allocate` into its establisher (killing the in-band `allocate_failed` frame on the channels path — ADR-010's reason for that frame disappears); alktunnels Phase 1 uses `plan` for the dialed handle; alkhttp's ferry passes `Option` through unchanged. ## Unit 2 — `OpenHandler` lifetime doc note (R-02) Doc-only (type docs + ADR-049 amendment). Optional debug warning. No break. ## Unit 3 (optional) — `pump_bidi` helper extraction (R-03) Additive pub fn + tests. No break. Do it in the same 0.6 or record the decision not to. ## What must NOT ride along Nothing else. The remaining alktunnels Phase 1 needs (UDP codec ADR, reverse-flow lifecycle, access-policy details) are alktunnels-local spec work — the POC validated the mechanics and found no further upstream gaps. The deliberate goal of this review: **0.6 is the last breaking sweep forced by known work**; anything after it should be a genuinely new discovery, not a known-dangling item. ## Verification gates - Unit 1: end-to-end test — establisher dials, `plan` carries the handle, pump handler receives it; concurrent same-resource opens each get their own handle (the race the POC documented is unreachable); alktty migration test (`allocate` in the establisher → `allocate_failed` is a call error, no in-band frame). - Unit 2: doc gate only (`cargo doc` clean; the note present on both the type and the registration entry points). - Unit 3: helper test — the POC's two-pump semantics (EOF from one direction completes the other; shutdown-on-completion) reproduced through the helper. ## References - alkcall review 006 (E-01 establishment gap; R-01 is the payload half of E-01's own remediation — ADR-047 §3's "channel plan" through the reserved `Establishment` field) and ADR-049 (the establishment phase; §1's original sketch already described the plan-carrying shape). - alkcall ADR-047 §3 ("return a 'channel plan'" — the original shape this review restores), ADR-022 §2 (both-sides serving — the reverse-flow non-finding), ADR-047 §5 (odd/even allocation — reverse-flow allocation on the serving side). - alknet ADR-078 (the two-pump contract; R-03's helper pins it upstream). - alktunnels `docs/research/poc-summary.md` (§Issues Surfaced #1/#2 — the empirical findings; §#4 — the convergence input for R-03) and the POC crate (`/workspace/alktunnels-udp-poc`, 17 tests — the empirical basis for every finding here). - alktty `src/channels.rs:253-257` — the existing documentation of the R-01 cost on the TTY side (`allocate` cannot cross; in-band `allocate_failed` remains), the second consumer confirming the gap is not tunnel-specific.