Consolidates three code-review passes (one main summary + sub-review A
on ADR-046 + sub-review B on channels ADRs 034–043/047) into a single
verified document under docs/reviews/. Every finding was re-verified
directly against the source at f305f8c with exact file:line refs.
Findings (all verified): 10 critical, 19 major, 7 minor.
- Pub end-to-end cannot work (chunks written to a different bi-stream
than the request); publish() takes Vec<Value> not a Stream; 30s
client timeout; abort-doesn't-cancel-Pub; dispatch re-implements
invoke_sink inline; from_call sink forwarding swallows errors.
- Channel 0 dead in both directions; register_openable absent;
resolve_channel_manager stub; backpressure drops chunks; buffer cap
in messages not bytes; ledger decrement on 1/4 teardown paths; demux
desyncs on oversized chunk; no channel-adoption/collision scheme.
- channel/control + resources/subscribe stubs; busy-wait spin; lost
EOF sentinel; TOCTOU on max_channels; ~50 lines of abandoned
deliberation in poll_write; cargo doc 4 warnings; spec docs
inconsistent post-047.
Includes a 6-unit remediation plan sequenced by dependency, with
acceptance gates. Units 1/2/4 need no spec decisions; Units 3 and 5
each need one written ADR decision first (ADR-047 §4 env-resolution;
§5 channel-id adoption). The overarching acceptance gate is the
end-to-end ChannelClient ↔ ChannelsAdapter test whose absence let
both commits land green.
Two corrections to the original reviews noted in the verification log:
- C-17 (lost EOF sentinel): Sub Review B overstated "the pump does not
write an EOF chunk" — the pump does write EOF when it receives the
sentinel; the real bug is narrower (sentinel lost on full buffer →
pump exits on recv→None without writing EOF).
- Warning count: main review said 5 cargo doc warnings; this pass sees
4 (2× register_openable, 1× default_policy, 1× env module/macro).
Verification:
- cargo test → 432 passed (no source changed; docs-only commit)
- cargo clippy --all-targets -- -D warnings → clean
- cargo fmt --check → clean
- cargo doc --no-deps → 4 warnings (documented as C-09; not addressed
here — fixing them requires the register_openable method to exist,
which is Unit 3 of the remediation plan)
1075 lines
56 KiB
Markdown
1075 lines
56 KiB
Markdown
# Review 001 — Pub (ADR-046) and Channels Integration (ADR-047)
|
||
|
||
## Status
|
||
|
||
Verified, open for remediation.
|
||
|
||
## Scope
|
||
|
||
Consolidated code review of the two commits that landed the `Pub`
|
||
operation type / `HandlerKind::Sink` (ADR-046, commit `ea66398`) and
|
||
the channels protocol + openable-ALPNs model (ADR-047, commit
|
||
`f305f8c`). The review consolidates three passes (one main summary +
|
||
two sub-reviews: one focused on ADR-046, one on the channels ADRs
|
||
034–043/047) and was produced against the tree at `f305f8c`.
|
||
|
||
Every finding below was **re-verified directly in the source** during
|
||
this consolidation pass (2026-08-12). Each finding carries: the ADR it
|
||
drifts from (if any), a severity, the exact `file:line` reference(s),
|
||
the verification status, and the unit-of-work bucket it belongs to.
|
||
Findings that did not survive verification, or that the original
|
||
reviews overstated, are called out as such.
|
||
|
||
## Baseline verification (this pass)
|
||
|
||
```
|
||
cargo test → 432 passed, 0 failed
|
||
cargo clippy --all-targets -D warnings → clean
|
||
cargo fmt --check → clean
|
||
cargo doc --no-deps → 4 warnings (see C-09)
|
||
```
|
||
|
||
The suite is green, but green is misleading here: **all coverage is
|
||
unit-level**. Not a single test wires two sides of either new protocol
|
||
together end-to-end (no `CallConnection::publish` ↔
|
||
`Dispatcher::handle_stream` round-trip; no `ChannelClient` ↔
|
||
`ChannelsAdapter` round-trip). The integration seams — which is exactly
|
||
where ADR-047 lives — are where the real problems live, and the absence
|
||
of end-to-end tests is what let both commits land green with multiple
|
||
implementation-killing defects.
|
||
|
||
## Verdict
|
||
|
||
- **ADR-046 registry half (types, `HandlerKind::Sink` validation,
|
||
`invoke_sink`, builders, wire strings, discovery strings): solid**,
|
||
close to spec, well tested at the unit level.
|
||
- **ADR-046 protocol half: has one implementation-killing bug** (P-01:
|
||
publish chunks are written to a different bi-stream than the request
|
||
— end-to-end publish cannot succeed on any transport), plus two
|
||
unimplemented decided behaviors and an abort story that exists only
|
||
in doc comments.
|
||
- **ADR-047: the components are good but the integration layer — the
|
||
part ADR-047 actually decided — is scaffolding.** As committed, the
|
||
channels protocol cannot open, use, or account for a single data
|
||
channel end-to-end, channel 0 is dead in both directions, and a
|
||
`publish()` cannot succeed on any transport.
|
||
|
||
The architecture is right: no channels types leaked into the call
|
||
crate, `OperationEnv` stayed a trait, the wire formats (envelope +
|
||
8-byte chunk header) match spec exactly, the `channel_open` marker
|
||
plumbing (Gap F) is complete and tested, and auth stays on the one
|
||
`AccessControl` path. What's missing is the connective tissue a
|
||
tty/docker crate would actually call.
|
||
|
||
## Severity legend
|
||
|
||
- **[critical]** — the protocol cannot function end-to-end as committed
|
||
on any transport; or a decided spec invariant is violated in a way
|
||
that corrupts data or causes silent permanent state damage.
|
||
- **[major]** — a decided behavior is missing, wrong, or a real
|
||
reliability hazard; works in the happy path but fails a spec-required
|
||
edge case or stores wrong data.
|
||
- **[minor]** — drift, convention violation, dead code, or a
|
||
doc/spec inconsistency with no correctness impact.
|
||
|
||
---
|
||
|
||
# Part A — Pub / `HandlerKind::Sink` (ADR-046) findings
|
||
|
||
## P-01 [critical] — Publish chunks are written to the wrong stream; end-to-end publish cannot work
|
||
|
||
**ADR drift:** ADR-046 §5, §8 (the stream lifecycle and `publish()`
|
||
client method).
|
||
**Verified:** YES. `src/protocol/connection.rs:285-311` (open stream A,
|
||
write `call.requested`), `src/protocol/connection.rs:334-362`
|
||
(`write_publish_chunks` opens a **fresh** `open_bi()` at line 343-346
|
||
and writes all `call.published` + `call.completed` on stream B).
|
||
**Responder side:** `src/protocol/dispatch.rs:433-435, 516`
|
||
(`pump_sink` reads chunks from the *same* stream the request arrived
|
||
on — stream A). Stream B arrives as a fresh `accept_bi` whose frames
|
||
hit the `handle_stream` "ignoring non-requested/non-aborted event"
|
||
branch (`src/protocol/dispatch.rs:443-444`) and are silently
|
||
discarded.
|
||
|
||
**Consequences (verified):**
|
||
- Multi-stream transport (QUIC): chunks are lost; `pump_sink` blocks
|
||
on stream A until the client's dropped write-half surfaces as
|
||
reset/EOF — best case the handler sees an *empty* stream and returns
|
||
a bogus result (e.g. `count: 0`), worst case it hangs until the 30s
|
||
client-side timeout (P-05).
|
||
- Single-stream transport (`Connection::from_bidi`): the second
|
||
`open_bi` returns `StreamClosed` (`src/core/types.rs`), so *every*
|
||
publish fails outright with "failed to open stream".
|
||
|
||
No end-to-end publish test exists (see P-12), which is how this landed
|
||
green. **Fix:** keep stream A's write half and pump chunks on it. This
|
||
is also what a `Stream`-typed `publish` (P-02) would force
|
||
structurally. **Acceptance gate:** one end-to-end publish test that
|
||
moves ≥2 chunks and asserts the responder sees them.
|
||
|
||
## P-02 [major] — `publish()` takes `Vec<Value>`, not a `Stream`
|
||
|
||
**ADR drift:** ADR-046 §8 specifies
|
||
`stream: Pin<Box<dyn Stream<Item = Value> + Send>>`. The implementation
|
||
is `chunks: Vec<Value>`.
|
||
**Verified:** YES. `src/protocol/connection.rs:249-254` (`publish`)
|
||
and `:268-272` (`publish_with_payload`) both take `chunks: Vec<Value>`;
|
||
`write_publish_chunks` (`:334-362`) iterates the `Vec`.
|
||
|
||
This buffers the entire publish in memory before anything is sent and
|
||
makes the ADR's flagship use cases (telemetry ingest, live upload,
|
||
drag-drop file streaming) impossible from this API — the chunk set
|
||
must be fully materialized up front. **Fix P-01 + P-02 together:**
|
||
hold stream A's write half, accept a `Stream`, pump it as
|
||
`call.published` events. This also resolves P-05's tension (a streaming
|
||
publish should register like `subscribe` with `timeout: None`, not the
|
||
30s `Call` deadline).
|
||
|
||
## P-03 [major] — `publish_schema` per-chunk validation is written but never read
|
||
|
||
**ADR drift:** ADR-046 §4, and the field's own doc comment at
|
||
`src/registry/spec.rs:181-186` which says "the dispatch path validates
|
||
each `call.published` event's `payload.input` against this schema
|
||
before yielding it to the `SinkHandler`."
|
||
**Verified:** YES. `grep publish_schema` outside `src/registry/spec.rs`
|
||
matches nothing. The field is declared (`spec.rs:186`), defaults to
|
||
`None` (`:228`), and has a builder (`with_publish_schema`, `:236-239`),
|
||
and is **never read anywhere**. `pump_sink` forwards chunks as-is
|
||
(`src/protocol/dispatch.rs:527-535`). It is also missing from
|
||
`spec_to_json`, from `operation_spec_schema()`, and from `from_call`'s
|
||
`rebuild_spec_for` — so an imported Pub op loses the schema entirely.
|
||
`alktype` is a declared dependency (`Cargo.toml:21`) and AGENTS.md §10
|
||
says to use it for exactly this; it is unused in `src/`. Dead field +
|
||
documented-but-unimplemented behavior.
|
||
|
||
## P-04 [major] — Initiator `call.error` does not terminate the publish stream
|
||
|
||
**ADR drift:** ADR-046 §5 event table and §6 ("`Err(call_error)` for an
|
||
initiator-side error (a `call.error` event from the initiator)"); plus
|
||
the `SinkHandler`/`PublishStream` doc contract
|
||
(`src/registry/registration.rs:26-31, 44-49`).
|
||
**Verified:** YES. `pump_sink` matches only `"call.published"`,
|
||
`"call.completed"`, `"call.aborted"` and drops everything else into a
|
||
debug-log ignore branch (`src/protocol/dispatch.rs:526-550`). A
|
||
`call.error` frame is silently ignored; the only `Err` ever injected is
|
||
the `call.aborted` case, and it's an ad-hoc
|
||
`CallError::internal("publish aborted by initiator")`
|
||
(`dispatch.rs:538-543`), not the initiator's error.
|
||
|
||
## P-05 [major] — Client-side 30s timeout on Pub contradicts "long-lived, same as Sub"
|
||
|
||
**ADR drift:** ADR-046 §7 ("the stream may be long-lived, same as
|
||
`Sub`") clears the deadline for Pub on the responder.
|
||
**Verified:** YES. `publish_with_payload` registers via `register_call`
|
||
with `DEFAULT_CALL_TIMEOUT` = 30s (`src/protocol/connection.rs:32,
|
||
296-303`). The sweeper resolves expired `Call` entries with `TIMEOUT`
|
||
(`src/protocol/pending.rs`). `subscribe` avoids this with
|
||
`register_subscribe(timeout: None)` (`connection.rs:215-218`). Any
|
||
publish whose stream takes >30s wall-clock gets a spurious client-side
|
||
`TIMEOUT` while the responder keeps consuming. **Fix:** register like
|
||
`subscribe` with `timeout: None` (naturally falls out of the P-01/P-02
|
||
streaming rework).
|
||
|
||
## P-06 [major] — Abort does not cancel a Pub
|
||
|
||
**ADR drift:** ADR-046 §5 table (`call.aborted` | either side | "Cancel
|
||
the Pub"); ADR-020 (stream drop on abort; cascade through sink
|
||
handlers).
|
||
**Verified:** YES, both abort paths are broken:
|
||
- **Cross-stream (the path the shipped client exercises):**
|
||
`CallConnection::abort()` sends `call.aborted` on a **new** stream
|
||
(`src/protocol/connection.rs:236-243` → `write_envelope` → `open_bi`
|
||
at `:388-392`). On the responder that frame is handled by a different
|
||
`handle_stream` task, whose `handle_abort` only mutates the
|
||
`PendingRequestMap` (`src/protocol/dispatch.rs:381-389`). Inbound
|
||
requests are never registered in that map, and nothing links the map
|
||
to the spawned `handle_stream`/`pump_sink` task — the running
|
||
`SinkHandler` and feed loop are untouched. The doc comments at
|
||
`dispatch.rs:494-498` (and `:459-462` for `pump_stream`) describe a
|
||
cancellation mechanism that does not exist.
|
||
- **Same-stream (dead-untested):** `pump_sink` injects an `Err` and
|
||
breaks the feed on `call.aborted` (`dispatch.rs:538-543`), then
|
||
**still `tokio::join!`s the handler to completion and writes the
|
||
response back to the aborting initiator** (`:562-568`). The handler
|
||
future is never dropped.
|
||
|
||
Net: an initiator cannot actually cancel an in-flight Pub. (The
|
||
`pump_stream` half of this predates ADR-046, but ADR-046 §5 explicitly
|
||
promises abort-cancels-Pub, so it is in scope here.) **Also:** during a
|
||
sink pump, `call.aborted` frames for **other** request IDs on the same
|
||
stream are dropped by the id-mismatch guard before the type match
|
||
(`dispatch.rs:518-525`) — a conforming peer that serializes calls on
|
||
one stream loses requests. Same for a second `call.requested`
|
||
multiplexed on that stream.
|
||
|
||
## P-07 [major] — Handler-returns-early: response withheld until the initiator finishes
|
||
|
||
**ADR drift:** ADR-046 §7 (long-lived publish streams).
|
||
**Verified:** YES. `pump_sink` uses `tokio::join!(feed_fut, handler)`
|
||
and only writes the response after **both** complete
|
||
(`src/protocol/dispatch.rs:562-568`). If the `SinkHandler` returns
|
||
early (e.g. rejects the upload after chunk 1), the feed loop notices
|
||
only when its next `chunk_tx.send` fails (`dispatch.rs:533-535`) — only
|
||
when another chunk arrives. An initiator that pauses between chunks, or
|
||
a long/infinite publish, never learns of the early error; the response
|
||
is deferred indefinitely. Not a deadlock, but an unbounded stall on a
|
||
path the ADR intends for long-lived streams.
|
||
|
||
## P-08 [major] — The dispatch Pub branch re-implements `invoke_sink` inline
|
||
|
||
**ADR drift:** ADR-046 §6, §7 (`Pub` → `invoke_sink()`; `dispatch()`
|
||
branches on `op_type` and routes Pub through `invoke_sink`).
|
||
**Verified:** YES. The `Pub` branch of `Dispatcher::dispatch`
|
||
re-implements the not-found / visibility / ACL / handler-kind checks
|
||
inline (`src/protocol/dispatch.rs:309-377`) and invokes the
|
||
`SinkHandler` directly, duplicating `OperationRegistry::invoke_sink`
|
||
(`src/registry/registration.rs:283-345`) line for line. `invoke_sink`
|
||
is only ever called from its own tests. Divergence hazard: any future
|
||
fix in `invoke_sink` (e.g. adding the missing `publish_schema`
|
||
validation, P-03) won't reach the wire path.
|
||
|
||
## P-09 [major] — `from_call` sink forwarding is store-and-forward and swallows errors
|
||
|
||
**ADR drift:** ADR-046 §9 (sink forwarding); the `SinkHandler` contract
|
||
(`src/registry/registration.rs:29-31`: an `Err` terminates the
|
||
stream).
|
||
**Verified:** YES. `make_sink_forwarding_handler` does
|
||
`publish_stream.filter_map(|item| async move { item.ok() }).collect::<Vec<_>>().await`
|
||
then one `publish_with_payload` call
|
||
(`src/client/from_call.rs:464-475`). Three problems:
|
||
1. The whole stream is buffered in memory — unbounded, contradicting
|
||
the doc comment "No truncation — the full stream is forwarded
|
||
end-to-end" (`from_call.rs:452-453`) in spirit (forwarded, but not
|
||
streamed; a hub relaying a large upload holds it all in RAM).
|
||
2. `Err` items are **silently discarded and iteration continues**
|
||
(`item.ok()` → `None` → filtered out), contradicting the contract
|
||
that an `Err` terminates the stream — a partial/errored publish is
|
||
forwarded upstream as if it completed cleanly.
|
||
3. It inherits P-01, so the forwarded publish is broken anyway.
|
||
|
||
## P-10 [minor] — String literals instead of wire constants in `pump_sink`
|
||
|
||
**Verified:** YES. `pump_sink` matches `"call.published"`,
|
||
`"call.completed"`, `"call.aborted"` (`src/protocol/dispatch.rs:527,
|
||
537, 538`) instead of `EVENT_PUBLISHED`/`EVENT_COMPLETED`/`EVENT_ABORTED`
|
||
(`src/protocol/wire.rs:12-17`), which the rest of the file imports and
|
||
uses. Refactor hazard, not a correctness issue.
|
||
|
||
## P-11 [minor] — ADR-046 §3 vs §6 `SinkHandler` stream-item-type inconsistency
|
||
|
||
**Verified:** YES (internal ADR inconsistency, not a code bug). ADR-046
|
||
§3 defines `SinkHandler` receiving `Pin<Box<dyn Stream<Item = Value> +
|
||
Send>>`; §6 defines `invoke_sink`'s `publish_stream` as
|
||
`Item = Result<Value, CallError>`. The code resolves this in favor of
|
||
§6 uniformly: `SinkHandler` itself takes `Item = Result<Value,
|
||
CallError>` (`src/registry/registration.rs:32-40`), aliased as
|
||
`PublishStream` (`:49`), and `invoke_sink` passes it straight through
|
||
(`:344`). This contradicts §3's literal type but is arguably the right
|
||
resolution (the handler must see initiator errors), and the ADR's
|
||
Door-type section explicitly marks the concrete stream item type a
|
||
two-way-door detail. **Action:** amend ADR-046 §3 text to match §6.
|
||
|
||
## P-12 — Test coverage gaps (ADR-046)
|
||
|
||
**Verified:** YES — all 23 new Pub tests are either registry-level
|
||
(`invoke_sink` with a synthetic stream), responder-side with
|
||
hand-crafted frames on **one** stream, or builder/wire-shape tests.
|
||
`src/protocol/connection.rs` has **zero** publish tests. Specific
|
||
decided behaviors with no test:
|
||
1. End-to-end publish through `CallConnection::publish` ↔
|
||
`Dispatcher::handle_stream` (would have caught P-01 immediately).
|
||
2. `deadline: None` for Pub — `dispatch_pub_clears_deadline_to_none`
|
||
asserts nothing about the deadline; the test name is misleading.
|
||
3. `publish_schema` validation (untested + unimplemented, P-03).
|
||
4. Initiator `call.error` terminating the publish stream (P-04).
|
||
5. Abort during a Pub, both directions (P-06 — same-stream path is
|
||
dead-untested; no initiator-side `abort()` of an in-flight publish).
|
||
6. `from_call` Pub forwarding — no test that `build_bundles` maps
|
||
`"pub"` → `HandlerKind::Sink`; no behavioral test of
|
||
`make_sink_forwarding_handler` (Err-dropping, forwarded_for
|
||
population).
|
||
7. Backpressure — nothing exercises a full 64-slot chunk channel
|
||
(`PUBLISH_CHANNEL_BUFFER`, `dispatch.rs:45`) with a slow handler,
|
||
or the handler-returns-early feed-termination path.
|
||
8. `OverlayOperationEnv` Sink arm (`connection.rs:523-528`) — the Sub
|
||
arm has a test; Sink does not.
|
||
|
||
## P-13 [minor] — `SinkDispatch` exposes `chunk_tx` publicly
|
||
|
||
**Verified:** YES. `SinkDispatch` exposes `chunk_tx:
|
||
futures::mpsc::Sender` publicly (`src/protocol/dispatch.rs:78-81`). The
|
||
channel type and buffer size are effectively public API now; consider
|
||
opaque wrapping before crates.io.
|
||
|
||
---
|
||
|
||
# Part B — Channels (ADR-034–043, 047) findings
|
||
|
||
## C-01 [critical] — Channel 0 is dead in both directions
|
||
|
||
**ADR drift:** ADR-036 (channel 0 pre-negotiated as `alknet/call`).
|
||
**Verified:** YES, both sides.
|
||
- **Accept side:** `src/channels/adapter.rs:181-188` — channel 0's
|
||
`Connection` is built and bound to `_channel0_conn`, which is
|
||
**never handed to anything**. The `InstallChannelZero` hook
|
||
(`adapter.rs:41-42`) receives only `(&ChannelManager, &AuthContext)`
|
||
— it has no way to reach the channel-0 `Connection` or its send/recv
|
||
halves (already consumed by `install_channel_zero` at `adapter.rs:181`,
|
||
which can't be called again — it returns `ChannelExists`). Inbound
|
||
`call.requested` chunks on channel 0 route into a receiver nobody
|
||
reads. Channel 0 on the accept side is a black hole.
|
||
- **Connect side:** `src/channels/client.rs:64-65` wraps channel 0 in
|
||
`CallConnection`, but `CallConnection::call_with_payload` opens a
|
||
**new bidi stream per request** via `connection.open_bi()`, and
|
||
`ChannelBidiStreamSource::open_bi` returns `Err(StreamClosed)`
|
||
unconditionally (`src/channels/source.rs:58-60`). So **every**
|
||
`ChannelClient::call_open_op` fails immediately with "failed to open
|
||
stream."
|
||
|
||
The call protocol's stream-per-request model
|
||
(`Dispatcher::run_loop` accepting streams in a loop,
|
||
`dispatch.rs:603-620`; `CallConnection` opening one stream per call)
|
||
is structurally incompatible with channel 0's single yield-once
|
||
`BiStream`. **This needs a single-stream call mode** (multiplex all
|
||
requests on one framed stream for both the client and the dispatch
|
||
side), which is a real design gap, not just a bug fix. **Acceptance
|
||
gate:** one end-to-end channel-0 test: `ChannelClient::call_open_op`
|
||
↔ `ChannelsAdapter` accept-side call dispatch, one round-trip.
|
||
|
||
## C-02 [critical] — `register_openable` / the ADR-047 §3 open flow does not exist
|
||
|
||
**ADR drift:** ADR-047 §3 (`ChannelCore` wrapper +
|
||
`register_openable(spec, open_handler, channel_core)` helper; the
|
||
ACL→cap→allocate→ledger→spawn→respond flow).
|
||
**Verified:** YES. `ChannelCore` has only `new`/`manager`/`check_open`/
|
||
`on_close` (`src/channels/operations.rs:280-311`). Doc comments link to
|
||
`ChannelCore::register_openable` (`operations.rs:10, 62`) — the
|
||
rustdoc warnings in C-09 are those dangling links. There is currently
|
||
**no way to register a channel-open op at all**; the
|
||
`channel:forbidden` / `channel:no_channels_session` /
|
||
`channel:allocation_failed` / `channel:too_many_channels` error-code
|
||
mapping to `CallError` exists only in comments. The whole open flow is
|
||
unreachable.
|
||
|
||
## C-03 [critical] — `resolve_channel_manager` (ADR-047 §4) is a stub
|
||
|
||
**ADR drift:** ADR-047 §4 (per-connection `ChannelManager` resolution
|
||
via the extension-trait downcast).
|
||
**Verified:** YES. `resolve_channel_manager` unconditionally returns
|
||
`None` (`src/channels/env.rs:108-128`). The comment at `:111-125` admits
|
||
a real downcast is impossible without `Any`-machinery and says "the
|
||
implementation uses the `ChannelCore`'s stored manager instead" —
|
||
which contradicts ADR-047 §4's core point (a statically-registered
|
||
handler closing over a static `ChannelCore` "has no way to know which
|
||
channels connection invoked it"). `ChannelsSessionEnv` exists
|
||
(`env.rs:46`) but nothing constructs it outside tests (`env.rs:148` is
|
||
the only construction site). `channel:no_channels_session` appears only
|
||
in comments. **Decision needed:** either add `as_any()` to
|
||
`OperationEnv` (or similar) to make §4 implementable, or amend ADR-047
|
||
to bless the "manager passed at registration via `ChannelCore`" shape.
|
||
The current stub gives you neither per-connection resolution nor the
|
||
amended spec.
|
||
|
||
## C-04 [critical] — Backpressure is inverted: the demux drops chunks
|
||
|
||
**ADR drift:** ADR-040 DP-5 / REQ-CH-05 (lossless bounded-buffer
|
||
backpressure — the demux stalls until the consumer drains).
|
||
**Verified:** YES. `route_payload` uses `try_send` and on `Full` logs a
|
||
warn and **drops the chunk** (`src/channels/manager.rs:253-258`).
|
||
`route_payload` is sync (`manager.rs:246`), so it *cannot* await — the
|
||
shape itself deviates. This is a reliability-layer protocol with no
|
||
retransmit; a dropped mid-stream data chunk silently corrupts the
|
||
channel's byte stream for the handler. **Fix:** make `route_payload`
|
||
async (or use a `blocking_send`/`reserve`-equivalent) and await on
|
||
full.
|
||
|
||
## C-05 [critical] — The buffer bound is in messages, not bytes
|
||
|
||
**ADR drift:** ADR-040 ("1 MiB default"; "256 × 1 MiB = 256 MiB worst
|
||
case per connection" memory bound).
|
||
**Verified:** YES. `DEFAULT_BUFFER_CAP = 1024 * 1024`
|
||
(`src/channels/reassembly.rs:35`) is passed as the **capacity of
|
||
`tokio::mpsc::channel`** (`reassembly.rs:77`, also `manager.rs:180,
|
||
219`, `mux.rs:115`), which counts *messages*, not bytes. The actual
|
||
per-channel bound is 1,048,576 chunks × up to 16 MiB each ≈ 16 TiB,
|
||
not 1 MiB. The doc comment at `reassembly.rs:31-34` even claims "1 MiB"
|
||
while the bound is ~6 orders of magnitude larger. ADR-040's
|
||
per-connection memory bound effectively does not exist. **Fix:** a
|
||
byte-budgeted bound (track accumulated `payload.len()` against the
|
||
cap, not message count).
|
||
|
||
## C-06 [critical] — Opener-ledger decrement happens on only 1 of 4 teardown paths
|
||
|
||
**ADR drift:** ADR-047 §7 (the self-DoS fix: decrement on
|
||
close-received, close-sent, handler-exit, connection-drop).
|
||
**Verified:** YES. Only close-received is wired
|
||
(`src/channels/operations.rs:197-204`). On transport EOF,
|
||
`clear_all()` returns the `(channel_id, opener)` list
|
||
(`src/channels/manager.rs:305-316`) and the demux loop **only logs its
|
||
length** (`src/channels/adapter.rs:139-143`) — `on_close` is never
|
||
called. Close-sent locally: nothing sends `channel/close` locally, no
|
||
decrement hook. Handler exit: a handler task that finishes naturally
|
||
triggers no teardown at all. The exact "peer whose connection dies at
|
||
cap is permanently at cap" self-DoS that ADR-047 §7 was written to fix
|
||
is back. The auth-blind adapter has no way to reach the policy — there
|
||
is no hook — so this is an architectural gap, not just a missed call.
|
||
|
||
## C-07 [critical] — Demux desynchronizes permanently on an oversized chunk
|
||
|
||
**ADR drift:** channels-wire.md §MAX_CHUNK_LEN (resync by skipping
|
||
`length` bytes; the parsed length is available in the error variant).
|
||
**Verified:** YES. On `ChunkError::TooLarge` (and any header-parse
|
||
error) the loop `continue`s without skipping the payload bytes
|
||
(`src/channels/adapter.rs:104-110`). The next 8-byte header read
|
||
consumes the first 8 bytes of the oversized payload → permanent stream
|
||
desync; every subsequent "header" is garbage routed to random
|
||
`channel_id`s (lenient drop hides the corruption). `ChunkError::TooLarge`
|
||
carries the parsed `length` (`src/channels/wire.rs:95-98`) — it is
|
||
discarded. **Fix:** on `TooLarge`, read and discard `length` bytes
|
||
before continuing.
|
||
|
||
## C-08 [critical] — No channel-adoption path → channel-id agreement is impossible
|
||
|
||
**ADR drift:** implied by ADR-037/047 §5 (connection-owner allocates).
|
||
**Verified:** YES. The side that did **not** allocate a `channel_id`
|
||
has no way to install routing state for it: `open_channel` always
|
||
allocates a fresh id (`manager.rs:167`), `install_channel_zero` is
|
||
hardcoded to 0. A consumer that receives `{channel_id: 7}` from the
|
||
responder cannot register 7 in its local manager/mux — inbound chunks
|
||
for 7 are dropped as unknown (`manager.rs:268-274`) and there's nothing
|
||
to write to. Additionally, both sides construct independent managers
|
||
with `next_id` starting at 1 (`manager.rs:98`; adapter side
|
||
`adapter.rs:170`; client side `client.rs:55`), so if both sides ever
|
||
allocate on one connection (Sub opened by A + Pub opened by A, per
|
||
ADR-047 §5's Pub case), **IDs collide** — one wire `channel_id`, two
|
||
different channels. No odd/even split or any collision-avoidance scheme
|
||
exists in either the ADRs (post-047) or the code. **This needs a spec
|
||
decision** (odd/even split, or an adoption op), not just a code fix.
|
||
|
||
## C-09 [major] — `cargo doc` emits 4 warnings (broken doc links)
|
||
|
||
**Verified:** YES — 4 warnings (the original reviews said 5; this pass
|
||
sees 4):
|
||
- `unresolved link to ChannelCore::register_openable` ×2 (from
|
||
`operations.rs:10, 62` — the nonexistent helper, C-02).
|
||
- `unresolved link to default_policy` (from `operations.rs:49`).
|
||
- `env is both a module and a macro` (from the `env` module name
|
||
colliding with the `env!` macro).
|
||
|
||
The commit message's "cargo doc generates" was technically true; clean
|
||
it is not.
|
||
|
||
## C-10 [major] — `channel/control` is a stub returning success for a dropped message
|
||
|
||
**ADR drift:** ADR-037 ("the channels layer routes `message` to the
|
||
handler's control handle for `channel_id`").
|
||
**Verified:** YES. `src/channels/operations.rs:236-248` parses
|
||
`channel_id`, ignores `message` (`_message`), and returns
|
||
`{"ok": true}`. There is no control-handle concept anywhere (no field
|
||
on `ChannelState`). Returning success for a silently-discarded control
|
||
message is worse than returning `unimplemented`.
|
||
|
||
## C-11 [major] — `channel/resources/subscribe` emits a one-shot snapshot of the wrong data
|
||
|
||
**ADR drift:** ADR-037 (as amended by ADR-047 §6): live subscription of
|
||
**openable resources** aggregated from ALPN-crate resource enumerators,
|
||
with subsequent events on any change.
|
||
**Verified:** YES, three problems
|
||
(`src/channels/operations.rs:257-277`):
|
||
1. `futures::stream::once(...)`: emits only the initial snapshot, never
|
||
"subsequent events on any change." ADR-037 explicitly rejected
|
||
poll-shaped behavior; this is a one-shot poll wearing a `Sub` type.
|
||
2. Wrong content: emits the ALPNs of **currently-open channels**, not
|
||
the set of **openable ALPNs** aggregated from ALPN-crate resource
|
||
enumerators ("which containers are running…"). A fresh connection
|
||
reports an empty resource set even when TTY/tunnel ops are
|
||
registered.
|
||
3. (Positive) The `access` preview **is** correctly dropped per
|
||
ADR-047 §6 (`operations.rs:255`; the output schema at `:148-161` has
|
||
no `access`).
|
||
|
||
## C-12 [major] — No guard on `channel/close` with `channel_id: 0`
|
||
|
||
**Verified:** YES. `channel/close` has no guard against `channel_id: 0`
|
||
(`src/channels/operations.rs:190`). Any peer can tear down channel-0
|
||
state via `teardown_channel(0)` (`manager.rs:284-298`), killing the
|
||
call channel's demux routing. **Fix:** reject `channel_id: 0` in the
|
||
`channel/close` handler (channel 0 is pre-negotiated and not
|
||
closeable).
|
||
|
||
## C-13 [major] — `channel/close` abort ≠ drain; REQ-CH-06 ordering absent
|
||
|
||
**ADR drift:** ADR-041 §3 (decrement "after the drain completes");
|
||
ADR-037 (responder drains and signals EOF to the handler);
|
||
channels-wire.md REQ-CH-06 (exit-chunk-before-close ordering — "the
|
||
channels layer owns the ordering guarantee").
|
||
**Verified:** YES. `channel/close`'s handler task is `abort()`ed
|
||
immediately and the decrement happens immediately
|
||
(`src/channels/operations.rs:190-204`); abort ≠ EOF-and-drain.
|
||
REQ-CH-06 (the close handler must observe data-pump completion) has no
|
||
implementation anywhere.
|
||
|
||
## C-14 [major] — Substrate modes: only in-line is implemented
|
||
|
||
**ADR drift:** ADR-034 §substrate modes, ADR-039 (QUIC-native
|
||
multi-stream substrate).
|
||
**Verified:** YES. `handle()` calls `connection.accept_bi()` **once**
|
||
and never loops (`src/channels/adapter.rs:159`). The doc comment at
|
||
`adapter.rs:80-84` claims the multi-stream behavior the code doesn't
|
||
have. The QUIC-native substrate ("accept remaining bidi streams… read
|
||
headers off each") is not implemented.
|
||
|
||
## C-15 [major] — `ChannelClient` API does not match ADR-043
|
||
|
||
**ADR drift:** ADR-043 (`open_channel(...) -> Channel`,
|
||
`Channel { channel_id, source }`, `subscribe_resources`, `call()`
|
||
accessor).
|
||
**Verified:** YES. `src/channels/client.rs` has `from_connection` ✓
|
||
(transport-agnostic, correct per the amendment), but: no
|
||
`open_channel(...) -> Channel`, no `Channel { channel_id, source }`,
|
||
no `subscribe_resources`, no `call()` accessor (instead `call_open_op`
|
||
+ `take_call_connection`). The doc comment at `client.rs:104-106`
|
||
refers to `manager.open_channel_stream(channel_id)` — **no such method
|
||
exists**. A consumer who receives `{channel_id: 7}` from the responder
|
||
has no way to obtain a `BiStream` for it (see C-08, channel adoption).
|
||
|
||
## C-16 [major] — Busy-wait spin on full write buffer
|
||
|
||
**Verified:** YES. `poll_write` on `TrySendError::Full` does
|
||
`cx.waker().wake_by_ref(); Poll::Pending`
|
||
(`src/channels/reassembly.rs:256-257`) — a hot busy-loop that pegs a
|
||
core until the mux drains (the waker is re-woken immediately, not
|
||
registered against capacity). The correct shape is storing a
|
||
`reserve()`/`poll_ready`-style future. The abandoned deliberation left
|
||
in comments (`reassembly.rs:205-255`) documents that the author knew
|
||
this (see C-20).
|
||
|
||
## C-17 [major] — Lost EOF sentinel path → remote handler hang
|
||
|
||
**Verified:** YES, with a precision correction to Sub Review B. The mux
|
||
pump **does** write an EOF chunk to the wire when it receives the
|
||
zero-length sentinel (`src/channels/mux.rs:124-133`). The real bug:
|
||
`MpscSendStream::poll_shutdown` (`reassembly.rs:283-291`) and `Drop`
|
||
(`:295-309`) emit the sentinel via `try_send`, best-effort; if the
|
||
buffer is full the sentinel is silently dropped. When the sentinel is
|
||
lost, the pump's `recv` yields `None` (sender dropped, no sentinel) and
|
||
the pump loop exits (`mux.rs:122-147`) **without** writing an EOF
|
||
chunk. Result: the remote handler never sees channel EOF until full
|
||
transport close. REQ-CH-01 violation on this path. The doc comments at
|
||
`reassembly.rs:283-291, 295-309` ("the peer's read will still EOF when
|
||
the sender drops — REQ-CH-02") are **wrong about which side of the
|
||
wire** this sender feeds (it feeds the local mux pump, not the peer's
|
||
demux). The doc comment at `mux.rs:109-112` ("the pump emits the EOF
|
||
sentinel") is misleading because it depends on the `Drop` impl's
|
||
`try_send` succeeding. **Fix:** the pump must write an EOF chunk when
|
||
its receiver ends without a sentinel (treat `recv → None` as implicit
|
||
EOF).
|
||
|
||
## C-18 [major] — `open_channel` TOCTOU on `max_channels`
|
||
|
||
**Verified:** YES. `src/channels/manager.rs:159-194` — the cap check
|
||
(`:161`) and the insert (`:188-189`) are under **separate** lock
|
||
acquisitions with `mux.register(...)` awaited in between
|
||
(`:170-176`). N concurrent opens at `len == max-1` all pass the check
|
||
and all insert → cap exceeded by N-1. **Fix:** hold the channels lock
|
||
across check + insert (release for the `mux.register` await, re-acquire
|
||
for insert, re-check on re-acquire).
|
||
|
||
## C-19 [minor] — Misc channels correctness issues
|
||
|
||
- **`next_id` wrap allocates channel 0:** `manager.rs:98` starts at 1;
|
||
`fetch_add(1, Relaxed)` wraps `u32::MAX → 0`, colliding with the
|
||
reserved channel 0. The `ChannelExists` defensive check catches the
|
||
map collision (`:189-193`), but only after registering a mux pump
|
||
that is never cleaned up. Effectively unreachable in practice
|
||
(ADR-040), but the wrap story is not handled as stated.
|
||
- **Unbounded loop in resources snapshot:** `operations.rs:267-274` —
|
||
`for channel_id in 0..u32::MAX` with a mutex lock per iteration,
|
||
break keyed on `resources.len() >= manager.open_count()`.
|
||
`open_count()` is re-read each iteration; a channel closing
|
||
mid-iteration (or sparse ids after churn — monotonic ids guarantee
|
||
sparseness) can iterate millions/billions of times.
|
||
- **Mux pump map leak + duplicate registration:** `mux.rs:148` —
|
||
finished pump `JoinHandle`s accumulate in `self.pumps` for the
|
||
connection's lifetime. Duplicate `register(channel_id)` silently
|
||
overwrites the handle while the old pump keeps running (two pumps,
|
||
one channel id).
|
||
- **Mux shutdown aborts pumps mid-`write_chunk`:** `mux.rs:154-158` —
|
||
pumps are `abort()`ed possibly mid-`write_chunk` while holding the
|
||
writer lock (a half-written chunk header corrupts the wire; moot
|
||
since the transport is going away, except the code then writes EOF
|
||
sentinels onto that possibly-mid-chunk stream).
|
||
- **`manager.rs:309`:** `drop(state.demux_sender.clone())` clones a
|
||
sender then drops the clone — a no-op (the real drop happens when
|
||
`drained` falls out of scope; the code works by accident, the line is
|
||
misleading).
|
||
- **`manager.rs:176, 218`:** mux `register` failure (runner closed)
|
||
mapped to `ManagerError::ChannelExists` — wrong error.
|
||
- **`client.rs:61`:** `install_channel_zero` error mapped to
|
||
`StreamError::StreamClosed` — conflation.
|
||
- **`policy.rs:127-140`:** `check_open` **increments** the count
|
||
(check-and-reserve). If allocation subsequently fails, no rollback
|
||
API exists other than `on_close(&Identity)`; a future open-op wrapper
|
||
must remember to decrement on every post-check failure path or quota
|
||
leaks. Not a bug today (nothing calls it), but a trap in the trait's
|
||
contract vs ADR-041's "check_open = check, on_close = decrement after
|
||
deallocation" framing.
|
||
- **REQ-CH-04's error counter (`Demux::stats()`):** no counter, no
|
||
stats surface; only a `debug!` log (`manager.rs:268-274`).
|
||
- **`derive_alpn_from_op_name`** (`src/client/from_call.rs:289-301`):
|
||
ADR-047 says non-`alknet/*` ALPNs "use their full ALPN string as the
|
||
path segment." The implementation takes only the **first** path
|
||
segment (`rest.split('/').next()` at `:291`), so
|
||
`channels/custom/proto/sub` derives `alknet/custom` — wrong. The
|
||
`segment.starts_with("alknet/")` branch at `:295` is dead code (a
|
||
single segment can't contain `/`), and `segment == "alknet"` yields
|
||
the nonsense ALPN `"alknet"` (`:296`).
|
||
- **`Box::leak` per discovery** (`from_call.rs:312-314`): bounded per
|
||
unique ALPN in theory, but it leaks on *every* rediscovery of every
|
||
marked op. Documented in-code as accepted; the `Arc<str>` refactor
|
||
should happen before any long-lived hub uses `from_call` against
|
||
churning peers.
|
||
- **`from_call.rs:269` "SAFETY:" comment** is factually wrong (describes
|
||
a `'static`-only return that isn't what `derive_alpn_from_op_name`
|
||
does — it returns `Option<String>`; the leak happens in `leak_alpn`),
|
||
and "SAFETY" is a loaded term that conventionally marks `unsafe`
|
||
blocks; there's no `unsafe` here.
|
||
- **Ledger placement:** ADR-047 §7 says "the ledger lives in
|
||
channels-call, not channels-core," but `OpenerLedger` is defined in
|
||
`src/channels/mux.rs:169-207` and owned by `ChannelManager`
|
||
(`manager.rs:81`) — the auth-blind core layer. Doc comment at
|
||
`manager.rs:62-69` argues for this; it's a deliberate deviation, but
|
||
it is a deviation, and the "which-crate" boundary matters when the
|
||
crate is split per ADR-044.
|
||
|
||
## C-20 [major → convention] — Abandoned deliberation + pervasive inline comments
|
||
|
||
**Convention drift:** AGENTS.md §1 (no inline `//` comments unless
|
||
asked; doc comments `///`/`//!` are fine).
|
||
**Verified:** YES. Worst instance: `src/channels/reassembly.rs:205-255`
|
||
— ~50 lines of abandoned stream-of-consciousness deliberation ("no, we
|
||
need `Sender::reserve_slot`… Actually, the simplest approach…") shipped
|
||
inside `poll_write`. Plus pervasive inline `//` comments across the
|
||
channels module: `adapter.rs:154-158, 163-165, 177-180, 189-194`,
|
||
`client.rs:57, 67, 74-76`, `manager.rs:170, 177-179, 190-191, 196,
|
||
234-237, 268-269, 291-292, 314`, `env.rs:112-126`,
|
||
`operations.rs:188-189, 195-196, 243-247, 262-266`,
|
||
`from_call.rs:256-262, 269-274`. A handful qualify as "non-obvious
|
||
correctness constraint" (REQ references); most don't. Several comments
|
||
are actively *wrong* (the mux EOF claim `mux.rs:109-112`, the
|
||
abort-cancels claim `dispatch.rs:495-498`, the
|
||
`reassembly.rs:283-291` "peer's read will still EOF" wrong-side claim,
|
||
the `from_call.rs:269` "SAFETY:" comment marking no `unsafe`).
|
||
|
||
## C-21 [minor → convention] — `write_header` panics on short buffers
|
||
|
||
**Convention drift:** AGENTS.md §2 (no panics in library code).
|
||
**Verified:** YES. `src/channels/wire.rs:110-114` — `write_header`
|
||
slices `out[..CHUNK_HEADER_LEN]`, panicking on short buffers; the doc
|
||
comment at `:106-107` declares the panic. All current callers pass
|
||
`[0u8; 8]`, but the API is `&mut [u8]`. Same class: a potential panic
|
||
path documented instead of typed. **Fix:** return a
|
||
`Result<_, ChunkError::HeaderTooShort>` (the error variant already
|
||
exists for the parse side, `wire.rs:87-90`).
|
||
|
||
## C-22 [minor → convention] — Filler tests padding the count
|
||
|
||
**Verified:** YES. `src/channels/client.rs:131-136` — a `PhantomData`
|
||
"smoke test" (`let _ = std::marker::PhantomData::<ChannelClient>;`).
|
||
`src/channels/env.rs:155` — `assert!(env.contains("anything") ||
|
||
!env.contains("anything"))` tautology. These pad the "66 new channels
|
||
tests" / "23 new Pub tests" counts claimed in the commit messages.
|
||
|
||
## C-23 [minor] — `dispatch_requested` Sink/Stream errors use empty request id
|
||
|
||
**Verified:** YES. `dispatch_requested`'s Sink error uses
|
||
`String::new()` as request id (`src/protocol/dispatch.rs:244-247`),
|
||
producing an envelope with an empty id (shared flaw with the Stream
|
||
arm at `:236-243`).
|
||
|
||
## C-24 [minor] — Producer/consumer naming leaked into docs
|
||
|
||
**Convention drift:** AGENTS.md §8 (producer/consumer, not
|
||
server/client).
|
||
**Verified:** YES, minor. Prod code is largely fine; "client→server
|
||
streaming" leaked from ADR-046 into `docs/architecture/call-protocol.md:335`
|
||
and the ADR-046 reference table at `:584`. No public API names offend;
|
||
only test-local `duplex` variable names use client/server
|
||
(`mux.rs:217, 253`, etc.) — acceptable.
|
||
|
||
## C-25 — Test coverage gaps (channels)
|
||
|
||
**Verified:** YES — present coverage is unit-level only; **no
|
||
end-to-end test wires two sides together**. Missing:
|
||
1. Channel 0 end-to-end (ADR-036): adapter ↔ client, one
|
||
`call_open_op` round-trip (would catch C-01 immediately).
|
||
2. Demux resync on `TooLarge` (channels-wire.md §MAX_CHUNK_LEN): send
|
||
oversized chunk + valid chunk; assert the valid one survives (C-07).
|
||
3. Backpressure semantics (ADR-040/REQ-CH-05): slow reader on channel
|
||
A, fast writer, assert no data loss and channel B unaffected (C-04,
|
||
C-05, C-16).
|
||
4. Policy decrement on connection drop / handler exit (ADR-047 §7):
|
||
open at cap, drop transport, reopen (C-06).
|
||
5. Per-connection `max_channels` via `open_channel` — no test fills 256
|
||
and asserts `TooManyChannels` (only `Display` is tested,
|
||
`manager.rs:446-452`); no concurrent-open race test (C-18).
|
||
6. REQ-CH-01 through the mux to the wire on drop-without-shutdown —
|
||
`mux_handle_register_and_write_round_trips_to_transport` covers
|
||
explicit shutdown only; the drop-with-full-buffer path is untested
|
||
(C-17).
|
||
7. REQ-CH-06 — untested (and unimplemented, C-13).
|
||
8. `channel/close` / `channel/control` / `resources/subscribe` handler
|
||
invocation — the handler bodies (`make_close_handler` etc.) have
|
||
zero tests; only spec-shape and registration are tested
|
||
(`operations.rs:329-364`).
|
||
9. `resolve_channel_manager` — untested (it's a stub; a test asserting
|
||
the ADR-047 §4 behavior would have flagged it, C-03).
|
||
10. ID wrap → 0 — untested (C-19).
|
||
11. Marker wire format: well tested ✓ (`discovery.rs:807-839`,
|
||
`from_call.rs:592-653`, `spec.rs:338-365`).
|
||
|
||
## C-26 — Spec docs internally inconsistent post-047
|
||
|
||
**Verified:** YES. Only `channel-operations.md` was updated. The other
|
||
channels spec docs still describe the pre-047 model that ADR-047
|
||
removed, and carry stale alknet ADR numbers (071/075/076/093/094/079/
|
||
080) that point to nonexistent files after the renumbering (alkcall
|
||
decisions only go up to 047):
|
||
- `docs/architecture/channel-client.md:54-59, 94-98` still specifies
|
||
`open_channel(alpn, params, direction)`, `ChannelDirection`, and
|
||
`ResourceEntry.access` — all removed by ADR-047.
|
||
- `docs/architecture/channels-wire.md:241` lifecycle table still says
|
||
"`channel/open` call operation… responder allocates."
|
||
- `docs/architecture/channels-adapter.md:142-194` still describes the
|
||
generic `channel/open` handler with `channel:unknown_alpn`.
|
||
- Stale ADR-number references: `channel-client.md:170-171`,
|
||
`channels-wire.md:273-274`, `channels-adapter.md:294-299`.
|
||
|
||
---
|
||
|
||
# Cross-cutting: what is solid (verified)
|
||
|
||
These were checked and are correct/close to spec — listed so the
|
||
remediation plan can focus on what's actually broken:
|
||
|
||
- **Wire formats:** `EventEnvelope` shape and the five+one event types
|
||
match ADR-014/046 exactly; the channels 8-byte chunk header
|
||
`[channel_id:u32 BE][length:u32 BE]` matches ADR-034/035 exactly
|
||
(`src/channels/wire.rs:85-114`). No `stream_type` anywhere in the
|
||
channels layer (verified by grep). ✓
|
||
- **`HandlerKind::Sink` validation:** Pub↔Sink both directions
|
||
(`src/registry/registration.rs:108-124, 371-411`), `make_sink_handler`
|
||
(`:587-593`), `with_local_sink`/`with_leaf_sink`/
|
||
`with_leaf_sink_provenance` (`:507-552`). ✓
|
||
- **Wire `op_type` strings:** `"query"`/`"mutation"`/`"sub"`/`"pub"`
|
||
emit (`src/registry/discovery.rs:163-169`) and parse with
|
||
`"subscription"` correctly rejected (`src/client/from_call.rs:316-326`).
|
||
✓
|
||
- **`call.published` payload `{ "input": <chunk> }`:** `src/protocol/wire.rs:54-56`,
|
||
correlated by envelope `id`. ✓
|
||
- **Responder-side `deadline: None` for Pub:** `dispatch.rs:310`
|
||
(implemented; untested — P-12).
|
||
- **`invoke()` / `invoke_streaming()` on Pub → `INVALID_OPERATION_TYPE`:**
|
||
`registration.rs:193-197, 262-271`. ✓
|
||
- **Gap F (`channel_open` marker):** `spec_to_json` emits
|
||
`"channel_open": true` only when set (`discovery.rs:213-215`),
|
||
schema documents it as optional (`:145-148`), `rebuild_spec_for`
|
||
parses it and ignores `false`/non-channels names (`from_call.rs:263-277`).
|
||
`ChannelOpenSpec { alpn: &'static str }` matches ADR-047 §2
|
||
(`spec.rs:38-40`). Well tested. ✓
|
||
- **Per-identity cap default 256:** `policy.rs:156`, `default_policy()`,
|
||
`PerIdentityChannelPolicy::new(256)` tested at `policy.rs:252-259`. ✓
|
||
- **Channel 0 pre-installed, skips the ledger:** correct
|
||
(`manager.rs:234-237`), ALPN recorded as `alknet/call`. ✓ (Functionally
|
||
dead per C-01, but the bookkeeping is right.)
|
||
- **Connection-owner allocation mechanism:** `ChannelManager::open_channel`
|
||
allocating via `next_id.fetch_add` (`manager.rs:167`) — "whoever holds
|
||
the manager allocates" ✓ — but nothing invokes it in the Sub flow (no
|
||
open-op wrapper, C-02) and the Pub-case flow doesn't exist.
|
||
- **Layering held:** no channels types leaked into the call crate,
|
||
`OperationEnv` stayed a trait, auth stays on the one
|
||
`AccessControl` path. ✓
|
||
- **Hub relay (ADR-042), broker (Gap B), `from_call` relay wrapper
|
||
(Gap C):** intentionally out of scope per ADR-047 — correctly not
|
||
flagged as bugs. ADR-047 Gap C expects alkcall's `from_call` to
|
||
reconstruct the marker so the consumer can branch — that half **is**
|
||
implemented ✓.
|
||
- **No `unwrap()`/`expect()`/panics** in the new library code outside
|
||
the `write_header` short-buffer path (C-21) and the channels
|
||
deliberation comments. No blocking I/O on async paths. Poisoning N/A
|
||
(`parking_lot`). ✓
|
||
|
||
---
|
||
|
||
# Remediation plan
|
||
|
||
The plan is split into **six units of work**, ordered by dependency and
|
||
severity. Each unit is a independently shippable commit (or small
|
||
sequence) with its own acceptance gate. The units are sized so that
|
||
none of the critical bugs is blocked behind a spec decision except
|
||
where noted (C-03, C-08). The overarching acceptance gate for the
|
||
whole effort is the single end-to-end test both reviews identify as
|
||
missing:
|
||
|
||
> **One end-to-end test:** `ChannelClient` ↔ `ChannelsAdapter`, open a
|
||
> channel via a registered op, move bytes, tear down, verify quota.
|
||
|
||
That test is the gate because its absence is what let both commits land
|
||
green. Each unit below either moves toward it or removes a defect it
|
||
would surface.
|
||
|
||
## Unit 1 — Fix Pub end-to-end (P-01, P-02, P-05, P-08, P-12 #1)
|
||
|
||
**Goal:** a `publish()` that actually works on any transport.
|
||
**Files:** `src/protocol/connection.rs`, `src/protocol/dispatch.rs`.
|
||
**Scope:**
|
||
- P-01: keep stream A's write half; pump `call.published` + `call.completed`
|
||
on it (do not re-open `open_bi` in `write_publish_chunks`).
|
||
- P-02: change `publish`/`publish_with_payload` to take
|
||
`Pin<Box<dyn Stream<Item = Value> + Send>>` (ADR-046 §8 shape). Hold
|
||
stream A's write half and pump the stream.
|
||
- P-05: register the publish like `subscribe` (`timeout: None`), not
|
||
with `DEFAULT_CALL_TIMEOUT`. Falls out of the streaming rework.
|
||
- P-08: route the dispatch Pub branch through
|
||
`OperationRegistry::invoke_sink` instead of re-implementing it inline
|
||
(removes the divergence hazard; the `invoke_sink` path becomes the
|
||
wire path).
|
||
- P-12 #1: add the end-to-end publish test (≥2 chunks, assert the
|
||
responder sees them and returns the right result). This is the
|
||
acceptance gate for the unit.
|
||
|
||
**Note:** P-02 changes a public API (`publish` signature) — but per
|
||
ADR-046 §8 the `Stream` shape *is* the spec, so the current `Vec<Value>`
|
||
was the drift. This is correcting drift, not introducing a wire break
|
||
(the wire format is unchanged). No deployments exist.
|
||
|
||
## Unit 2 — Channel 0 single-stream call mode, both sides (C-01, C-25 #1)
|
||
|
||
**Goal:** channel 0 actually carries call traffic in both directions.
|
||
**Files:** `src/protocol/connection.rs`, `src/protocol/dispatch.rs`,
|
||
`src/channels/source.rs`, `src/channels/client.rs`,
|
||
`src/channels/adapter.rs`.
|
||
**Scope:** This is the real design gap, not a one-line fix. The call
|
||
protocol's stream-per-request model is incompatible with channel 0's
|
||
single yield-once `BiStream`. Options:
|
||
- (a) A single-stream call mode: multiplex all requests/responses on
|
||
one framed stream (length-prefixed `EventEnvelope`s on channel 0's
|
||
one `BiStream`). Both `CallConnection` (client) and `Dispatcher`
|
||
(accept side) gain a "single-stream mode" that reads/writes frames
|
||
on the one `BiStream` instead of opening/accepting one stream per
|
||
request. `ChannelBidiStreamSource::open_bi` stops being called.
|
||
- (b) Make `ChannelBidiStreamSource` yield a fresh `BiStream` per
|
||
`accept_bi`/`open_bi` by framing sub-streams over the one channel —
|
||
more complex, probably wrong.
|
||
Recommend (a). This unblocks C-01's connect side (every `call_open_op`
|
||
no longer fails) and accept side (channel 0's `Connection` is actually
|
||
driven by a call dispatch loop). **Acceptance gate:** C-25 #1 — one
|
||
end-to-end channel-0 round-trip test. **May require a short ADR or
|
||
ADR-036 amendment** recording the single-stream-mode decision (it's a
|
||
two-way-door implementation detail per ADR-046/036, but worth pinning).
|
||
|
||
## Unit 3 — `register_openable` + decide the §4 env-resolution question (C-02, C-03)
|
||
|
||
**Goal:** a channel-open op can actually be registered and invoked.
|
||
**Files:** `src/channels/operations.rs`, `src/channels/env.rs`,
|
||
`src/registry/env.rs` (possibly, if `as_any()` is added).
|
||
**Scope:** This unit requires a **spec decision first**:
|
||
- **Option A (implement §4 as written):** add `as_any()` (or equivalent
|
||
downcast machinery) to `OperationEnv`, implement
|
||
`resolve_channel_manager` for real, and have the open-op wrapper
|
||
resolve the per-connection manager at invocation time.
|
||
- **Option B (amend ADR-047 §4):** bless the "manager passed at
|
||
registration via `ChannelCore`" shape that the stub's comment
|
||
describes. This drops per-connection resolution but is simpler; it
|
||
works only if a `ChannelCore` is registered per-connection (an
|
||
overlay concern), which interacts with ADR-019's layering.
|
||
Recommend Option A (it preserves the ADR's stated design and the
|
||
session/connection overlay patterns from ADR-024). Whichever is
|
||
chosen, **amend or affirm ADR-047 §4 in writing** before coding. Then:
|
||
- Implement `ChannelCore::register_openable(spec, open_handler,
|
||
channel_core)` wrapping the ALPN open handler with the
|
||
ACL→`check_open`→allocate→ledger→spawn→respond flow
|
||
(`src/channels/operations.rs:280-311`).
|
||
- Map `channel:forbidden` / `channel:no_channels_session` /
|
||
`channel:allocation_failed` / `channel:too_many_channels` to
|
||
`CallError`.
|
||
- Fix the broken rustdoc links (C-09's `register_openable` ×2) once the
|
||
method exists.
|
||
- Test: register a no-op open op via `register_openable`, invoke it
|
||
end-to-end through channel 0 (depends on Unit 2), assert
|
||
`channel_id` is returned and quota is reserved.
|
||
|
||
## Unit 4 — Backpressure fixes (C-04, C-05, C-07, C-16, C-17, C-25 #2 #3 #6)
|
||
|
||
**Goal:** the channels layer is lossless and bounded as ADR-040
|
||
promises.
|
||
**Files:** `src/channels/manager.rs`, `src/channels/reassembly.rs`,
|
||
`src/channels/adapter.rs`, `src/channels/mux.rs`.
|
||
**Scope:**
|
||
- C-04: make `route_payload` async (or use `blocking_send`/`reserve`),
|
||
await on full instead of dropping. The demux loop calls it on the
|
||
read path — make the demux loop await it.
|
||
- C-05: replace the message-count buffer cap with a byte-budgeted cap
|
||
(track accumulated `payload.len()` against the cap). Update
|
||
`DEFAULT_BUFFER_CAP`'s doc comment to match reality.
|
||
- C-07: on `ChunkError::TooLarge`, read and discard `length` bytes
|
||
before continuing. The parsed length is in the error variant.
|
||
- C-16: replace the `wake_by_ref` + `Pending` busy-loop with a stored
|
||
`reserve()` future (or `Poll`-friendly ready notification). Remove
|
||
the ~50 lines of abandoned deliberation while here (C-20 overlap).
|
||
- C-17: the mux pump must write an EOF chunk when its receiver ends
|
||
without a sentinel (treat `recv → None` as implicit EOF). Fix the
|
||
wrong-side doc comments in `reassembly.rs:283-291, 295-309` and the
|
||
misleading `mux.rs:109-112` claim.
|
||
- C-25 #2 #3 #6: add the demux-resync test, the backpressure test, and
|
||
the drop-without-shutdown EOF test.
|
||
|
||
## Unit 5 — Ledger decrement on all teardown paths + channel-id adoption/collision decision (C-06, C-08, C-12, C-13, C-18)
|
||
|
||
**Goal:** quota accounting is correct and channel ids don't collide.
|
||
**Files:** `src/channels/adapter.rs`, `src/channels/manager.rs`,
|
||
`src/channels/operations.rs`, `src/channels/mux.rs`.
|
||
**Scope:**
|
||
- C-06: wire `on_close` into the three missing teardown paths:
|
||
close-sent locally, handler-exit, connection-drop. On transport EOF,
|
||
`clear_all()` already returns the `(channel_id, opener)` list — call
|
||
`policy.on_close(&opener)` for each (the adapter needs a hook to the
|
||
policy; this is the architectural gap Sub Review B flagged — add a
|
||
`DecrementOnDrop` hook or have the adapter hold a `ChannelCore`).
|
||
- C-08: **spec decision needed** — odd/even id split, or an adoption op
|
||
so the non-allocating side can install routing for a remotely-assigned
|
||
id. Recommend odd/even (initiator allocates odd, responder even, or
|
||
vice versa) as the smaller change; record the decision in ADR-047 §5
|
||
or a follow-on ADR. Then implement the adoption path: the
|
||
non-allocating side registers the remotely-assigned id in its
|
||
manager/mux.
|
||
- C-12: reject `channel_id: 0` in the `channel/close` handler.
|
||
- C-13: `channel/close` must drain (signal EOF to the handler, await
|
||
pump completion) before `abort`/decrement; implement REQ-CH-06
|
||
ordering (the close handler observes data-pump completion).
|
||
- C-18: hold the channels lock across the `max_channels` check + insert
|
||
in `open_channel` (re-check on re-acquire after the `mux.register`
|
||
await).
|
||
- Test: C-25 #4 #5 — open-at-cap-then-drop-transport-then-reopen, and
|
||
the concurrent-open race test.
|
||
|
||
## Unit 6 — Cleanup: comments, doc links, stale spec docs, stubs (C-09, C-10, C-11, C-14, C-15, C-19, C-20, C-21, C-22, C-23, C-24, C-26, P-03, P-04, P-06, P-07, P-09, P-10, P-11, P-13)
|
||
|
||
**Goal:** conventions satisfied, docs consistent, the remaining
|
||
decided-but-unimplemented behaviors either implemented or explicitly
|
||
deferred with an OQ.
|
||
**Scope:** This is the largest unit by item count but the lowest risk;
|
||
do it last, after the integration is end-to-end functional. Items:
|
||
- **C-20:** remove the ~50 lines of abandoned deliberation in
|
||
`reassembly.rs:205-255` and the pervasive inline `//` comments
|
||
across the channels module that aren't carrying a non-obvious
|
||
correctness constraint. Fix the actively-wrong comments (mux EOF
|
||
claim, abort-cancels claim, wrong-side peer-EOF claim, "SAFETY:"
|
||
comment marking no `unsafe`).
|
||
- **C-09:** fix the remaining `cargo doc` warnings (`default_policy`
|
||
link, `env is both a module and a macro`).
|
||
- **C-26:** update `channel-client.md`, `channels-wire.md`,
|
||
`channels-adapter.md` to the post-047 model (remove
|
||
`open_channel(..., direction)`, `ChannelDirection`,
|
||
`ResourceEntry.access`, the generic `channel/open` handler,
|
||
`channel:unknown_alpn`; renumber stale alknet ADR refs to alkcall
|
||
ADR-001..047).
|
||
- **C-21:** make `write_header` return `Result<_, ChunkError>` instead
|
||
of panicking.
|
||
- **C-22:** remove the filler tests (`client.rs:131-136`, `env.rs:155`)
|
||
or replace them with real assertions.
|
||
- **C-23:** give `dispatch_requested`'s Sink/Stream error envelopes a
|
||
real request id (or document why empty is correct).
|
||
- **C-24:** replace "client→server streaming" in
|
||
`call-protocol.md:335` with producer/consumer phrasing; amend
|
||
ADR-046 §3 to match §6 (P-11).
|
||
- **C-19 misc:** fix `next_id` wrap, the unbounded resources-snapshot
|
||
loop, the mux pump map leak / duplicate registration, the wrong
|
||
`ManagerError` mapping on mux register failure, the misleading
|
||
`drop(state.demux_sender.clone())`, the `derive_alpn_from_op_name`
|
||
multi-segment bug + dead branch, the `Box::leak` `Arc<str>` refactor,
|
||
the `policy.rs:127-140` check-and-reserve rollback trap (document or
|
||
add a rollback API), the `Demux::stats()` error counter (REQ-CH-04).
|
||
- **P-03:** implement `publish_schema` per-chunk validation using
|
||
`alktype` (the dependency is already declared and unused), and wire
|
||
it through `spec_to_json` / `operation_spec_schema()` /
|
||
`rebuild_spec_for` so imported Pub ops keep the schema. Or, if
|
||
deferred, file an OQ and remove the misleading doc comment at
|
||
`spec.rs:181-186`.
|
||
- **P-04:** make `pump_sink` match `call.error` and inject the
|
||
initiator's `CallError` as an `Err` item that terminates the stream
|
||
(per ADR-046 §6).
|
||
- **P-06:** implement abort-cancels-Pub. This likely needs the
|
||
single-stream call mode from Unit 2 (so an abort on the same stream
|
||
can reach the running pump) plus a mechanism to drop the handler
|
||
future on abort (the doc comments at `dispatch.rs:494-498` describe
|
||
the target shape). Decide whether to fix the cross-stream path
|
||
(`abort()` opening a new stream) or deprecate it in favor of
|
||
same-stream abort.
|
||
- **P-07:** fix the handler-returns-early stall — when the handler
|
||
returns, the feed should be short-circuited and the response written
|
||
immediately (don't `join!` to completion). Use `tokio::select!` or
|
||
feed the handler's completion back into the feed loop.
|
||
- **P-09:** fix `make_sink_forwarding_handler` to stream (not
|
||
`collect`), and to terminate on `Err` (not `filter_map` it away).
|
||
Depends on P-01/P-02.
|
||
- **P-10:** use the `EVENT_*` wire constants in `pump_sink` instead of
|
||
string literals.
|
||
- **P-13:** opaque-wrap `SinkDispatch::chunk_tx` before crates.io.
|
||
- **C-10 / C-11:** either implement `channel/control` routing and live
|
||
`resources/subscribe`, or file OQs and make the stubs honest (return
|
||
`unimplemented`-style errors, not fake success).
|
||
- **C-14 / C-15:** either implement the QUIC-native substrate and the
|
||
full `ChannelClient` API (`open_channel`, `Channel`,
|
||
`subscribe_resources`), or file OQs and remove the misleading doc
|
||
comments / nonexistent method references.
|
||
|
||
## Suggested sequencing
|
||
|
||
```
|
||
Unit 1 (Pub end-to-end) → unblocks the Pub flagship use cases
|
||
Unit 2 (channel 0 single-stream) → unblocks all channel-0 integration tests
|
||
Unit 3 (register_openable) → needs a §4 spec decision first; unblocks opening channels
|
||
Unit 4 (backpressure) → independent; can run in parallel with 2/3
|
||
Unit 5 (ledger + adoption) → needs a §5 spec decision first; depends on 3
|
||
Unit 6 (cleanup) → last; after the integration is end-to-end functional
|
||
```
|
||
|
||
Units 1, 2, 4 can proceed immediately with no spec decisions. Units 3
|
||
and 5 each need one written decision (ADR-047 §4 affirm/amend; ADR-047
|
||
§5 odd/even-or-adoption). Unit 6 is the long tail.
|
||
|
||
## On the baseline objective
|
||
|
||
The layering held, the wire formats are right, the architecture is
|
||
right. What's missing is the connective tissue a tty/docker crate would
|
||
actually call: a working `publish()`, a working channel 0,
|
||
`register_openable`, channel adoption, and a streaming `publish()`.
|
||
**A tty extraction cannot start on this foundation yet** — treat the
|
||
end-to-end `ChannelClient` ↔ `ChannelsAdapter` test (Unit 2's gate,
|
||
then Unit 3's gate) as the acceptance gate for the next round.
|
||
|
||
---
|
||
|
||
## Verification log (this consolidation pass)
|
||
|
||
Findings re-verified directly in source on 2026-08-12 against tree
|
||
`f305f8c`. Every `file:line` reference above was checked. Findings that
|
||
did not survive verification or were overstated in the original
|
||
reviews, corrected here:
|
||
|
||
- **Sub Review B's "the pump does not write an EOF chunk" (C-17):**
|
||
overstated. The pump *does* write EOF when it receives the sentinel
|
||
(`mux.rs:124-133`). The real bug is narrower: when the sentinel is
|
||
lost (full buffer `try_send`), the pump exits on `recv → None`
|
||
without writing EOF. C-17 above states the corrected mechanism.
|
||
- **Warning count (C-09):** the main review said 5; this pass sees 4
|
||
`cargo doc` warnings (2× `register_openable`, 1× `default_policy`,
|
||
1× `env` module/macro). Documented as 4.
|
||
- **P-11:** flagged in Sub Review A as drift; on verification this is
|
||
an internal ADR-046 inconsistency (§3 vs §6) that the code resolved
|
||
correctly and consistently. Reclassified from "drift" to "amend the
|
||
ADR §3 text."
|
||
|
||
All other findings (P-01 through P-10, C-01 through C-08, C-12 through
|
||
C-26) verified as stated. |