diff --git a/docs/reviews/003-websocket-data-channel-review.md b/docs/reviews/003-websocket-data-channel-review.md new file mode 100644 index 0000000..1901717 --- /dev/null +++ b/docs/reviews/003-websocket-data-channel-review.md @@ -0,0 +1,467 @@ +# Review 003 — WebSocket data-channel wiring gap (the OQ-05 drill-down) + +## Status + +Verified, open for remediation. + +## Scope + +Focused review of the deferred half of the WS path: the data-channel +wiring recorded as OQ-05 (review-001 WS-03, deferred 2026-08-29), plus +the bidirectionality half of ADR-048 it leans on. The trigger for this +pass was a session-level drill-down into +[websocket.md](../architecture/websocket.md) §"Data channels for +browsers" and the OQ-05 deferral: what exactly is missing, whether the +deferral rationale still holds, and whether the "nothing new to +design" assumption recorded in OQ-05 is actually true. + +The pass covered: + +- the WS accept-side wiring end to end (`src/websocket/upgrade.rs`, + the alkcall `ChannelsAdapter` accept path it drives), +- the alkcall channel machinery the deferred wiring is expected to + reuse (`ChannelCore::register_openable`, `ChannelOperations`, + `ChannelManager` open/adopt/ledger, the demux loop), including + alkcall's own end-to-end test of the exact wiring shape, +- the dispatch path that would resolve an open op on channel 0 + (`Dispatcher::dispatch` / `run_loop_single_stream`, + `compose_root_env`, the connection overlay), +- the deployment surface (`HttpAdapter` builders, `SessionState`, + `RouterState`) an openable-ALPN declaration would ride, +- the spec record (ADR-047 §4 amendment, ADR-067 v1-cut note, + ADR-048 reconciliation note, OQ-05, alkcall ADR-047 §6 discovery + split), and +- the consumer framing (OQ-05's "reopened when a browser consumer + lands" trigger) against the clarified product position: WS is the + browser path **and** the native-client fallback behind hostile + NAT/firewall — the consumer set is wider than browsers, and the + wasm-capable protocol stack means a browser client needs only + binary-frame parsing and byte reassembly before handing bytes to + alkcall-wasm. + +Cross-crate findings (alkcall-side) are flagged as such. Findings +continue review-002's numbering (WS-20..). + +## Baseline verification (this pass) + +``` +cargo test → 453 passed, 0 failed +cargo test --all-features → 575 passed, 0 failed +cargo clippy --all-targets -- -D warnings → clean +cargo clippy --all-features --all-targets -- -D warnings → clean +cargo fmt --check → clean +``` + +Verified against tree `58665f2`. No source changes were made in this +pass; the findings are analysis-verified against source (file:line +below) rather than introduced by new code. + +## Verdict + +- **OQ-05's core assumption is now known to be wrong, and that is the + headline.** OQ-05 records the deferral as *"the design is decided + (ADR-067: the browser opens data channels exactly as any channels + consumer — nothing new to design); what is deferred is the + wiring."* That held for the alkcall-side machinery — which is + genuinely complete and well tested — but two wiring-blockers turned + out to be **design gaps, not mechanical work**: + - **WS-24 (cross-crate):** the ADR-047 §4 amendment's registration + mechanism ("`register_openable` is called on the connection's + overlay registry, Layer 2") cannot resolve an open op on the + wire — the top-level dispatch consults only the dispatcher's base + registry and never the connection overlay. The only shape proven + to work (alkcall's own e2e test) registers open ops on a + per-connection registry installed **as the dispatcher's base + registry** — a shape the ADR does not describe and that has no + fork/clone surface on `OperationRegistry` to support. + - **WS-25:** there is no wire mechanism for a WS client to + register an op at all — the call protocol's envelope kinds are + `call.requested`/`responded`/`completed`/`aborted`/`published`/ + `error` only. ADR-048's browser-side bidirectionality ("the hub + can call browser-registered ops") is not designable until + alkcall defines client-side op registration (or amends the + promise). Every overlay test to date pokes + `register_imported` in-process. +- **The mechanical half of the gap is real but small** (WS-20/21/22): + the accept-side hook discards the `ChannelManager` alkcall hands + it, the generic channel lifecycle ops are never registered, and no + deployment surface exists to declare openable ALPNs. The alkcall + machinery these would consume (`ChannelCore`, `ChannelOperations`, + opener ledger, demux, odd/even id split, cap policy) is built, + tested, and ready — alkcall even ships an end-to-end test of the + exact wiring over duplex. +- **The deferral trigger no longer matches the consumer set.** OQ-05 + says "reopened when a browser consumer lands," but WS is also the + native fallback behind ugly NAT/firewall; a Rust WS consumer + (exactly the `test_support` client shape) is a legitimate first + consumer and needs no JS library — OQ-04 does not block this + wiring (WS-23). +- No regressions and no new robustness holes were found in the + channel-0 path itself; the WS-13/17/18-era pump work is sound and + the idle-knob/data-channel interplay (silent-but-alive SSH-style + channels) is already documented with a deployment posture. + +## Severity legend + +- **[critical]** — the protocol cannot function end-to-end as + committed; or a decided spec invariant is violated in a way that + corrupts data or causes silent permanent state damage. +- **[major]** — a decided behavior is missing, wrong, or a real + reliability hazard; works in the happy path but fails a + spec-required edge case, or is a realistic security weakness. +- **[minor]** — drift, convention violation, dead code, or a + doc/spec inconsistency with no correctness impact. + +--- + +# Part A — The deferred wiring (mechanical) + +## WS-20 [major] — `install_channel_zero` discards the `ChannelManager`; no `ChannelCore`, no open ops, ever + +**Residual of:** review-001 WS-03 / OQ-05 (the v1 cut itself). +**Verified:** YES. The hook alkhttp supplies to +`ChannelsAdapter::new` ignores the manager: +`Arc::new(move |_manager, channel0_conn, auth| ...)` — +`src/websocket/upgrade.rs:255`. alkcall's accept path passes the +per-connection `ChannelManager` precisely so the hook can build a +`ChannelCore` and register openable ALPNs on it +(`alkcall/src/channels/adapter.rs:268` — +`(self.install_channel_zero)(manager.clone(), channel0_conn, auth.clone())`; +the `InstallChannelZero` doc at `adapter.rs:50-54` says the manager is +reachable via the hook "e.g., to register channel lifecycle ops' +`ChannelCore`"). The reference wiring exists in alkcall's e2e test +(`channels/client.rs:780-837`): the hook builds +`ChannelCore::new(manager, policy)`, registers a per-ALPN open op via +`register_openable`, and dispatches channel 0 with it. In alkhttp the +manager is dropped on the floor, so a browser/native WS client can +never open a data channel — the capability ADR-067 says the channels +design exists to provide, and the only part of the WS path the HTTP +gateway cannot already do. + +**Fix (Unit 2):** rework the hook to take the manager, build the +per-connection `ChannelCore`, register the generic ops (WS-21) and the +deployment's openable ALPNs (WS-22), then run the dispatcher over the +per-session registry — shape per the WS-24 decision. The policy is +already threaded (`ChannelsPolicy` extension, review-001 SRV-10's fix); +the openables ride the same path. + +## WS-21 [major] — Generic channel lifecycle ops are never registered: no `channel/close`, `channel/control`, `channel/resources/subscribe` on WS sessions + +**Verified:** YES. alkcall's `ChannelOperations::register_on` +(`alkcall/src/channels/operations.rs:42-98`) registers the three +generic ops — close (with drain-before-close + opener-ledger +decrement), control (loud `not_implemented` per alkcall OQ-39), and +resources/subscribe (loud `not_implemented` per alkcall OQ-40). +Grep-verified: nothing in alkhttp calls it, and the per-session hook +(WS-20) never registers them. Consequence: even if open ops were +wired today, a WS client could not close a channel +(`channel/close` → `NOT_FOUND`) or observe resources — the open +ledger would only drain via connection teardown. This is also the +discovery seam for the dynamic half of alkcall ADR-047 §6's +discovery split. + +**Fix:** fold into the WS-20 hook rework — `register_on` on the +per-session registry next to the openables. Note the +`not_implemented` ops are correctly loud today; registering them is +still right (fail loudly at the op, not `NOT_FOUND` at discovery). + +## WS-22 [major] — No deployment surface to declare openable ALPNs for the WS path + +**Verified:** YES. The `HttpAdapter` builder surface is +`with_decoy` / `with_extra_routes` / `with_ws_max_sessions` / +`with_ws_idle_timeout` (`src/server/adapter.rs:158-233`) — nothing +expresses "these ALPNs are openable on WS sessions." `SessionState` +carries registry/sessions/slots/idle only (`upgrade.rs:89-97`), and +`RouterState` likewise. The deployment decision ADR-067 requires +("the deployment decides which ALPNs are openable — the browser +reaches exactly what a Rust consumer on an in-line connection would +reach") has no expression point. Note the distinction from the +existing `ChannelsPolicy` extension: that is the *cap* policy +(`ChannelLifecyclePolicy::check_open`); the *openable set* (per-ALPN +`OperationSpec` with the `channel_open` marker + `OpenHandler`) is a +separate, currently nonexistent input. + +**Fix (Unit 2):** an `OpenableAlpn { spec, open_handler }` list + +`HttpAdapter::with_ws_openable_alpns(...)`, threaded +`RouterState` → `SessionState` → `run_channels_session` → hook, with +a request-extension fallback mirroring `ChannelsPolicy` so +bare-registry/custom upgrade routes can pass their own set. The +ALPN-specific `OpenHandler` implementations stay in the ALPN crates +(alktty et al.); alkhttp only ferries the registrations. + +## WS-23 [minor] — No browser-opened-channel e2e test; OQ-05's reopen trigger underestimates the consumer set + +**Verified:** YES. The test suite covers channel-0 dispatch +(`tests/ws_*`), overlay mechanics (in-process pokes, +`tests/ws_overlay_ops.rs`), and pump robustness — but no test opens a +data channel through a live WS session (the OQ-05 acceptance item +"browser-opened-channel end-to-end test" exists precisely because +none does). Everything needed already ships behind +`test-support`: `WsClient`, `frame_channel0_chunk`, +`ChunkAssembler`, `FrameAssembler` (`src/websocket/upgrade.rs:444-709`) +— a Rust WS client speaking raw channels framing, which is also +exactly the shape of the native-client-behind-a-firewall consumer. +alkcall has the analogous e2e test over duplex +(`channels/client.rs:767-870`) — the alkhttp one is the missing gate. + +**Also record here (product framing, no code change):** OQ-05's +status — `deferred(scope: v1 cut — no browser consumer yet; reopened +when one lands)` — frames the consumer as a browser needing the JS/TS +library (OQ-04). The clarified position: WS is the bidirectional path +for browsers **and** the fallback for native clients behind hostile +NAT/firewall/proxy; a native WS consumer is feasible today with zero +new library work (Rust client, same framing as any in-line +transport), and a browser consumer is mostly byte parsing + +reassembly ahead of wasm-targeted alkcall (WebTransport remains out +of scope per ADR-069). The wiring is therefore not blocked on OQ-04, +and the reopen trigger should be reframed accordingly when OQ-05 is +picked up. + +**Fix:** the Unit-3 e2e test (open → `{channel_id}` → chunks both +ways → handler sees bytes → `channel/close` → cap denial → teardown +decrements), plus the OQ-05 reframe when the task lands. + +--- + +# Part B — Design/spec mismatches (planning bugs; need decisions before wiring) + +## WS-24 [major, cross-crate] — Top-level dispatch never consults the connection overlay; the ADR-047 §4 amendment mechanism cannot resolve open ops on the wire + +**Verified:** YES. The amendment +(`alkcall/docs/architecture/decisions/047-openable-alpns-are-operations.md`, +"Amendment (§4 per-connection registration, 2026-08-13)") decides: +*"open ops are registered per-connection … `register_openable` is +called on the connection overlay registry (Layer 2 per ADR-019), +closing over a per-connection `ChannelCore`."* But the dispatcher's +top-level resolution never looks at that layer: +`Dispatcher::dispatch` reads the op's type from +`self.registry.registration(...)` (`dispatch.rs:316-320`) and invokes +via `self.registry.invoke/invoke_streaming/invoke_sink` +(`dispatch.rs:330-341`) — the base registry only. The connection +overlay (`CallConnection::overlay_env()`, +`connection.rs:174`; `OverlayOperationEnv`) is reachable solely as a +layer of `context.env` via `compose_root_env` +(`dispatch.rs:195-215`) — i.e., for *nested* invocation by handlers, +not for resolving the incoming `call.requested` itself. An open op +registered per the amendment's mechanism would resolve `NOT_FOUND` +on the wire. + +The only shape proven to work is alkcall's own e2e test +(`channels/client.rs:767-870`): the hook builds a **fresh +per-connection registry containing the open op and passes it as the +dispatcher's base registry** (`Dispatcher::new(registry, ...)` at +`:837`). That shape works but implies a per-session fork of the +deployment's base registry (base ops + generic channel ops + +openables) — and `OperationRegistry`/`HandlerRegistration` have no +`Clone`/fork surface (registration.rs:68-75, 97-100; the inner +payloads `OperationSpec`/`HandlerKind` do derive `Clone`), so the +fork needs an alkcall-side addition or a re-registration walk. It +also implies per-session re-registration of `services/list` / +`services/schema` closed over the fork, or per-session open ops stay +undiscoverable (alkcall ADR-047 §6's "what may I open" half). + +This is the "planning bug": OQ-05 says *nothing new to design*; +mechanically the alkcall machinery is done, but the registration +*mechanism* the ADR names does not dispatch. Either resolution is +fine — the decision just needs to be made deliberately: + +- **(a)** per-session base registry (the proven shape): alkcall + amends ADR-047 §4's mechanism wording and adds a fork/clone + surface on `OperationRegistry` (most registration payloads already + `Clone`); alkhttp builds the per-session fork in the hook. No + dispatch-path change; discovery re-registered per session. +- **(b)** overlay-aware top-level dispatch: alkcall adds an overlay + fallback in `dispatch`/`run_loop_single_stream` (consult + `connection.overlay_env()` when the base registry misses). Larger + blast radius (touches the shared dispatch loop); keeps one static + registry; makes the ADR-047 amendment's wording true as written. + +Cross-crate: the decision and (for (a)) the fork surface live in +alkcall; alkhttp consumes whichever lands. + +## WS-25 [major] — No wire mechanism for a WS client to register an op; ADR-048's browser-side bidirectionality is undesignable as specified + +**Verified:** YES. The call protocol's envelope kinds are +`call.requested` / `call.responded` / `call.completed` / +`call.aborted` / `call.published` / `call.error` +(`alkcall/src/protocol/wire.rs:12-17`) — there is no +registration/announce kind, and grep-verified no wire op by which a +client could register a handler spec. Client-side registration +onto an overlay (`register_imported` / `register_imported_all`, +`connection.rs:162-172`) is reached only in-process — the +`from_call`/`from_wss` import path (a Rust consumer importing a +remote peer's ops) and tests. Every "browser-registered op" test +pokes the overlay directly in-process +(`tests/ws_overlay_ops.rs:210-251`; the alknet-http `overlay.rs` +prior art is the same shape). So the decided behavior — *the hub can +call browser-registered ops over the same session* +(websocket.md §"Bidirectionality"; ADR-048's reconciliation note +records it as decided design, deferred) — has no wire contract for +the browser half: nothing tells the hub the op exists, and the +browser cannot run a Rust-side `import()`. + +This is an alkcall-level protocol design gap (a new envelope kind / +handshake, an explicit "not supported for WS-native clients" scope +cut, or a channel-0 bootstrap op). It does **not** block the +data-channel half (open ops are hub-registered; the browser only +calls them), which is why Unit 1 can decide WS-24 first and WS-25 in +parallel — but ADR-048's promise should not be treated as +implementable by wiring alone. + +## WS-26 [major] — The upgrade path retains no live connection handle; the hub cannot reach a session's overlay even once registration exists (and cannot push anything, ever) + +**Verified:** YES. `run_channels_session` hands channel 0's +`Connection` into the hook, which moves it into the spawned +dispatcher task (`upgrade.rs:255-287`) — nothing retains a handle the +assembly layer can reach. `WsSessions` tracks pump handles only (the +WS-08 abort lever). Consequences: ADR-048's "the hub holds that +connection as a live handle" and "the hub reaches browser ops through +the connection handle, not through `PeerRef::Specific`" +(websocket.md §"Browsers are not alk peers") has no object to hold — +the deployment cannot enumerate live WS sessions, cannot +hub→browser-call an overlay op even after WS-25 defines +registration, and cannot push-initiate anything toward a session. +The alknet prior art retained the handle +(`alknet-http/src/websocket/upgrade.rs:104` — +`Arc::new(CallConnection::new_overlay_only(identity))` held by the +session state). + +**Fix:** rides the WS-20 rework — the hook (or the session) retains an +`Arc` in a deployment-visible registry (shape decided +in Unit 2 alongside the openables surface; eviction hooks into +`WsSessions` so handles die with the session). + +## WS-27 [minor] — `adapter_install_channel_zero` is compiled unconditionally on `server` under `#[allow(dead_code)]`; its only caller is test-gated + +**Verified:** YES. `src/websocket/upgrade.rs:290-295` exports the +`pub(crate)` wrapper; `mod.rs:36-37` re-exports it under +`#[cfg(feature = "server")]`; the sole caller is `from_wss`'s +`#[cfg(all(test, feature = "server"))]` test server +(`src/adapters/from_wss.rs:352,394`). The `#[allow(dead_code)]` +documents the current shape but the cleaner cut is gating the wrapper +with the same cfg as its caller — moot after WS-20's rework, which +will give both the upgrade path and the test server a real hook; +fold the cleanup into that task. + +--- + +# Non-findings (verified correct, recorded to bound the re-review) + +- **Channel-id collision:** accept side allocates even ids starting + at 2, connect side odd starting at 1 + (`alkcall/src/channels/manager.rs:105-125`); `adopt_channel` is the + non-allocating side's install and returns `ChannelExists` on + collision (`manager.rs:273-309`). The WS browser (connect side) and + hub (accept side) allocation split is safe. +- **Demux hardening already landed** (review-001 WS-12's cross-crate + fix): `TooLarge` skip with a 64 KiB stack buffer, cumulative + 256 MiB skipped-bytes budget, EOF-mid-skip ends the loop + (`alkcall/src/channels/adapter.rs:131-222`) — the DoS shape is + bounded before browser traffic ever hits it. +- **Open-op wrapper completeness:** `register_openable` covers + Query/Mutation (`Once`), Sub (`Stream`, one envelope then + out-of-band data), Pub (loud `not_implemented` stub pending the + channel-adoption path, alkcall C-08) — cap check → allocation → + ledger → spawn → teardown decrement all wired + (`alkcall/src/channels/operations.rs:399-453, 483-555`). +- **Idle-knob vs silent data channels** is already documented with a + deployment posture (websocket.md §"Idle-read timeout": progress- + based deadline, `with_ws_idle_timeout(None)` posture, no + keepalive by decision). Wiring data channels makes the 60 s + default bite more often (an idle SSH-style channel is + silent-but-alive); the remediation plan should surface that knob + in the task's docs note, not change the semantics. +- **`NoCap` default on the built-in surface** is review-001 SRV-10's + recorded resolution (`ChannelsPolicy` extension exists for + deployments; `upgrade.rs:308-317, 391-393`) — unchanged by this + review. + +--- + +# Remediation plan + +Sequenced by dependency; Unit 1 is a decision task and gates the +shape of Unit 2. + +## Unit 1 — alkcall decision task (WS-24, WS-25) + +Decide the open-op dispatch-resolution mechanism and the client-side +op-registration question; record as alkcall ADR amendments (ADR-047 +§4 mechanism; ADR-048's bidirectionality scope) with alkhttp-side +consequences noted. + +- WS-24: pick (a) per-session base registry (amend ADR-047 §4's + mechanism wording; add the `OperationRegistry` fork/clone surface) + or (b) overlay-aware top-level dispatch. Gate: an alkcall e2e test + where an open op registered per the *amended* mechanism resolves + via `run_loop_single_stream` dispatch over a channels connection. +- WS-25: decide the client-registration story (new envelope kind / + channel-0 bootstrap op / explicit scope cut). Gate: the ADR (or + scope-cut note) exists and OQ-05/ADR-048 point at it. + +## Unit 2 — alkhttp wiring (WS-20, WS-21, WS-22, WS-26) + +- `OpenableAlpn` list + `HttpAdapter::with_ws_openable_alpns(...)`, + threaded through `RouterState`/`SessionState` with a + request-extension fallback (mirroring `ChannelsPolicy`). +- Rework `install_channel_zero`: take the manager + policy + + openables; build the per-connection `ChannelCore`; register + `ChannelOperations` generic ops + the openables (per Unit 1's + shape); retain the `Arc` in a deployment-visible, + `WsSessions`-evicted handle registry. +- `adapter_install_channel_zero` cleanup (WS-27) rides this rework. +- Gate: a live WS session lists `channels//sub` in + `services/list` (per Unit 1's discovery shape) and an open op on + channel 0 returns a `channel_id`. + +## Unit 3 — e2e tests (WS-23) + +Using `test_support::WsClient` (the Rust-native-consumer shape): + +1. open → `{channel_id}` → chunks flow both directions on that + channel → the `OpenHandler`'s `Connection` sees the bytes. +2. `channel/close` → handler EOF + opener-ledger decrement + (`policy` count drops). +3. per-identity cap denial (`channel:too_many_channels`) and + per-connection cap (`channel:too_many_channels` from the manager). +4. disconnect mid-open → teardown cascade, no leak. +5. `TooLarge` chunk on a data channel survives (demux resync) — + already proven alkcall-side; assert it through the WS path. + +## Unit 4 — spec reconciliation (docs) + +- OQ-05 → resolved (or narrowed to the remaining piece), with the + consumer-set reframe recorded (WS-23): browser + native fallback; + OQ-04 not blocking. +- ADR-067 v1-cut note and ADR-048 reconciliation note updated to the + landed state; websocket.md §"Data channels for browsers" status + block removed; the alknet-ADR-044 §5 pointer checked for staleness. +- websocket.md gains the idle-knob deployment note for silent data + channels (already true, just surfaced where a deployer reads). + +--- + +## Verification log (this pass) + +- All findings carry `file:line` references verified against tree + `58665f2`. +- The WS-24 dispatch claim was triple-checked: `dispatch()`'s + op-type lookup (`dispatch.rs:316-320`) and invoke paths + (`:330-341`) consult `self.registry` only; `OverlayOperationEnv` + (`connection.rs:731-770`) is reachable only via + `compose_root_env` → `context.env` layers + (`dispatch.rs:195-215`); alkcall's e2e test passes the + per-connection registry **as** the dispatcher's base registry + (`channels/client.rs:789-837`), not as an overlay. +- The WS-25 claim was grep-verified across alkcall `src/`: the only + `register_imported*` callers are in-process import paths + (`protocol/adapter.rs:541,610`, `client/from_call.rs:71` doc); + the wire kind set is closed over the six `call.*` constants. +- The WS-20 reference shape was read in full + (`channels/client.rs:767-870`), including the fresh-registry + detail that motivates the fork-surface ask. +- `adapter_install_channel_zero`'s caller census: + `from_wss.rs:394` only (cfg(test) module). +- Baseline gates re-run for this pass (see above): cargo test + (default + all-features), clippy (both), fmt. No coverage run — + no source changed. \ No newline at end of file