docs: review 007 — establishment follow-ups (from the alktunnels UDP POC)
Findings filed to prevent a second fix->publish->update-dependents cycle; each was reached by building working code against 0.5.0: - R-01 [major] — Establishment is payloadless but the channel plan is exactly what establishers need to hand to the pump handler (ADR-049's own 'reserved for a channel plan' note). Costs verified in two consumers: alktunnels POC side-channel handoff (same-resource opens race the slot), alktty forced to keep backend allocate post-open in-band (allocate_failed stays an in-band frame — the shape ADR-049 eliminates, alive one layer down). Ask: fill the reserved field (plan: Option<Value>, process-local, wire unchanged) in a 0.6.0 sweep; the break is mechanical (Establishment::default). - R-02 [minor] — the OpenHandler JoinHandle lifetime contract is undocumented and load-bearing: the wrapper's await of the returned handle IS the teardown trigger; a handler that returns before its pumps finish tears the channel down at birth (the POC found this empirically — every tunnel EOF'd instantly). Doc note + ADR-049 amendment; optional debug warning. - R-03 [minor, optional] — the ADR-078 two-pump helper's convergence test is satisfied (POC gives both shapes); extracting pump_bidi now is additive (no break) and pins the contract upstream. Decide in the same 0.6 sweep. Non-findings: reverse-flow (-R) needs no upstream mechanism (from_connection_with_serving + serving-side allocation, verified by trace); establisher receives registry-validated input; typed establishment errors complete on the wire; early-arrival cap unchanged; EstablishmentError reason set sufficient. Goal stated in the review: 0.6 is the last breaking sweep forced by known work. alkcall tests: 617 passed (docs-only change; baseline check).
This commit is contained in:
glm-5.3-flash committed
1 parent
36e74cda11
commit
6590ab005f
1 file changed
+325
@@ -0,0 +1,325 @@
|
||||
# Review 007 — Establishment Follow-Ups (from the alktunnels UDP POC)
|
||||
|
||||
## Status
|
||||
|
||||
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).
|
||||
|
||||
## 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.
|
||||
Reference in new issue
Block a user