spec(slice-1): adversarial review fixes

API-surface corrections (verified against str0m 0.21 docs):
- str0m::Live -> str0m::Rtc (no Live type exists)
- needs_input_timeout() -> Rtc::poll_output() returning Output::Timeout(Instant)
- RtcConfig::with_dtls_cert -> RtcConfig::set_dtls_cert (also: str0m auto-gens
  a cert if none passed; slice 1 still passes explicitly for ownership clarity)
- Event::RtpPacket(pkt, rid) -> Event::RtpPacket(RtpPacket) { single struct,
  RID is a field }
- str0m::ice::IceAgent -> no public 'ice' module; ICE types at crate root:
  Candidate, CandidateKind, IceCreds, IceConnectionState
- str0m.write_rtp() on Rtclive -> Frame API path: Rtc::writer(mid) -> Writer::write

New design decision surfaced by API verification:
- Media API path: Frame API (default) vs RTP API (set_rtp_mode(true)).
  Slice 1 uses Frame API — the proof target is the codec-to-PCM boundary,
  not RTP packetization. RTP API is a candidate for step 5.

Design-level fixes from close reading:
- §3.3 AudioSource/AudioSink: clarify who drives the traits (the poll loop
  drives both; sink on inbound MediaData, source on Output::Timeout deadline)
- §3.6: clarify cert/CrossRef — fresh in-memory cert each restart is safe
  because browser fetches fresh SDP each session, no caching layer to go stale
- §4.1: add Content-Type: application/sdp to /offer (browser-side fetch needs
  it)
- §4.5: resolve 5-min-vs-60s idle-timeout contradiction (60s, not 5 min)
- §4.5: add graceful-shutdown story (drop DashMap on SIGINT, browsers see
  dead peer — acceptable for dev loop)
- §6.4: fix 'mocked str0m Rtc' — str0m's Rtc is a concrete struct, not a
  trait; sans-IO means drive real Rtc with synthetic Inputs, not mock it.
  Stronger than mocking: production path exercised exactly.

Resolves 5 load-bearing API errors + 3 design-level ambiguities. Spec is now
implementation-ready pending user review.
This commit is contained in:
adlee-was-taken
2026-06-28 10:41:18 -04:00
parent 504d0020f4
commit 1164770d98

View File

