docs: review 006 — channel-open establishment gap (from alktunnels phase 0)

Design review from the alktunnels Phase 0 research pass, verified
against tree a22b2b8 (0.4.1). Findings numbered E-01..E-04:

- E-01 [major] — the open op cannot fail after allocation: the
  wrapper replies {channel_id} the moment the OpenHandler is spawned;
  establishment failures (params-valid-but-rejected, backend lookup
  failure, target dial failure) present to the consumer as a
  successful open followed by an instant, indistinguishable clean
  EOF (implicit-EOF mux path + unified poll_read EOF arms). SSH
  semantics (RFC 4254 §5.1 open-failure reply with reason codes;
  channel never exists opener-side), SOCKS5 reply codes, and
  udpgw's opaque ERR bit (counterexample) surveyed in
  alktunnels/docs/research/ssh-socks5-survey.md. alktty's in-band
  error-frame mechanism (send_negotiation_error, 0x00-peek) is the
  per-crate workaround this upstream establisher obsoletes for the
  channels path. Proposed shape: an awaited establishment hook
  (OpenEstablisher) or await-and-inspect OpenHandler, tearing down on
  failure and replying channel:open_failed with SSH-four reason codes
  in ADR-016 details. Remediation sketch + verification gates
  included.
- E-02 [minor] — services/list discloses no per-op metadata; OQ-40
  (channel/resources/subscribe) becomes load-bearing for the first
  time via the alktunnels discovery resolution (OQ-TN-08).
- E-03 [minor] — OpenHandler-exit vs channel/close teardown race is
  benign (ledger take is the gate) but the let _ = discard at
  operations.rs:509 is silent; recommend log-or-comment.
- E-04 [minor] — early-arrival park cap (64) is an observable bound
  for push-first producers under slow adopters; no change requested,
  filed so the constraint is visible to the next consumer.

Non-findings recorded: open-op ACL path complete across all three
dispatch entry points; input_schema enforcement covers open params;
EOF arms unified; channel_open marker + resource_id_path wire
round-trip intact; opener-ledger decrement atomic at every call site.

