The publish path's decided-but-unimplemented behaviors land: per-chunk
schema validation, initiator call.error terminates the stream, and an
early handler return short-circuits the feed. alktype::validation::
build_validator is now used on both dispatch paths (the dependency was
declared and unused); jsonschema was added as a direct dependency
because its Validator type is part of alktype's public API.
P-03 — OperationSpec::publish_schema was declared, had a builder,
defaulted to None, and was never read. It now round-trips through
discovery: spec_to_json emits "publish_schema" when Some,
operation_spec_schema() advertises the property, and rebuild_spec_for
parses it back (null/absent treated as None, so an imported Pub op no
longer loses the schema). Both dispatch paths (pump_sink and
run_loop_single_stream) validate each call.published chunk's input
against the schema before yielding it to the SinkHandler; on
validation failure an Err(CallError::invalid_input(...)) with the
offending chunk in `details` is injected and the feed terminates,
matching the SinkHandler contract that an Err terminates the stream.
The validator is built once at dispatch time and carried in the
SinkDispatch (and, for the single-stream path, in a new InFlightSink
struct alongside chunk_tx); a schema that fails to compile logs a
warn and falls back to no validation. When publish_schema is None,
chunks are yielded as-is (unchanged behavior).
P-04 — pump_sink matched only call.published/call.completed/call.
aborted and dropped everything else into a debug-log ignore branch; a
call.error from the initiator was silently ignored. Both dispatch
paths now match call.error, parse the CallError from the payload,
inject it as Err(call_error) into the sink's chunk_tx, and terminate
the feed — the handler sees the initiator's error, not a synthetic
"aborted" message. A malformed payload falls back to
CallError::internal("publish error from initiator (malformed)").
P-07 — pump_sink used tokio::join!(feed_fut, handler) and only wrote
the response after both completed; an early-returning handler (e.g.
rejects after chunk 1) had its response deferred until the initiator
finished publishing or the next chunk_tx.send failed. Replaced with a
tokio::select! loop over handler.fuse() and reader.read_frame(): when
the handler completes first, the response is captured and the loop
breaks immediately, the feed is short-circuited (chunk_tx dropped,
the handler's PublishStream sees EOF), and the response is written
without waiting for the feed. The feed-wins branch (natural end /
abort / error / read failure) awaits the handler after the loop as
before. This removes the unbounded stall on a path ADR-046 intends
for long-lived streams.
Tests (15 new, 480 total):
- discovery: spec_to_json emits/omits publish_schema; operation_spec_
schema documents the property (3).
- from_call: rebuild_spec_for parses publish_schema (present/omitted/
null) and round-trips with spec_to_json (4).
- dispatch (stream-per-request): publish_schema rejects invalid chunk
+ passes valid chunks + no-schema yields as-is (3); initiator
call.error terminates with the initiator's error + malformed
fallback (2); early handler return short-circuits a slow feed and
the response is not deferred (1).
- channels/client (single-stream): publish_schema rejects invalid
chunk over channel 0; initiator call.error terminates the publish
over channel 0 (drives run_loop_single_stream directly with crafted
frames since the public publish() API takes Stream<Item = Value> and
cannot emit an initiator call.error) (2).
Verification:
- cargo test → 480 passed, 0 failed (was 465; +15 new tests)
- cargo clippy --all-targets -- -D warnings → clean
- cargo fmt --check → clean
- cargo doc --no-deps → 0 warnings
72 KiB
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::Sinkvalidation,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_sinkblocks 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 secondopen_bireturnsStreamClosed(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
Fixed (2026-08-13, Unit 8). publish_schema now round-trips through
spec_to_json / operation_spec_schema() / rebuild_spec_for, and both
dispatch paths (pump_sink and run_loop_single_stream) validate each
call.published chunk's input against the schema via
alktype::validation::build_validator; a validation failure injects
Err(CallError::invalid_input(...)) and terminates the stream.
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
Fixed (2026-08-13, Unit 8). Both pump_sink and
run_loop_single_stream now match call.error, parse the CallError
from the payload, inject it as Err(call_error) into the sink's
chunk_tx, and terminate the feed. A malformed payload falls back to
CallError::internal("publish error from initiator (malformed)").
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()sendscall.abortedon a new stream (src/protocol/connection.rs:236-243→write_envelope→open_biat:388-392). On the responder that frame is handled by a differenthandle_streamtask, whosehandle_abortonly mutates thePendingRequestMap(src/protocol/dispatch.rs:381-389). Inbound requests are never registered in that map, and nothing links the map to the spawnedhandle_stream/pump_sinktask — the runningSinkHandlerand feed loop are untouched. The doc comments atdispatch.rs:494-498(and:459-462forpump_stream) describe a cancellation mechanism that does not exist. - Same-stream (dead-untested):
pump_sinkinjects anErrand breaks the feed oncall.aborted(dispatch.rs:538-543), then stilltokio::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
Fixed (2026-08-13, Unit 8). pump_sink now uses
tokio::select! over handler.fuse() and reader.read_frame(); when
the handler completes first, the response is written immediately and
the feed is short-circuited (the loop breaks, chunk_tx is dropped,
the handler's PublishStream sees EOF). The feed-wins branch awaits
the handler after the loop as before.
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:
- 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). Erritems are silently discarded and iteration continues (item.ok()→None→ filtered out), contradicting the contract that anErrterminates the stream — a partial/errored publish is forwarded upstream as if it completed cleanly.- 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:
- End-to-end publish through
CallConnection::publish↔Dispatcher::handle_stream(would have caught P-01 immediately). deadline: Nonefor Pub —dispatch_pub_clears_deadline_to_noneasserts nothing about the deadline; the test name is misleading.publish_schemavalidation (untested + unimplemented, P-03).- Initiator
call.errorterminating the publish stream (P-04). - Abort during a Pub, both directions (P-06 — same-stream path is
dead-untested; no initiator-side
abort()of an in-flight publish). from_callPub forwarding — no test thatbuild_bundlesmaps"pub"→HandlerKind::Sink; no behavioral test ofmake_sink_forwarding_handler(Err-dropping, forwarded_for population).- 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. OverlayOperationEnvSink 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'sConnectionis built and bound to_channel0_conn, which is never handed to anything. TheInstallChannelZerohook (adapter.rs:41-42) receives only(&ChannelManager, &AuthContext)— it has no way to reach the channel-0Connectionor its send/recv halves (already consumed byinstall_channel_zeroatadapter.rs:181, which can't be called again — it returnsChannelExists). Inboundcall.requestedchunks 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-65wraps channel 0 inCallConnection, butCallConnection::call_with_payloadopens a new bidi stream per request viaconnection.open_bi(), andChannelBidiStreamSource::open_bireturnsErr(StreamClosed)unconditionally (src/channels/source.rs:58-60). So everyChannelClient::call_open_opfails 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 continues 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_ids (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 (fromoperations.rs:10, 62— the nonexistent helper, C-02).unresolved link to default_policy(fromoperations.rs:49).env is both a module and a macro(from theenvmodule name colliding with theenv!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):
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 aSubtype.- 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.
- (Positive) The
accesspreview is correctly dropped per ADR-047 §6 (operations.rs:255; the output schema at:148-161has noaccess).
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 atclient.rs:104-106refers tomanager.open_channel_stream(channel_id)— no such method exists. A consumer who receives{channel_id: 7}from the responder has no way to obtain aBiStreamfor 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_idwrap allocates channel 0:manager.rs:98starts at 1;fetch_add(1, Relaxed)wrapsu32::MAX → 0, colliding with the reserved channel 0. TheChannelExistsdefensive 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::MAXwith a mutex lock per iteration, break keyed onresources.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 pumpJoinHandles accumulate inself.pumpsfor the connection's lifetime. Duplicateregister(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 areabort()ed possibly mid-write_chunkwhile 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 whendrainedfalls out of scope; the code works by accident, the line is misleading).manager.rs:176, 218: muxregisterfailure (runner closed) mapped toManagerError::ChannelExists— wrong error.client.rs:61:install_channel_zeroerror mapped toStreamError::StreamClosed— conflation.policy.rs:127-140:check_openincrements the count (check-and-reserve). If allocation subsequently fails, no rollback API exists other thanon_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 adebug!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), sochannels/custom/proto/subderivesalknet/custom— wrong. Thesegment.starts_with("alknet/")branch at:295is dead code (a single segment can't contain/), andsegment == "alknet"yields the nonsense ALPN"alknet"(:296).Box::leakper 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; theArc<str>refactor should happen before any long-lived hub usesfrom_callagainst churning peers.from_call.rs:269"SAFETY:" comment is factually wrong (describes a'static-only return that isn't whatderive_alpn_from_op_namedoes — it returnsOption<String>; the leak happens inleak_alpn), and "SAFETY" is a loaded term that conventionally marksunsafeblocks; there's nounsafehere.- Ledger placement: ADR-047 §7 says "the ledger lives in
channels-call, not channels-core," but
OpenerLedgeris defined insrc/channels/mux.rs:169-207and owned byChannelManager(manager.rs:81) — the auth-blind core layer. Doc comment atmanager.rs:62-69argues 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:
- Channel 0 end-to-end (ADR-036): adapter ↔ client, one
call_open_opround-trip (would catch C-01 immediately). - Demux resync on
TooLarge(channels-wire.md §MAX_CHUNK_LEN): send oversized chunk + valid chunk; assert the valid one survives (C-07). - 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).
- Policy decrement on connection drop / handler exit (ADR-047 §7): open at cap, drop transport, reopen (C-06).
- Per-connection
max_channelsviaopen_channel— no test fills 256 and assertsTooManyChannels(onlyDisplayis tested,manager.rs:446-452); no concurrent-open race test (C-18). - REQ-CH-01 through the mux to the wire on drop-without-shutdown —
mux_handle_register_and_write_round_trips_to_transportcovers explicit shutdown only; the drop-with-full-buffer path is untested (C-17). - REQ-CH-06 — untested (and unimplemented, C-13).
channel/close/channel/control/resources/subscribehandler invocation — the handler bodies (make_close_handleretc.) have zero tests; only spec-shape and registration are tested (operations.rs:329-364).resolve_channel_manager— untested (it's a stub; a test asserting the ADR-047 §4 behavior would have flagged it, C-03).- ID wrap → 0 — untested (C-19).
- 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-98still specifiesopen_channel(alpn, params, direction),ChannelDirection, andResourceEntry.access— all removed by ADR-047.docs/architecture/channels-wire.md:241lifecycle table still says "channel/opencall operation… responder allocates."docs/architecture/channels-adapter.md:142-194still describes the genericchannel/openhandler withchannel: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:
EventEnvelopeshape 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). Nostream_typeanywhere in the channels layer (verified by grep). ✓ HandlerKind::Sinkvalidation: 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_typestrings:"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.publishedpayload{ "input": <chunk> }:src/protocol/wire.rs:54-56, correlated by envelopeid. ✓- Responder-side
deadline: Nonefor 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_openmarker):spec_to_jsonemits"channel_open": trueonly when set (discovery.rs:213-215), schema documents it as optional (:145-148),rebuild_spec_forparses it and ignoresfalse/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 atpolicy.rs:252-259. ✓ - Channel 0 pre-installed, skips the ledger: correct
(
manager.rs:234-237), ALPN recorded asalknet/call. ✓ (Functionally dead per C-01, but the bookkeeping is right.) - Connection-owner allocation mechanism:
ChannelManager::open_channelallocating vianext_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,
OperationEnvstayed a trait, auth stays on the oneAccessControlpath. ✓ - Hub relay (ADR-042), broker (Gap B),
from_callrelay wrapper (Gap C): intentionally out of scope per ADR-047 — correctly not flagged as bugs. ADR-047 Gap C expects alkcall'sfrom_callto reconstruct the marker so the consumer can branch — that half is implemented ✓. - No
unwrap()/expect()/panics in the new library code outside thewrite_headershort-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.completedon it (do not re-openopen_biinwrite_publish_chunks). - P-02: change
publish/publish_with_payloadto takePin<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 withDEFAULT_CALL_TIMEOUT. Falls out of the streaming rework. - P-08: route the dispatch Pub branch through
OperationRegistry::invoke_sinkinstead of re-implementing it inline (removes the divergence hazard; theinvoke_sinkpath 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
EventEnvelopes on channel 0's oneBiStream). BothCallConnection(client) andDispatcher(accept side) gain a "single-stream mode" that reads/writes frames on the oneBiStreaminstead of opening/accepting one stream per request.ChannelBidiStreamSource::open_bistops being called. - (b) Make
ChannelBidiStreamSourceyield a freshBiStreamperaccept_bi/open_biby framing sub-streams over the one channel — more complex, probably wrong. Recommend (a). This unblocks C-01's connect side (everycall_open_opno longer fails) and accept side (channel 0'sConnectionis 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) toOperationEnv, implementresolve_channel_managerfor 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 aChannelCoreis 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_channelstoCallError. - 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), assertchannel_idis 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_payloadasync (or useblocking_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). UpdateDEFAULT_BUFFER_CAP's doc comment to match reality. - C-07: on
ChunkError::TooLarge, read and discardlengthbytes before continuing. The parsed length is in the error variant. - C-16: replace the
wake_by_ref+Pendingbusy-loop with a storedreserve()future (orPoll-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 → Noneas implicit EOF). Fix the wrong-side doc comments inreassembly.rs:283-291, 295-309and the misleadingmux.rs:109-112claim. - 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_closeinto the three missing teardown paths: close-sent locally, handler-exit, connection-drop. On transport EOF,clear_all()already returns the(channel_id, opener)list — callpolicy.on_close(&opener)for each (the adapter needs a hook to the policy; this is the architectural gap Sub Review B flagged — add aDecrementOnDrophook or have the adapter hold aChannelCore). - 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: 0in thechannel/closehandler. - C-13:
channel/closemust drain (signal EOF to the handler, await pump completion) beforeabort/decrement; implement REQ-CH-06 ordering (the close handler observes data-pump completion). - C-18: hold the channels lock across the
max_channelscheck + insert inopen_channel(re-check on re-acquire after themux.registerawait). - Test: C-25 #4 #5 — open-at-cap-then-drop-transport-then-reopen, and the concurrent-open race test.
Units 6-10 — the post-integration long tail
The original Unit 6 was the largest unit by item count and mixed trivial doc fixes with real protocol-semantics work (P-03/P-04/P-06/ P-07). It has been decomposed into five focused units, ordered by risk and dependency. Several original Unit 6 items were already absorbed by Units 1-5 during their implementation:
- P-09 fell out of Unit 1's P-01/P-02 streaming rework
(
make_sink_forwarding_handlernow streams + terminates onErr). - C-20's biggest item — the ~50 lines of abandoned deliberation in
reassembly.rs— landed with Unit 4's backpressure rewrite (reassembly.rs was rewritten from scratch). - C-22's
env.rs:155tautology test was removed in Unit 3. - C-19's
next_idwrap-to-0, the misleadingdrop(state.demux_sender.clone()), and themanager.rsmux-register error mapping were fixed in Units 4/5. - C-13 (drain-before-close) and C-06 (all 4 teardown paths) landed in Unit 5.
The units below cover what remains. Each is independently shippable.
Unit 6 — Convention + doc cleanup (C-09, C-20-rem, C-22-rem, C-24, P-10, P-11)
Goal: conventions satisfied, cargo doc clean, no actively-wrong
comments, producer/consumer naming consistent in docs.
Files: src/protocol/dispatch.rs, src/client/from_call.rs,
src/channels/operations.rs, src/channels/mod.rs,
docs/architecture/call-protocol.md,
docs/architecture/operation-registry.md,
docs/architecture/decisions/046-publish-operation-type-and-handler-kind-sink.md.
Scope:
- P-10:
pump_sinkmatches the string literals"call.published","call.completed","call.aborted"(dispatch.rs:475,485,486) instead of theEVENT_PUBLISHED/EVENT_COMPLETED/EVENT_ABORTEDconstants the rest of the file imports (wire.rs:12-17). Pure refactor hazard; no behavior change. - C-20 remainder: the wrong "SAFETY:" comment at
from_call.rs:271(marks nounsafeblock —derive_alpn_from_op_namereturnsOption<String>, the leak happens inleak_alpn). Reword to a plain note about the'staticlifetime requirement. The mux EOF claim, abort-cancels claim, and wrong-side peer-EOF claim were fixed with Unit 4's reassembly rewrite — verify they're gone and remove any remaining stray inline//comments in the channels module that aren't carrying a non-obvious correctness constraint. - C-09: fix the 2 remaining
cargo docwarnings (verified 2026-08-13):unresolved link to default_policy(operations.rs:50— the[default_policy]intra-doc link resolves tosuper::policy::default_policy; use the full path[super::policy::default_policy]or re-export).env is both a module and a macro(channels/mod.rs:30— the[env]link in the module doc is ambiguous; qualify it as[env][self::env]or use the full path in the prose).
- C-22 remainder: remove the filler
PhantomDatatest atclient.rs:291-294(let _ = std::marker::PhantomData::<ChannelClient>;— asserts nothing). Theenv.rs:155tautology is already gone. - C-24: replace "client→server streaming" with producer/consumer
phrasing in
call-protocol.md(the Pub reference-table line) andoperation-registry.md(the ADR-046 reference-table line). No public API names offend; this is doc-only. - P-11: amend ADR-046 §3's
SinkHandlertype so the stream item type matches §6. §3 (decisions/046-...md:146) declaresPin<Box<dyn Stream<Item = Value> + Send>>; §6 (:268) declaresPin<Box<dyn Stream<Item = Result<Value, CallError>> + Send>>. The code uses §6's shape uniformly (registration.rs:32-40, aliased asPublishStream). §3 is the one to amend (§6'sResult-carrying shape is correct — the handler must see initiator errors). The ADR's Door-type section explicitly marks the concrete stream item type a two-way-door detail, so this is a text amendment, not a design change. Acceptance gate:cargo doc --no-depsemits 0 warnings;cargo clippy --all-targets -- -D warningsclean;cargo fmt --checkclean;cargo testgreen.
Unit 7 — Small correctness fixes (C-19-misc, C-21, C-23, P-13)
Goal: no panic paths in library code, no misleading public fields,
honest error envelopes, latent bugs in derive_alpn_from_op_name and
the mux pump map fixed.
Files: src/channels/wire.rs, src/channels/mux.rs,
src/channels/operations.rs, src/client/from_call.rs,
src/protocol/dispatch.rs, src/registry/spec.rs.
Scope:
- C-21:
write_header(wire.rs:110-114) slicesout[..CHUNK_HEADER_LEN]and panics on short buffers. The doc comment at:106-107declares the panic. ReturnResult<_, ChunkError::HeaderTooShort>instead (the variant already exists for the parse side,wire.rs:67-68). Updatewrite_chunk(:138-150) and all callers (they pass[0u8; 8]today, so theResultwill always beOk— but the API is&mut [u8]and the contract should be typed, not panic-documented). - C-23:
dispatch_requested's Sink and Stream error arms useString::new()as the request id (dispatch.rs:236,242), producing envelopes with an empty id. Pass the realrequest_id(theOncearm at:298already does this correctly viainvoke). - P-13:
SinkDispatch::chunk_txispub(dispatch.rs:76-79), exposing thefutures::mpsc::Sendertype and the 64-slot buffer size as effective public API. Make the field private (orpub(crate)) before crates.io;handle_streamandpump_sinkare the only readers and they're in the same module. - C-19 (mux pump map leak + duplicate registration):
mux.rs:159inserts each pump'sJoinHandleintoself.pumpsand never removes finished ones — they accumulate for the connection's lifetime. Duplicateregister(channel_id)silently overwrites the old handle while the old pump keeps running (two pumps, one channel id). Fix: detect a finished pump before insert (poll theJoinHandleor useis_finished()), and reject duplicate registration (return an error the caller maps toChannelExists). - C-19 (
derive_alpn_from_op_name):from_call.rs:291-303takes only the first path segment (rest.split('/').next()), sochannels/custom/proto/subderivesalknet/custominstead of the full ALPNcustom/proto. Thesegment.starts_with("alknet/")branch at:297is dead (a single segment can't contain/), andsegment == "alknet"yields the nonsense ALPN"alknet". Fix: strip the known/sub|/pubsuffix fromrestinstead of taking the first segment, so multi-segment ALPNs survive. Thechannels/tty/subcommon case already works; this fixes the latent non-alknet/*multi-segment case (no live callers yet, but theArc<str>refactor in Unit 8 will make this path reachable). - C-19 (
Box::leakper discovery):from_call.rs:314-316leaks aStringto'staticon everyleak_alpncall. Bounded per unique ALPN in theory, but leaks on every rediscovery of every marked op. RefactorChannelOpenSpec::alpnfrom&'static strtoCow<'static, str>(orArc<str>) soleak_alpncan return an owned value. This is a small public-API change toChannelOpenSpec(spec.rs:38-46); no deployments exist, and the ADR-047 §2 shape said&'static strwas "for now" (theBox::leakwas the workaround for that constraint). TheCow/Arc<str>is the deferred refactor. (If this grows beyond a quick edit, split it into its own unit.) - C-19 (REQ-CH-04 error counter): the demux's lenient
unknown-channel drop (
manager.rs:389-394) onlydebug!-logs; there is no counter or stats surface. Add a simpleAtomicU64dropped-chunks counter toChannelManagerwith adropped_unknown_chunks()accessor for observability. (Low priority — file an OQ if the counter surface isn't worth the API surface today.) - C-19 (resources-snapshot unbounded loop):
operations.rs:278-285iterates0..u32::MAXwith a per-iteration mutex lock, breaking whenresources.len() >= manager.open_count(). With sparse ids (monotonic after churn) or a channel closing mid-iteration, this can iterate millions of times. Replace with an iterator over the channel map's keys (add aChannelManager::channel_ids() -> Vec<u32>accessor) so the snapshot is O(open channels), not O(max id). (This is also touched by Unit 9'sresources/subscriberewrite — coordinate or fold the loop fix into Unit 9.) Acceptance gate:cargo testgreen;cargo clippy --all-targets -- -D warningsclean; no panics reachable fromwrite_header; thederive_alpn_from_op_nametest covers a multi-segment non-alknet/*ALPN.
Unit 8 — Pub protocol semantics (P-03, P-04, P-07)
Status: complete (2026-08-13). All three findings fixed; 15 new
tests (7 in dispatch.rs, 4 in discovery.rs, 4 in from_call.rs…
the single-stream P-03/P-04 tests live in channels/client.rs); full
suite 480 passed, clippy/fmt/doc clean. alktype::validation::build_validator
is now used on both dispatch paths (pump_sink and
run_loop_single_stream); jsonschema was added as a direct
dependency (the validator return type is part of alktype's public
API). The SinkDispatch struct gained a publish_validator field
built once at dispatch time; the single-stream in_flight_sinks map
gained an InFlightSink struct carrying the validator alongside
chunk_tx. The P-07 rework replaced tokio::join! with a
tokio::select! loop over handler.fuse() and reader.read_frame()
so an early handler return writes the response immediately without
waiting for the feed.
Goal: the publish path's decided-but-unimplemented behaviors land:
per-chunk schema validation, initiator call.error terminates the
stream, and an early handler return short-circuits the feed.
Files: src/registry/spec.rs, src/registry/discovery.rs,
src/client/from_call.rs, src/protocol/dispatch.rs.
Scope:
- P-03:
OperationSpec::publish_schema(spec.rs:181-186) is declared, has a builder (with_publish_schema,:236-239), defaults toNone, and is never read.pump_sinkforwards chunks as-is (dispatch.rs:475-484). It is also missing fromspec_to_json(discovery.rs:197-217), fromoperation_spec_schema()(discovery.rs:102-161), and fromfrom_call'srebuild_spec_for(from_call.rs:205-282) — so an imported Pub op loses the schema. Implement: (a) addpublish_schematospec_to_json(emit as"publish_schema"whenSome),operation_spec_schema()(add the property to the schema), andrebuild_spec_for(parse it back); (b) inpump_sink, validate eachcall.publishedchunk'sinputagainstpublish_schemabefore yielding it — on validation failure, inject anErr(CallError::invalid_input(...))and terminate the stream (matching theSinkHandlercontract). Usealktype'svalidation::build_validator(the dependency is declared and unused;jsonschemais transitively available). Whenpublish_schemaisNone, chunks are yielded as-is (current behavior). - P-04:
pump_sinkmatches only"call.published","call.completed","call.aborted"and drops everything else into a debug-log ignore branch (dispatch.rs:492-497). Acall.errorframe from the initiator is silently ignored. Per ADR-046 §6, an initiator-sidecall.errorshould inject the initiator'sCallErroras anErritem that terminates the stream. Match"call.error"in thepump_sinkevent match, parse theCallErrorfrom the payload, sendErr(call_error)intochunk_tx, and break the feed. - P-07:
pump_sinkusestokio::join!(feed_fut, handler)(dispatch.rs:510) and only writes the response after both complete. If theSinkHandlerreturns early (e.g. rejects after chunk 1), the feed loop notices only when its nextchunk_tx.sendfails — a paused or long initiator never learns of the early error. Fix: usetokio::select!(or race the handler against the feed) so that when the handler completes, the feed is short-circuited and the response is written immediately. Droppingchunk_txon the handler side signals the feed to stop. Add a test: a sink handler that returns after 1 chunk; an initiator that publishes 3; assert the response arrives without waiting for all 3 chunks. Acceptance gate: a test that registers a Pub op with apublish_schema, publishes a chunk that violates it, and asserts the handler receives anErrand the stream terminates; a test that an initiatorcall.errorterminates the publish stream; a test that an early-returning handler's response is not deferred behind a slow feed.
Unit 9 — Abort-cancels-Pub (P-06)
Goal: an initiator can actually cancel an in-flight Pub; the
handler future is dropped on abort, not just join!-ed to completion.
Files: src/protocol/dispatch.rs, src/protocol/connection.rs,
possibly src/protocol/pending.rs.
Scope: This is the one Unit 6 item with a design decision, so it
gets its own unit. The single-stream call mode from Unit 2 already
routes call.aborted for an in-flight sink's request_id to the
matching chunk_tx (dispatch.rs:684-693), injecting an Err — but
the handler future is spawned separately (dispatch.rs:670-680) and
is not dropped on abort; it runs to completion and its response is
written to the wire after the abort. The stream-per-request
pump_sink path (dispatch.rs:447-517) join!s the handler to
completion regardless of abort. Decisions:
- Single-stream path: wire a cancellation token (or
oneshot) per in-flight sink socall.abortedboth injects theErrand aborts the spawned handler task. The handler'sJoinHandleis already stored implicitly viatokio::spawn— keep it in thein_flight_sinksmap alongsidechunk_txso abort can.abort()it. - Stream-per-request path:
pump_sinkneeds the same: oncall.abortedfor thisrequest_id, dropchunk_tx(feed gets EOF) and drop the handler future (useselect!with an abort signal instead ofjoin!). The cross-streamabort()path (connection.rs:398-415) opens a new stream and only mutatesPendingRequestMap— on the responder, a differenthandle_streamtask receives it and callshandle_abort, which doesn't reach the runningpump_sink. The single-stream mode makes the cross-stream abort path less relevant for channel 0; document whether stream-per-request abort is worth fixing or should be deprecated in favor of single-stream. - Also (from P-06): during a sink pump,
call.abortedframes for other request IDs on the same stream are dropped by the id-mismatch guard before the type match (dispatch.rs:466-473). On the single-stream path this is handled (the outer loop routes byrequest_idfirst); on the stream-per-request path it's a latent bug. Verify the single-stream path is correct and decide whether the stream-per-request path needs the multiplexing fix or just deprecation. Acceptance gate: a test that aborts an in-flight publish and asserts the handler future is cancelled (the handler observes a drop/cancellation, not just anErrinjected into the stream it's no longer reading); a test that the aborting initiator receives confirmation and the responder's resources are released.
Unit 10 — Stubs, substrate modes, and spec-doc renumbering (C-10, C-11, C-14, C-15, C-26)
Goal: the remaining decided-but-deferred behaviors are honestly
stubbed (no fake success) with OQs filed; the spec docs are consistent
with the post-047 model and the alkcall ADR numbering.
Files: src/channels/operations.rs, src/channels/adapter.rs,
src/channels/client.rs, docs/architecture/channel-client.md,
docs/architecture/channels-adapter.md,
docs/architecture/channels-wire.md, docs/architecture/open-questions.md.
Scope: This is the lowest-risk unit and can land last. It is mostly
docs + OQ filing.
- C-10 / C-11:
channel/control(operations.rs:234-262) returns{"ok": true}for a silently-discardedmessage; there is no control-handle concept.channel/resources/subscribe(operations.rs:268-289) isfutures::stream::once(...)(one-shot, not live), emits currently-open channels' ALPNs (not the set of openable ALPNs aggregated from ALPN-crate resource enumerators), and has the unbounded0..u32::MAXloop (see Unit 7). Either implement (control-handle routing; live subscription aggregated from enumerators) or file OQs and make the stubs honest: return an explicitunimplemented-styleCallError(codechannel:control_not_implemented/channel:resources_not_implemented) so callers fail loudly, not with fake success. Recommend: file OQs (the control-handle and resource-enumerator surfaces are real design work, not cleanup) and make the stubs honest. - C-14 / C-15: only the in-line substrate mode is implemented
(
adapter.rs:199-259accepts one bidi stream once). The QUIC-native multi-stream substrate ("accept remaining bidi streams, read headers off each") is not implemented.ChannelClienthasopen_channel(Unit 5) but notsubscribe_resourcesor theChannel { channel_id, source }struct from ADR-043. Either implement or file OQs and remove the misleading doc comments / nonexistent method references. Recommend: file OQs (QUIC-native substrate + the fullChannelClientAPI are features, not fixes) and remove the stale doc claims (client.rsdoc comment referencing a nonexistentmanager.open_channel_stream). - C-26: update
channel-client.md,channels-adapter.md,channels-wire.mdto the post-047 model: removeopen_channel(..., direction),ChannelDirection,ResourceEntry.access, the genericchannel/openhandler,channel:unknown_alpn(replaced by per-ALPN ops in ADR-047 §3). Renumber stale alknet ADR refs (071/075/076/093/094/079/080 and others >047) to their alkcall renumbered equivalents (ADR-001..047), or mark them as "alknet source ADR, not yet ported" where no alkcall equivalent exists. This is a doc-only pass; verify againstdocs/architecture/decisions/(which only goes to 047). Acceptance gate: stubs return explicit errors (not{"ok": true});cargo doc --no-depshas no broken links to nonexistent methods; the channels spec docs no longer describe the pre-047channel/openhandler orChannelDirection; OQs filed for C-10/C-11/C-14/C-15.
Suggested sequencing (post-Units 1-5)
Unit 6 (convention + doc cleanup) → no deps; mechanical; do first
Unit 7 (small correctness fixes) → no deps; independent
Unit 8 (Pub protocol semantics) → no deps beyond Units 1-5; P-03 uses alktype
Unit 9 (abort-cancels-Pub) → depends on Unit 8's pump_sink changes (shared file)
Unit 10 (stubs + substrate + doc renumber) → no deps; can land any time
Units 6, 7, 8, 10 are independent and can proceed in any order (or in
parallel across separate branches). Unit 9 touches the same pump_sink
code as Unit 8, so land 8 before 9 to avoid merge conflicts. None of
Units 6-10 require a spec decision (the ADR-047 §4 and §5 decisions
were made in Units 3 and 5). Units 1-5 are complete (commits
502488c through f25d0a6); the integration is end-to-end functional.
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 buffertry_send), the pump exits onrecv → Nonewithout writing EOF. C-17 above states the corrected mechanism. - Warning count (C-09): the main review said 5; this pass sees 4
cargo docwarnings (2×register_openable, 1×default_policy, 1×envmodule/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.