diff --git a/docs/reviews/007-ws-data-channel-surface-review.md b/docs/reviews/007-ws-data-channel-surface-review.md new file mode 100644 index 0000000..b2d2eea --- /dev/null +++ b/docs/reviews/007-ws-data-channel-surface-review.md @@ -0,0 +1,371 @@ +# Review 007 — Post-remediation review of the landed WS data-channel surface (review 006 Units 2–4) + +## Status + +Open for remediation. + +## Scope + +Fresh-eyes review of the now-landed WS surface — the tree after review +006 fully closed: the data-channel + `op/register` wiring (Unit 2, +`030c5ef`), the e2e gates (Unit 3, same commit), the 0.3.1 consumption +(`2053420`), and the spec reconciliation (Unit 4, `64fa10b`). This is +the "natural next work" review 006's status block pointed at: the first +pass whose subject is the landed wiring itself, not the gap to it. + +The pass covered: + +- the reworked `install_channel_zero` hook end to end + (`src/websocket/upgrade.rs` — fork, per-session registrations, + `op/register`, the dispatch loop, the WS-26 handle retention), +- the deployment surface (`HttpAdapter` builders, `RouterState` / + `SessionState`, the three request extensions and their precedence), +- the Unit-3 gates (`tests/ws_upgrade_session.rs`) against the code + they claim to pin, and the two surfaces the gates do not touch, +- the from_wss import exclusion against the exact set the hook now + serves (`src/adapters/from_wss.rs`), +- the UP-01 error-table arm (`src/gateway/error.rs`), and +- the Unit-4 spec records (ADR-048's landed note, OQ-05's resolution, + websocket.md, ADR-067) against the code they describe. + +Findings continue review-003/006's WS numbering (last used WS-27). +Cross-crate references are to alkcall 0.3.1 as consumed by the +lockfile. + +## Baseline verification (this pass) + +``` +cargo test → 454 passed, 0 failed +cargo test --all-features → 582 passed, 0 failed +cargo clippy --all-targets -- -D warnings → clean +cargo clippy --all-features --all-targets -- -D warnings → clean +cargo fmt --check → clean +cargo doc --no-deps → clean +``` + +Verified at tree `64fa10b`, working tree clean. No source changes were +made in this pass; findings are analysis-verified, and the headline +finding (WS-28) was additionally reproduced empirically through the +live WS path with a scratch integration test (since removed — see the +verification log). + +## Verdict + +- **The wiring is sound where it is exercised.** The fork shape matches + the upstream reference (`fork → register_on → register_openable → + install_bootstrap_discovery → op/register → dispatch over the fork`), + the policy threading is the single-instance shape the ledger gate + caught the first draft missing, the extension precedence mirrors + `ChannelsPolicy`/`WsTimeouts` exactly, UP-01's 409 arm is in with its + test, and the from_wss exclusion set matches the served bootstrap set + name-for-name. The six Unit-3 gates do what they claim. +- **One major defect: WS-26's landed surface is dead on arrival.** The + `ConnectionGuard` that should hold the session's `Arc` + in `WsSessions` for the channel-0 task's lifetime is bound *inside* + the `if let Some(sessions)` block — it drops when that block ends, + microseconds after insertion, while the dispatcher task is still + starting. `live_connections()` / `live_connection_count()` are + permanently empty for every session, on every path. Reproduced + empirically (WS-28). ADR-048's landed note ("the session retains its + live `Arc` in `WsSessions`") and review-006's + remediation log describe behavior the code does not have; nothing + else consumes the handles yet, which is why the Unit-3 gates — none + of which assert handle visibility — stayed green. +- **Two decision records describe surfaces that don't exist, and one + upstream-mandated consequence is unrecorded (WS-29/30/31):** the + `op/register` ACL override that review-006's UP-02 log and ADR-048's + landed note both claim deployments can apply is not implemented (the + ACL is hardcoded permissive); the bare-registry "default session cap" + is constructed per request and therefore bounds nothing; and the + bootstrap discovery install silently shadows a deployment's own + `services/list` on WS sessions. +- **One coverage gap (WS-32):** every Unit-3 gate rides the + `OpenableAlpns` request-extension fallback on bare-registry routers; + the built-in router-state threading (`with_ws_openable_alpns` → + `RouterState` → `SessionState` → hook) has no gate at all. + +## Severity legend + +Same scale as reviews 002/003/006: **[major]** — a decided behavior is +missing, wrong, or a real reliability hazard; **[minor]** — drift, +convention violation, or doc/spec inconsistency with no (or marginal) +correctness impact. + +--- + +# Findings + +## WS-28 [major] — The WS-26 `ConnectionGuard` drops at the end of the `if let` block, not when the channel-0 task ends: `live_connections()` is permanently empty + +**Verified:** YES (empirically). `src/websocket/upgrade.rs:451-459` — +the guard is created and bound *inside* the block whose binding it is +meant to outlive: + +```rust +if let Some(sessions) = &sessions { + let id = sessions.insert_connection(Arc::clone(&call_connection)); + let _conn_guard = ConnectionGuard { sessions: sessions.clone(), id }; + // The guard lives for this task — its drop removes the + // handle when the dispatcher loop returns. +} + +let dispatcher = … Dispatcher::new(fork, …); +dispatcher.run_loop_single_stream(…).await; // the loop the comment names +``` + +`_conn_guard` is a block-scoped binding: its `Drop` runs when the `if +let` block closes — immediately, before `Dispatcher::new` is even +constructed — removing the handle it just inserted. The comment +("lives for this task … when the dispatcher loop returns") describes +the intended scope, not the actual one. Reproduced through the live +path: a scratch integration test established a WS session, completed a +call on channel 0 (the dispatcher task fully up), confirmed the pump +registry saw the session (`sessions.len() == 1`), and then polled +`live_connection_count()` for 2 s — it stayed 0. (Scratch test removed +after the repro; the fix's acceptance gate re-adds it.) + +Consequences, in order of what breaks first: + +- **WS-26's deliverable does not function.** The deployment-visible + handle registry — the object ADR-048's landed note says gives §4's + hub→browser direction "its object" — is empty from the microsecond + each session starts to the moment it ends. There is never a handle to + reach, so hub→session composition through `live_connections()` is + unimplementable by callers today. +- **ADR-048:40-53 and review-006:367-372 record behavior that does not + exist.** Both say the session "retains its live `Arc` + in `WsSessions` … dropped when the channel-0 task ends — any teardown + path". The retention exists for microseconds only. +- **Nothing else regresses.** The pump registry (`SessionGuard`) is a + separate, correctly-scoped mechanism — `ws_sessions_registry_tracks_ + and_aborts_live_sessions` passes because that guard is bound in + `run_channels_session`'s frame, which genuinely lives for the + session. No other consumer reads `connections`. That is also why all + 454/582 tests stay green: the defect is only observable through the + new surface itself. + +**Fix (small):** bind the guard outside the block — `let conn_guard = +sessions.as_ref().map(…)` as an `Option` in the +task's frame (the same shape `run_channels_session` already gets +right), or move the `if let` body's guard construction up. Acceptance: +re-add the scratch repro as a Unit-3 gate (`live_connections` sees the +handle mid-session, drains after teardown) — it is the only test that +exercises the WS-26 surface, and its absence is why `030c5ef` landed +green. + +## WS-29 [minor] — The `op/register` ACL override is recorded as landed (review-006 UP-02, ADR-048) but is not implemented + +**Verified:** YES. The decided posture (review-006:352-358): *"`op/ +register` is registered with `AccessControl::default()` on the +built-in surface … and an assembly layer overrides via a stricter +policy passed through the hook."* ADR-048:50-53 repeats it +("deployments gate via a stricter `ChannelsPolicy` passed through the +hook"). The landed code hardcodes the permissive ACL: +`upgrade.rs:429-443` registers +`op_register_spec(AccessControl::default())` with no parameter in the +hook signature (`install_channel_zero(registry, sessions, policy, +openable_alpns)` — `upgrade.rs:367-372`) and no extension surface. The +`ChannelsPolicy` value named by both records is +`Arc` — the channel-cap policy consulted +by the open wrappers and the demux teardown (`upgrade.rs:412-426`); it +cannot express an operation `AccessControl`. A deployment that wants +to restrict which authenticated peers may announce ops has no surface +to do it with, on either the built-in or the extension path. + +Impact is bounded — an announced op lands `Visibility::Internal` / +`FromCall` (composition material only), the G-03 gate prevents +shadowing the serving registry, and the announce surface requires an +authenticated session — which is why this is not [major]: the +permissive default is itself the recorded decision; what is wrong is +the second half (the override) being recorded as available when it +does not exist. **Fix (pick one):** thread an `AccessControl` (or an +`op_register_spec` override) through the hook the way the policy +threads, with an `OpRegisterAcl` extension mirroring +`ChannelsPolicy`/`OpenableAlpns`; or amend review-006's UP-02 log and +ADR-048's landed note to say the override surface is pending and name +it when it lands. The first option is a ~20-line change; the second is +two paragraphs. + +## WS-30 [minor] — The bare-registry "default session cap" is constructed per request and bounds nothing (also corrects review-002 WS-17's claim) + +**Verified:** YES. For a bare-registry upgrade route (router state = +`Arc`), `FromRef` builds the handler's +`SessionState` per request (`upgrade.rs:164-168` → `from_registry`, +`upgrade.rs:114-122`): a fresh 64-permit semaphore and a fresh +handler-private `WsSessions` **per request**. The doc comment +("custom upgrade routes / integration tests get … the default cap", +`upgrade.rs:91-95`) and review-002 WS-17's record ("one authenticated +client per custom route is bounded at 64 sessions/60 s", +review-002:186-189) both read as a per-route bound; in reality every +upgrade acquires its permit from its own brand-new semaphore, so the +cap never accumulates across requests and bounds nothing. The idle +knob (WS-01) and write window remain real per-session knobs; only the +cap and the (already "not shared") eviction registry are vacuous. The +built-in `RouterState` path is correct — `state.rs:58-68` clones the +shared semaphore, and the 503 gate (`session_cap_rejects_over_limit…`) +exercises that path. + +This is the WS-17 class one layer short of its fix: WS-17 asked for +extension surfaces, `WsTimeouts`/`WsSessions`/`OpenableAlpns` landed +(the `WsSessions` extension *is* consulted, `upgrade.rs:572-591`) — +but the session-cap half got no extension and no correction of the +review-002 claim. Bare-registry routes are a real deployment shape +(ADR-046 custom surfaces). **Fix:** a `SessionSlots` request extension +mirroring the other three (or a documented `Arc` in the +bare-registry state contract), plus a one-line correction in +review-002's WS-17 entry; minimum viable is fixing the `SessionState` +doc to say "per-request (no effective cap)". + +## WS-31 [minor] — The bootstrap discovery install silently shadows a deployment's own `services/list` on WS sessions + +**Verified:** YES. The hook forks the base registry (which deep-copies +the deployment's registrations, including any custom +`services/list`), then calls `install_bootstrap_discovery(&fork)` +(`upgrade.rs:428`) — which registers alkcall's `services/list` / +`services/schema` / `services/list-peers` closed over the fork +(`alkcall/src/registry/discovery.rs:293-318`). `register` inserts by +name (`alkcall/src/registry/registration.rs:184-187`), so the fork's +bootstrap ops **overwrite** the deployment's own discovery ops for +every WS session. The overwrite is upstream-mandated (the F-06 shape: +per-session discovery must see the fork's openables — a deployment +handler closed over the base registry cannot) and arguably correct, +but it is unrecorded: a deployment with a custom `services/list` (the +`registry_with_services_list` shape in the test suite) silently gets +alkcall's listing on WS sessions while every other transport keeps +theirs. The existing gate passes only because its assertions hold for +both handlers. **Fix:** a sentence in websocket.md §"Data channels for +browsers" (or the ADR-067 landed note): WS-session discovery is the +bootstrap set; a base-registry `services/*` registration is shadowed +on the WS path by design. + +## WS-32 [minor] — The router-state openables threading has no gate; every Unit-3 gate rides the extension fallback + +**Verified:** YES. `HttpAdapter::with_ws_openable_alpns` → +`RouterState.ws_openable_alpns` → `SessionState` → +`run_channels_session` → hook (`adapter.rs:282-299`, `state.rs:58-68`, +`upgrade.rs:296-297, 601-603`) is the built-in surface — the path a +real deployment (an `HttpAdapter` consumer, not a hand-built axum +router) uses. Every openables-bearing gate in +`tests/ws_upgrade_session.rs` constructs the bare-registry router + +`OpenableAlpns` extension instead (`spawn_openable_ws_server`, +tests/ws_upgrade_session.rs:1039-1079); grep-verified: no test touches +`with_ws_openable_alpns`. The threading is the same fields the +extension path exercises at the hook, so the risk is low — but the +builder is public API, its `RouterState`→`SessionState` hop is +distinct code, and the exact class of bug this pass found (WS-28) is +one a builder-path gate would have caught sooner. **Fix:** one +integration gate through `HttpAdapter::new(…).with_ws_openable_alpns(…)` ++ `.router()` (the session-cap test already shows the pattern, +tests/ws_upgrade_session.rs:740-747): open a data channel through the +built-in surface and assert the round trip. Rides the WS-28 gate work. + +--- + +# Non-findings (verified correct, recorded to bound the re-review) + +- **UP-01's arm is in and tested:** `ALREADY_EXISTS` → 409 + (`src/gateway/error.rs:32, 79`) with the test arm + (`error.rs:191-194`) and the module doc naming the seventh code + (`error.rs:5-7`). Matches review-006's fix description exactly. +- **Policy threading is the corrected shape:** one `Arc` instance flows from the `ChannelsPolicy` + extension resolution (or `NoCap`) through both + `ChannelOperations::new` and `ChannelCore::new` + (`upgrade.rs:296-299, 412-426`) — the ledger-decrement gate + (`channel_close_resolves_and_decrements_the_opener_ledger`) is the + test that pins the interaction SRV-10's extension exists for. +- **from_wss's exclusion set matches the served set name-for-name:** + the hook serves the bootstrap trio + `op/register` + the three + channel ops per session; `is_protocol_session_op` + (`src/adapters/from_wss.rs:338-346`) excludes exactly those seven + names from import. No drift in either direction. +- **Extension precedence is uniform:** `ChannelsPolicy`, `WsTimeouts`, + `OpenableAlpns`, and `Extension` each take precedence + over the `SessionState` values, with the same `Option>` + extraction shape (`upgrade.rs:571-603`); the bare-registry defaults + match the documented 60 s/60 s/64 trio. +- **The registration-failure path kills the session loudly:** a fork + registration error (schema compile) logs at `error` and returns + before the dispatch loop starts (`upgrade.rs:446-449`) — the session + ends via the dropped connection rather than mis-discovering. +- **`op/register` over the WS path exercises the full G-03 matrix:** + announce-ok, overlay re-announce → `ALREADY_EXISTS`, serving-registry + collision with `replace: true` → `ALREADY_EXISTS` + (`tests/ws_upgrade_session.rs:1309-1441`), plus the 0.3.1 + `services/list-peers` discovery step (`:1351-1374`) — the UP-03 + gate that fails against 0.3.0 and passes against 0.3.1, as recorded. +- **The demux-resync gate sends the filler bytes** the skip will + consume (`tests/ws_upgrade_session.rs:1506-1524`) — the subtle + detail (the `TooLarge` skip reads the declared payload off the + transport; a naive test would desync itself and pass for the wrong + reason) is handled. +- **Unit-4 spec records match the code they describe** — with the + single exception of the WS-26 retention claim (WS-28) and the + `op/register` override (WS-29). websocket.md's landed-surface + section, OQ-05's resolution, and ADR-067's landed note otherwise + agree with `030c5ef`/`2053420` as written, including the + idle-knob-sharpening note and the UP-02 ACL posture sentence (the + posture's default half is real; only the override half is not). + +--- + +# Remediation plan + +## Unit 1 — WS-28 (the guard scope) + its gate + +Bind the `ConnectionGuard` for the channel-0 task's lifetime +(`Option` in the task frame, mirroring +`SessionGuard`'s shape). Acceptance: the re-added repro gate — +`live_connections()` sees the handle while the session is live and +drains after teardown — plus the existing suite staying green. Update +the block comment to describe the (now real) scope. + +## Unit 2 — record corrections + the two small surfaces + +- WS-29: either thread the `op/register` ACL (hook param + + `OpRegisterAcl` extension) or amend review-006's UP-02 log and + ADR-048's landed note to "override surface pending". +- WS-30: `SessionSlots` extension (or a doc correction) + + the review-002 WS-17 one-liner. +- WS-31: the websocket.md/ADR-067 sentence. +- WS-32: the builder-path openables gate (rides Unit 1's test work). + +Verification: `cargo test` (default + all-features), clippy both, fmt, +doc — the standard gate set. + +--- + +## Verification log (this pass) + +- All findings carry `file:line` references verified against tree + `64fa10b`. +- WS-28 was reproduced empirically, not just read: a scratch + integration test (added, run, removed) established a live WS session + over `ws_upgrade_handler` + `ws_bearer_auth`, completed a channel-0 + call, asserted the pump registry saw the session, then polled + `live_connection_count()` for 2 s — stayed 0 (panicked at the assert, + message: "WS-28 REPRO"). The `Drop`-ordering mechanism was + independently confirmed with a minimal drop-log probe of the + `if let` binding shape. +- WS-29's "no override surface" claim: read the full hook signature + and body (`upgrade.rs:367-470`); grep-verified `op_register_spec`'s + single call site with `AccessControl::default()`; confirmed + `ChannelsPolicy`'s type (`Arc`) cannot + carry an op ACL. The recorded-posture quotes were checked against + review-006:352-358 and ADR-048:50-53. +- WS-30's per-request claim rests on axum `FromRef` semantics + (per-request extraction, no caching) applied to + `upgrade.rs:161-168`; the review-002 WS-17 quote was checked + (review-002:182-192). +- WS-31's overwrite claim: `register` → `insert` by name + (`alkcall/src/registry/registration.rs:184-187`); bootstrap + registration of all three names confirmed + (`alkcall/src/registry/discovery.rs:293-318`); the shadowing is + consistent with the existing `services_list_over_channel0…` gate + passing (its assertions hold for both handlers). +- WS-32's coverage census: grep for `with_ws_openable_alpns` across + `tests/` — zero hits; the only openables-bearing spawner is + `spawn_openable_ws_server` (extension path). +- Baseline gates re-run for this pass (see above): cargo test (default + + all-features), clippy (both), fmt, doc. No source changed; the + scratch repro was removed after the run (working tree clean). \ No newline at end of file