Adversarial review's test-coverage gap finding: no test drives
`loop_driver::drive()`, the central function of the slice. F1, F2,
and F4 would have been caught by one. The obstacle was that `drive()`
called `session.socket.recv_from` on a concrete `UdpSocket`, so
unit tests couldn't feed synthetic STUN/RTP/DTLS bytes without a real
UDP loopback pair + hand-rolled valid packets on the wire.
Retrofit a sans-IO test seam shaped the way str0m itself is built:
- New `crates/rutster-media/src/packet_io.rs` defines `PacketIo`
(`recv_from`, `send_to`) with a blanket impl for `UdpSocket`.
- `RtcSession.socket` goes from `std::net::UdpSocket` to
`Box<dyn PacketIo + Send>`. Dispatch cost: a vtable call (~1 ns)
on a 20 ms tick at single-digit pps; ~10,000× headroom vs the 200 ms
latency bar. If step 4's dedicated timing thread measures it, swap
`Box<dyn>` → a generic `RtcSession<S>` parameterization (same
trait interface).
- `RtcSession::new()` keeps its behavior; new hidden `new_with_socket`
is the test constructor (injects a fake + a synthetic local_addr).
This is the infrastructural refactor only — no behavior change. F4
and F1+F3 land on top of it (separate commits, TDD-style: failing
test first, then fix).
Adversarial review F6 (Low, confirmed): Channel.created_at: Instant was
drafted 'for the idle timeout' (spec §5.1) but the timer keys off
RtcSession.last_rtp_rx (and now last_rtp_rx after F4), never creation
time. The field was never read; the Instant import it pulled is dropped
with it.
Spec §5.1 already reconciled in 8680f2b (struct sketch + comment).
Spec §4.5 already specified 'no RTP' (60 s); the contradiction with
the old 5-min lineage is closed.
Refactor phase — full test suite + clippy green; no behavior change.
Adversarial review F7 (Low, confirmed): the Step-2 Output::Timeout arm
checked `needs_redrain` but the flag could only be set inside Step 3
(which runs after Step 2 exits). The arm was unreachable dead code that
misled readers into thinking Step 2 re-drains inter-iteration.
The flag itself remains load-bearing for the bottom re-drain after
Step 3's Writer::write (str0m's mutation → drain-to-Timeout invariant).
Only the inner-loop check is removed; production behavior unchanged.
Refactor phase — existing tests stay green.
Adversarial review F2 (Med-High, confirmed): accept_offer asserted
audio_mid.is_none() on a path fed by unauthenticated external input.
A second POST /v1/sessions/:id/offer panicked the handler task instead
of returning a clean 4xx.
Replace the assert! with a typed RtcSessionError::AlreadyNegotiated.
routes.rs maps the variant to 409 Conflict, distinct from 400 Bad
Request for malformed SDP. debug_assert! documents the invariant for
tests; the production path returns Err, never panics.
TDD: unit test second_accept_offer_returns_already_negotiated_not_panic
red on the panic message; green after the typed error. Integration test
double_post_offer_returns_409_conflict_not_panic red on 400; green after
routes.rs is updated.
Two Important findings from the final whole-branch review:
T4-M3: RtcSession::channel_id/channel_state/is_closed lacked /// docs
(AGENTS.md mandates /// on every public item; learner-facing code is a
primary product). One-line docs added per existing /// style in rtc_session.rs.
T7-M2: README "Status" section still read "Scoping. No code yet" — stale
post-slice-1. Updated to reflect slice-1 implementation status; minimal
edit, surrounding content preserved.
Four routes on axum 0.7 per spec §4.1: POST /v1/sessions (mint),
POST /v1/sessions/:id/offer (str0m-native SDP accept), DELETE
/v1/sessions/:id (close), GET / (static HTML client). Session store is
a DashMap<ChannelId, Arc<Mutex<RtcSession>>> (spec §4.5); one tokio
task drives all session poll loops — per-session tasks would pre-pave
the wrong pattern for step 4's dedicated thread. Graceful shutdown
drops the DashMap on Ctrl-C / SIGTERM. Integration test exercises the
REST surface; manual browser e2e per README §6.5.
RtcSession owns a str0m::Rtc + Opus decoder/encoder + EchoAudioPipe +
a bound UDP socket (spec §3.1, §4.5). accept_offer calls str0m 0.21's
sdp_api().accept_offer() natively — no hand-rolled SDP munger; str0m
fills DTLS fingerprint + ICE creds + Opus codec. The loop driver
drains poll_output per str0m's single-mutation invariant, routes
inbound MediaData through Opus decode + EchoAudioPipe sink, sends
Transmit packets on the UDP socket, and checks the 60 s idle timeout.
DEV-DEVIATION: loop runs on tokio (spec §3.4); step 4 replaces with
a dedicated timing thread per ARCHITECTURE.md.
str0m 0.21 API adjustments from the brief's sketch (verified against
src/str0m-0.21.0):
- SdpAnswer has no .mid() accessor; use answer.media_lines[0].mid()
via the Deref<Target=Sdp> impl.
- Input::Receive carries the Instant as the first tuple element
(Input::Receive(now, recv)); handle_input takes a single Input arg.
- Receive constructed via Receive::new(proto, src, dst, buf) (the
DatagramRecv field is private).
- UDP socket binds 127.0.0.1:0 (Candidate::host rejects the
unspecified address 0.0.0.0).
- BROWSER_SDP_OFFER fixture restored a=group:BUNDLE 0 (str0m's
Sdp::assert_consistency rejects SDP without a session group).
PcmFrame is the canonical tap format (16-bit mono @ 24 kHz, 480 samples
per 20 ms frame — ARCHITECTURE.md). AudioSource/AudioSink are the seam
step 2 splices the tap client into (spec §3.3); EchoAudioPipe is the
slice-1 wiring of that seam. OpusDecoder/OpusEncoder wrap the opus
crate's libopus FFI with hot-path match-and-continue (no ? on the 20 ms
loop, spec §3.8); decode/encode return Option<PcmFrame>/Option<Vec<u8>>
so a dropped frame is logged + counted, never propagated to crash the
peer.
rutster-call-model is real-but-minimal (spec §5): the unifying leg
object the future API exposes. ChannelId is a Uuid newtype for
type-safety (the slice-1 worked example of the newtype pattern).
Channel is signaling-state only — media lives in rutster-media as a
leaf concern of the Channel, surfaced only when a second consumer needs
to observe it (spec §5.3). ChannelState matches the New→Connecting→
Connected→Closing→Closed flow from §5.4.
Workspace root, pinned toolchain, and the three stub crates whose only
job in slice 1 is to lock the ADR-0002 boundary shape. Each ships a
lib.rs module doc (what it will hold, why deferred, which spearhead step
fills it) and a crate_compiles test. Spec §2.2.
