alkcall/docs/reviews/007-establishment-follow-ups-review.md

# 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<dyn Any + Send + Sync>`), not
`Option<Value>` — 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<HashMap<String, SubstrateHandle>>` + 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<Value>,
}
```

or, keeping it typed at the boundary:

```rust
pub struct Establishment { pub plan: Option<Value> }
```

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, B>(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<Value>` (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.