alkcall/docs/reviews/013-adopt-window-ordering-race.md

Review 013 — The adoption-window ordering race: a chunk routed in the adopt_channel install→drain window overtakes earlier-parked early arrivals and misorders the channel's byte stream

Status

Resolved — filed 2026-10-21, resolved 2026-10-22 (same session). C-01 was re-verified (code walk plus a latch-based repro: holding the old install→drain window open via a test-only gate makes a receiver read CP1P2 where the wire order was P1, P2, C — the deterministically observed inversion), then closed by making the install and its park drain one atomic section (the re-review's fix direction 1+3 hybrid: route_payload's map-miss-and-park is now also one lock section, so nothing can straddle either edge of the window). The resolution is fully synchronous — no settle flag, no extra task, no pending queue per channel:

• route_payload holds the channels-map lock across the miss → park decision (park_early_arrival runs nested under it) — a park can never straddle the install, closing the orphaned-chunk loss variant the re-review added to the finding (a chunk that missed the map pre-install but parked after the post-install drain would sit parked forever, invisible to dropped_unknown_chunks).
• install_locked (new, shared by open_channel / adopt_channel / install_channel_zero) swaps the parked queue out, inserts the state, and try_sends the parked queue into the fresh, empty receiver before releasing the channels lock — a direct route (map hit) can only begin after that release, so parked chunks always precede it in mpsc send order. No await runs inside the section: the effective park cap is min(EARLY_ARRIVAL_CAP, buffer_cap) so the parked queue always fits the fresh receiver. drain_early_arrivals remains as the defensive full-buffer fallback (previously-unreachable-by-invariant) and is now called by all three install paths.
• Semver: patch release of the 0.10 line (0.10.1) — ChannelState and the fix surface are private; the API surface, the wire formats, and the documented cap behavior (review 006 E-04) are unchanged; behavior only strengthens (in-window drops and the orphan loss become unreachable).

