fix: replace fast_channel with tokio::sync::mpsc to eliminate spin-deadlock

[sanity]

Problem

A live freenet client peer (v0.2.49) on Linux went unresponsive after running for several hours: the HTTP API on 127.0.0.1:7509 stopped accepting requests, no log output for 4+ hours, but the process kept running with two tokio worker threads pegged near 100% CPU. strace -p <tid> -c over 5 s showed 100% sched_yield, ~30 kHz, on each thread. gdb caught both threads inside crossbeam-channel:

• TX: UdpPacketsListener::listen → ConnectionStateManager::try_send_established → crossbeam_channel::Sender::try_send → sched_yield
• RX: PeerConnection::recv → fast_channel::FastReceiver::recv_async → crossbeam_channel::Receiver::try_recv → sched_yield

fast_channel.rs wraps crossbeam's bounded array channel. That channel uses a slot-reservation protocol — a sender CAS-bumps tail, then writes the slot's value, then updates the slot's stamp. Receivers do the symmetric dance on head. When either side encounters a slot whose stamp hasn't been finalized yet, they call Backoff::snooze() (→ sched_yield) — and Backoff::snooze has no upper bound. Once a slot is left in a permanent in-flight state, every subsequent try_send/try_recv that lands on it spins forever.

The listener calls try_send_established synchronously from its event loop. A single wedged per-peer channel therefore freezes the listener for every peer plus the entire runtime, including the Axum HTTP server task.

