From 6590ab005f912deeb433d9571b690fa17504f847 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Mon, 7 Sep 2026 08:18:11 +0000 Subject: [PATCH] =?UTF-8?q?docs:=20review=20007=20=E2=80=94=20establishmen?= =?UTF-8?q?t=20follow-ups=20(from=20the=20alktunnels=20UDP=20POC)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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, 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). --- .../007-establishment-follow-ups-review.md | 325 ++++++++++++++++++ 1 file changed, 325 insertions(+) create mode 100644 docs/reviews/007-establishment-follow-ups-review.md diff --git a/docs/reviews/007-establishment-follow-ups-review.md b/docs/reviews/007-establishment-follow-ups-review.md new file mode 100644 index 0000000..83ff034 --- /dev/null +++ b/docs/reviews/007-establishment-follow-ups-review.md @@ -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>` + 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. \ No newline at end of file