Regression tests (src/channels/manager.rs): the chunk-granularity ordering test the fix sketch called for (adopt_window_does_not_reorder_chunks — 200 iterations on a real 2-worker runtime, a racer routing C the moment the install becomes visible, receiver byte stream asserted byte-identical to the producer's — catches the old window probabilistically under genuine preemption, exactly the alkgit repro shape), the pre-install park FIFO shape (adopt_drains_parked_and_raced_chunks_in_fifo), and the park-cap normalization (park_cap_bounded_by_receiver_buffer). Downstream re-verification gate still applies: alkgit's flaking row (tests/native_transport.rs … under multi-process load, finding F-2) must pass against the fixed crate before publishing 0.10.1.

Triggered by a downstream finding: alkgit's wave-2 review gate (/workspace/@alkdev/alkgit/ — core/review-wave-2) caught its channels open-op integration test flaking under multi-process load, instrumented the failing byte stream, and traced every failure to the window analyzed below. The downstream diagnostic data (two distinct desync shapes at two distinct offsets) is reproduced here in full; alkgit carries the panic improvement (alkgit b6db513) that makes the shape observable.

As filed — before the resolution — this review contained no alkcall code change: it was the finding record, the repro evidence, and the fix-shape sketch the implementing patch builds from. Recommended urgency: patch release of the published 0.10 line (the ruling the Status section implements), because the failure mode is silent byte-stream corruption on a freshly adopted channel and the documented downstream consumers (the git producer half in alkgit; any dial-then-pump tunnel shape) sit exactly on the affected window.

Scope

src/channels/manager.rs — the adopt/adopt-equivalent paths and the demux receive side they race:

• ChannelManager::adopt_channel (manager.rs:338-402): the install (channels.insert, manager.rs:389-394) and the post-install drain (drain_early_arrivals, manager.rs:402).
• ChannelManager::route_payload (manager.rs:515-533): the channels-map lookup → direct send path, and the not-yet-installed → park_early_arrival fallback.
• ChannelManager::park_early_arrival (manager.rs:539-561).
• ChannelManager::drain_early_arrivals (manager.rs:567+).

The open-reply adoption path (ChannelClient/open-op consumers calling adopt_channel after the open response) and the open_channel path share the same shape; the observed failures are on the adopt_channel path (the consumer side of the open op).

This is the ordering half of the adopt-race family review 006 E-04 already bounded ("early-arrival park cap (64) interacts with push-first producers under slow adopters"): E-04 covered the cap drop of chunks arriving before adopt and classed it not-a-correctness-bug because the cap is documented behavior. This review covers the ordering of chunks around the moment routing state becomes visible — nothing documented bounds it, and nothing in the implementation preserves it.

Severity legend note

Same scale as reviews 004/005/006. Tagged [critical] by E-01's second clause ("corrupts data"): the channel's reassembled byte stream is delivered with misordered bytes — which, at the layer every real consumer sits on (a framing codec), is corrupted data. The mitigating fact (the window is narrow and adoption-time-only) bounds frequency, not impact; a consumer cannot detect or recover from a misordered chunk, and the REQ-CH series pins delivery as a byte stream on a "single ordered, reliable" substrate (docs/architecture/channels-wire.md:9) — reassembly delivering out-of-order violates the one property the demux exists to preserve.

The finding

C-01 [critical] — a chunk arriving between install and drain is delivered ahead of the chunks parked before install

Sequence (single channel id, one demux):

1. The demux receives chunk P1 for channel_id; route_payload's map lookup misses (the adopting side has not run adopt_channel yet) → P1 is parked in early_arrivals (FIFO). Possibly P2…
2. The adopting side's adopt_channel runs: channels.insert stores the state carrying the new demux_sender. The map lock is released here.
3. The demux receives chunk C for the same channel_id now, inside the window between the insert (step 2) and the drain (drain_early_arrivals, step 4): route_payload's lookup hits → C is sent directly into the mpsc.
4. drain_early_arrivals then pushes P1 (and any other parked chunks) into the same mpsc.

Delivery order on the byte stream: C, then P1 — with P1 having arrived on the wire earlier. The mpsc preserves send order, and the send order is inverted relative to wire-arrival order. The receiver (MpscRecvStream) has no way to know; it reassembles faithfully in send order = wrong order.

The window is the wall-clock span between manager.rs:394 (insert completes) and the drain's first take — normally nanoseconds apart, which is why isolated runs never observe it; under scheduling pressure (spawned adoption task descheduled immediately after the insert, or the pending demux send completing faster than the adoption task's next lock acquisition) the window is real and reproducible.

Observed evidence (alkgit tests/native_transport.rs the_channels_open_op_serves_a_fetch_session_on_the_opened_channel; 11 tests, both processes multi-threaded, ~2 failures in ~10 suite-adjacent runs, 0 in ~15 isolated single-test runs). The test reads the server's V2 capability advertisement back from the channel as a raw pkt-line stream (read_exact 4-byte hex headers). The advertisement is a fixed byte sequence (4+10 bytes version 2, hex headers + ls-refs=unborn / fetch=wait-for-done / object-format=sha1 lines), so any desync is attributable to delivery, not content:

• Failure shape 1 — after 0 lines, the first 4-byte header read came out fetc = [0x66,0x65,0x74,0x63], followed by h=wait-for-done. That is offset ~16 bytes into the advertisement (inside the fetch=wait-for-done line, four bytes past its header): the leading chunks were delivered late; the reader started mid-stream.
• Failure shape 2 — after 1 line (version 2 parsed correctly), the next header read came out 00ev = [0x30,0x30,0x65,0x76] (the 00 prefix of a hex length straddled with text): a chunk boundary two bytes into the second line's header — the second chunk was delivered ahead of (or its tail interleaved against) the chunk carrying the line it belongs to.

Both shapes are exactly the C-befor-P1 inversion at different chunk granularities (chunk sizes vary with scheduling).

A further aggravation this review flags on the same paths: the window also interacts with park_early_arrival's EARLY_ARRIVAL_CAP overflow branch (manager.rs:542-551) — a burst that parks while the window delays the drain can additionally drop chunks. That combination is loss, not just misorder; any fix must be ordering-exact and keep the drop path unreachable while the channel is installed-but-undrained.

