glm-5.2
f362c9e596
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
1333 lines
71 KiB
Markdown
1333 lines
71 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
|
||
|
||
**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()` 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
|
||
|
||
**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:
|
||
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.
|
||
|
||
## 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_handler` now streams + terminates on `Err`).
|
||
- **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:155` tautology test was removed in Unit 3.
|
||
- **C-19's** `next_id` wrap-to-0, the misleading
|
||
`drop(state.demux_sender.clone())`, and the `manager.rs` mux-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_sink` matches the string literals `"call.published"`,
|
||
`"call.completed"`, `"call.aborted"` (`dispatch.rs:475,485,486`)
|
||
instead of the `EVENT_PUBLISHED`/`EVENT_COMPLETED`/`EVENT_ABORTED`
|
||
constants 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 no `unsafe` block — `derive_alpn_from_op_name`
|
||
returns `Option<String>`, the leak happens in `leak_alpn`). Reword to
|
||
a plain note about the `'static` lifetime 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 doc` warnings (verified 2026-08-13):
|
||
- `unresolved link to default_policy` (`operations.rs:50` — the
|
||
`[`default_policy`]` intra-doc link resolves to `super::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 `PhantomData` test at
|
||
`client.rs:291-294` (`let _ = std::marker::PhantomData::<ChannelClient>;`
|
||
— asserts nothing). The `env.rs:155` tautology is already gone.
|
||
- **C-24:** replace "client→server streaming" with producer/consumer
|
||
phrasing in `call-protocol.md` (the Pub reference-table line) and
|
||
`operation-registry.md` (the ADR-046 reference-table line). No public
|
||
API names offend; this is doc-only.
|
||
- **P-11:** amend ADR-046 §3's `SinkHandler` type so the stream item
|
||
type matches §6. §3 (`decisions/046-...md:146`) declares
|
||
`Pin<Box<dyn Stream<Item = Value> + Send>>`; §6 (`:268`) declares
|
||
`Pin<Box<dyn Stream<Item = Result<Value, CallError>> + Send>>`. The
|
||
code uses §6's shape uniformly (`registration.rs:32-40`, aliased as
|
||
`PublishStream`). §3 is the one to amend (§6's `Result`-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-deps` emits 0 warnings;
|
||
`cargo clippy --all-targets -- -D warnings` clean; `cargo fmt --check`
|
||
clean; `cargo test` green.
|
||
|
||
## 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`) slices `out[..CHUNK_HEADER_LEN]`
|
||
and panics on short buffers. The doc comment at `:106-107` declares
|
||
the panic. Return `Result<_, ChunkError::HeaderTooShort>` instead
|
||
(the variant already exists for the parse side, `wire.rs:67-68`).
|
||
Update `write_chunk` (`:138-150`) and all callers (they pass
|
||
`[0u8; 8]` today, so the `Result` will always be `Ok` — 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 use
|
||
`String::new()` as the request id (`dispatch.rs:236,242`), producing
|
||
envelopes with an empty id. Pass the real `request_id` (the `Once`
|
||
arm at `:298` already does this correctly via `invoke`).
|
||
- **P-13:** `SinkDispatch::chunk_tx` is `pub` (`dispatch.rs:76-79`),
|
||
exposing the `futures::mpsc::Sender` type and the 64-slot buffer size
|
||
as effective public API. Make the field private (or `pub(crate)`)
|
||
before crates.io; `handle_stream` and `pump_sink` are the only
|
||
readers and they're in the same module.
|
||
- **C-19 (mux pump map leak + duplicate registration):** `mux.rs:159`
|
||
inserts each pump's `JoinHandle` into `self.pumps` and never removes
|
||
finished ones — they accumulate for the connection's lifetime.
|
||
Duplicate `register(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 the `JoinHandle` or use
|
||
`is_finished()`), and reject duplicate registration (return an error
|
||
the caller maps to `ChannelExists`).
|
||
- **C-19 (`derive_alpn_from_op_name`):** `from_call.rs:291-303` takes
|
||
only the first path segment (`rest.split('/').next()`), so
|
||
`channels/custom/proto/sub` derives `alknet/custom` instead of the
|
||
full ALPN `custom/proto`. The `segment.starts_with("alknet/")` branch
|
||
at `:297` is dead (a single segment can't contain `/`), and
|
||
`segment == "alknet"` yields the nonsense ALPN `"alknet"`. Fix: strip
|
||
the known `/sub`|`/pub` suffix from `rest` instead of taking the
|
||
first segment, so multi-segment ALPNs survive. The `channels/tty/sub`
|
||
common case already works; this fixes the latent non-`alknet/*`
|
||
multi-segment case (no live callers yet, but the `Arc<str>` refactor
|
||
in Unit 8 will make this path reachable).
|
||
- **C-19 (`Box::leak` per discovery):** `from_call.rs:314-316` leaks
|
||
a `String` to `'static` on every `leak_alpn` call. Bounded per unique
|
||
ALPN in theory, but leaks on every *rediscovery* of every marked op.
|
||
Refactor `ChannelOpenSpec::alpn` from `&'static str` to
|
||
`Cow<'static, str>` (or `Arc<str>`) so `leak_alpn` can return an
|
||
owned value. This is a small public-API change to `ChannelOpenSpec`
|
||
(`spec.rs:38-46`); no deployments exist, and the ADR-047 §2 shape
|
||
said `&'static str` was "for now" (the `Box::leak` was the workaround
|
||
for that constraint). The `Cow`/`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`) only `debug!`-logs; there
|
||
is no counter or stats surface. Add a simple `AtomicU64` dropped-chunks
|
||
counter to `ChannelManager` with a `dropped_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-285`
|
||
iterates `0..u32::MAX` with a per-iteration mutex lock, breaking when
|
||
`resources.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 a `ChannelManager::channel_ids() -> Vec<u32>` accessor) so
|
||
the snapshot is O(open channels), not O(max id). (This is also
|
||
touched by Unit 9's `resources/subscribe` rewrite — coordinate or
|
||
fold the loop fix into Unit 9.)
|
||
**Acceptance gate:** `cargo test` green; `cargo clippy --all-targets
|
||
-- -D warnings` clean; no panics reachable from `write_header`; the
|
||
`derive_alpn_from_op_name` test 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
|
||
to `None`, and is **never read**. `pump_sink` forwards chunks as-is
|
||
(`dispatch.rs:475-484`). It is also missing from `spec_to_json`
|
||
(`discovery.rs:197-217`), from `operation_spec_schema()`
|
||
(`discovery.rs:102-161`), and from `from_call`'s `rebuild_spec_for`
|
||
(`from_call.rs:205-282`) — so an imported Pub op loses the schema.
|
||
Implement: (a) add `publish_schema` to `spec_to_json` (emit as
|
||
`"publish_schema"` when `Some`), `operation_spec_schema()` (add the
|
||
property to the schema), and `rebuild_spec_for` (parse it back); (b)
|
||
in `pump_sink`, validate each `call.published` chunk's `input`
|
||
against `publish_schema` before yielding it — on validation failure,
|
||
inject an `Err(CallError::invalid_input(...))` and terminate the
|
||
stream (matching the `SinkHandler` contract). Use `alktype`'s
|
||
`validation::build_validator` (the dependency is declared and unused;
|
||
`jsonschema` is transitively available). When `publish_schema` is
|
||
`None`, chunks are yielded as-is (current behavior).
|
||
- **P-04:** `pump_sink` matches only `"call.published"`, `"call.completed"`,
|
||
`"call.aborted"` and drops everything else into a debug-log ignore
|
||
branch (`dispatch.rs:492-497`). A `call.error` frame from the
|
||
initiator is silently ignored. Per ADR-046 §6, an initiator-side
|
||
`call.error` should inject the initiator's `CallError` as an `Err`
|
||
item that terminates the stream. Match `"call.error"` in the
|
||
`pump_sink` event match, parse the `CallError` from the payload, send
|
||
`Err(call_error)` into `chunk_tx`, and break the feed.
|
||
- **P-07:** `pump_sink` uses `tokio::join!(feed_fut, handler)`
|
||
(`dispatch.rs:510`) and only writes the response after **both**
|
||
complete. If the `SinkHandler` returns early (e.g. rejects after chunk
|
||
1), the feed loop notices only when its next `chunk_tx.send` fails —
|
||
a paused or long initiator never learns of the early error. Fix:
|
||
use `tokio::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. Dropping `chunk_tx` on 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 a
|
||
`publish_schema`, publishes a chunk that violates it, and asserts the
|
||
handler receives an `Err` and the stream terminates; a test that an
|
||
initiator `call.error` terminates 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 so `call.aborted` both injects the `Err` **and**
|
||
aborts the spawned handler task. The handler's `JoinHandle` is
|
||
already stored implicitly via `tokio::spawn` — keep it in the
|
||
`in_flight_sinks` map alongside `chunk_tx` so abort can `.abort()` it.
|
||
- **Stream-per-request path:** `pump_sink` needs the same: on
|
||
`call.aborted` for this `request_id`, drop `chunk_tx` (feed gets EOF)
|
||
**and** drop the handler future (use `select!` with an abort signal
|
||
instead of `join!`). The cross-stream `abort()` path
|
||
(`connection.rs:398-415`) opens a new stream and only mutates
|
||
`PendingRequestMap` — on the responder, a different `handle_stream`
|
||
task receives it and calls `handle_abort`, which doesn't reach the
|
||
running `pump_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.aborted` frames 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 by
|
||
`request_id` first); 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 an `Err` injected 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-discarded `message`; there is no
|
||
control-handle concept. `channel/resources/subscribe`
|
||
(`operations.rs:268-289`) is `futures::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 unbounded `0..u32::MAX` loop (see Unit 7). Either implement
|
||
(control-handle routing; live subscription aggregated from
|
||
enumerators) or file OQs and make the stubs honest: return an
|
||
explicit `unimplemented`-style `CallError` (code
|
||
`channel: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-259` accepts one bidi stream once). The QUIC-native
|
||
multi-stream substrate ("accept remaining bidi streams, read headers
|
||
off each") is not implemented. `ChannelClient` has `open_channel`
|
||
(Unit 5) but not `subscribe_resources` or the `Channel { 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 full `ChannelClient`
|
||
API are features, not fixes) and remove the stale doc claims
|
||
(`client.rs` doc comment referencing a nonexistent
|
||
`manager.open_channel_stream`).
|
||
- **C-26:** update `channel-client.md`, `channels-adapter.md`,
|
||
`channels-wire.md` to the post-047 model: remove `open_channel(...,
|
||
direction)`, `ChannelDirection`, `ResourceEntry.access`, the generic
|
||
`channel/open` handler, `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 against
|
||
`docs/architecture/decisions/` (which only goes to 047).
|
||
**Acceptance gate:** stubs return explicit errors (not `{"ok": true}`);
|
||
`cargo doc --no-deps` has no broken links to nonexistent methods; the
|
||
channels spec docs no longer describe the pre-047 `channel/open`
|
||
handler or `ChannelDirection`; 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 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. |