spec(slice-1): adversarial review patches
Apply findings from the Claude adversarial assessment (docs/reviews/2026-06-28-slice-1-claude-adversarial-assessment.md) after GLM-5.2 verification against the codebase + str0m 0.21 source. Spec-only patches (code fixes follow as atomic commits): - §1.2: add F5 (resource ceiling) to out-of-scope (deferred to step 5). - §3.4: pin both str0m inputs as mandatory (Input::Receive + Input::Timeout) and the deadline-honoring sleep semantics (Option B from brainstorming). Without Input::Timeout, str0m's clock froze during inbound silence; consent checks stopped; the browser tore the call down before the 60s idle timeout (F1). - §4.5: clarify the idle timeout keys on MediaData only. STUN/DTLS keepalives do not reset it. Spec already said 'no RTP'; wording now makes the intent unambiguous so the deviation cannot return (F4). - §5.1: drop dead Channel.created_at field. The idle timeout keys on RtcSession.last_rtp_rx, not creation time. The field was never read (F6). - §10 (new): record the review patches lineage + cleared findings (F8, F9). Findings F8 and F9 cleared by reading str0m 0.21 source (packet/payload.rs:91 rebases MediaTime to the negotiated Opus clock; RFC 7587 fixes SDP at opus/48000/2 regardless of internal rate). Cleared with the same standard the review used for 'Browser ICE gathering'. See AGENTS.md slice-1 boundaries — no scope additions; this is ratification of fixes for bugs the spec already prohibited or that the dev-loop surfaced.
This commit is contained in:
@@ -56,6 +56,7 @@ though it pre-paves the tap by exposing the PCM boundary as a clean trait seam.
|
||||
| Latency benchmarking harness | Spearhead step 4 | Latency matters when barge-in needs to beat the brain round-trip; slice 1's bar is "no perceptible delay" (~≤200 ms), not sub-10 ms. |
|
||||
| Fuzz harnesses for wire parsers | Spearhead step 5 (SIP/SDP/RTP) | PORT_PLAN §10 mandates continuous fuzzing of every wire parser; slice 1 has no hostile-bytes surface (browser is trusted) and the fuzz story lands with the SIP trunk. A `fuzz/` placeholder dir pre-paves the layout. |
|
||||
| Resumability / re-INVITE / session migration | Later | Refresh the page → new session. Acceptable for dev loop. |
|
||||
| Session / FD resource ceiling on `POST /v1/sessions` | Spearhead step 5 (PSTN trunk + deployment posture) | A resource ceiling is a separate concern from authn/authz but isn't slice-1's wedge. Each session binds a UDP socket; orphan sessions are reaped after 60 s idle, so a tight create loop can hold bounded FDs at `(rate × 60 s)`. Acceptable for the loopback dev build; lands with the deployment posture / spend-gate step. |
|
||||
|
||||
### 1.3 What this slice does NOT prove
|
||||
|
||||
@@ -194,9 +195,25 @@ indirection when a real tap client replaces the pipe).
|
||||
|
||||
### 3.4 Polling & the tokio-vs-dedicated-thread deviation
|
||||
|
||||
str0m is sans-IO; its `Rtc` API exposes `handle_input(Input)` (feed network input) and
|
||||
`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.
|
||||
str0m is sans-IO; its `Rtc` API exposes `handle_input(Input)` (feed network input **or**
|
||||
advance the internal clock via `Input::Timeout(Instant)`) and `poll_output() -> Output`
|
||||
(the poll interface, where `Output::Timeout(Instant)` gives str0m's next deadline).
|
||||
Slice 1 runs this poll on a fixed 10 ms tokio tick.
|
||||
|
||||
**Both inputs are mandatory.** Inbound packets carry `Input::Receive(now, recv)`, but
|
||||
str0m's internal clock (DTLS retransmit, ICE consent freshness RFC 7675, RTCP) **only
|
||||
advances via `Input::Timeout(now)`**. The 10 ms tick feeds `Input::Timeout(now)` at the
|
||||
top of each `drive()` call before draining the UDP socket. Without this, sustained inbound
|
||||
silence (muted mic, hold, peer paused brain-side) freezes str0m's clock, stops consent
|
||||
checks, and the browser tears the call down (~15–30 s) **before** the 60 s idle timeout
|
||||
triggers. (Adversarial review F1 surfaced this; pinned here so the deviation cannot
|
||||
return.)
|
||||
|
||||
**Deadline-honoring sleep.** `drive()` returns `Option<Duration>` = str0m's next deadline
|
||||
− now. The session-map's poll task sleeps `min(deadline, 10 ms)` between ticks: honoring
|
||||
the deadline saves wakeups when str0m has nothing pending; the 10 ms cap guarantees the
|
||||
outbound 20 ms encode tick fires on time. (F3: the return value is load-bearing — it is
|
||||
wired into the sleep, not discarded.)
|
||||
|
||||
**This is an explicit, documented deviation from ARCHITECTURE.md**, which mandates
|
||||
*dedicated timing threads, never the shared tokio pool*. The deviation is **scoped to
|
||||
@@ -331,9 +348,13 @@ A single self-contained HTML file with inline JS, no build step. Behavior:
|
||||
The `ChannelId` (a UUID newtype from `rutster-call-model`) is the session id surfaced
|
||||
in the REST API. `RtcSession` owns both the str0m `Rtc` + codecs and the `Channel`
|
||||
(signaling state); see §3.1 and §5.
|
||||
- **Idle timeout: 60 s of no RTP packets received from the peer → close the session.**
|
||||
(RTC quiet periods are normal but 60 s of dead air is a real "the browser tab is
|
||||
dead" signal —browser-refresh, network drop, etc. 5 min was originally considered but
|
||||
- **Idle timeout: 60 s of no **RTP packets** received from the peer → close the session.**
|
||||
STUN consent checks and DTLS keepalives do **not** reset the timer — only
|
||||
`Event::MediaData` (depacketized audio) advances `last_rtp_rx`. A peer that keeps ICE
|
||||
alive but sends no audio (muted mic, revoked permission) is reaped on schedule.
|
||||
(RTC quiet periods are normal but 60 s of dead air — particularly STUN-only keepalives
|
||||
without any media flow — is a real "the browser tab is dead" signal —browser-refresh,
|
||||
network drop, mic-permission-revoked, etc. 5 min was originally considered but
|
||||
rejected as too long to hold dead state; 60 s is tight enough to reclaim resources
|
||||
promptly while tolerating natural inter-speech silences.) Implemented as a
|
||||
per-session deadline checked on each poll cycle; no per-session tokio task spawned
|
||||
@@ -368,14 +389,16 @@ pub struct Channel {
|
||||
pub id: ChannelId, // UUID newtype for type-safety
|
||||
pub state: ChannelState, // New | Connecting | Connected | Closing | Closed
|
||||
pub direction: Direction, // Inbound (browser-initiated in slice 1)
|
||||
pub created_at: Instant, // for the 5-min idle timeout
|
||||
}
|
||||
|
||||
pub enum ChannelState { New, Connecting, Connected, Closing, Closed }
|
||||
pub enum Direction { Inbound } // Outbound lands with the dialer (later)
|
||||
```
|
||||
|
||||
That's the whole crate. ~80 lines.
|
||||
That's the whole crate. ~70 lines. (Review-patch F6: a `created_at: Instant`
|
||||
field was originally drafted "for the idle timeout," but the timer keys off
|
||||
`RtcSession.last_rtp_rx`, not the channel's creation time. The field is dead;
|
||||
the `Instant` import it pulled is dropped with it.)
|
||||
|
||||
### 5.2 Why this is the Channel, not a throwaway peer type
|
||||
|
||||
@@ -583,3 +606,44 @@ them, later slices can be sparser on the well-trodden patterns).
|
||||
- [ADR-0003](../../adr/0003-sip-rust-native-trunk.md) — Rust-native stack stance
|
||||
- [ADR-0004](../../adr/0004-license.md) — GPL-3.0-or-later
|
||||
- [ADR-0006](../../adr/0006-ingress-posture.md) — WebRTC-first ingress
|
||||
|
||||
---
|
||||
|
||||
## 10. Review patches (adversarial assessment 2026-06-28)
|
||||
|
||||
The slice-1 spec was patched per the Claude adversarial assessment (filed at
|
||||
`docs/reviews/2026-06-28-slice-1-claude-adversarial-assessment.md`) after GLM-5.2
|
||||
verification of each finding against the codebase + str0m 0.21 source.
|
||||
|
||||
- **F1 (High, confirmed):** `Input::Timeout` was never fed; str0m's clock froze during
|
||||
inbound silence. §3.4 now pins both inputs (`Input::Receive` + `Input::Timeout`) as
|
||||
mandatory and the deadline-honoring sleep semantics. Code fix: feed
|
||||
`Input::Timeout(now)` at the top of `drive()`; route the return value into `session_map`'s
|
||||
sleep.
|
||||
- **F2 (Med-High, confirmed):** `accept_offer` asserted on a client-reachable path
|
||||
(double-`POST /offer` panic). Code fix: typed `RtcSessionError::AlreadyNegotiated` →
|
||||
`409 Conflict`.
|
||||
- **F3 (Med, confirmed):** `next_timeout` field written, return value discarded.
|
||||
Folded into the F1 fix (the return becomes load-bearing in the sleep).
|
||||
- **F4 (Med, confirmed):** `last_rx` bumped on every datagram, not RTP-specific.
|
||||
§4.5 already specified "no RTP"; the code now matches (bump `last_rtp_rx` on
|
||||
`Event::MediaData` only).
|
||||
- **F5 (Low-Med, confirmed):** No cap / rate limit on `POST /v1/sessions`. Deferred to
|
||||
step 5 — added to §1.2 out-of-scope.
|
||||
- **F6 (Low, confirmed):** `Channel.created_at` dead + spec contradicted itself on
|
||||
"5-min" vs "60 s". §5.1 drops the field; spec wording reconciled to 60 s in §4.5.
|
||||
- **F7 (Low, confirmed):** Unreachable `needs_redrain` branch in Step-2 drain loop.
|
||||
Removed.
|
||||
- **F8 (Low, **cleared**):** str0m 0.21 `packet/payload.rs:91` calls
|
||||
`rtp_time.rebase(self.clock_rate).numer()` — rebases `MediaTime` to the negotiated
|
||||
Opus 48 kHz wire clock. 20 ms → 960 ticks. Correct; no fix.
|
||||
- **F9 (Low, **cleared as bug**):** RFC 7587 fixes SDP at `opus/48000/2` regardless of
|
||||
internal rate; libopus self-describes internal rate. A 40/60 ms frame yields
|
||||
`OPUS_BUFFER_TOO_SMALL` → rutster routes to `None` → drop+observe per §3.8 (already the
|
||||
documented correct behavior). One-line comment added to `OpusDecoder::pcm_buf` noting
|
||||
the 20-ms assumption.
|
||||
- **Test-gap (confirmed):** No test drives `loop_driver::drive()` — the central function
|
||||
of the slice. Retrofitted a sans-IO harness (synthetic `Input::Receive` /
|
||||
`Input::Timeout` in, assert `Output::Transmit` + state transitions out). F1, F2, F4
|
||||
each carry regression tests on the new harness.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user