From 435ae9da2f339f2e9b378c9f5711fc76b2cb88c2 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Fri, 4 Sep 2026 07:25:41 +0000 Subject: [PATCH] docs(review 005): serving-loop concurrency + op/register composition findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-remediation review of f84d214 (review 004 Units 1-3). Five findings, verified in source and (for G-01) empirically via a probe test that was added, run, and removed: - G-01 [major]: serve_single_stream awaits dispatch inline; a wire-dispatched handler composing a peer-announced op (or a from_call import) over the same connection deadlocks — the nested call resolves only via the 30s sweeper (probe: TIMEOUT at 30.0007s). - G-02 [major]: the F-05 e2e gate calls the announced op directly, bypassing the forwarding stub — the one path G-01 breaks. - G-03 [major]: op/register's collision gate is overlay-only; PeerCompositeEnv resolves connections before base, so an announced op can shadow the serving side's own ops in nested composition. - G-04 [minor]: resource_id_path does not survive the spec wire round-trip (pre-existing shape, load-bearing for op/register). - G-05 [minor]: install_bootstrap_discovery registers services/list-peers; ADR-022's bootstrap set doesn't name it. Non-findings bound the re-review: fork surface lock discipline, bootstrap discovery closure, frame-arm equivalence of the composed loop, unchanged pure-consumer default, alkhttp cross-repo claims, CJK sweep (none), all gates reproduce (581/598, clippy, fmt, wasm, doc). Remediation plan: Unit 1 (concurrent serving loop + stub-exercising gate) gates Unit 4 downstream; Unit 2 (collision policy); Unit 3 (round-trip completeness + doc alignment). Verification: cargo doc --no-deps clean; tree unchanged apart from this review doc. --- ...loop-concurrency-and-op-register-review.md | 448 ++++++++++++++++++ 1 file changed, 448 insertions(+) create mode 100644 docs/reviews/005-serving-loop-concurrency-and-op-register-review.md diff --git a/docs/reviews/005-serving-loop-concurrency-and-op-register-review.md b/docs/reviews/005-serving-loop-concurrency-and-op-register-review.md new file mode 100644 index 0000000..c5144ad --- /dev/null +++ b/docs/reviews/005-serving-loop-concurrency-and-op-register-review.md @@ -0,0 +1,448 @@ +# Review 005 — Serving-Loop Concurrency and `op/register` Composition (post-remediation review of `f84d214`) + +## Status + +Verified, open for remediation. + +## Scope + +Post-remediation review of commit `f84d214` (review 004 Units 1–3: +per-session fork registry, connect-side serving loop, `op/register`). +The remediation landed all six F-findings' mechanisms; this pass +reviews the landed implementation against the decided spec (ADR-022 +amendment 2026-09-03, ADR-047 §4 amendment #2, review 004's +acceptance gates) rather than re-litigating the findings. + +Every finding below was verified directly in source at tree +`f84d214`. The headline finding (G-01) was verified **empirically**: +a probe test was added to the tree, run, observed to reproduce the +defect, and removed — the tree at this review's baseline carries no +source changes. Findings continue alkcall review numbering with the +new prefix `G` (001–004 used P/C/R/A/B/C/D/F — each review numbers +independently). + +Cross-repo context: the alkhttp re-pointing claimed by the remediation +log was verified in that repo (`5b62307` — `open-questions.md:102-112`, +`decisions/048…md:19-25`). Unit 4 (alkhttp wiring) remains downstream +and is not reviewed here. + +## Baseline verification (this pass) + +``` +cargo test → 581 passed, 0 failed +cargo test --all-features → 598 passed, 0 failed +cargo clippy --all-targets -- -D warnings → clean +cargo clippy --all-features --all-targets -- -D warnings → clean +cargo fmt --check → clean +cargo check --target wasm32-unknown-unknown → clean +cargo clippy --target wasm32-unknown-unknown -- -D warnings → clean +cargo doc --no-deps → clean +``` + +All gates the remediation log claims reproduce. The suite is green — +and again the green is partial: the gates test the flows they name, +and the one flow the ADR amendment promises most loudly (nested +composition of peer-announced ops) is the one no gate exercises. See +G-02 for why the existing gate cannot catch G-01. + +## Verdict + +- **Unit 2's fork surface is solid.** Interior mutability, lock + discipline, fork independence, validator carry, and the + self-referential bootstrap-discovery closure are all correct and + tested (see Non-findings). +- **Unit 3's serving loop resolves the F-04 frame gap but inherits the + dispatch loop's serial-inline shape** — and the F-05 flow it exists + to enable is exactly the shape that deadlocks under it. The ADR-022 + amendment's "invocable via nested composition" promise is + unmechanized for wire-dispatched handlers (the primary caller + shape); a 30s sweeper timeout masquerades as resolution. +- **The F-05 acceptance gate does not exercise the forwarding stub** — + the one component that is genuinely new protocol surface. It + verifies announce-lands-in-overlay and the consumer serves, by + calling the announced op *directly*, bypassing the stub entirely. +- **`op/register`'s collision gate is overlay-only**, so a + peer-announced op can shadow the serving side's own ops in nested + composition (connections resolve before base in `PeerCompositeEnv`). + +The architecture decisions (fork as dispatch registry; overlay as +landing zone; bootstrap-op set; opt-in serving) remain sound. The +defects are in the serving loop's concurrency model and in what the +gates measure. + +## Severity legend + +Same scale as review 004: + +- **[critical]** — a decided spec invariant is violated in a way that + makes a promised capability unreachable end-to-end; or corrupts + data. +- **[major]** — a core protocol path cannot serve a decided behavior; + works only via shapes the spec does not describe. +- **[minor]** — drift, doc/spec inconsistency, or a missing + convenience with no correctness impact. + +--- + +# Part A — The serving loop's concurrency model + +## G-01 [major] — Same-connection nested composition deadlocks the serving loop; the forwarding stub's nested call resolves only via the 30s sweeper + +**ADR drift:** ADR-022 amendment (2026-09-03) §`op/register`: the +announced op is *"invocable via nested composition (`env.invoke`)"*; +the amendment's whole point is that the hub's handlers compose +peer-announced ops. Review 004 F-05's recommendation said the same: +the hub-side handler wraps announcements *"as a forwarding handler +that issues a nested `call.requested` back over channel 0"*. + +**Verified:** YES, by code trace and by an empirical probe (see +Verification log). + +Mechanism, in four steps: + +1. `Dispatcher::serve_single_stream` awaits `dispatch()` **inline** in + its read loop for `call.requested` frames + (`src/protocol/dispatch.rs:1009-1014`) — the same serial-inline + shape as the pre-existing accept-side `run_loop_single_stream` + (`:765-770`). Only the Sink arm is spawned + (`:1031-1053`); Once responses and Sub pumps + (`pump_stream_single_stream`, awaited inline at `:1027-1029`) + hold the loop. +2. The F-05 forwarding stub (`make_forwarding_handler`, + `src/client/from_call.rs:392-415`) issues its nested call via + `connection.call_with_payload` (`:408`) — over **the same + connection** whose loop dispatched the parent. In single-stream + mode the pending resolves only when *some* loop reads the + `call.responded` frame (`connection.rs:274-303` — register + pending, write frame, await receiver; resolution is the reader's + job). +3. The only reader of that connection is the serving loop itself — + which is blocked in step 1 awaiting the parent dispatch, whose + handler is blocked in step 2 awaiting the nested call. Circular + wait. The transport buffers the response frame; nobody reads it. +4. The 30s sweeper (`DEFAULT_CALL_TIMEOUT`, `connection.rs:32`; + `evict_expired`, run every 10s per `SWEEPER_INTERVAL`, + `dispatch.rs:44`) is the only thing that breaks the cycle — + the nested call resolves as `CallError::timeout`. + +**Empirical probe (this pass):** hub serves `op/register` + a +`hub/compose` handler that invokes the connection overlay's +registration (the forwarding stub — the exact resolution +`PeerCompositeEnv` → `OverlayOperationEnv` produces for nested +composition) from inside a wire-dispatched handler; consumer announces +`consumer/exec`, then calls `hub/compose` over the wire. Result: + +``` +PROBE: hub/compose resolved after 30.000761619s: + Err(CallError { code: "TIMEOUT", message: "request timed out", retryable: true }) +``` + +A second run with an 8s timeout confirmed the hang is unconditional +(not load-dependent): zero progress until the sweeper. The probe was +removed after evidence capture; the tree is unchanged. + +**Consequences (verified):** + +- The ADR-022 amendment's flagship flow — a hub handler composing a + peer-announced op — fails for every wire-dispatched parent handler. + It "works" only for parents dispatched from *in-process* contexts + (where the serving loop is idle and free to resolve the nested + call's response) — a shape the ADR does not describe. Per the + severity legend this is a decided behavior served only via an + undescribed shape; it borders [critical] for the composition flow + specifically. +- The same circular wait applies to composition of `from_call` + *imports* on the accept side: `run_loop_single_stream` has the same + inline shape, and the connection overlay's imported-op stubs ride + the same connection. That hazard is **latent pre-`f84d214`** (the + accept side had the inline loop and `register_imported` before this + commit) — `f84d214` makes it load-bearing by (a) adding the wire + path that populates overlays with stubs, (b) putting serving loops + on the connect side where both directions are live by design, and + (c) writing the composition promise into ADR-022. +- The same starvation is not limited to the stub: while the loop + serves *any* Sub (`pump_stream_single_stream` inline), the side's + own outbound pendings queue unresolved. A serving-enabled consumer + that calls out while serving a long-lived Sub to the same peer eats + 30s timeouts on its own calls. The F-04 gate tests the two + directions only **sequentially** (hub→consumer resolves, *then* + consumer→hub) — never interleaved. +- Head-of-line blocking: the peer's subsequent requests queue behind + each inline dispatch. + +**Fix (Unit 1):** spawn the Once dispatch and the Sub pump the way the +Sink arm already is (the `SharedFrameWriter` already serializes +concurrent frame writes, so per-request frame ordering is preserved); +track spawned task handles for teardown on loop exit (the Sink arm's +`in_flight_sinks` pattern, generalized). Keep read-loop resolution +unblocked at all times. Apply the same rework to +`run_loop_single_stream` (or factor the shared loop) — the accept side +has the identical latent hazard via imported-op composition. +Acceptance: the probe above (recreated as a permanent gate) resolves +in < 1s; a concurrent-interleaving gate (outbound call resolving while +a Sub is being served) passes without sweeper intervention. + +## G-02 [major] — The F-05 acceptance gate bypasses the forwarding stub it claims to prove + +**Verified:** YES. `op_register_announce_then_hub_call_routes_back_to_consumer` +(`src/channels/client.rs:1412-1543`) asserts the announce resolves and +the overlay holds the op — then calls +`accept_conn.call("consumer/exec", …)` (`:1529`). That is a **direct +wire call**: the hub's dispatcher resolves `consumer/exec`… which is +not on the hub at all — the frame crosses to the consumer, whose +serving loop dispatches it from its *local* registry. The forwarding +stub (`op_register_handler`'s `register_imported` bundle) is never +invoked. The gate's comment — *"the forwarding stub routes the nested +call back over channel 0"* — describes a path the test does not take. + +**Consequence:** the one behavior that is genuinely new protocol +surface (stub routing) is untested, and the one defect it hides (G-01) +is precisely in that path. This is the same failure class review 001 +flagged ("all coverage is unit-level / the integration seams are where +the real problems live") — the remediation added e2e gates, but the +F-05 gate measures an adjacent, easier path. The F-04 gate is +likewise sequential-only (see G-01's third consequence). + +**Fix (fold into Unit 1):** add a gate that exercises the stub — +announce → a wire-dispatched hub handler composes the announced op via +`ctx.env` (or directly via the overlay registration) → resolves from +the consumer. This is the removed probe, productized; it is the +acceptance test G-01's fix must pass. Optionally extend the F-04 gate +with an interleaved-directions phase. + +--- + +# Part B — The `op/register` composition surface + +## G-03 [major] — Announced ops can shadow the serving side's own ops in nested composition; the collision gate checks the overlay only + +**ADR drift:** ADR-018 (composition authority) and ADR-019 (the +overlay is the *landing zone* for imported ops — a layer beneath the +deployment's own registry, not a rival to it); ADR-022 amendment +(silent on name collisions with the serving side's base registry). + +**Verified:** YES. `op_register_handler`'s collision gate is +`connection.overlay_contains(&request.spec.name)` +(`src/registry/op_register.rs:142`) — it consults **only the +connection overlay**. The serving registry (the fork the session +dispatches over) is not consulted. Meanwhile `PeerCompositeEnv` +resolves **connections before base** (`src/registry/env.rs:230-245` — +session, then `connection_order`, then base), and `compose_root_env` +attaches the dispatching connection's overlay +(`dispatch.rs:207-214`). + +Consequence: a peer announces an op named `fs/readFile` (or any name +the serving side has registered — `services/list` itself is fair +game). The registration lands in the overlay (forced `Internal`, +`FromCall` — both per spec). From then on, every handler +**wire-dispatched from that peer's connection** that nested-composes +that name via `ctx.env` resolves to the peer's forwarding stub instead +of the serving side's own op. The forced `Visibility::Internal` does +not help here — it blocks wire invocation, not `env.invoke` +(`OverlayOperationEnv` gates on `AccessControl` only, +`connection.rs:749-836`; the composed child context is `internal: +true` by design). + +Concretely: a hub handler processing peer A's request composes +`fs/readFile` expecting *its own* registry op (with the deployment's +`scoped_env`, capabilities, and ACL); it silently gets peer A's stub — +peer A chooses where the call goes and what it returns. Composition +authority (ADR-018) is decided by the composing handler's *deployer*; +a same-name peer registration silently rewrites it. The effect is +scoped to handlers whose root context is that connection's +(`compose_root_env` attaches that connection's overlay), which bounds +the blast radius — but that is exactly the flagship F-05 flow. + +**Fix (Unit 2):** decide the collision policy and record it in the +ADR-022 amendment. Recommended: `op_register_handler` also rejects +names registered on the serving registry (pass the fork — or a +name-set closure — into the handler alongside the connection, the +same way `install_bootstrap_discovery` closes over its registry); +`ALREADY_EXISTS` for base collisions regardless of `replace` (the +reconnect path needs to re-announce *peer* ops, not to overwrite the +deployment's). The alternative — changing `PeerCompositeEnv` to +resolve base before connections — would break `from_call`'s +namespace-prefixed imports (prefixed names don't collide) is not +needed for that, but would change long-decided layering semantics; the +registration-side gate is the cheaper, narrower door. + +## G-04 [minor] — `resource_id_path` does not survive the spec wire round-trip + +**Verified:** YES. `spec_to_json_pub` serializes +name/namespace/op_type/visibility/schemas/error_schemas/access_control +(+ `channel_open`/`publish_schema` markers; +`src/registry/discovery.rs:211-234`) — `resource_id_path` is not +serialized. `rebuild_spec_for` cannot parse it +(`src/client/from_call.rs:209-286`), and the field exists on +`OperationSpec` (`src/registry/spec.rs:190`). An announced (or +`from_call`-imported) op that declares ownership-scoped resource +extraction silently loses it; the rebuilt spec's ACL checks run with +`resource_id: None`. + +This is a pre-existing gap in the `from_call` import shape, not +introduced by `f84d214` — but `op/register` makes it load-bearing for +a second wire surface (peer-announced specs) that explicitly promises +"the serializable parts" of a registration. Note `namespace` *is* +serialized but `rebuild_spec_for` ignores it in favor of the +`namespace_prefix` parameter — consistent for `op/register` +(`replace`/re-announce uses `None`), worth a comment either way. + +**Fix (Unit 3):** add `resource_id_path` to both halves of the +round-trip plus a round-trip test. Additive optional field in the +`services/schema` JSON shape — no existing consumer breaks. + +## G-05 [minor] — `install_bootstrap_discovery` registers `services/list-peers`; the ADR-022 amendment's bootstrap set doesn't name it + +**Verified:** YES. `install_bootstrap_discovery` registers +`services/list`, `services/list-peers`, and `services/schema` +(`src/registry/discovery.rs:290-313`; `list-peers` at `:300`). The +ADR-022 amendment's bootstrap set lists exactly three ops — +`services/list`, `services/schema`, `op/register` +(`022-…contract.md:380-393`). Review 004's remediation log says +"list-peers" too; the ADR text was never updated. + +Consequence: none at runtime (installing a fourth op is strictly +additive, and `services/list-peers` is what makes F-05's +"peer-announced ops are discoverable" true). It is a +doc/spec-consistency drift on a set the ADR declares *"closed."* + +**Fix (Unit 3):** add `services/list-peers` to the ADR-022 +amendment's bootstrap-op list (recommended — `from_call`-import +discovery already relies on it) or stop installing it; one line +either way. + +--- + +# Non-findings (verified correct, recorded to bound the re-review) + +- **The fork surface (F-02/F-03) is solid.** Lock discipline is + correct: `registration()` clones under a read guard and drops it + before handler invocation (`registration.rs:190-192`), so + self-referential discovery handlers cannot deadlock their own + registry; `register` takes the two maps' write locks sequentially + and never across an `await`. `fork()` deep-copies both maps + (handlers are `Arc` closures — cheap, and the F-03 constraint that + closures must carry is met); validators carry; fork independence is + tested both directions; `OperationRegistryBuilder::from_registry` + seeds the builder path. +- **`install_bootstrap_discovery` (F-06) is correct** — handlers + closed over the same `Arc` they're registered on, so post-install + per-session registrations are visible at call time; ACL filtering + stays per-caller; the fork gate + (`fork_registry_open_op_resolves_and_is_discoverable`) exercises + exactly the review-004 acceptance shape. +- **`serve_single_stream`'s pending-resolution arms match the two + half-loops they compose.** Frame by frame against + `run_loop_single_stream` (`dispatch.rs:765-905`) and + `dispatch_envelope` (`connection.rs:717-741`): responded/completed/ + error resolve pendings; aborted tries both tables (in-flight sink + aborts, then the pending cascade via `handle_abort`); published + routes inbound sinks with the same validation/keep semantics; teardown + (fail_all + sink drop + sweeper abort) matches. The direction + disambiguation by table membership (documented at + `dispatch.rs:940-966`) is correct as written — G-01 is *not* a + frame-routing bug; the arms are right, the loop's scheduling is the + defect. +- **The pure-consumer default is unchanged.** `from_connection` + delegates with `None` and keeps the resolution-only read pump; no + behavior change for existing callers. +- **`op/register`'s DTO and unit semantics are clean** — one spec + serialization on the wire (`spec_to_json` shape), forced + `Internal`/`FromCall` on landing (unit-tested), `replace` semantics + tested, `ALREADY_EXISTS` collision gate tested (against the + overlay — see G-03 for the gap). +- **The alkhttp cross-repo claims check out.** `open-questions.md` + OQ-05 and ADR-048's re-point are present in that repo at `5b62307`. +- **No non-English text reached the tree.** The remediation session's + language slip (reported anecdotally) did not land in any code, + doc, or metadata: a CJK-range regex sweep over all `.rs`/`.md`/ + `.toml`/`.json` in the repo returns zero matches at `f84d214`. +- **Gates reproduce.** All commands in Baseline verification pass at + the review tree, matching the remediation log's claims + (581 default / 598 all-features). + +--- + +# Remediation plan + +Sequenced by dependency. All units are alkcall work; Unit 4 (alkhttp +wiring) stays downstream and should **not** start before Unit 1 — +alkhttp's serving consumers would compose over the same connection +and hit G-01 immediately. + +## Unit 1 — Concurrent serving loop + a stub-exercising gate (G-01, G-02) + +- Rework `serve_single_stream`'s `EVENT_REQUESTED` arm: spawn the Once + dispatch and the Sub pump (the Sink arm's existing pattern, + generalized; `SharedFrameWriter` serializes frames; track handles + for teardown). Apply the same rework to `run_loop_single_stream` or + extract the shared loop. +- Add the G-02 gate: announce → wire-dispatched hub handler composes + the announced op via nested composition → resolves from the + consumer. This is the removed probe, productized; it is the + acceptance test for G-01. +- Optional: an interleaved-directions phase on the F-04 gate + (outbound call resolving while a Sub is being served). +- Gate: the probe recipe (Verification log) resolves in < 1s; no + sweeper evictions in the gates. + +## Unit 2 — `op/register` collision policy (G-03) + +- `op_register_handler` gains a serving-registry name-set (closure or + `Arc`); base-registry name collisions reject + with `ALREADY_EXISTS` regardless of `replace`. +- ADR-022 amendment: record the collision policy (peer-announced ops + may collide with peer-announced ops — `replace` governs; never with + the serving side's own registrations). +- Gates: announcing a base-registered name (External *and* Internal) + fails loudly; announcing a distinct name still lands; nested + composition of a base op is unaffected by an unrelated announce. + +## Unit 3 — Round-trip completeness + doc alignment (G-04, G-05) + +- `resource_id_path` through `spec_to_json_pub` + `rebuild_spec_for` + + round-trip test. +- ADR-022 bootstrap list gains `services/list-peers` (or drop it from + the installer; recommend adding). + +--- + +## Verification log (this pass) + +- All gates in Baseline verification reproduced at tree `f84d214` + (clean working tree; the probe was added, run, and removed — + `git status` verified clean after removal). +- G-01 verified by code trace (the four steps above, each line + read at `f84d214`) **and** empirically: probe + `probe_hub_handler_composes_peer_announced_op` (in + `src/channels/client.rs#[cfg(test)]`, since removed) — hub registry + carries `op/register` + `hub/compose` (whose handler invokes the + connection overlay's registration for the announced op, the shape + `PeerCompositeEnv` → `OverlayOperationEnv` resolution produces); + consumer announces `consumer/exec` then calls `hub/compose`. + 8s-timeout run: `Elapsed` (no progress). 45s-cap run: resolved at + **30.0007s** with `CallError::TIMEOUT` (retryable) — the + `DEFAULT_CALL_TIMEOUT` sweeper eviction, not live resolution. +- The existing gates' blind spot verified by reading both e2e gates' + call directions: `serving_loop_hub_to_consumer_call_resolves` + (`client.rs:1373-1399`) and + `op_register_announce_then_hub_call_routes_back_to_consumer` + (`client.rs:1529`) — the latter's `accept_conn.call("consumer/exec")` + crosses the wire to the consumer's local registry; the forwarding + stub is unreachable from it. +- G-03 verified by reading the collision gate (`op_register.rs:142`) + against `PeerCompositeEnv::invoke_with_policy`'s resolution order + (`env.rs:230-245`) and `compose_root_env`'s attach + (`dispatch.rs:207-214`). +- G-04 verified by field audit: `OperationSpec`'s fields + (`spec.rs:175-206`) vs `spec_to_json_pub` (`discovery.rs:211-234`) + vs `rebuild_spec_for` (`from_call.rs:209-286`). +- G-05 verified against `discovery.rs:290-313` and ADR-022 + amendment's set (`022-…contract.md:380-393`). +- Frame-arm equivalence of `serve_single_stream` vs the two half-loops + verified read-through (Non-findings). +- alkhttp cross-repo claims verified in `/workspace/@alkdev/alkhttp` + at `5b62307` (OQ-05 `open-questions.md:102-112`; ADR-048 + `decisions/048-…md:19-25`). +- CJK sweep: no CJK-range codepoints in any `.rs`/`.md`/`.toml`/ + `.json` under the repo at `f84d214`. \ No newline at end of file