diff --git a/docs/reviews/006-alkcall-0.3.0-consequence-review.md b/docs/reviews/006-alkcall-0.3.0-consequence-review.md index 43e02e8..a0db8fc 100644 --- a/docs/reviews/006-alkcall-0.3.0-consequence-review.md +++ b/docs/reviews/006-alkcall-0.3.0-consequence-review.md @@ -2,8 +2,10 @@ ## Status -Verified, open for Unit 2. One new finding (UP-01); the 0.3 bump -committed and verified (`3dee46a`). +Unit 2 + Unit 3 remediated and verified (`030c5ef`). Remaining: Unit 4 +(docs reconciliation). UP-02 decided (recorded below). One new +upstream gap surfaced during the gates (UP-03 — alkcall-side). See +Remediation log. ## Scope @@ -321,39 +323,182 @@ the one review-005's original gate missed and G-02 called out). --- -# Remediation plan (alkhttp side; supersedes review 003's plan where noted) +# Remediation log -Review 003's Units 1 and 4 are partially closed by upstream; the -remaining work re-sequences as: +## Unit 2 — alkhttp wiring (WS-20/21/22/25/26/27 + UP-01) — LANDED (`030c5ef`) -## Unit 2 — alkhttp wiring (WS-20/21/22/26/27 + UP-01) — next +**Hook rework (WS-20/21/25).** `install_channel_zero` +(`src/websocket/upgrade.rs`) now takes `(registry, sessions, policy, +openable_alpns)` and, per the alkcall ADR-047 §4 amendment #2 shape +(verified against alkcall's own e2e reference +`fork_registry_open_op_resolves_and_is_discoverable`): -1. `OpenableAlpn { spec, open_handler }` + - `HttpAdapter::with_ws_openable_alpns(...)`, threaded - `RouterState` → `SessionState` → `run_channels_session` → hook, - with a request-extension fallback (mirror `ChannelsPolicy`). -2. Hook rework per the upstream reference shape: fork → channel ops - → openables → bootstrap discovery → `op/register` (ACL per - UP-02's decided posture) → dispatch over the fork; retain the - `Arc` in a `WsSessions`-evicted registry (WS-26); - `adapter_install_channel_zero` cfg cleanup (WS-27). -3. UP-01: `ALREADY_EXISTS` → 409 in the gateway error map + test. +1. Forks the deployment's base registry. +2. Registers on the fork: `ChannelOperations::register_on` (the + generic `channel/close` / `channel/control` / + `channel/resources/subscribe` — WS-21), the deployment's openables + via `ChannelCore::register_openable` (WS-22), + `install_bootstrap_discovery` closed over the fork (F-06 — the + session's `services/list` sees its own openables), and + `op/register` (WS-25 — `op_register_spec` + + `op_register_handler(connection, fork)`; the collision set is the + fork, the production shape review-005's gates use). +3. Dispatches over the fork (`Dispatcher::run_loop_single_stream` + unchanged as the loop; the loop's pending-resolution arms are the + review-005 G-01 mechanization the bump inherited). -Gate: a live WS session lists its openable in `services/list`; an -open op on channel 0 returns a `channel_id`; `op/register` collides -loudly against fork-registered names (409 over the WS path). +**UP-02 decided.** `op/register` is registered with +`AccessControl::default()` on the built-in surface (the SRV-10 +precedent: permissive crate default), and an assembly layer overrides +via a stricter policy passed through the hook — the posture mirrors +`ChannelsPolicy`. The override surface rides the existing `policy` +parameter; the assembly layer's `ChannelsPolicy` value is the same +object the open wrappers consult. -## Unit 3 — e2e tests (WS-23) — after Unit 2 +**Openable-ALPN surface (WS-22).** `OpenableAlpn { spec, +open_handler }` + `HttpAdapter::with_ws_openable_alpns(Vec<..>)`, +threaded `RouterState` → `SessionState` → `run_channels_session` → +hook, with the `OpenableAlpns` request-extension fallback on the +upgrade handler (mirroring `ChannelsPolicy`/`WsTimeouts`). Default: +no openables (channel 0 only). -Review 003's five scenarios stand, plus the two upstream-mirroring -gates named in Part B (openable discoverable on the live session; -announce → nested composition → routes back). +**WS-26.** `WsSessions` gains a `connections` map + +`live_connections()` / `live_connection_count()`; the hook retains +the channel-0 `Arc` under a self-removing +`ConnectionGuard` (dropped when the channel-0 task ends — any +teardown path). Shared only when a shared `WsSessions` instance is in +play (the bare-registry/test-server path passes `None`). -## Unit 4 — spec reconciliation (docs) — after Unit 2 +**Policy threading.** The hook takes the session's resolved +`ChannelsPolicy` (the extension resolution, `NoCap` default) and +passes it to both `ChannelOperations` and `ChannelCore` — one policy +instance across the open wrappers and the demux-loop teardown path. +(First cut used a fresh `default_policy()` per hook; the +ledger-decrement gate caught it — the `ChannelsPolicy` extension was +never consulted. Fixed by threading; that interaction is exactly the +seam SRV-10's extension was built for.) -Review 003's list stands (OQ-05 resolution, ADR-067/ADR-048 notes, -websocket.md status block, idle-knob deployment note), with the -`op/register` ACL posture recorded per UP-02. +**UP-01.** `ALREADY_EXISTS` → 409 Conflict in +`call_error_to_http_status_with_identity` + the test arm; the module +doc names the seventh code. + +**from_wss import exclusion (new in this unit, surfaced by the +gates).** The reworked hook serves the bootstrap + channel-lifecycle +set on every WS session, so a `from_wss` import's `services/list` +listing now carries them. Proxying session-scoped machinery would be +nonsense (`channel/close` proxied to the remote would forward a close +for *this* session's channel ids; `op/register` proxied duplicates +the import itself). `FromWss::import` now runs discovery first and +passes the listing minus the protocol-session ops +(`services/list`, `services/schema`, `services/list-peers`, +`op/register`, `channel/close`, `channel/control`, +`channel/resources/subscribe`) as the `operation_filter` — the +`from_call` filter surface, used for the first time by alkhttp. The +three from_wss discovery tests assert the original op sets and pass. + +**WS-27.** `adapter_install_channel_zero` cfg-gated to exactly its +caller's cfg (`all(test, feature = "server", feature = "wss")`); the +`#[allow(dead_code)]` is gone; it inherits the reworked hook (the +from_wss test-server producer now serves the session ops too). + +**Gate results (Unit 3, `tests/ws_upgrade_session.rs`):** + +- `data_channel_open_discoverable_and_bytes_round_trip` — + `services/list` lists the session's openable + generic channel ops + + `op/register` (F-06 shape); open op returns an even `channel_id`; + a data chunk echoes back through the `OpenHandler`'s `Connection`. +- `channel_close_resolves_and_decrements_the_opener_ledger` — close + resolves `{"closed": true}`; the ledger drops to 0. +- `channel_cap_denies_open_over_the_limit` — the second open over the + cap denies with the `channel:too_many_channels` code (the + `ChannelsPolicy` extension is consulted). +- `disconnect_mid_open_tears_down_and_decrements` — hard disconnect → + the demux teardown decrements the ledger, no leak. +- `too_large_data_channel_chunk_survives_demux_resync` — a + `MAX_CHUNK_LEN + 1` declared chunk is skipped (the skip consumes + the declared payload off the transport, so the gate sends the + filler), the next chunk routes and echoes — resync through the WS + path (scenario 5). +- `op_register_served_per_session_and_collision_is_already_exists` — + announce resolves `registered: true`; a second announce without + `replace` rejects with the overlay gate's `ALREADY_EXISTS`; + announcing the serving side's own op name with `replace: true` + rejects `ALREADY_EXISTS` (the G-03 gate, through the WS path). +- `disconnect_mid_call_does_not_hang_the_server` and the full + pre-existing suite stay green. + +Test-harness hardening the gates required (recorded so the next +pass doesn't re-derive it): `call_and_await` filters by request id +(a prior Sub's trailing `call.completed` may interleave) and +tolerates data-channel chunks; `await_envelope` is the same filter +for hand-built frames; `CallError`-payload envelopes serialize flat +(`env.payload["code"]`, not `env.payload["error"]["code"]`). + +**Verification (post-fix):** + +``` +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 +``` + +--- + +## Unit 4 — spec reconciliation (docs) — open + +As planned in this review's remediation plan: OQ-05 resolution → +resolved, ADR-067 v1-cut note + ADR-048 amendment note updated, +websocket.md §"Data channels for browsers" status block removed, the +idle-knob deployment note surfaced, and the `op/register` ACL posture +(UP-02's decision) recorded where a deployer reads. + +--- + +## Findings added during remediation + +### UP-03 [minor, alkcall-side] — `services/list-peers` cannot list peer-announced ops; `PeerCompositeEnv` does not override `peer_operations` + +Discovered by the Unit-3 gates: the first draft asserted announced-op +discoverability through `services/list-peers` and failed — the peers +array carries only `peer_id` entries with empty operation lists. + +**Verified in both the published 0.3.0 crate and the alkcall tree:** +`services_list_peers_handler` maps `ctx.env.peer_operations(&peer_id)` +per peer (`alkcall/src/registry/discovery.rs:347-351`), but +`PeerCompositeEnv` overrides `peer_ids` only — `peer_operations` falls +to the trait default (`Vec::new()`, +`alkcall/src/registry/env.rs:63-65`); the `connections` map and +`connection_order` it would list from are in scope but unused there. +`OverlayOperationEnv` likewise overrides `contains` only. Net effect: +the peer-keyed listing shows `peer_id` entries whose `operations` +array is always empty, and the ADR-022 amendment's "announced op is +discoverable via `services/list-peers`" promise does not resolve on +the wire today. The alkcall gates never caught it because the F-05 +gate calls the announced op directly (bypassing discovery) and the +G-02 gate composes it — neither lists peers after announcing. + +**Fix lives in alkcall** (one method override: +`PeerCompositeEnv::peer_operations(peer)` → the connection env's +overlay registration names, ACL-filtered like the local listing; +`OverlayOperationEnv` may want a matching helper). Flagged for the +next alkcall review pass; the alkhttp-side gate +(`op_register_served_per_session_and_collision_is_already_exists`) +asserts the overlay-landing + collision semantics instead, which is +the behavior Unit 2 actually wires, and is not blocked by UP-03. + +### UP-02 decision (recorded) + +`op/register` is served per WS session with +`AccessControl::default()` on the built-in surface (the SRV-10 +posture: the crate default is permissive, deployments gate via the +`ChannelsPolicy` extension whose value the hook now consults). The +wire-callability tension in this review's UP-02 note resolves the +same way: the peer must call `op/register` (it is in the bootstrap +set), so Internal visibility was never an option; the ACL parameter +is the deployment's gate, not the visibility flag. ---