fast_channel (introduced in #2263) has been patched four times for related correctness/perf issues: #2336, #3081, #3208, #3215. This is the fifth instance of the same family.

Approach

Delete fast_channel and migrate every call site to tokio::sync::mpsc directly:

• tokio's mpsc has no slot-reservation/commit split, so it cannot wedge in the way crossbeam's array channel can. try_send returns Full/Closed immediately under all conditions.
• recv().await integrates with the runtime via parker/notify primitives owned by the channel, so the Notify wrapper in fast_channel is unnecessary.
• The "2 × microbenchmark advantage" of crossbeam over tokio mpsc cited in the original fast_channel doc never translated to production throughput — per-channel throughput is bounded by network speed and by per-packet processing in PeerConnection::recv, not by channel ops/sec. Trading the microbench delta for elimination of node-wide-hang tail risk is the right call.
• The crossbeam workspace dependency is removed (no remaining users).

API shifts:

• FastSender<T> → mpsc::Sender<T>
• FastReceiver<T> → mpsc::Receiver<T> (now needs &mut self for recv()/try_recv())
• fast_channel::bounded(n) → mpsc::channel(n)
• recv_async() → recv().await (returns Option<T>, not Result<T, RecvError>; matches updated accordingly)
• send_async(x).await → send(x).await
• Sync .send(x) for blocking test sockets → .blocking_send(x)

Testing

• New regression test test_try_send_established_never_blocks_when_full exercises the listener forwarding path: fills a per-peer channel with no consumer, calls try_send_established 1000 times, asserts the whole loop completes within 1 s. Under crossbeam-backed fast_channel, the wedge required unlucky thread interleaving inside crossbeam internals and was not triggerable in a unit test, but the test pins the invariant the fix establishes — try_send from the listener path must never block, regardless of receiver state.
• Existing 530 transport unit tests: all pass.
• Existing 2453 lib tests: all pass.
• cargo clippy --all-targets -- -D warnings: clean.
• Drive-by: added a missing // SAFETY: comment to a pre-existing unsafe block in service.rs that was tripping clippy::undocumented_unsafe_blocks even on main.

Why didn't CI catch this?

The wedge requires a specific cross-thread interleaving inside crossbeam-channel-array — a slot reservation that completes its CAS but doesn't update the stamp before the next sender or receiver lands on it. Reproducing this synthetically would require either a custom-built test that hooks into crossbeam internals or a fault-injection layer above it. Neither was practical to add for one library's internal protocol.

The right CI gap to close is the architectural one: the listener's forwarding path must be guaranteed non-blocking. The new test pins exactly that invariant — and tokio mpsc's API guarantees try_send returns immediately by construction, so it cannot regress without changing channels again.

Closes #3959

[AI-assisted - Claude]

[github-actions]

Now I have everything needed for a complete review. Let me compile my findings.

---

Rule Review: send_nat_traversal uses .send().await in listener loop after migration

Rules checked: channel-safety.md, code-style.md, testing.md, transport.md, git-workflow.md
Files reviewed: 9 (Cargo.lock, Cargo.toml, crates/core/Cargo.toml, benches/transport/blackbox.rs, bin/commands/service.rs, transport.rs, transport/connection_handler.rs, transport/fast_channel.rs [deleted])

---

Warnings

• crates/core/src/transport/connection_handler.rs:790 — send_nat_traversal now uses packet_sender.send(packet).await (replacing the equivalent fast_channel::send_async().await). The call site at line 1371 is inside the UDP listener loop (ConnectionState::NatTraversal match arm). Per channel-safety.md, .send().await in event loops is explicitly banned — it has caused 4+ production deadlocks — and the channel-safety audit grep now directly catches this call. If the NAT traversal task stalls or the channel fills (capacity 100), the entire listener loop blocks, halting packet delivery for all peers. The old fast_channel::send_async() had identical semantics so this is not a regression, but the PR migrates to the standard primitive without adding a timeout, and the channel-safety audit will flag it going forward. Fix: replace with try_send() + drop-and-log-on-Full, or wrap with tokio::time::timeout. (rule: channel-safety.md)

---

Info

None.

---

Summary: The core fix (removing the crossbeam spin-deadlock via fast_channel deletion and migrating to tokio::sync::mpsc) is correct and well-tested. The regression test suite for try_send_established and try_send_gateway_handshake is thorough. The one WARNING is that the async send_nat_traversal path carries the same indefinite-block behavior into a standard mpsc::send().await that is now directly catchable by the project's own channel-safety audit grep, and should be fixed before merge.

---

Rule review against .claude/rules/. WARNING findings block merge. ⚠️ 1 warning(s) — fix or add review-override label

[sanity]

Review feedback summary

Ran multi-model review (4 internal Claude agents — code-first, testing, skeptical, big-picture — plus Codex). Codex found no regressions. The other four converged on three actionable categories:

Addressed in this PR (commits 9ed6319, c154da2)

• Stale fast_channel references in docs/comments — code-simplifier swept 7 sites in benches/transport/blackbox.rs, tests/simulation_integration.rs, docs/architecture/transport/DOCUMENTATION_GAPS.md, and docs/architecture/transport/benchmarking/methodology.md. The historical perf analysis doc (2025-12-performance-analysis-01.md) got a "Superseded by fix: replace fast_channel with tokio::sync::mpsc to eliminate spin-deadlock #3960" header rather than a rewrite, since it captures decisions made at that time.
• Add a parallel regression test for try_send_gateway_handshake — same forwarding path from the listener loop, equally vulnerable to the original wedge. Added test_try_send_gateway_handshake_never_blocks_when_full alongside the existing test.
• Convert regression tests to #[tokio::test] — keeps them forward-compatible if ConnectionStateManager later uses async primitives.
• Soften the regression-test docstring claim — the tests do not reproduce the original wedge (which required cross-thread interleaving inside crossbeam internals); they pin the structural invariant the fix establishes. That distinction is now explicit in the docstring.

Tracked as follow-up (#3961), not addressed in this PR

Reviewers flagged two pre-existing .send().await call sites in transport hot paths that violate .claude/rules/channel-safety.md:

• send_nat_traversal at connection_handler.rs:790 (called synchronously from the listener event loop)
• inbound_streams fragment forwarding at peer_connection.rs:1483 (legacy stream path inside PeerConnection::recv)

Both predate this PR (they were .send_async().await under crossbeam — same blocking semantics, just renamed by the migration). Converting them to try_send requires real design decisions about backpressure semantics for two distinct subsystems (drop the packet vs tear down the handshake; drop the fragment vs close the stream), not a mechanical rename. Tracked in #3961 with the suggested approach.

Acknowledged but not changed

• Service.rs SAFETY-comment drive-by is a one-line clippy fix on a pre-existing unsafe block that was failing cargo clippy --all-targets -- -D warnings even on main. Disclosed in the original PR description.
• No bench numbers backing the "2x microbench delta is irrelevant" claim — softened the language; the load-bearing argument is "per-channel throughput is bounded by network speed and per-packet processing in PeerConnection::recv," not the microbench delta itself.
• Pre-existing race between gateway-handshake completion and packet drain (skeptical reviewer) — the same race exists with crossbeam, unchanged by this PR.

Test status

• 2455 lib tests pass (was 2453; +2 for the new regression tests)
• cargo fmt --check clean
• cargo clippy --all-targets -- -D warnings clean

[AI-assisted - Claude]