diff --git a/docs/reviews/004-per-connection-dispatch-and-client-serving-review.md b/docs/reviews/004-per-connection-dispatch-and-client-serving-review.md new file mode 100644 index 0000000..1aa8833 --- /dev/null +++ b/docs/reviews/004-per-connection-dispatch-and-client-serving-review.md @@ -0,0 +1,447 @@ +# Review 004 — Per-Connection Dispatch Resolution and Client-Side Op Serving (found via alkhttp's WS data-channel drill-down) + +## Status + +Verified, open for remediation. + +## Scope + +Focused review of two design/spec mismatches in the call + channels +integration, uncovered while drilling into the deferred half of +alkhttp's WebSocket path (alkhttp review 003, findings WS-24/WS-25 — +the OQ-05 "browser data channels" deferral). The drill-down +established that the alkhttp-side gap is mostly wiring, but two +assumptions underneath it resolve to *this* crate: + +1. **Dispatch resolution for per-connection operations.** ADR-047's + §4 amendment (2026-08-13) decided that openable-ALPN open ops are + registered per-connection on the connection overlay registry + (Layer 2). The top-level dispatch path never consults that layer, + so an op registered per the amendment's mechanism cannot be + invoked over the wire. The only shape proven to dispatch is + different from the one the ADR describes. +2. **Client-side operation serving.** The connection-local overlay + (`CallConnection::register_imported` → `overlay_env()`) and the + session overlay (`compose_root_env` attaching peer-keyed overlays) + are fully built and tested — but only the *accept side* has a loop + that serves inbound `call.requested` frames. The connect side's + read pump resolves responses and silently drops requests. There + is also no wire mechanism by which a client could announce an op + in the first place. Together these leave the "both sides can be + both" symmetry (AGENTS.md §8; ADR-022 §direction semantics; + alkhttp ADR-048's browser bidirectionality) unreachable on the + single-stream channel-0 shape. + +The pass re-verified every claim directly in source at tree +`c16b069`. Cross-repo context: these findings block alkhttp review +003's Unit 2 (data-channel wiring for WS sessions, both browser and +native-fallback consumers); the decision work and the fixes live +here. Findings continue alkcall review numbering (F-01..; reviews +001–003 used P/C/R/A/B/C/D prefixes — F is the new prefix for this +focused review, findings numbered independently). + +## Baseline verification (this pass) + +``` +cargo test → 565 passed, 0 failed +cargo clippy --all-targets -- -D warnings → clean +cargo fmt --check → clean +``` + +The suite is green; these findings are design/spec mismatches, not +regressions. Notably, one of them (F-04's serving gap) is invisible +to the suite *because* the connect side has never been asked to +serve — the existing e2e tests all put the dispatcher on the accept +side. + +## Verdict + +- **The channel machinery is done and proven.** `ChannelCore` / + `register_openable` / `ChannelOperations`, the opener ledger, the + odd/even id split, the demux hardening, and the full e2e wiring + test (`channels/client.rs:767-870`) are all in place and tested. + Nothing in Part A below re-opens that machinery. +- **The gap is the dispatch-resolution *mechanism* (F-01/F-02/F-03) + and the connect-side serving half (F-04)** — both upstream of any + transport (TCP+TLS, WS, whatever). Fixing them here unblocks + alkhttp's data-channel wiring, and — since the crate is + wasm-targetable and not yet published to consumers — the fixes are + cheap now and required regardless of alkhttp. +- **The recommended resolution is assembly, not invention:** per- + session registry forks (F-02/F-03) + a bootstrap `op/register` op + (F-01's mechanism choice) + a connect-side serving loop (F-04). + Three of the four pieces have working reference shapes in-tree; + the genuinely new protocol surface is one op handler and one read + loop. + +## Severity legend + +- **[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 — Dispatch resolution vs the ADR-047 §4 amendment + +## F-01 [major] — Top-level dispatch never consults the connection overlay; ops registered per the ADR-047 §4 amendment mechanism resolve `NOT_FOUND` on the wire + +**ADR drift:** ADR-047 "Amendment (§4 per-connection registration, +2026-08-13)": *"open ops are registered per-connection … +`register_openable` is called on the connection overlay registry +(Layer 2 per ADR-019), closing over the per-connection `ChannelCore`."* +**Verified:** YES, three ways: + +1. `Dispatcher::dispatch` reads the operation's `op_type` from + `self.registry.registration(...)` only + (`src/protocol/dispatch.rs:316-320`) and invokes via + `self.registry.invoke` / `invoke_streaming` / `resolve_sink_handler` + (`:330-357`) — the dispatcher's **base registry** only. There is + no fallback to `connection.overlay_env()`. +2. The connection overlay is reachable solely as a layer of + `context.env`: `compose_root_env` attaches it via + `env.attach_peer(peer_id, connection.overlay_env())` + (`dispatch.rs:195-215`). `context.env` is consulted for *nested* + invocations (a handler calling `context.env.invoke(...)`) — not + for resolving the incoming `call.requested` itself. An open op + registered on the overlay resolves `NOT_FOUND` when the wire + request arrives. +3. The one place this wiring is proven end-to-end uses a different + shape than the amendment describes: alkcall's own e2e test + (`src/channels/client.rs:767-870`) has the `install_channel_zero` + hook build a **fresh per-connection registry containing the open + op and pass it as the dispatcher's base registry** + (`Dispatcher::new(registry, ...)` at `:837`) — not as an overlay. + That shape works; the amendment's shape cannot. + +Consequence: ADR-047's data-plane promise (per-connection open ops +invocable on channel 0) is unreachable through the mechanism the +amendment names. Every consumer of `register_openable` must either +adopt the undocumented per-session-base-registry shape or fail. + +**Fix (Unit 1):** decide the mechanism deliberately (see F-02) and +amend ADR-047 §4's wording to describe the mechanism that actually +dispatches. The amendment's *rationale* (per-connection state, no +`context.env` downcast) stands under either option. + +## F-02 [major] — `OperationRegistry` has no fork/clone surface; the proven per-session-registry shape requires one + +**Verified:** YES. `OperationRegistry` +(`src/registry/registration.rs:97-100`) is +`HashMap` + a validators map, with +`new`/`register`/`registration`/`publish_validator`/`list_operations` +(`:102-180`) — no `Clone` impl and no fork API. Every type inside a +`HandlerRegistration` *is* clone-able (verified): + +- `HandlerKind` — `#[derive(Clone)]` (`registration.rs:51`) +- `OperationSpec` — `#[derive(Debug, Clone, PartialEq)]` + (`spec.rs:174-175`) +- `OperationProvenance` — `#[derive(Debug, Clone, Copy, PartialEq, + Eq)]` (`registration.rs:58-59`) +- `CompositionAuthority` — `#[derive(Debug, Clone)]` + (`context.rs:82-83`) +- `Capabilities` — manual `impl Clone` (`core/types.rs:75-80`) +- `jsonschema::Validator` — `#[derive(Clone, Debug)]` + (jsonschema 0.46.10 `validator.rs:294`) + +So the fork is a small addition (derive `Clone` on +`HandlerRegistration` + `OperationRegistry`, or an explicit +`fork() -> OperationRegistry` that copies both maps). Two shape +options: + +- **(a) per-session base registry (recommended).** The + `install_channel_zero` hook (and any future session-establishment + seam) forks the deployment's base registry, registers the + per-connection operations on the fork (generic channel ops, the + openable ALPNs, and — with F-05 — the bootstrap op), and dispatches + channel 0 over the fork. This is the proven shape; the ADR wording + moves from "overlay registry" to "per-connection registry installed + as the session's dispatch registry." Cost: `services/list` / + `services/schema` registered on the base registry must be + re-registered (or inherited via fork) per session to stay + discoverable — the fork makes that free if the bootstrap ops are + part of the fork source. +- **(b) overlay-aware top-level dispatch.** Add an overlay fallback + in `dispatch` / `run_loop_single_stream`: when the base registry + misses, consult `connection.overlay_env()`. Keeps one static + registry and makes the amendment's wording true as written, but + touches the shared dispatch loop (higher blast radius, and the + overlay's `invoke_with_policy` shape — namespace-scoped, parent- + context-driven — does not match the top-level call shape, so the + fallback needs care: op-type lookup, ACL, visibility). + +Recommendation: (a). It is the only shape with an end-to-end proof, +it needs no dispatch-loop change, and it matches ADR-019's layering +(the overlay stays what it is today: the nested-invocation landing +zone for imported ops — which F-05 needs). + +**Fix (Unit 2):** `Clone`/fork surface + the ADR wording amendment; +acceptance = the review-001-style e2e gate (open op resolves over a +live channels connection through the *fork*). + +## F-03 [minor] — Per-session fork + static base: no `Default`/builder seam exists to compose "base ops + per-session ops" without re-registration + +**Verified:** YES. Even with a `Clone` surface (F-02), the hook must +compose three sources into the per-session registry: the deployment's +base ops (already registered, not retained anywhere as a "source" +list), the generic channel ops (`ChannelOperations::register_on`), +and the openables. A fork of the base registry makes the first source +free — this finding exists only if (a) is chosen *without* the fork +(i.e., rebuilding per-session registries from scratch). Record it as +a constraint on the fork API: it must clone *registered handlers* +(`HandlerKind` clones carry their closures), not just specs, so +`services/list` keeps its closure over the base registry. +`publish_validator`'s `validators` map must be carried too (schema +enforcement would silently vanish otherwise). + +**Fix:** fold into F-02's fork surface; the acceptance test covers it +(`services/schema` on the fork still validates input). + +--- + +# Part B — The connect side cannot serve; no wire path announces ops + +## F-04 [major] — The connect side's channel-0 read pump resolves responses only; inbound `call.requested` frames are silently dropped — there is no serving half on channel 0 + +**Verified:** YES. `ChannelClient::from_connection` +(`src/channels/client.rs:85-134`) spawns the read pump as +`read_single_stream_until_closed(single_stream_reader, &pending_map)` +(`connection.rs:681-686`), whose only job is resolving the +`PendingRequestMap` — its frame handler (`dispatch_envelope`, +`connection.rs:705-...`) branches on `call.responded` / +`call.completed` / `call.aborted` / `call.error` and **has no arm for +`call.requested`**. The accept side's mirror +(`Dispatcher::run_loop_single_stream`, `dispatch.rs:727-850`) serves +requests and has no pending-resolution arm. Each side silently drops +the other half's frames. + +Consequences: + +- A consumer that dials a producer via `ChannelClient` can call ops + and open channels, but if the producer later calls an op *the + consumer serves*, the request is dropped (the caller's pending + hangs until the sweeper or connection close). This is the exact + shape ADR-022 §connection-direction-independence promises ("both + sides can be both simultaneously" — AGENTS.md §8), and ADR-036's + channel-0 pre-negotiation makes channel 0 the single control + plane where it must work. +- It blocks the bootstrap registration story (F-05): the hub cannot + call anything the WS/native client serves, and the client cannot + even receive the call that would let it register. + +The fix is well-understood and symmetric: a full-duplex +single-stream read loop that branches both ways — `call.requested` +→ dispatch and write a response frame (the `run_loop_single_stream` +arms), `call.responded`/`completed`/`aborted`/`published` → resolve +pendings and route sink chunks (the read-pump arms). IDs are +UUID-generated on each side (`generate_request_id()`), so +cross-correlation is not a hazard; the abort arm needs to try both +tables (in-flight sink aborts *and* the pending map's cascade), which +`run_loop_single_stream`'s `EVENT_ABORTED` arm already does within +its own scope. The wasm-targetable browser story compounds the +value: the same serving loop is what a wasm alkcall browser +compiles. + +**Fix (Unit 3):** a serving-capable single-stream loop usable from +the connect side (e.g., a `Dispatcher` variant or a +`CallConnection::serve_single_stream` that composes both frame +directions), wired into `ChannelClient` behind an opt-in (a serving +registry is not always present — a pure consumer may have none). +Acceptance gates: hub→consumer call over an existing `ChannelClient` +session resolves; consumer-served op participates in ACL (its +`AccessControl` gates the hub's call); disconnect mid-call still +fails pendings on both sides. + +## F-05 [major] — No wire mechanism announces client-side ops; ADR-022's import flow is hub→consumer only and the browser-native path has none + +**Verified:** YES. The envelope kind set is closed at six — +`call.requested` / `call.responded` / `call.completed` / +`call.aborted` / `call.error` / `call.published` +(`src/protocol/wire.rs:12-17`). Op registration onto an overlay +(`register_imported` / `register_imported_all`, +`connection.rs:162-172`) happens only in-process: `from_call` +(`client/from_call.rs:82-91`) builds bundles the *local* process +registers after *it* dialed out — the consumer importing a hub's +ops. There is no op, frame, or handshake by which a connected peer +announces "here are the ops I serve." Every consumer-facing +registration surface today requires the registrant to hold the +local `CallConnection` handle in-process. + +Consequence: the decided bidirectionality model — either side +registers ops the other can call (AGENTS.md §8; ADR-022 §direction +semantics; alkhttp ADR-048's connection-local overlay for +browser-registered ops) — has no on-the-wire expression for the +non-in-process side. It is exactly the assumed-bootstrap-op set the +protocol already leans on (`services/list` is dialed and called by +`from_call` on every import; each side is *expected* to serve it) +extended by one op: e.g. `op/register` carrying the +`HandlerRegistration`'s serializable parts (spec + provenance + +access control), whose hub-side per-session handler writes the +registration into that connection's overlay via +`register_imported`. The overlay is already the landing zone +(`compose_root_env` attaches it keyed by identity, +`dispatch.rs:211-213`), and discovery of what a peer registered is +already built: `services/list-peers` reads +`ctx.env.peer_ids()` / `peer_operations()` +(`registry/discovery.rs:260-307`) — the same overlay-populated env. + +Scope decision this forces (record it in the ADR, either way): + +- **Design it** (recommended): `op/register` (+ a deregister or + replace semantics for the reconnect path) as an assumed op in the + bootstrap set, served per-session; the wire stays six kinds. The + `HandlerRegistration`'s `Handler` closures cannot cross the wire — + the client sends spec + a call-forwarding contract, and the + hub-side handler wraps it as a forwarding handler that issues a + nested `call.requested` back over channel 0 (the same shape + `from_call`'s imported bundles use — `protocol/adapter.rs:541,610` + show the in-process twin). +- **Or scope-cut**: amend ADR-022/alkhttp ADR-048 to say peer-side + op registration is hub-initiated-import only until a contract + exists. Cheaper, but it re-opens the same hedge the deferral was + criticized for — the promise would remain written-but-unmechanized. + +Recommendation: design it. The pieces are all present; the cost is +one op handler, an `AccessControl`-gated registration surface (the +registry's existing ACL path covers it — an op with restrictive +`AccessControl` cannot be overwritten by an unprivileged peer), and +the F-04 serving loop to make it reachable in both directions. + +**Fix (Unit 3):** with F-04 — bootstrap op + client serving loop + +ADR-022 amendment naming the bootstrap op set (assumed ops each side +may serve: `services/list`, `services/schema`, and `op/register`). + +## F-06 [minor] — `services/list` on a per-session fork needs a closed-over fork reference, or per-session ops are undiscoverable + +**Verified:** YES. `services_list_handler` closes over a *specific* +`Arc` (`registry/discovery.rs:233-258`) — it +lists that registry's ops. Under the F-02(a) shape, per-session +openables live on the fork, so a `services/list` handler closed over +the base registry cannot see them. Two clean outs, both cheap: (i) +fork *before* registering the bootstrap discovery ops (then the +handler closes over the fork), or (ii) the fork API registers +base-registry bootstrap ops fresh on the fork. Either way the +constraint is: discovery ops must be registered against the +per-session registry, not the static one. Also note `services/list` +on the fork still ACL-filters (the handler re-checks per-caller) — +no privilege regression. + +**Fix:** fold into Unit 2 (the fork seam registers bootstrap ops on +the fork); acceptance: a per-session openable appears in +`services/list` for an authorized caller and not for an unauthorized +one. + +--- + +# Non-findings (verified correct, recorded to bound the re-review) + +- **The e2e reference shape is real and passes** — + `channels/client.rs:767-870` wires `ChannelClient` ↔ + `ChannelsAdapter` over duplex with an open op registered on the + per-connection registry, invoked end-to-end, quota reserved. The + F-02(a) shape is not speculative; it is in-tree. +- **`register_openable`'s wrapper machinery is complete** — ACL via + the registry's normal invoke path, `check_open` → `open_channel` → + ledger → spawn → teardown decrement, with `Once`/`Stream`/`Sink` + arms (`channels/operations.rs:399-453, 483-555`). Nothing in F-01 + re-opens it; the ops it registers just need a dispatch-reachable + home. +- **Channel-id allocation is collision-safe** for the bidirectional + open story (accept side even from 2, connect side odd from 1, + `manager.rs:105-125`; `adopt_channel` for the non-allocating side, + `manager.rs:273-309`). +- **The `call.*` envelope kind set staying closed is correct** — + AGENTS.md §7 allows *adding* kinds but F-05's resolution does not + need one; bootstrap ops over channel 0 are the lighter door. +- **Wasm-cleanliness is unaffected** — the recommended fixes (fork + + handler + read-loop branch) touch no transport, no fs/net tokio + features; `cargo check --target wasm32-unknown-unknown` remains + the release gate. + +--- + +# Remediation plan + +Sequenced by dependency. Units 1–3 are alkcall work; the alkhttp +consequences are tracked as alkhttp review 003 Unit 1 (updated to +point here). + +## Unit 1 — Decision recording (F-01, F-02 direction) + +ADR work only: + +- ADR-047 §4 amendment #2: the open-op registration mechanism is the + **per-connection registry installed as the session's dispatch + registry** (option (a)); the overlay registry remains the landing + zone for peer-announced ops (F-05) and nested invocation. +- ADR-022 amendment: the bootstrap-op set (each side may serve + `services/list`, `services/schema`, `op/register`), and the + direction-semantics note that serving on the connect side is + opt-in (F-04). +- alkhttp ADR-048's bidirectionality promise is re-pointed at these + ADRs instead of standing as an unmechanized commitment. + +Gate: ADRs recorded; alkhttp OQ-05 and ADR-048 cross-reference them. + +## Unit 2 — Fork surface + per-session composition (F-02, F-03, F-06) + +- `Clone` (or explicit fork) on `HandlerRegistration` + + `OperationRegistry`, carrying handlers and validators. +- A composition helper (e.g. `OperationRegistry::fork_with(...)`) or + documented pattern: fork base → register generic channel ops + + openables + bootstrap ops against the fork. +- Gate: e2e over a live channels connection — open op resolves + through the fork; `services/list` shows per-session openables + (ACL-filtered); `services/schema` on the fork still validates. + +## Unit 3 — Client serving half + bootstrap registration (F-04, F-05) + +- Serving-capable single-stream loop (compose the read-pump and + dispatch arms); wired into `ChannelClient` opt-in. +- `op/register` bootstrap op (per-session handler → + `register_imported` into the connection overlay), with + `AccessControl` gating and reconnect/replace semantics decided in + the ADR. +- Gates: hub→consumer call over an existing session resolves; + peer-registered op is discoverable via `services/list-peers` and + callable back over channel 0; unregistered/unauthorized + registration attempts fail loudly; disconnect drops the overlay + and fails both sides' pendings. + +## Unit 4 — alkhttp wiring (downstream, after Units 1–3) + +Tracked in alkhttp review 003 (Units 2–4 there): openables surface, +`install_channel_zero` rework over the fork, retained session +handle, e2e tests, spec reconciliation (OQ-05, ADR-067/048 v1-cut +notes). + +--- + +## Verification log (this pass) + +- All claims verified at tree `c16b069`; the F-01 dispatch claim was + checked against both dispatch paths (`run_loop` stream-per-request + reads `handle_stream` → same base-registry-only resolution; + `run_loop_single_stream` at `dispatch.rs:727-850`). +- The F-04 claim was verified by reading both loops' frame arms: + `read_single_stream_until_closed` → `dispatch_envelope` + (`connection.rs:681-705`, no `EVENT_REQUESTED` arm) vs + `run_loop_single_stream` (`dispatch.rs:754-850`, no + pending-resolution arm). +- The F-02 clone claims were verified type-by-type, including the + third-party `jsonschema::Validator` (0.46.10) and the vendored + `Capabilities` (manual `Clone` preserving `Secret`'s clone). +- The F-05 wire-closure claim was verified against the full kind set + (`wire.rs:12-17`) and grep over `src/` for any wire-side + registration path (none; `register_imported*` callers are + in-process only). +- `services/list-peers`' overlay-backed discovery verified at + `registry/discovery.rs:260-307` (reads `ctx.env.peer_ids()` / + `peer_operations()`, populated by `compose_root_env` at + `dispatch.rs:211-213`). +- Baseline gates re-run for this pass: cargo test (565), clippy + (all-targets), fmt — all clean. No source changes. \ No newline at end of file