- 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
561 lines
30 KiB
Markdown
561 lines
30 KiB
Markdown
# 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). |