feat(review 006 Unit 3): teardown-race log, early-arrival bound docs, count accessor (E-03, E-04, N-2)
- E-03: the open wrapper's handler-exit teardown no longer discards UnknownChannel silently — debug log + benign-race pinning comment (ledger take is the atomic gate; no double-decrement) - E-04: consumer-facing doc note on the 64-parked-chunks observable bound (channel-client.md + EARLY_ARRIVAL_CAP const doc) for tunnel-style push-first producers - N-2: ChannelManager::early_arrival_count() accessor (the observability choice over removing the write-only counter), documented monotonic, with a park/adopt-drain monotonicity test - review 006: Unit 3 marked implemented in Status and remediation plan Verification: 617 tests pass; clippy -D warnings clean (host + wasm32 check); fmt clean; doc clean
This commit is contained in:
@@ -108,6 +108,35 @@ and `subscribe_resources` method are deferred (OQ-40, OQ-41). The
|
||||
`ResourceEntry.access` preview was dropped by ADR-047 §6 — it is
|
||||
available via `services/schema` on the op spec.
|
||||
|
||||
## The early-arrival park bound (push-first producers)
|
||||
|
||||
The open-op response carrying `channel_id` races the producer's first
|
||||
data-plane writes: the producer's handler can start pumping before the
|
||||
consumer's `adopt_channel` runs. The demux parks those first chunks
|
||||
per-channel (FIFO) instead of dropping them, and `adopt_channel` drains
|
||||
the parked chunks into the new receiver — a push-first producer (a TTY
|
||||
backend's banner, a sub protocol's greeting) must not lose its first
|
||||
chunks.
|
||||
|
||||
The park is bounded: **up to 64 chunks per channel** are parked
|
||||
(`EARLY_ARRIVAL_CAP`, ADR-040's memory bounds); chunks arriving past
|
||||
the cap are dropped silently — at the consumer this presents as a
|
||||
truncated stream with clean framing everywhere else, not as an error.
|
||||
Consumer-facing consequences (review 006 E-04, noted for tunnel-style
|
||||
consumers):
|
||||
|
||||
- Size any pre-adopt buffering math (e.g. a UDP POC's MTU-vs-buffer
|
||||
sizing) against **64 parked chunks as the observable bound**, and
|
||||
adopt promptly — the open reply resolves *before* the first data
|
||||
arrives by design, so the adopt is the consumer's next step, not a
|
||||
slow path.
|
||||
- The producer side observes both halves of the race via
|
||||
`ChannelManager::early_arrival_count()` (chunks parked) and
|
||||
`ChannelManager::dropped_unknown_chunks()` (chunks lost to the cap) —
|
||||
a non-zero dropped counter under a push-first producer means the
|
||||
adopter was too slow for the producer's burst, and the affected
|
||||
streams were truncated at the park boundary.
|
||||
|
||||
## Transport-agnostic by construction
|
||||
|
||||
`ChannelClient` is the client side of the channels protocol. The channels
|
||||
|
||||
@@ -2,12 +2,12 @@
|
||||
|
||||
## Status
|
||||
|
||||
**Resolved-by-ADR (E-01, N-1) / Verified + planned (E-02, E-03, E-04,
|
||||
N-2).** Findings filed from the alktunnels Phase 0 research pass
|
||||
(2026-09-06). This is a design review, not a code-defect review: the
|
||||
establishment gap (E-01) is real, POC-observable, and load-bearing for
|
||||
the next downstream crate; the remaining findings are smaller
|
||||
mechanism/coverage gaps noticed in the same sweep.
|
||||
**Resolved-by-ADR (E-01, N-1) / Implemented (E-02, E-03, E-04, N-2).**
|
||||
Findings filed from the alktunnels Phase 0
|
||||
research pass (2026-09-06). This is a design review, not a code-defect
|
||||
review: the establishment gap (E-01) is real, POC-observable, and
|
||||
load-bearing for the next downstream crate; the remaining findings are
|
||||
smaller mechanism/coverage gaps noticed in the same sweep.
|
||||
|
||||
**2026-09-06 verification + remediation pass.** All four findings were
|
||||
independently re-verified against source at tree `88e3f5e` (0.4.1 +
|
||||
@@ -33,6 +33,14 @@ timeout). **Unit 2 (E-02) is implemented** in alkcall 0.5.0
|
||||
`rebuild_spec_for` parses it (shared by `from_call` and `op/register`),
|
||||
`services/list` and `services/list-peers` local listings emit it when
|
||||
set; the ADR-047 §6 amendment below records the discovery decision).
|
||||
**Unit 3 (E-03, E-04, N-2) is implemented** in alkcall 0.5.0 (E-03: the
|
||||
wrapper's handler-exit teardown discards `UnknownChannel` through a
|
||||
debug log + a benign-race pinning comment; E-04: the 64-parked-chunks
|
||||
observable bound is documented consumer-side in `channel-client.md` §
|
||||
"The early-arrival park bound (push-first producers)" and on the
|
||||
`EARLY_ARRIVAL_CAP` const; N-2: `early_arrival_count` is exposed via
|
||||
`ChannelManager::early_arrival_count()` — the observability choice,
|
||||
paired with `dropped_unknown_chunks` — with a monotonicity test).
|
||||
The original remediation sketch below is superseded by the
|
||||
"Remediation plan (post-verification)" section; the original sketch
|
||||
is retained for the record.
|
||||
@@ -426,7 +434,13 @@ E-03: debug log (or pinning comment) on the discarded `UnknownChannel`
|
||||
at the wrapper's teardown discard. E-04: doc note on `EARLY_ARRIVAL_CAP`
|
||||
(the 64-parked-chunks observable bound) for tunnel-crate-facing
|
||||
consumers. N-2: expose an accessor for `early_arrival_count` or remove
|
||||
the write-only counter.
|
||||
the write-only counter. **Status: IMPLEMENTED (2026-09-06)** — E-03:
|
||||
debug log + benign-race pinning comment on the wrapper's handler-exit
|
||||
teardown (mirroring the sibling log the Unit 1 establisher-failure
|
||||
teardown already carries); E-04: consumer-side doc note in
|
||||
`channel-client.md` plus the const doc; N-2: accessor
|
||||
(`ChannelManager::early_arrival_count()`, documented as monotonic —
|
||||
adopt-drain does not decrement) + test.
|
||||
|
||||
**Sequencing:** ADR-049 (landed) → alkcall 0.5.0 (Units 1+2+3) →
|
||||
alktty migration (channels-path semantic failures move into an
|
||||
|
||||
@@ -108,6 +108,16 @@ struct Inner {
|
||||
/// the open-op response/first-data race). A channel whose adopter never
|
||||
/// arrives leaks its parked chunks until `clear_all`; the cap bounds
|
||||
/// that leak per channel.
|
||||
///
|
||||
/// Consumer-facing bound (review 006 E-04): for a push-first producer
|
||||
/// (dial-then-pump is the common tunnel shape), up to `EARLY_ARRIVAL_CAP`
|
||||
/// chunks per channel can be parked before the consumer's
|
||||
/// `adopt_channel` runs — and chunks past the cap drop silently at the
|
||||
/// consumer (data loss presents as a truncated stream with clean
|
||||
/// framing elsewhere; `dropped_unknown_chunks` /
|
||||
/// `early_arrival_count` are the producer-side observability). A
|
||||
/// tunnel crate's sizing (e.g. a UDP POC's MTU-vs-buffer math) should
|
||||
/// treat 64 parked chunks as the observable bound and adopt promptly.
|
||||
const EARLY_ARRIVAL_CAP: usize = 64;
|
||||
|
||||
impl ChannelManager {
|
||||
@@ -192,6 +202,19 @@ impl ChannelManager {
|
||||
self.inner.dropped_unknown_chunks.load(Ordering::Relaxed)
|
||||
}
|
||||
|
||||
/// The number of chunks parked in the early-arrival buffer since
|
||||
/// the connection started (monotonic — parked chunks handed to the
|
||||
/// adopter are not subtracted). Together with
|
||||
/// [`Self::dropped_unknown_chunks`] this bounds the two observable
|
||||
/// halves of the open-op-response/first-data race: how many of a
|
||||
/// push-first producer's first chunks were buffered
|
||||
/// (`early_arrival_count`) versus lost to the per-channel cap
|
||||
/// (`dropped_unknown_chunks` — see `EARLY_ARRIVAL_CAP`, review 006
|
||||
/// E-04). Observability only — neither counter drives control flow.
|
||||
pub fn early_arrival_count(&self) -> u64 {
|
||||
self.inner.early_arrival_count.load(Ordering::Relaxed)
|
||||
}
|
||||
|
||||
/// The mux handle — for registering new channels' write halves.
|
||||
pub fn mux(&self) -> &MuxHandle {
|
||||
&self.inner.mux
|
||||
@@ -712,6 +735,42 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// `early_arrival_count` observes parked chunks (review 006 N-2 —
|
||||
/// the counter was write-only before the accessor). Monotonic by
|
||||
/// design: it counts chunks parked since connection start; draining
|
||||
/// on adopt does not subtract (paired with `dropped_unknown_chunks`
|
||||
/// it bounds the open-response/first-data race's two halves).
|
||||
#[tokio::test]
|
||||
async fn early_arrival_count_tracks_parked_chunks_monotonically() {
|
||||
let manager = make_manager_with_runner().await;
|
||||
assert_eq!(manager.early_arrival_count(), 0);
|
||||
for _ in 0..3 {
|
||||
manager
|
||||
.route_payload(7, Bytes::from_static(b"parked"))
|
||||
.await;
|
||||
}
|
||||
assert_eq!(
|
||||
manager.early_arrival_count(),
|
||||
3,
|
||||
"each parked chunk increments the counter"
|
||||
);
|
||||
// Adoption drains the buffer into the receiver but does not
|
||||
// decrement (the accessor is an observability counter, not a
|
||||
// live-depth gauge).
|
||||
let (_send, mut recv) = manager
|
||||
.adopt_channel(7, "alk/tty", None)
|
||||
.await
|
||||
.expect("adopt");
|
||||
assert_eq!(
|
||||
manager.early_arrival_count(),
|
||||
3,
|
||||
"adopt-drain does not decrement the monotonic counter"
|
||||
);
|
||||
use tokio::io::AsyncReadExt;
|
||||
let mut buf = [0u8; 6 * 3];
|
||||
recv.read_exact(&mut buf).await.expect("read parked chunks");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn route_payload_to_known_channel_does_not_increment_dropped_counter() {
|
||||
let manager = make_manager_with_runner().await;
|
||||
|
||||
@@ -750,7 +750,23 @@ async fn run_open_wrapper(
|
||||
let teardown_policy = Arc::clone(policy);
|
||||
let task = tokio::spawn(async move {
|
||||
let _ = raw_task.await;
|
||||
let _ = teardown_manager.teardown_channel(id);
|
||||
// Benign-race pin (review 006 E-03): `channel/close`'s
|
||||
// awaited teardown and this handler-exit teardown both
|
||||
// gate on the atomic `opener_ledger().take(id)`, so the
|
||||
// loser of the race gets `UnknownChannel` here and skips
|
||||
// the ledger/policy half — no double-decrement. A
|
||||
// vanished-after-install channel (transport EOF
|
||||
// `clear_all`) is equally benign: `clear_all` drained
|
||||
// the ledger and decremented the policy itself. The
|
||||
// vanished-between-open-and-install case surfaces as the
|
||||
// `set_handler_task` warn below.
|
||||
if let Err(e) = teardown_manager.teardown_channel(id) {
|
||||
tracing::debug!(
|
||||
channel_id = id,
|
||||
error = %e,
|
||||
"open wrapper: handler-exit teardown found no channel to remove (benign when racing channel/close)"
|
||||
);
|
||||
}
|
||||
if let Some(opener_id) = teardown_manager.opener_ledger().take(id) {
|
||||
let opener = Identity {
|
||||
id: opener_id,
|
||||
|
||||
Reference in New Issue
Block a user