rutster/docs/reviews/2026-06-28-slice-1-claude-adversarial-assessment.md

Claude Adversarial Assessment — Slice 1 (WebRTC media loopback)

• Reviewer: Claude (Opus 4.8), static read of the tree. No code executed; no browser e2e run.
• Target: crates/rutster* as of main@22d3f03 (slice-1 implemented; slice-2 is spec/plan only and out of scope for this code review).
• Spec under test: docs/superpowers/specs/2026-06-28-slice-1-webrtc-loopback-design.md.
• Scope: ~1,550 LoC of Rust across the workspace. Media core (rutster-media), call model (rutster-call-model), signaling binary (rutster). Stub crates (-tap, -spend, -signaling-sip) are empty and not assessed.

How to use this document (for the GLM-5.2 reviewers)

This is adversarial, not authoritative. Each finding is a claim Claude is asking you to refute. For every one:

1. Read the cited file:line yourself — do not trust the quote.
2. Run the Falsification test. If it passes (i.e. the bug does not reproduce), the finding is rejected — say so and show why.
3. Where Claude marks Confidence: verify, that means Claude could not confirm it by static reading alone (usually a str0m 0.21 API semantic). Treat those as open questions, not bugs, until you check the dependency source.

Claude has already cleared one suspected issue (the browser client's ICE-gathering wait — see "Cleared" at the bottom). Hold this review to the same standard: try to clear findings, don't just nod along.

---

TL;DR — findings

#	Severity	Confidence	One-line	Location
F1	High	High (contract) / Med (user-visible)	str0m is never fed Input::Timeout; its clock only advances on inbound packets, so all timer-driven behavior (DTLS retransmit, ICE consent, RTCP) stalls during inbound silence	loop_driver.rs:56
F2	Medium-High	High	Client-triggerable panic: a second offer to the same session hits assert!	rtc_session.rs:183, routes.rs:60
F3	Medium	High	next_timeout is computed with great care, then discarded; session.next_timeout is written but never read — dead plumbing that hides F1	loop_driver.rs:227, session_map.rs:114
F4	Medium	High	Idle timeout is spec'd as "no RTP" but resets on any datagram (STUN/DTLS keepalives included), so a silent-but-connected peer is never reaped within 60 s	loop_driver.rs:87
F5	Low-Med	High	No cap / rate-limit on POST /v1/sessions; each session binds a UDP socket — unbounded FD/port growth (reaped only after 60 s idle)	routes.rs:28, session_map.rs:53
F6	Low	High	Channel.created_at is dead and mis-documented ("for the idle timeout"); spec also says "5-min" in one place, "60 s" in another	rutster-call-model/src/lib.rs:140, spec §4.5 vs §5.6
F7	Low	High	Unreachable needs_redrain branch in the Step-2 drain loop (always false there)	loop_driver.rs:113
F8	Low	verify	Outbound RTP timestamp via MediaTime::from(Duration) — correctness depends on str0m rebasing to the 48 kHz payload clock in Writer::write	loop_driver.rs:187
F9	Low	verify	Codec is 24 kHz mono while SDP advertises opus/48000/2; decode buffer is exactly 480 i16, so any inbound frame > 20 ms overflows → dropped	opus_codec.rs:23, :45

Plus: a material test-coverage gap — there is no automated test that drives loop_driver::drive(), the heart of the slice. F1, F2, and F4 would all have been caught by one.

---

F1 — str0m is never given Input::Timeout; its clock is frozen during inbound silence

Severity: High. Confidence: High that the contract is violated; Medium on how visible it is in the happy-path demo.

Evidence. grep -rn 'Input::Timeout' crates/ returns only comments — no call site. The sole handle_input call is Input::Receive:

// loop_driver.rs:83
if session.rtc.handle_input(Input::Receive(now, recv)).is_err() { … }

str0m::Rtc::poll_output() takes no now. The only way str0m's internal clock advances is a handle_input(...) call carrying an Instant. Since the loop only calls handle_input when a datagram is actually waiting (recv_from returns Ok), during any stretch with no inbound packets str0m's notion of "now" is stuck at the last received packet's timestamp.

The spec (§3.4, lines 197-199) describes the intended drive loop as: "poll_output() -> Output … where Output::Timeout(Instant) gives the next deadline we sleep tokio until." The implied second half — sleep, then feed Input::Timeout(now) back — is missing.

Why it matters. str0m schedules on timers: DTLS handshake retransmission (on loss), ICE connectivity checks + consent freshness (RFC 7675, ~every few seconds), RTCP SR/RR, pacing/BWE. All of these fire from poll_output only after the clock has advanced past their deadline. With the clock frozen during inbound silence, none of them fire.

The honest nuance (test this). During an active two-way echo, the browser sends RTP every 20 ms, so Input::Receive advances str0m's clock ~every 20 ms and the late-but-present clock lets RTCP/consent fire (a little behind). That is why the loopback demo can appear to work. The failure modes are specifically:

• One-way / sustained silence (mic muted, hold): no inbound RTP → clock frozen → str0m stops emitting consent checks → the browser may tear the call down on consent failure (~15-30 s) before the 60 s idle timeout ever triggers.
• Handshake packet loss with no other inbound traffic to advance the clock → server-side DTLS/ICE retransmit never scheduled → potential stall.

Adversarial challenge. Refute by showing either (a) str0m 0.21 advances internal time without Input::Timeout (check Rtc::poll_output / Rtc::handle_input in the str0m 0.21 source — Claude believes it does not), or (b) that the browser's continuous traffic provably masks every timer path that matters for the slice-1 acceptance ("hear yourself echo"), in which case downgrade severity but keep it as a latent bug for slices 2-4 (barge-in especially needs reliable timing).

Suggested fix. Feed the deadline back. Minimal shape:

// near the top of drive(), after Step 1, before the drain:
if session.next_timeout.map_or(true, |t| now >= t) {
    let _ = session.rtc.handle_input(Input::Timeout(now));
}

…and/or have the driver actually honor the returned Duration (sleep until min(str0m_deadline, outbound_tick)), which makes the existing next_timeout computation load-bearing instead of dead (see F3).

Falsification test. Add a headless test that: constructs a session, feeds a synthetic Input::Receive, drains, then advances wall-clock 6 s with no further packets and asserts that drive() causes a handle_input(Input::Timeout(_)) (spy via a wrapper) — or, end-to-end, that str0m emits an RTCP/consent Transmit on schedule during inbound silence. Today it will not.

---

F2 — A second offer to a session panics (assert! on an attacker-reachable path)

Severity: Medium-High. Confidence: High.

// rtc_session.rs:183
pub fn accept_offer(&mut self, offer_sdp: &str) -> Result<String, RtcSessionError> {
    assert!(self.audio_mid.is_none(), "accept_offer called twice");

post_offer calls this under the session lock with no guard against repeats (routes.rs:60). The endpoint is unauthenticated (slice-1 by design). A client that POSTs /v1/sessions/:id/offer twice for the same id panics the handler task on the second call.

tokio::sync::Mutex does not poison on panic, so the process survives and other sessions are unaffected — but it is still a remotely-triggerable panic returning a broken/aborted response instead of a clean 409. assert! is the wrong tool on a request path fed by external input.

Suggested fix. Return a typed error (RtcSessionError::AlreadyNegotiated or similar) and map it to 409 Conflict in post_offer. Keep debug_assert! if you want the invariant documented for tests.

Falsification test. POST the bundled BROWSER_SDP_OFFER twice to one session id; assert the second returns a 4xx, not a panic. Today it panics.

---

F3 — Dead deadline plumbing (couples to F1)

Severity: Medium. Confidence: High.

drive() computes next_timeout through three arms (incl. borrow-checker-driven comments), stores it (session.next_timeout = next_timeout, loop_driver.rs:227) and returns next_timeout.map(|t| t.saturating_duration_since(now)). But:

• grep next_timeout shows session.next_timeout is written, never read.
• The caller discards the return: let _ = s.run_poll_once(now); (session_map.rs:114), driving instead on a fixed tokio::time::interval(10ms) (session_map.rs:86).

So the loop carefully derives str0m's next deadline and routes it to /dev/null, while the thing str0m actually needs (the Input::Timeout feed, F1) is absent. This is the smell that most corroborates F1: the loop looks like a deadline-driven sans-IO loop that lost its final wiring step.

Decide one way: either (a) the fixed 10 ms tick is the intended slice-1 design — then delete next_timeout/session.next_timeout/the return value and update the spec §3.4 "sleep until the deadline" language, and still fix F1; or (b) honor the deadline — wire the return into the sleep and feed Input::Timeout, which fixes F1 and F3 together. Right now it is neither, which is how F1 hid.

---

F4 — Idle timeout keys on "any datagram," not "RTP" (spec deviation)

Severity: Medium. Confidence: High.

Spec §4.5: "Idle timeout: 60 s of no RTP packets received from the peer → close." Implementation:

// loop_driver.rs:83-87 — runs for EVERY datagram str0m demuxes (STUN/DTLS/RTP)
if session.rtc.handle_input(Input::Receive(now, recv)).is_err() { … }
session.last_rx = now;

last_rx is bumped on every inbound datagram, including the browser's periodic STUN consent checks. A peer that stops sending audio but keeps ICE alive resets the timer forever and is never reaped within 60 s. The code comment ("60 s no RX") quietly redefines the spec's guarantee.

Fix. Bump an RTP-specific timestamp only on Event::MediaData in handle_event (that is "RTP received, depacketized"), and key the idle check off that. Or amend the spec to say "no datagrams" and accept the weaker guarantee.

Falsification test. Drive a session past DTLS, then feed only STUN-shaped keepalives (no MediaData) for 61 s; assert it closes. Today it stays open.

---

F5 — Unbounded session creation (resource exhaustion)

Severity: Low-Medium. Confidence: High.

POST /v1/sessions → create_session → RtcSession::new() binds a UDP socket (rtc_session.rs:132) with no cap, no rate limit, no per-client accounting. Orphan sessions are reclaimed only after the 60 s idle timeout, so a tight create loop can hold up to (rate × 60 s) live sockets/FDs/ephemeral ports. "No authn" is documented as deferred (§1.2), but a resource ceiling is a separate concern from auth. Acceptable for the loopback dev build; flag explicitly for the step-5 deployment posture so it isn't forgotten.

Note: axum's default body limit (~2 MB) does cap the offer body, so the SDP path itself isn't an unbounded-memory vector — only the session/FD count is.

---

F6 — created_at is dead and mis-documented; spec self-contradicts (5-min vs 60 s)

Severity: Low. Confidence: High.

Channel.created_at (rutster-call-model/src/lib.rs:140) is documented "For the 60 s idle timeout," but the idle timeout uses RtcSession.last_rx, not created_at; the field is never read. The spec adds confusion: §4.5 says "60 s," but the struct sketch (spec line 371) and created_at's lineage say "5-min idle timeout." Pick one number, and either wire created_at or drop it.

---

F7 — Unreachable needs_redrain branch in Step 2

Severity: Low (clarity). Confidence: High.

// loop_driver.rs:108-119
let mut needs_redrain = false;     // set true ONLY in Step 3, below
loop {
    match session.rtc.poll_output() {
        Ok(Output::Timeout(t)) => {
            next_timeout = Some(t);
            if needs_redrain { needs_redrain = false; continue; }  // always false here → dead
            break;
        }
        …

needs_redrain cannot be true during the Step-2 loop (Step 3 sets it afterward; the real re-drain is the separate loop at :201). The branch is dead and misleads the reader into thinking Step 2 re-drains. Remove it.

---

F8 — Outbound RTP timestamp clock — verify against str0m 0.21

Severity: Low. Confidence: verify.

// loop_driver.rs:187
session.next_media_time += str0m::media::MediaTime::from(Duration::from_millis(20));

The comment reasons "960 ticks at 48 kHz," but the code adds a MediaTime built from a Duration. Whether the wire RTP timestamp advances by the correct 960/frame depends entirely on str0m::media::Writer::write rebasing the supplied MediaTime to the negotiated Opus payload clock (48 kHz). Claude believes str0m rebases (so this is probably fine), but could not confirm statically.

Verify: read Writer::write + MediaTime::from<Duration> in str0m 0.21. If write uses the numerator directly without rebasing, the timestamps are microsecond-scaled and playout timing is wrong (audible as garbled/late audio). Confirm with a packet capture of the answer stream if in doubt.

---

F9 — 24 kHz mono codec vs opus/48000/2 SDP, and a tight decode buffer — verify

Severity: Low. Confidence: verify.

SAMPLE_RATE = 24_000, Channels::Mono (opus_codec.rs:23-27) while the negotiated rtpmap is opus/48000/2. Opus is self-describing (RFC 7587 fixes the SDP clock at 48000/2 regardless of internal rate), so this is likely legal and Chrome should decode it — verify the browser actually renders it cleanly.

Separately, the decode buffer is exactly [i16; 480] (opus_codec.rs:45). A 20 ms frame at 24 kHz mono is exactly 480 samples, so anything larger — a 40/60 ms frame, or stereo — returns OPUS_BUFFER_TOO_SMALL → decode() yields None → frame silently dropped. Chrome defaults to 20 ms mono-ish, so the echo works, but it's a latent assumption worth a one-line comment or a guard.

---

Test-coverage gap (not a bug, but the reason F1/F2/F4 shipped)

There is no automated test that calls loop_driver::drive() — the central function of the entire slice. Current coverage:

• rtc_session tests: accept_offer returns an answer of the right shape; state goes New → Connecting.
• opus_codec tests: encode→decode RMS; garbage → None.
• pcm tests: frame size; echo round-trip; bounded sink.
• api_integration tests: POST /v1/sessions shape; GET / content-type.

None feed packets through the poll loop. A sans-IO test harness (synthetic Input::Receive / Input::Timeout in, assert Output::Transmit / state out — exactly the pattern str0m is designed for) would have caught the missing Input::Timeout (F1), the double-offer panic (F2), and the idle-timeout semantics (F4). This is the single highest-leverage addition.

---

Informational — design notes (deliberate, not defects)

• tokio::sync::Mutex for a synchronous critical section. No lock is held across an .await anywhere (accept_offer, run_poll_once, close are all sync under the guard). A std::sync::Mutex / parking_lot::Mutex would be cheaper and avoid async-mutex overhead. Defensible given handlers are async; noting for later.
• Single poll task, O(n) per 10 ms tick, sequential lock acquisition (drive_all_sessions). Fine at slice-1 scale; tokio::time::interval defaults to MissedTickBehavior::Burst, so if a tick ever overruns 10 ms it will catch up in a burst. Revisit before multi-hundred-session scale (the spec already earmarks a dedicated timing thread for step 4).
• drive_all_sessions does two map lookups (iter().collect() then get(id)); a single iter_mut/retain pass would halve it. Micro.

What's solid (calibration — so the findings above are read in context)

This is careful work, and saying so keeps the critique honest:

• Clean workspace with pinned [workspace.dependencies]; Cargo.lock committed for the binary.
• Sound error taxonomy: cold-path RtcSessionError enum vs hot-path Option "drop + observe" (§3.8) applied consistently in the codec and the loop.
• The EchoAudioPipe is genuinely non-blocking and bounded, with an overflow-drop test (pcm.rs:147).
• ChannelId newtype; ChannelState as a closed enum for exhaustiveness.
• Graceful shutdown on Ctrl-C/SIGTERM; sensible tracing throughout.
• CI gates are real: fmt --check, clippy -D warnings, test --all, cargo deny. (The current CI red is infra — runner DNS — not the code.)
• Doc comments explain why, not just what, and even record prior dead-ends (the BUNDLE-line fixture note, the [0u8;8] "silence not garbage" note). Above-average.

Cleared during this review (so you don't re-flag it)

• Browser client ICE gathering. Suspected the client POSTs the offer before candidates are gathered. False — static/index.html awaits iceGatheringState === 'complete' before the POST (non-trickle done correctly). Not a finding.

---

Verification checklist for the GLM-5.2 reviewers

1. F1 is the one that matters. Confirm str0m 0.21 does not self-advance time, then decide severity by testing the muted-mic / one-way-silence path against a real browser. Everything else is secondary.
2. Reproduce F2 in under a minute (double POST) — cheapest confirmable bug.
3. Decide the F3/F1 wiring direction (fixed tick vs honor-deadline) as one coherent fix, not two.
4. Treat F8/F9 as str0m/Opus semantics homework — read the dependency, don't guess.
5. Push back on anything here you can falsify. A finding Claude can't defend should be struck, with the reasoning recorded, same as the cleared item above.