alkhttp/docs/reviews/007-ws-data-channel-surface-review.md

# Review 007 — Post-remediation review of the landed WS data-channel surface (review 006 Units 2–4)

## Status

Remediated (2026-09-05). WS-28: the `ConnectionGuard` now binds in the
channel-0 task's frame (`Option<ConnectionGuard>`, the `SessionGuard`
shape), with the re-added repro gate
(`live_connections_visible_mid_session_and_drain_after_teardown`) —
verified to fail against the pre-fix tree and pass with the fix. The
decisions taken during remediation, both the "implement" options: WS-29
landed the recorded override surface
(`HttpAdapter::with_ws_op_register_acl` + the `OpRegisterAcl` request
extension; builder + extension gates); WS-30 landed the `SessionSlots`
extension (the shared-cap surface for bare-registry routes, with the
review-002 WS-17 correction). WS-31: the discovery-shadowing sentence
is in (websocket.md §"Data channels for browsers", ADR-067's landed
note). WS-32: the builder-path openables gate
(`builder_path_openables_serve_the_data_channel_surface`). The
review-006 UP-02 / WS-26 log entries and ADR-048's landed note carry
corrections where the pre-fix code did not match the recorded claim.
Verification: the standard gate set (below), extended to the new gates.

## 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<CallConnection>`
  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<CallConnection>` 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<CallConnection>`
  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<ConnectionGuard>` 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<dyn ChannelLifecyclePolicy>` — 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<OperationRegistry>`), `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<Semaphore>` 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<dyn
  ChannelLifecyclePolicy>` 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<WsSessions>` each take precedence
  over the `SessionState` values, with the same `Option<Extension<_>>`
  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<ConnectionGuard>` 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<dyn ChannelLifecyclePolicy>`) 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).