alkdev/alkhttp
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(review 007): fresh-eyes pass over the landed WS data-channel surface

Browse Source 
First review whose subject is the landed wiring itself (review 006
Units 2-4, 030c5ef/2053420/64fa10b), not the gap to it.

Findings:
- WS-28 [major] — the WS-26 ConnectionGuard is bound inside the
  `if let` block and drops when it ends, not when the channel-0 task
  ends: live_connections() is permanently empty for every session.
  Reproduced empirically through the live WS path (scratch test,
  run + removed). ADR-048's landed note and review-006's log record
  behavior the code does not have.
- WS-29 [minor] — the op/register ACL override recorded as landed
  (review-006 UP-02, ADR-048) is not implemented; the hook hardcodes
  AccessControl::default() and ChannelsPolicy cannot express an op ACL.
- WS-30 [minor] — the bare-registry default session cap is built per
  request (FromRef) and bounds nothing; corrects review-002 WS-17.
- WS-31 [minor] — install_bootstrap_discovery silently shadows a
  deployment's own services/list on WS sessions (upstream-mandated,
  unrecorded).
- WS-32 [minor] — the router-state openables threading
  (with_ws_openable_alpns) has no gate; every Unit-3 gate rides the
  extension fallback.

Plus non-findings bounding the re-review (UP-01 arm, policy threading,
from_wss exclusion set, extension precedence, gate fidelity) and a
sequenced remediation plan.

Baseline at 64fa10b: 454 / 582 tests, clippy both, fmt, doc — all clean.
This commit is contained in:
glm-5.3-flash
2026-09-05 00:46:22 +00:00
parent 64fa10be31
commit b36ec49cc1
1 changed files with 371 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/reviews/007-ws-data-channel-surface-review.md
+371
View File
@@ -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<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).