@@ -118,28 +118,50 @@ Stubs **do not** anticipate code. They lock the boundary shape; that is their on
### 3.1 Components ### 3.1 Components
- **`RtcSession`** — owns a str0m `Live` instance plus the per-peer codec state (an Opus - **`RtcSession`** — owns a `str0m::Rtc` instance (the main sans-IO driver type; str0m
decoder + an Opus encoder). One per WebRTC peer. has no `Live` struct) plus the per-peer codec state (an Opus decoder + an Opus
encoder). One per WebRTC peer.
- **`PcmFrame`** — the canonical tap format from ARCHITECTURE.md: 16-bit signed mono PCM - **`PcmFrame`** — the canonical tap format from ARCHITECTURE.md: 16-bit signed mono PCM
@ 24 kHz, fixed frame size (20 ms = 480 samples). This is the single format every @ 24 kHz, fixed frame size (20 ms = 480 samples). This is the single format every
future brain/tap consumer speaks. Defined here in slice 1; `rutster-tap` will future brain/tap consumer speaks. Defined here in slice 1; `rutster-tap` will
re-export it once that crate fills in (step 2 — keeps one canonical home). re-export it once that crate fills in (step 2 — keeps one canonical home).
- **Codec pair** — `opus` crate (libopus via FFI; PORT_PLAN §7 disposition for Opus is - **Codec pair** — `opus` crate (libopus via FFI; PORT_PLAN §7 disposition for Opus is
🦀 Core (FFI)). Decoder: `opus_decode` → fills a `PcmFrame`; Encoder: `opus_encode` 🦀 Core (FFI)). Decoder: `opus_decode` → fills a `PcmFrame`; Encoder: `opus_encode`
from ring-buffered PCM → RTP packet payload. from ring-buffered PCM → Opus payload for str0m's Frame API.
### 3.2 Loop shape (Approach A from brainstorming) ### 3.2 Loop shape (Approach A from brainstorming)
str0m exposes two API surfaces: the **Frame API** (default; str0m handles
packetization internally, applications push/pop decoded media frames) and the **RTP API**
(opt-in via `RtcConfig::set_rtp_mode(true)`; raw RTP packets, the app does
packetization). Slice 1 uses the **Frame API** because slice 1's goal is to prove the
codec-to-PCM boundary, not to prove RTP packetization. The Frame API gives us decoded
audio frames as `MediaData` and accepts encoded audio back; str0m does the RTP/SRTP
framing underneath. RTP API is a candidate for step 5 (PSTN trunk) where raw-packet
control over the SIP/RTP boundary may matter.
``` ```
str0m Live (sans-IO, polled on tokio — see §3.4) str0m Rtc (sans-IO, polled on tokio — see §3.4)
↓ Event::RtpPacket(pkt, rid) ↓ Event::RtpPacket(RtpPacket) { …, payload } (single struct; RID is a field if present)
decode Opus → PcmFrame (24 kHz mono, 20 ms) ↓ str0m delivers decoded audio as MediaData via the Frame API
decode Opus payload → PcmFrame (24 kHz mono, 20 ms)
buffer PCM into playout ring (slice 1: echo; step 2 taps here) buffer PCM into playout ring (slice 1: echo; step 2 taps here)
↓ on each 20 ms tick ↓ on each 20 ms tick
take 480 samples → encode Opus → str0m.write_rtp() take 480 samples → encode Opus → push to str0m via Rtc::writer(mid)->Writer::write
``` ```
> **API-surface notes (verified against str0m 0.21 docs):**
> - `str0m::Rtc` is the main sans-IO driver; there is no `Live` type. `Rtc::handle_input(Input)`
> feeds network input; `Rtc::poll_output() -> Output` is the poll interface, where
> `Output::Timeout(Instant)` gives the next-deadline signal we sleep tokio until.
> - `Event::RtpPacket(RtpPacket)` is a single-struct variant (RID is a field on `RtpPacket`,
> not a second variant payload).
> - Inbound/`Event::RtpPacket`: in RTP mode you get raw packets; in slice 1's Frame-mode
> the canonical inbound event is `Event::MediaData(MediaData)` containing decoded media.
> - Outbound: `StreamTx::write_rtp` exists in RTP mode only; under the Frame API the path is
> `Rtc::writer(mid) -> Writer::write(...)`.
### 3.3 The PCM-tap seam ### 3.3 The PCM-tap seam
The point where decoded `PcmFrame`s emerge from the inbound side and where `PcmFrame`s The point where decoded `PcmFrame`s emerge from the inbound side and where `PcmFrame`s
@@ -163,10 +185,18 @@ Slice 1 wires an `EchoAudioPipe` (implements both traits) between sink and sourc
Step 2 replaces `EchoAudioPipe` with a real tap client. No code changes to Step 2 replaces `EchoAudioPipe` with a real tap client. No code changes to
`RtcSession` itself in step 2 — that's the test of the seam. `RtcSession` itself in step 2 — that's the test of the seam.
**Who drives the traits:** the poll loop (§3.4) drives both. On each `poll_output()` cycle:
inbound `Event::MediaData` → decode → `sink.on_pcm_frame(PcmFrame)`; the outbound side
runs on the `Output::Timeout(Instant)` deadline → `source.next_pcm_frame()` → encode →
push to str0m. `RtcSession` owns the `Arc<Mutex<dyn AudioSource + AudioSink>>` (or a
concrete `EchoAudioPipe` in slice 1 — no trait object needed yet; step 2 introduces
indirection when a real tap client replaces the pipe).
### 3.4 Polling & the tokio-vs-dedicated-thread deviation ### 3.4 Polling & the tokio-vs-dedicated-thread deviation
str0m is sans-IO; its `Live` API exposes `handle_input` / `needs_input_timeout` to drive str0m is sans-IO; its `Rtc` API exposes `handle_input(Input)` (feed network input) and
the poll. Slice 1 runs this poll on the tokio runtime. `poll_output() -> Output` (the poll interface, where `Output::Timeout(Instant)` gives the
next deadline we sleep tokio until). Slice 1 runs this poll on the tokio runtime.
**This is an explicit, documented deviation from ARCHITECTURE.md**, which mandates **This is an explicit, documented deviation from ARCHITECTURE.md**, which mandates
*dedicated timing threads, never the shared tokio pool*. The deviation is **scoped to *dedicated timing threads, never the shared tokio pool*. The deviation is **scoped to
@@ -187,17 +217,23 @@ spec so it cannot be forgotten.
### 3.5 SRTP / DTLS ### 3.5 SRTP / DTLS
DTLS-SRTP is mandatory from slice 1 (Security-as-product pillar). str0m handles DTLS+ DTLS-SRTP is mandatory from slice 1 (Security-as-product pillar). str0m handles DTLS+
SRTP natively once fed a certificate; configured via `RtcConfig::with_dtls_cert` at SRTP natively; configured via `RtcConfig::set_dtls_cert` at startup. If no cert is
startup. passed, str0m auto-generates one — in slice 1 we explicitly pass our self-signed DTLS
cert (see §3.6) to make the cert ownership clear, but the auto-gen path is also
acceptable for a dev loop.
**No plaintext RTP fallback path exists** in code or config. This is a deliberate floor, **No plaintext RTP fallback path exists** in code or config. This is a deliberate floor,
not a default. not a default.
### 3.6 DTLS certificate ### 3.6 DTLS certificate
Self-signed DTLS cert generated at startup, held in memory. No cert file in slice 1 Self-signed DTLS cert generated at startup, held in memory, **passed explicitly to
(cert rotation / persistence is a step-5 / trunk-integration concern). The cert is `RtcConfig::set_dtls_cert`** (per §3.5 — making cert ownership clear in code rather than
regenerated on every server restart — fine for a dev loop. relying on str0m's auto-gen default). No cert file on disk in slice 1 (cert rotation /
persistence is a step-5 / trunk-integration concern). The cert is regenerated on every
server restart — acceptable for slice 1 because the browser fetches a fresh SDP answer
each session, so the DTLS fingerprint in the SDP always matches the current cert. No
caching layer exists to go stale.
### 3.7 Codec negotiation ### 3.7 Codec negotiation
@@ -213,7 +249,9 @@ point.
Slice 1's SDP module is a focused ~50-line mapper: parse the browser offer, extract the Slice 1's SDP module is a focused ~50-line mapper: parse the browser offer, extract the
audio m-line, munge into our answer (Opus only, recv+send, DTLS fingerprint from our audio m-line, munge into our answer (Opus only, recv+send, DTLS fingerprint from our
cert, ICE ufrag/pwd from str0m). cert, ICE ufrag/pwd via str0m's `IceCreds` / `Candidate` types at the crate root — str0m
has no public `str0m::ice` module; the ICE-relevant public surface lives at `str0m::`:
`Candidate`, `CandidateKind`, `IceCreds`, `IceConnectionState`).
### 3.8 Error handling on the hot path ### 3.8 Error handling on the hot path
@@ -245,9 +283,9 @@ policy is match-and-continue).
- `POST /v1/sessions` → mint a `RtcSession` (which owns a fresh `Channel`; the - `POST /v1/sessions` → mint a `RtcSession` (which owns a fresh `Channel`; the
`ChannelId` *is* the session id). Returns `{ "session_id": "<uuid>" }`. `ChannelId` *is* the session id). Returns `{ "session_id": "<uuid>" }`.
- `POST /v1/sessions/:id/offer` (body: browser SDP offer, including all ICE - `POST /v1/sessions/:id/offer` (body: browser SDP offer, `Content-Type: application/sdp`,
candidates — non-trickle) → core produces SDP answer (including its ICE candidates), including all ICE candidates — non-trickle) → core produces SDP answer (including its
feeds candidates to str0m, returns the answer as `application/sdp`. ICE candidates), feeds candidates to str0m, returns the answer as `application/sdp`.
- `DELETE /v1/sessions/:id` → tear down: transition `Channel` to `Closing → Closed`, - `DELETE /v1/sessions/:id` → tear down: transition `Channel` to `Closing → Closed`,
drop the `RtcSession`, close the peer connection cleanly via str0m. drop the `RtcSession`, close the peer connection cleanly via str0m.
- `GET /` → serve the static HTML test client. - `GET /` → serve the static HTML test client.
@@ -291,14 +329,22 @@ A single self-contained HTML file with inline JS, no build step. Behavior:
- Sessions held in an in-process `DashMap<ChannelId, RtcSession>` in the binary crate. - Sessions held in an in-process `DashMap<ChannelId, RtcSession>` in the binary crate.
The `ChannelId` (a UUID newtype from `rutster-call-model`) is the session id surfaced The `ChannelId` (a UUID newtype from `rutster-call-model`) is the session id surfaced
in the REST API. `RtcSession` owns both the str0m `Live` + codecs and the `Channel` in the REST API. `RtcSession` owns both the str0m `Rtc` + codecs and the `Channel`
(signaling state); see §3.1 and §5. (signaling state); see §3.1 and §5.
- Idle timeout: 5 min hard cutoff. "Idle" = no RTP packets received from the peer in - **Idle timeout: 60 s of no RTP packets received from the peer → close the session.**
the last 60 s (a longer window than the strict "no RTCP" definition, because RTC (RTC quiet periods are normal but 60 s of dead air is a real "the browser tab is
quiet periods are normal; no-RTP-for-60s is a real "the browser tab is dead" signal). dead" signal —browser-refresh, network drop, etc. 5 min was originally considered but
Implemented as a per-session deadline checked on each poll cycle; no per-session rejected as too long to hold dead state; 60 s is tight enough to reclaim resources
tokio task spawned (which would clutter the runtime and pre-pave the wrong pattern promptly while tolerating natural inter-speech silences.) Implemented as a
for the dedicated timing thread in step 4). per-session deadline checked on each poll cycle; no per-session tokio task spawned
(which would clutter the runtime and pre-pave the wrong pattern for the dedicated
timing thread in step 4).
- **Graceful shutdown (Ctrl-C / SIGTERM):** the slice installs a tokio signal handler
that drops the `DashMap` (and thus every `RtcSession`) on shutdown. Browsers see a
dead peer connection — acceptable for the dev loop; no in-flight call preservation
story in slice 1. Cleanly closing each str0m `Rtc` is a nice-to-have — we attempt it
but don't block shutdown on a peer that won't ack. POST/DELETE callers get TCP RST,
also acceptable for dev-loop.
- Restart on browser refresh: new session. No resumability. - Restart on browser refresh: new session. No resumability.
### 4.6 API posture ### 4.6 API posture
@@ -429,9 +475,11 @@ audiohook primitive) needs to observe it — not before.
- Opus⇄PCM roundtrip: encode known PCM → decode → assert RMS within tolerance. - Opus⇄PCM roundtrip: encode known PCM → decode → assert RMS within tolerance.
- SDP answer munger: feed a captured browser offer fixture, assert the answer is - SDP answer munger: feed a captured browser offer fixture, assert the answer is
well-formed and Opus-only. well-formed and Opus-only.
- `RtcSession` state transitions: drive a mocked str0m `Live` by injecting fake - `RtcSession` state transitions: drive a **real** `str0m::Rtc` instance with
events (str0m is sans-IO — its `Live` is fully testable without a network. This is synthetic `Input` events (str0m's `Rtc` is a concrete struct, not a trait — there's
the sans-IO payoff we chose in Approach A.) nothing to mock. Sans-IO means we drive the real type with fake inputs, not a test
double. This is the sans-IO payoff we chose in Approach A, and it's stronger than
mocking: the production code path is exercised exactly.)
- **Integration test in `rutster` binary crate:** - **Integration test in `rutster` binary crate:**
- Spin up the axum server on an ephemeral port, use `reqwest` + a `webrtc-rs` *client* - Spin up the axum server on an ephemeral port, use `reqwest` + a `webrtc-rs` *client*
(or a minimal hand-rolled SDP answer parser) to simulate a peer. (or a minimal hand-rolled SDP answer parser) to simulate a peer.
@@ -514,7 +562,8 @@ them, later slices can be sparser on the well-trodden patterns).
| WebRTC stack | **str0m** | webrtc-rs; defer-and-compare | Sans-IO, Rust-native, designed for embedding in a custom media loop. Maps directly onto ARCHITECTURE.md's "dedicated timing threads, not the shared tokio pool." Smaller community but the right shape. | | WebRTC stack | **str0m** | webrtc-rs; defer-and-compare | Sans-IO, Rust-native, designed for embedding in a custom media loop. Maps directly onto ARCHITECTURE.md's "dedicated timing threads, not the shared tokio pool." Smaller community but the right shape. |
| Workspace shape | **Full architecture-shaped workspace** | Workspace: media lib + binary; single binary crate | Locks the ADR-0002 fused-vertical boundary from day one. Speculative crate boundaries risk not surviving real code, but stub crates cost ~zero and prevent a future rename churn. | | Workspace shape | **Full architecture-shaped workspace** | Workspace: media lib + binary; single binary crate | Locks the ADR-0002 fused-vertical boundary from day one. Speculative crate boundaries risk not surviving real code, but stub crates cost ~zero and prevent a future rename churn. |
| Loopback scope | **+ Terminate codec to PCM** | Bare RTP passthrough; audio-only echo | Proves the canonical PCM tap format, not just RTP passthrough. Pre-paves step 2 (the tap). | | Loopback scope | **+ Terminate codec to PCM** | Bare RTP passthrough; audio-only echo | Proves the canonical PCM tap format, not just RTP passthrough. Pre-paves step 2 (the tap). |
| Media-loop structure | **A. str0m Live + codec hook** | B. Full sans-IO decomposition; C. Opaque Peer + dedicated thread now | Leans on str0m's pacing for slice 1; dedicated timing thread arrives in step 4 when barge-in needs determinism. Smallest code that proves the codec boundary. | | Media-loop structure | **A. str0m Rtc + codec hook** | B. Full sans-IO decomposition; C. Opaque Peer + dedicated thread now | Leans on str0m's pacing for slice 1; dedicated timing thread arrives in step 4 when barge-in needs determinism. Smallest code that proves the codec boundary. |
| Media API path | **str0m Frame API** (default) | RTP API (`set_rtp_mode(true)`) | Frame API handles RTP packetization internally; slice 1's proof target is the codec-to-PCM boundary, not packetization. RTP API is a candidate for step 5 (PSTN trunk). |
| Poll execution | tokio (slice 1 deviation) | Dedicated thread now | No reflex to time against; dedicated thread now is theater. Step 4 lands the dedicated thread. Explicit, documented deviation. | | Poll execution | tokio (slice 1 deviation) | Dedicated thread now | No reflex to time against; dedicated thread now is theater. Step 4 lands the dedicated thread. Explicit, documented deviation. |
| HTTP TLS | None (dev-only plaintext) | Self-signed TLS now | Dev loop is loopback; the real security surface (DTLS-SRTP) is already mandatory. TLS needs the cert story from ARCHITECTURE.md, which lands with deployment posture. | | HTTP TLS | None (dev-only plaintext) | Self-signed TLS now | Dev loop is loopback; the real security surface (DTLS-SRTP) is already mandatory. TLS needs the cert story from ARCHITECTURE.md, which lands with deployment posture. |
| ICE | Non-trickle | Trickle | Simpler server; one round-trip. Real-world NATs (likely step 5) trigger the switch. | | ICE | Non-trickle | Trickle | Simpler server; one round-trip. Real-world NATs (likely step 5) trigger the switch. |