alkcall tests: 597 passed (docs-only change; baseline check).
This commit is contained in:
2026-09-06 09:54:13 +00:00
parent a22b2b84c9
commit 88e3f5e9c3
@@ -0,0 +1,377 @@
# Review 006 — Channel-Open Establishment Gap (from alktunnels Phase 0)
## Status
Open — findings filed from the alktunnels Phase 0 research pass
(2026-09-06). This is a design review, not a code-defect review: the
establishment gap (E-01) is real, POC-observable, and load-bearing for
the next downstream crate; the remaining findings are smaller
mechanism/coverage gaps noticed in the same sweep.
Findings continue the review numbering with prefix `E` (001005 used
P/C/R/A/B/C/D/F/G — each review numbers independently).
## Scope
The channels open-op path (`run_open_wrapper` and the `OpenHandler`
contract) reviewed from the perspective of the next consumer crate
(alktunnels — arbitrary TCP/UDP tunnels over channels), cross-checked
against the two existing consumers (alktty, alkhttp) and the SSH
forwarding prior art recorded in
`/workspace/@alkdev/alktunnels/docs/research/ssh-socks5-survey.md`.
Everything below was verified directly in source at tree `a22b2b8`
(0.4.1 + the early-arrival park fix). No code changes were made in
this repo by this review.
```
Verified against: alkcall a22b2b8 (0.4.1)
Reading list: src/channels/operations.rs (run_open_wrapper, OpenHandler,
make_open_handler_once/stream/sink), src/channels/manager.rs
(open_channel, teardown_channel, route_payload, early-arrival park),
src/channels/reassembly.rs (MpscSendStream::poll_shutdown, Drop,
MpscRecvStream::poll_read EOF arms), src/channels/mux.rs (implicit-EOF
pump path), src/protocol/wire.rs (CallError), src/registry/
registration.rs (invoke/invoke_streaming gates), src/registry/
discovery.rs (services/list, spec_to_json_pub), src/client/from_call.rs
(rebuild_spec_for), docs/architecture/decisions/047,
docs/architecture/channel-operations.md
```
## Severity legend
Same scale as reviews 004/005:
- **[critical]** — a decided spec invariant is violated in a way that
makes a promised capability unreachable end-to-end; or corrupts data.
- **[major]** — a core protocol path cannot serve a decided behavior;
works only via shapes the spec does not describe.
- **[minor]** — drift, doc/spec inconsistency, or a missing
convenience with no correctness impact.
---
# Part A — The open-op establishment gap
## E-01 [major] — The open op cannot fail after allocation: the consumer receives a live channel for a tunnel whose establishment failed, with no error channel
**Verified:** YES, by code trace and mechanism analysis.
### The mechanics
`OpenHandler` (ADR-047 §3) is the ALPN crate's hook for "validate
params, consult ownership, prepare the backend" — i.e., the
establishment phase of a channel. But the wrapper does not await it:
1. `run_open_wrapper` (`src/channels/operations.rs:483-528`):
`policy.check_open``manager.open_channel(alpn, opener_id, None)`
→ build the channel `Connection``open_handler(input, channel_conn,
auth)` **spawns** the handler and collects its `JoinHandle`
`ResponseEnvelope::ok(request_id, json!({ "channel_id": channel_id }))`
(`operations.rs:503-527, 537`). The reply is written to the wire the
moment the handler task is *spawned*, not when the handler has done
its establishment work.
2. The `OpenHandler` type (`operations.rs:333-334`) returns
`tokio::task::JoinHandle<()>` — there is no result, no error variant,
no establishment phase the wrapper can consult. Any failure inside
the handler (params valid at the schema level but semantically
rejected, backend lookup failure, target dial failure for a
`direct-tcpip`-shaped tunnel, resource no longer available) is
invisible to the open op's reply.
3. What the consumer observes on handler-side failure: the call op
**succeeds** with `{channel_id}`, the channel is adopted
(`ChannelClient::open_channel`, `src/channels/client.rs:227-250`),
and then the channel EOFs — the handler wrote nothing before
exiting, the mux pump writes the implicit-EOF chunk on receiver end
(`src/channels/mux.rs:78-81`, REQ-CH-01 implicit-EOF path), and
`MpscRecvStream::poll_read` returns clean EOF for both the sentinel
and sender-drop arms (`src/channels/reassembly.rs:111-135`). A
dial-failure EOF is byte-for-byte indistinguishable from a target
that closed immediately after connecting — the two most different
failure/success stories map to the same consumer-visible event.
### Why this is the wrong shape (SSH's semantics, prior-art checked)
Every established tunnel/forwarding protocol puts establishment
failure in the open reply, not in the data stream:
- **SSH** (RFC 4254 §5.1): `SSH_MSG_CHANNEL_OPEN_FAILURE` is a
first-class reply to the open, carrying a reason code
(`ADMINISTRATIVELY_PROHIBITED` / `CONNECT_FAILED` /
`UNKNOWN_CHANNEL_TYPE` / `RESOURCE_SHORTAGE`) plus a description
string; the channel never exists on the opener's side afterward
(verified end-to-end in russh:
`src/server/encrypted.rs:1244-1278`, `src/client/encrypted.rs:414-434`
— see `/workspace/@alkdev/alktunnels/docs/research/ssh-socks5-survey.md`
§"Open-failure path").
- **SOCKS5** (RFC 1928 §6): the reply carries REP codes 0x010x08 and
the connection closes within 10s on failure — error-then-close, never
"success then in-stream error."
- **udpgw** (`tun2proxy/src/udpgw.rs:21-26`) is the counterexample: an
opaque ERR bit with zero reason information — the survey flags it as
the vocabulary to avoid.
- **alktty was forced to reinvent the missing mechanism in-band.** The
direct-ALPN path answers the negotiation with a length-prefixed JSON
error frame *on the stream* (`send_negotiation_error`,
`alktty/src/adapter.rs:177-187`; the `0x00`-prefix first-byte
disambiguation trick, `alktty/docs/architecture/tty-adapter.md:186-197`),
and the channels path retains the same error-frame shape even though
the open op's `input` is the negotiation (alktty ADR-009) — because
post-allocation failures have nowhere better to go. That is
per-crate reinvention of a protocol-level capability every ALPN
crate will need: a structured, typed, **establishment-failure reply
to the open op**.
### The cost today, concretely
- A consumer cannot distinguish "ACL denied" (call error,
`channel:forbidden` — never allocated) from "dial refused" (open
succeeded, instant EOF) from "target accepted then instantly closed"
(open succeeded, instant EOF). Retry policy, client UX, and error
reporting are impossible on the second and third.
- ADR-016's typed error details (`CallError.details`) — which the
wrapper already uses for `channel:too_many_channels` with
`{count, max}` details (`operations.rs:530-545`) — cannot carry
dial-failure reasons.
- The alktty in-band error-frame path exists *only* because the
wrapper can't fail the open post-allocation; every future ALPN crate
faces the same fork: reinvent an in-band error vocabulary or
silently-EOF.
### The ask (proposed shape, for the ADR — not a prescriptive API)
Give the open-op wrapper an **establishment phase it awaits before
replying**. Minimal, backward-compatible shape:
1. `OpenHandler` gains an establishment result. Two candidate shapes:
- **Split the hook**: `OpenEstablisher` (async, awaited by the
wrapper — validates params semantically, prepares/dials the
backend, returns `Result<Establishment, EstablishmentError>`)
followed by `OpenHandler` (spawned on the established channel, as
today). The dial is the natural establisher step for tunnels; the
pumps remain the spawned handler.
- **Await-and-inspect**: keep the single `OpenHandler` returning
`JoinHandle<OpenResult>`; the wrapper awaits a bounded
establishment phase (a `JoinHandle::timeout` equivalent — select
on the handle vs an establishment deadline) before replying. The
handler signals "established, continue" via an agreed value
(e.g. the handler resolves a first `Result<(), HandlerError>`
promptly, or the wrapper watches a oneshot the handler signals).
2. On establishment failure: the wrapper tears down the just-allocated
channel (`teardown_channel` — the ledger/policy paths already
handle this atomically) and replies with a **new typed CallError**,
e.g. `channel:open_failed`, with `details` carrying a reason code +
message. Reason-code vocabulary per the SSH four (the survey's
finding): policy-denied (already distinct — `channel:forbidden`),
dial-failed, unknown-resource-or-substrate, resource-shortage —
mapping 1:1 onto what an open handler can actually produce. Wire
addition is additive (new error code string + optional details
shape), no existing consumer breaks.
3. Backward compatibility: the existing `OpenHandler` signature is
preserved if the split shape is chosen (old handlers still compile —
the establisher is a new, separately-registered hook, defaulting to
an always-OK establisher for the no-establishment-work case).
### Alternative considered and rejected
An in-band establishment/error frame (alktty-style, on the channel
stream, alktunnels' original OQ-TN-09 direction) works without an
upstream change, but: (a) it forces every ALPN crate to define a frame
vocabulary and a disambiguation scheme (alktty's `0x00` peek is
exactly this cost, paid once per crate); (b) it cannot carry typed
`CallError.details` or participate in ADR-016 error schemas; (c) it
leaves the phantom-opened channel in the manager (allocation/ledger/
policy all fire for a channel that never carried data); (d) SSH's
semantics — the failure is the open's reply, the channel never exists
opener-side — are the cleaner contract, and channels is at exactly the
maturity point (three downstream dependents, alkcall 0.4.x) to fix it
upstream cheaply.
### Severity
[major] — not [critical]: no data corruption, and the capability is
reachable via per-crate workarounds (alktty proves it). But it is the
shape every future protocol crate will fight, the fix is cheaper now
than after the next consumer, and the workaround path (in-band frames)
bakes in a wire format that would then need to stay stable per crate.
---
# Part B — Smaller findings from the same sweep
## E-02 [minor] — `services/list` discloses no per-op metadata; openable resources are indistinguishable by name alone
`services_list_handler` (`src/registry/discovery.rs:247-269`) maps
`list_operations()` to `{name, namespace, op_type}` only. For a tunnel
registry, the consumer's discovery question is "which tunnel resources
may I open" — and the answer arrives as a bare list of op names
(`channels/tunnel/sub`...). Per-resource identity (which target, which
substrate, human description) has no field to live in:
- `services/schema` (`spec_to_json_pub`, `discovery.rs:211-236`) can
disclose it *per op* (input schema, access_control), so the data
path exists — but it is N+1 round-trips, and the input schema
describes the *open params contract*, not the *set of produced
resources* (a tunnel producer registers one op and N resources).
- ADR-047 §6 already anticipated the dynamic half:
`channel/resources/subscribe` aggregates per-ALPN resource
enumerators — but the handler is a not-implemented stub
(`channel:resources_not_implemented`, OQ-40,
`operations.rs:274-299`).
This is not an alkcall defect — OQ-40 is decided-deferred and the stub
fails loudly by design. Filing it because alktunnels' discovery need
(OQ-TN-08 resolution: "the ops listing IS tunnel-resource discovery")
makes OQ-40 load-bearing for the first time: a consumer UI cannot
distinguish produced tunnel resources without either the enumerator
aggregation or a listing enrichment. Recommend deciding (small ADR or
OQ-40 update) whether:
1. `channel/resources/subscribe` lands (per-ALPN enumerators — the
decided shape), or
2. `services/list` gains an additive per-op `description`/`metadata`
field (static, registry-side — cheap, but describes the op, not the
resource set), or
3. Both: subscribe for live resource sets, list enriched for op
descriptions.
alktunnels Phase 1 can proceed with (3)'s shape assumed; the minimal
v1 consumer can also just know the op name out-of-band (config), which
is why this is [minor] today.
## E-03 [minor] — `OpenHandler`-exit teardown races `channel/close`'s awaited teardown, but the ledger `take` makes it benign (verify + document)
`run_open_wrapper`'s spawned teardown task (`operations.rs:505-518`)
and the `channel/close` handler's teardown path
(`make_close_handler`, `operations.rs:170-238` — 5s await on the
handler task, then ledger `take` + `on_close`) both walk
`opener_ledger().take(id)``policy.on_close(opener)`. The `take` is
atomic (first caller removes the entry), so no double-decrement — the
cap cannot drift upward. But the loser of the race:
- The wrapper task's `teardown_channel(id)` returns
`Err(UnknownChannel)` (logged? no — `let _ =` discard,
`operations.rs:509`) and skips the ledger/policy half silently.
- The close handler's `Ok(task)` arm then awaits a task that is
already exiting.
Verified benign for the cap invariant (`take` is the gate —
`operations.rs:510`, `operations.rs:210`), but the `let _ =` discard
at `operations.rs:509` means a real `UnknownChannel` after a
`set_handler_task` failure (`operations.rs:520-527` — the "channel
vanished between open and handler-task install" warn) is
indistinguishable from a benign race. Recommend either a debug log on
the discard or a comment pinning the race as designed. No correctness
impact; filing for the record since it was checked during the E-01
trace.
## E-04 [minor] — Early-arrival park cap (64) interacts with push-first producers under slow adopters — cap-drop is silent at the consumer
The early-arrival buffer (`EARLY_ARRIVAL_CAP = 64`,
`manager.rs:111`, `park_early_arrival` `manager.rs:440-462`) is
per-channel and drops past the cap with a debug log + counter —
correct per the adopt-race design. But for a *tunnel* producer whose
handler dials then immediately pumps (the common case), 64 chunks can
arrive before the consumer's `adopt_channel` runs if the open-op
response is delayed (relay hops, scheduling). The drops are counted
(`dropped_unknown_chunks`) but not visible to the channel's
consumer — data loss presents as a truncated stream with clean framing
elsewhere. Not a correctness bug (the cap is the documented behavior;
the fix shipped in `a22b2b8` is the right shape), but worth a note in
the tunnel-crate's POC checklist: the UDP POC's MTU-vs-buffer sizing
should treat 64 parked chunks as the observable bound. No alkcall
change requested; filed so the constraint is visible to the next
consumer.
## Non-findings (verified correct, recorded to bound the re-review)
- **The open-op ACL path is complete**: `invoke_streaming` runs the
same visibility + `AccessControl::check` + `input_schema` gates as
`invoke` (`registration.rs:380-404` vs `:347+`); the Sub-typed open
op's ACL failure is a `call.error` on the open, no channel
allocated. Verified against the `invoke_streaming_acl_denied_yields_
forbidden` test (`registration.rs:1628`).
- **`input_schema` enforcement covers open params** (0.4.0, the
alktty-review-L1 fix): `check_input_schema` runs before the handler
in all three dispatch entry points; schema-invalid open params are
`INVALID_INPUT` call errors — never a phantom channel. The
semantic-beyond-schema gap is E-01's subject and is properly
post-schema.
- **EOF arms are unified and clean**: both the sentinel (`Bytes::new`)
and sender-drop arms of `MpscRecvStream::poll_read` yield clean EOF
(`reassembly.rs:111-135`); the mux pump writes the implicit-EOF
chunk for handler-drop-without-shutdown (`mux.rs:78-81` +
`mux_pump_writes_eof_on_implicit_close` test). The two-pump
shutdown contract (alknet ADR-078) has its upstream half in place.
- **The `channel_open` marker survives the wire round-trip**
(`spec_to_json_pub` emits the boolean; `rebuild_spec_for` re-derives
the ALPN from the op name — `from_call.rs:265-280`, `:298-314`),
including the `custom/proto` multi-segment case. The G-04
`resource_id_path` fix is present (`from_call.rs:260`,
round-trip test `from_call.rs:610-617`).
- **The opener-ledger cap decrement is atomic with removal**
(`opener_ledger().take` gates every `on_close` path); the
double-decrement concern from ADR-047 §7 is closed at every call
site traced.
---
# Remediation sketch (for the ADR discussion, not landed)
**Unit 1 — E-01 (the establishment phase).** Decided-shape ADR first
(this is ADR-047 §3 contract territory — the `OpenHandler` type shape
is a cross-crate API surface, and both existing consumers' handlers
must be considered; alktty's `TtyOpenHandler` is the migration
prototype). Implementation sketch: `ChannelCore::register_openable`
gains an optional establisher hook; `run_open_wrapper` awaits it
bounded (establishment deadline — a constant, e.g. 10s, or per-spec),
tears down on failure, replies `channel:open_failed` with
`{reason, message}` details on failure and `{channel_id}` on success.
alktty migrates its channels-path error-frame to the call-error path
where applicable (its direct-ALPN path keeps the in-band frame — two
transports, two contracts). alkhttp unaffected (no openable ops).
**Unit 2 — E-02 (discovery enrichment).** Decide via OQ-40 update +
small ADR amendment: recommend (3) — subscribe for live resource sets
(the decided shape, now load-bearing), plus an additive
`description` field on the listing (cheap, immediately useful).
**Unit 3 — E-03/E-04.** E-03: log-or-comment; E-04: doc note. Trivial.
## Verification gates for the E-01 remediation
- A channels end-to-end test: producer handler whose establisher fails
after channel allocation → consumer's `open_channel` resolves
`Err` with `channel:open_failed` + reason details; no channel in the
manager's `channel_ids()` afterward (the SSH "channel never exists
opener-side" property, consumer-visible as "no `channel_id` was
ever returned").
- A bounded-establishment test: establisher that never completes →
open op fails with a timeout-flavored error within the deadline,
channel torn down, ledger decremented.
- An alktty-migration test: the existing
`open_via_channels_surfaces_negotiation_rejected` scenario resolves
via call error (or retained error-frame — per the ADR's chosen
compat shape) unchanged in behavior.
## References
- alknet ADR-078 / channels two-pump contract — the teardown half this
review's EOF-arms non-finding confirms upstream.
- alkcall ADR-047 (openable ALPNs are operations), ADR-016 (typed
error schemas — the vehicle for `channel:open_failed` details),
ADR-040/041 (backpressure/caps — untouched by E-01).
- alktty ADR-009 (the open op's input is the negotiation) + review
#001 L1/L3 resolution — the per-crate workaround E-01 obsoletes;
`send_negotiation_error` (`alktty/src/adapter.rs:177-187`) and the
`0x00`-peek (`alktty/docs/architecture/tty-adapter.md:186-197`) are
the in-band mechanism the upstream establisher replaces for the
channels path.
- alktunnels Phase 0 (`docs/research/phase-0-findings.md` OQ-TN-09)
and the SSH/SOCKS5 survey (`docs/research/ssh-socks5-survey.md`
§"Open-failure path", §"Comparison") — the prior art motivating
E-01's reason-code vocabulary.
- The consumer-findings ledger convention (`docs/reviews/consumer-
findings-ledger.md`) — findings here follow the same spirit
(downstream-discovered, filed for upstream action); E-01..E-04 are
alktunnels-discovered but numbered in alkcall's review series since
they are alkcall findings.