Why the consumer cannot fix it (and why no downstream workaround exists or should): the reassembled BiStream is the promised byte stream; a framing codec (alkgit's pkt-line reader, a tunnel's inner protocol) consumes bytes with no awareness that chunk reassembly happened beneath it. Alkgit carries no compensating logic and must not — the fix belongs here, in the one place that owns delivery order.

Fix-shape sketch (for the implementing patch)

The window closes when all chunks of a channel are serialized through one ordering decision. Candidate directions, in increasing invasiveness:

1. Drain under the channels-map lock. Hold the map lock across the insert and the parked-queue swap-out, then push the swapped queue into the mpsc (the direct route_payload paths arriving behind the lock block until the drain's queue is out; they then send directly — after the parked bytes). The push of the swapped queue must not await while the lock is held: try_send first, draining backpressure by holding the queue, not the lock, gets complicated — see direction 3 for a clean cut.
2. Re-check in route_payload's park branch. After parking, re-check the map; if the channel now exists, take the queue back out (FIFO) and push it through the sender, then the just-parked chunk. This closes the "parked-after-install" half but leaves the window's other half (a direct send overtaking parked chunks whose drain hasn't run yet) — a direct route still sends C before the adopter pushes P1. Not sufficient alone.
3. Per-channel ordered mailbox. Make every chunk for an installed channel take the channel's own queue-lock, append, and hand off to a single per-channel drainer (or drain under the queue lock with try_send + a parked-overflow flag the drainer resumes). This serializes park, install-drain, and direct route on one lock per channel and closes every interleave, including the push-first producer shapes of 006 E-04.

Whichever shape lands must add a chunk-granularity ordering regression test: a producer that emits many small chunks with an open-reply-then-immediate-pump shape, the adopter parked mid-window under injected scheduling (e.g. a park latch between insert and drain — a test-only seam), asserting the receiver's byte stream is byte-identical to the producer's. The two observed desync shapes above are the acceptance shapes. The route_payload_to_open_channel_succeeds single-chunk test does not exercise the ordering and must not be counted as covering this.

Downstream re-verification gate: alkcall's fix must be re-verified against alkgit's flaking row under multi-process load (the condition that reproduced it) — the alkgit review task (core/review-wave-2, finding F-2) records this; wave 8's end-to-end git push/git clone rows ride channel adoption with push-first producers and would exercise the window hard without the fix.

Non-findings (verified, recorded to bound the re-review)

• The demux/mux wire format is not implicated: the 8-byte chunk header is per-chunk framing only, and the misorder below it is a manager scheduling property (ADR-034's wire surface untouched; no ADR-level wire change is needed for the fix).
• MpscRecvStream's pending-chunk continuation is correct — the observed corruptions are all order corruptions, never content corruption; the reassembly layer faithfully delivered what it was handed.
• The install-side duplicate/collision checks in adopt_channel (ChannelExists + the 0.8.1 regression pin) are unaffected by this finding; the fix must preserve them and the adopt-parity rules.

References

• alkgit wave-2 gate review: /workspace/@alkdev/alkgit/tasks/implementation/core/review-wave-2.md (finding F-2, with the repro data and the fix-urgency routing) and the diagnostic commit alkgit b6db513 (tests/native_transport.rs → read_channel_pkts's desync panic).
• Review 006 E-04 (the early-arrival cap half of this family; docs/reviews/006-channel-open-establishment-gap-review.md), review 007 (the establishment follow-ups).
• docs/architecture/channels-adapter.md (the demux invariants REQ-CH-02/04 this finding sits between), docs/architecture/channels-wire.md (the ordered-substrate premise, line 9; REQ-CH-01..05 wire invariants), docs/architecture/decisions/039-channelsadapter-and-channelmanager.md (the substrate-agnostic demux loop contract).
• alkcall a22b2b8 — the original park-early-arrival fix (the open/first-data race this review's window is the residue of).