docs(review 006): remediation log for Unit 2+3; UP-03 filed upstream-side
- Unit 2/Unit 3 recorded as landed (030c5ef): hook rework shape,
policy threading (the ChannelsPolicy seam the ledger gate caught),
WS-26 handle retention, from_wss protocol-op exclusion, UP-01, the
six new gates, and the harness hardening notes (request-id filter,
flat CallError payload shape).
- UP-02 decided: op/register serves per session with the default ACL
(SRV-10 posture); the visibility tension resolves as ACL-gate.
- UP-03 [minor, alkcall-side] filed: services/list-peers cannot list
peer-announced ops — PeerCompositeEnv overrides peer_ids but not
peer_operations (trait default Vec::new()), so the amendment's
peer-discovery promise does not resolve on the wire. Discovered by
the gates; fix lives in alkcall; alkhttp's gate asserts
overlay-landing + collision semantics instead and is not blocked.
- Unit 4 (docs reconciliation) remains open.
Verification unchanged: cargo test 454/582, clippy both sides, fmt,
doc — clean.
This commit is contained in:
glm-5.3-flash committed
1 parent
030c5efa51
commit
214fd213ff
1 file changed
+172
-27
@@ -2,8 +2,10 @@
|
|||||||
|
|
||||||
## Status
|
## Status
|
||||||
|
|
||||||
Verified, open for Unit 2. One new finding (UP-01); the 0.3 bump
|
Unit 2 + Unit 3 remediated and verified (`030c5ef`). Remaining: Unit 4
|
||||||
committed and verified (`3dee46a`).
|
(docs reconciliation). UP-02 decided (recorded below). One new
|
||||||
|
upstream gap surfaced during the gates (UP-03 — alkcall-side). See
|
||||||
|
Remediation log.
|
||||||
|
|
||||||
## Scope
|
## 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
|
## Unit 2 — alkhttp wiring (WS-20/21/22/25/26/27 + UP-01) — LANDED (`030c5ef`)
|
||||||
remaining work re-sequences as:
|
|
||||||
|
|
||||||
## 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 }` +
|
1. Forks the deployment's base registry.
|
||||||
`HttpAdapter::with_ws_openable_alpns(...)`, threaded
|
2. Registers on the fork: `ChannelOperations::register_on` (the
|
||||||
`RouterState` → `SessionState` → `run_channels_session` → hook,
|
generic `channel/close` / `channel/control` /
|
||||||
with a request-extension fallback (mirror `ChannelsPolicy`).
|
`channel/resources/subscribe` — WS-21), the deployment's openables
|
||||||
2. Hook rework per the upstream reference shape: fork → channel ops
|
via `ChannelCore::register_openable` (WS-22),
|
||||||
→ openables → bootstrap discovery → `op/register` (ACL per
|
`install_bootstrap_discovery` closed over the fork (F-06 — the
|
||||||
UP-02's decided posture) → dispatch over the fork; retain the
|
session's `services/list` sees its own openables), and
|
||||||
`Arc<CallConnection>` in a `WsSessions`-evicted registry (WS-26);
|
`op/register` (WS-25 — `op_register_spec` +
|
||||||
`adapter_install_channel_zero` cfg cleanup (WS-27).
|
`op_register_handler(connection, fork)`; the collision set is the
|
||||||
3. UP-01: `ALREADY_EXISTS` → 409 in the gateway error map + test.
|
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
|
**UP-02 decided.** `op/register` is registered with
|
||||||
open op on channel 0 returns a `channel_id`; `op/register` collides
|
`AccessControl::default()` on the built-in surface (the SRV-10
|
||||||
loudly against fork-registered names (409 over the WS path).
|
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
|
**WS-26.** `WsSessions` gains a `connections` map +
|
||||||
gates named in Part B (openable discoverable on the live session;
|
`live_connections()` / `live_connection_count()`; the hook retains
|
||||||
announce → nested composition → routes back).
|
the channel-0 `Arc<CallConnection>` 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,
|
**UP-01.** `ALREADY_EXISTS` → 409 Conflict in
|
||||||
websocket.md status block, idle-knob deployment note), with the
|
`call_error_to_http_status_with_identity` + the test arm; the module
|
||||||
`op/register` ACL posture recorded per UP-02.
|
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.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
Reference in new issue
Block a user