Files
alkcall/docs/reviews/006-channel-open-establishment-gap-review.md
T
glm-5.3-flash 36e74cda11 feat(review 006 Unit 3): teardown-race log, early-arrival bound docs, count accessor (E-03, E-04, N-2)
- E-03: the open wrapper's handler-exit teardown no longer discards
  UnknownChannel silently — debug log + benign-race pinning comment
  (ledger take is the atomic gate; no double-decrement)
- E-04: consumer-facing doc note on the 64-parked-chunks observable
  bound (channel-client.md + EARLY_ARRIVAL_CAP const doc) for
  tunnel-style push-first producers
- N-2: ChannelManager::early_arrival_count() accessor (the
  observability choice over removing the write-only counter),
  documented monotonic, with a park/adopt-drain monotonicity test
- review 006: Unit 3 marked implemented in Status and remediation plan

Verification: 617 tests pass; clippy -D warnings clean (host +
wasm32 check); fmt clean; doc clean
2026-09-06 19:35:18 +00:00

561 lines
30 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Review 006 — Channel-Open Establishment Gap (from alktunnels Phase 0)
## Status
**Resolved-by-ADR (E-01, N-1) / Implemented (E-02, E-03, E-04, N-2).**
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.
**2026-09-06 verification + remediation pass.** All four findings were
independently re-verified against source at tree `88e3f5e` (0.4.1 +
this review's own commit) — verdicts CONFIRMED for E-01..E-04, with
one correction to E-02's cost estimate (see the verification appendix
at the bottom of this file). Three additional findings filed from the
same sweep: **N-1** (client error-type gap — blocking for E-01's
consumer visibility), **N-2** (dead counter), **N-3** (panic posture,
pinned). **E-01 + N-1 are resolved by ADR-049**
(`docs/architecture/decisions/049-channel-open-establishment-phase.md`
— split-hook `OpenEstablisher`, bounded await, `channel:open_failed`
typed error). **Unit 1 (E-01 + N-1) is implemented** in alkcall 0.5.0
(`OpenEstablisher` + `register_openable_with_establisher`, wrapper
establishment phase with bounded await, `channel:open_failed` typed
error, `ChannelOpenError` typed client error — all four verification
gates landed as tests; two implementation-shape notes recorded in
ADR-049's amendment: the establisher does not receive the channel
`Connection` (yield-once BiStream belongs to the pump handler), and
the bound is the earlier of dispatch deadline and per-registration
timeout). **Unit 2 (E-02) is implemented** in alkcall 0.5.0
(`OperationSpec.description: Option<String>` +
`with_description`, `spec_to_json_pub` emits it when set,
`rebuild_spec_for` parses it (shared by `from_call` and `op/register`),
`services/list` and `services/list-peers` local listings emit it when
set; the ADR-047 §6 amendment below records the discovery decision).
**Unit 3 (E-03, E-04, N-2) is implemented** in alkcall 0.5.0 (E-03: the
wrapper's handler-exit teardown discards `UnknownChannel` through a
debug log + a benign-race pinning comment; E-04: the 64-parked-chunks
observable bound is documented consumer-side in `channel-client.md` §
"The early-arrival park bound (push-first producers)" and on the
`EARLY_ARRIVAL_CAP` const; N-2: `early_arrival_count` is exposed via
`ChannelManager::early_arrival_count()` — the observability choice,
paired with `dropped_unknown_chunks` — with a monotonicity test).
The original remediation sketch below is superseded by the
"Remediation plan (post-verification)" section; the original sketch
is retained for the record.
Findings continue the review numbering with prefix `E` (001–005 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. The 2026-09-06 re-verification pass
(verification appendix) re-traced all findings at tree `88e3f5e` —
no source changes touched the reviewed paths between the two trees
(the only delta is this review's own commit).
```
Verified against: alkcall a22b2b8 (0.4.1); re-verified at 88e3f5e
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 0x01–0x08 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 (original, superseded — see "Remediation plan (post-verification)")
**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).
**Implemented 2026-09-06 (alkcall 0.5.0):** the listing half landed
(`OperationSpec.description`, four touchpoints as the appendix
corrected); the subscribe half stays deferred (OQ-40, now with the
load-bearing note).
**Unit 3 — E-03/E-04.** E-03: log-or-comment; E-04: doc note. Trivial.
# Remediation plan (post-verification)
Firm ordering, decided 2026-09-06. E-01's ADR is **ADR-049**
(`docs/architecture/decisions/049-channel-open-establishment-phase.md`)
— decided shape: the **split hook** (`OpenEstablisher` awaited bounded
by the wrapper, `OpenHandler` spawned unchanged after success), chosen
over await-and-inspect because it preserves the `OpenHandler` type
(backward compatible by construction), keeps establishment and pump
lifecycles separate (SSH semantics: the dial is synchronous with the
open reply), and restores ADR-047 §3's original "channel plan" wrapper
shape. N-1 rides the same unit — without the client error-type fix,
`channel:open_failed`'s typed reason is unreachable through the
primary client path.
**Unit 1 — E-01 + N-1 (alkcall 0.5.0).** `OpenEstablisher` +
`register_openable_with_establisher` (no-establisher = always-OK,
existing registrations compile unchanged); wrapper flow: `check_open`
→ `open_channel` → await establisher bounded (dispatch deadline when
`Some`, else `ESTABLISHMENT_TIMEOUT` = 10s) → on failure:
`teardown_channel` + ledger `take` + `policy.on_close` + reply
`channel:open_failed` with `details: {reason, message}` (reason ∈
`dial_failed` / `unknown_resource` / `resource_shortage` /
`handler_error` / `timeout`); on success: spawn pumps,
`set_handler_task`, reply `{channel_id}`. `ChannelClient::open_channel`
returns a typed error carrying the `CallError`. Concurrency is safe:
the dispatcher spawns Once invocations as independent tasks
(`dispatch.rs` `spawn_once_dispatch`), so an awaited establisher does
not head-of-line-block channel 0. Version 0.5.0 (the `open_channel`
error-type change is semver-relevant). **Status: IMPLEMENTED
(2026-09-06)** — with two implementation-shape notes recorded in
ADR-049's amendment: the establisher takes `(input, auth)` only (the
yield-once channel `BiStream` belongs exclusively to the pump
handler), and the effective bound is `min(dispatch deadline,
registration timeout | ESTABLISHMENT_TIMEOUT)`. All four verification
gates below are tests in the crate.
**Unit 2 — E-02 (ride the 0.5.0 release).** Additive
`description: Option<String>` on `OperationSpec` — note this is four
touchpoints, not one (see the verification appendix's E-02
correction): struct field + `spec_to_json_pub` emit +
`rebuild_spec_for` parse + the `services/list` output-schema doc.
`services/list` emits it when set. OQ-40 stays deferred but gains a
"load-bearing for alktunnels discovery UI" note; the
`channel/resources/subscribe` half stays deferred (alktunnels v1 uses
config-known op names). **Status: IMPLEMENTED (2026-09-06)** — the
discovery decision (3: enriched listing now, subscribe deferred) is
recorded as an amendment to ADR-047 §6.
**Unit 3 — E-03/E-04/N-2 (trivial batch, same PR series as Unit 1).**
E-03: debug log (or pinning comment) on the discarded `UnknownChannel`
at the wrapper's teardown discard. E-04: doc note on `EARLY_ARRIVAL_CAP`
(the 64-parked-chunks observable bound) for tunnel-crate-facing
consumers. N-2: expose an accessor for `early_arrival_count` or remove
the write-only counter. **Status: IMPLEMENTED (2026-09-06)** — E-03:
debug log + benign-race pinning comment on the wrapper's handler-exit
teardown (mirroring the sibling log the Unit 1 establisher-failure
teardown already carries); E-04: consumer-side doc note in
`channel-client.md` plus the const doc; N-2: accessor
(`ChannelManager::early_arrival_count()`, documented as monotonic —
adopt-drain does not decrement) + test.
**Sequencing:** ADR-049 (landed) → alkcall 0.5.0 (Units 1+2+3) →
alktty migration (channels-path semantic failures move into an
establisher; the direct-ALPN in-band error frame is retained — two
transports, two contracts) → alkhttp mechanical pass
(`OpenableAlpn.establisher: Option<...>`, default `None`). alktunnels
Phase 1 blocks only on the ADR decision (landed), not the
implementation: its OQ-TN-09 direction ("self-contained control frame")
shrinks to *post-establishment* control only once `channel:open_failed`
exists.
## 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.
- (Added post-verification) A compat gate: a no-establisher
registration behaves exactly as today — `{channel_id}` reply,
handler spawned, teardown on handler exit.
## Verification appendix (2026-09-06 re-verification pass)
All findings re-verified by independent source trace at tree `88e3f5e`.
Verdicts, corrections, and the additional findings:
- **E-01 CONFIRMED — and strengthened.** The wrapper's
spawn-then-reply flow is as described
(`src/channels/operations.rs` `run_open_wrapper`: handler spawned,
reply written before the handler performs any work). New supporting
evidence the original pass missed: **ADR-047 §3's decision text**
describes the ALPN open handler as "validate params, consult
ownership, prepare the backend, return a 'channel plan'" — an
awaited preparation the wrapper consults before replying. The
implemented `OpenHandler` (`JoinHandle<()>`) collapsed that phase
into a fire-and-forget spawn. E-01 is therefore **drift from
ADR-047 §3's own wrapper shape**, not merely a missing convenience —
the remediation restores the decided design, which lowers the ADR's
contention cost. Also verified: the serving loop spawns Once
invocations as independent tasks, so an awaited establishment phase
does not head-of-line-block other calls on channel 0 (a fix-feasibility
question the original pass did not address).
- **E-02 CONFIRMED — one correction.** `services_list_handler` emits
`{name, namespace, op_type}` only, as described. But the "cheap,
additive `description` field" framing understates the work:
`OperationSpec` has **no `description` field at all** (the
`description` in `spec.rs` is on `ErrorDefinition`). The change is
four touchpoints: struct field, `spec_to_json_pub` emit,
`rebuild_spec_for` parse, and the `services/list` output-schema doc.
Still small; still [minor].
- **E-03 CONFIRMED.** Both teardown paths gate on the atomic
`opener_ledger().take(id)`; the race loser gets `UnknownChannel` /
`take → None` — no double-decrement. The `let _ =` discard on the
wrapper's `teardown_channel` result is as described. Also traced:
the `set_handler_task` failure path still runs the wrapper task to
completion (self-teardown), so no leak in that arm either. The
log-or-comment remedy stands.
- **E-04 CONFIRMED.** `EARLY_ARRIVAL_CAP = 64`, per-channel, FIFO,
drop-past-cap with debug log + `dropped_unknown_chunks` counter.
Doc-note-only remedy stands.
- **N-1 [minor today, blocking for E-01's consumer visibility] —
`ChannelClient::open_channel` erases the typed error.**
(`src/channels/client.rs`) It flattens the `CallError` into a
`String` via `format!("open op failed: {e:?}")`. Even once the
wrapper replies `channel:open_failed` with reason details, a
consumer cannot branch on the reason — the error type destroys it.
E-01's verification gate ("consumer's `open_channel` resolves `Err`
with `channel:open_failed` + reason details") is unreachable without
changing this error type, so the fix is in scope for the E-01 unit
(ADR-049 §4), not deferred.
- **N-2 [trivial] — `early_arrival_count` is write-only.** Incremented
in `park_early_arrival`, never read anywhere (no accessor; only
`dropped_unknown_chunks` is exposed). Either expose an accessor
(observability for the E-04 bound) or remove the counter.
- **N-3 [observation, pinned by ADR-049 §6] — panicked pump handlers
are EOF-shaped by design.** The wrapper's teardown task swallows the
pump handler's `JoinError` (`let _ = raw_task.await`). A panicked
handler = phantom channel + instant EOF, indistinguishable from a
clean short-lived channel. With the establisher split, the pump-phase
panic stays in this category (correct — there is no mid-stream error
channel by design; establishment errors are the only kind that
belong in the open reply). ADR-049 §6 pins this posture; no change.
## 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.
- **ADR-049** (`docs/architecture/decisions/049-channel-open-
establishment-phase.md`) — the E-01/N-1 resolution (split-hook
`OpenEstablisher`, bounded establishment, `channel:open_failed`
typed error, client error-type fix).