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`:

```rust
// 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:

```rust
// 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.**

```rust
// 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:

```rust
// 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.**

```rust
// 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.**

```rust
// 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.