fix(whip): let a rejoining client take over a dead session's slot

[wagenet]

Problem

max_sessions is 1 per WHIP endpoint. When a publisher dies without sending a WHIP DELETE — network loss, the common case for a real participant — the slot stays occupied and the rejoining client gets 503 - All session slots are occupied until the inactivity watchdog reclaims it. Measured on a 5-seat meeting rig: rejoin accepted ~20 s after an abrupt drop. A clean leave (SIGINT, so the DELETE is sent) frees the slot immediately, so this only bites on real network loss.

The ICE callback does not cover it: an abruptly killed peer leaves webrtcbin at completed until consent freshness expires, so failed/disconnected does not fire promptly.

What this does

When every slot is taken, whip_post now watches the sitting session for up to 3 s instead of refusing outright.

Each session carries a SessionActivity counter, stamped by the appsink callback for every buffer that crosses the appsink→appsrc bridge (the same value the inactivity watchdog already reads, now shared rather than private to whip.rs).

• Counter moves between two polls → a publisher is still sending. The new client gets its 503 after one poll interval (~100 ms) and the live session is never touched.
• Counter frozen past 2 s → dead transport. The session is handed to the ordinary cleanup path — SessionCleanupRequest on the existing channel, claimed via cleanup_sent.swap(true) so it is torn down exactly once and its watchdog thread stops — and the new client takes the slot that path releases.
• Never delivered a buffer → never displaced. It may just be slow to negotiate through a TURN relay, and evicting it would let two clients take turns throwing each other off before either sends media.

Why "counter moved", not "idle time"

The cheaper rule — refuse if the sitting session is idle less than some bound — does not work, and was measured not to work on the rig: the reconnect that matters arrives a few hundred milliseconds after the drop, while the dead session still looks freshly fed, so the rejoin was refused and the watchdog reclaimed the slot 12 s later, exactly as before the change. A healthy session and a just-died one are indistinguishable from a single reading; only the counter advancing separates them.

The 2 s threshold sits well under the watchdog's 10 s INACTIVITY_TIMEOUT, so takeover reclaims the slot before the watchdog would. It does not depend on the watchdog's timing (a separate PR is making that loop faster and more deterministic; these two changes are independent).

Verification

Headless backend, one builtin.whip_input block (endpoint_id=p1, max_sessions=1) into fakesinks, publisher a whipclientsink fed by videotestsrc/audiotestsrc with H264 pinned. Media flow confirmed by Pad video_0 in the server log, not by ICE state.

Takeover — SIGKILL the publisher, relaunch immediately:

09:50:18.720  WARN  Displacing session '14f9642f-…' on port 49409 (2000 ms without media) so a new client can take its slot on endpoint 'p1'
09:50:18.822  INFO  Allocated slot 0 for session '15c44012-…'

Displaced at exactly 2000 ms without media, slot taken one poll later, media flowing again (Pad video_0) 2.74 s after the SIGKILL against ~20 s before.

Healthy session not stolen — second client while the first streams: 503 in 105 ms, to curl and to a real whipclientsink alike. The first publisher kept streaming; no displacement logged.

Clean leave unchanged — SIGINT still sends the DELETE, slot released immediately, rejoin works at once.

Flow stop — no pipeline-survival ERROR from stop_flow().

Tests

backend/src/whip_session_manager.rs, four #[tokio::test]s against the real allocate_slot_or_take_over with a real WhipSessionManager and its cleanup task running (core fakesrc / gst::Pipeline only, so they run in CI):

• a_dead_session_gives_its_slot_to_a_new_client — frozen counter well past the threshold; asserts the slot is reassigned, cleanup_sent is set, and the session is gone from the manager (i.e. the ordinary cleanup path ran, not a parallel teardown).
• a_session_that_died_moments_before_the_post_is_still_displaced — frozen counter at zero idle, the measured case; asserts it is displaced, and not before it has been quiet long enough.
• a_live_session_is_never_displaced — a thread stamps the counter every 20 ms the way the appsink callback does; asserts 503, no teardown, still registered, and answered in well under the idle threshold.
• a_session_that_has_not_delivered_media_yet_is_not_displaced.

Reverting the fix (making allocate_slot_or_take_over fall back to a plain allocate_slot) fails the first two and leaves the other two passing, as intended.

Ran locally: cargo test (all 20 suites green, 545 lib tests), cargo clippy --all-targets clean, cargo fmt --check clean. Nothing skipped.

Known limitation

The bundled ingest page cannot reach this window yet. It waits 10 s for ICE disconnected to recover (backend/static/whip/whip.js:224) and then another flat 10 s before retrying (backend/static/whip/ingest.html:117), so its first re-POST lands ~20 s after a drop — past the 10 s watchdog, which has already freed the slot by then. Today this change helps fast retries: a participant who hits Connect while frozen, and publishers with tighter retry loops. whipclientsink does not retry at all — it exits on the 503. Tightening those two timers is a follow-up, deliberately separate: it fails differently, it needs browser verification rather than the headless rig, and it is worthless without this change (retry at 2 s and you collect 503s until the watchdog fires).

A wrongly displaced publisher is not told. WebRTC has no goodbye, so it discovers the eviction through its own consent-freshness timeout — seconds to tens of seconds during which its UI still reads Connected. What bounds the risk is that endpoints are per-participant (p1…p5, one seat each), so a POST to a full endpoint is overwhelmingly that participant returning rather than an intruder. On a shared endpoint the trade would be worse.

The counter is stamped inside the appsink callback, so a session whose new_sample is blocked pushing into a stalled main pipeline looks frozen even though its publisher is fine. Such a session is already torn down by the 10 s inactivity watchdog today; takeover only makes that happen sooner, and only while someone is actively trying to join the endpoint.

🤖 Generated with Claude Code

[wagenet]

(from Claude, on behalf of @wagenet)

Checked against #752, which touches the inactivity watchdog in the same file.

Independent, confirmed. allocate_slot_or_take_over derives liveness itself, and
decides on the buffer counter moving between polls rather than on the watchdog having
marked the slot dead — so it does not inherit the watchdog's detection latency. #752
makes that latency 10s flat instead of 10-20s but leaves INACTIVITY_TIMEOUT at 10s,
so the TAKEOVER_IDLE_THRESHOLD ≪ watchdog-timeout assumption still holds.

They conflict textually. Both branches add a new top-level item immediately after
wait_until_deadline_or_stop — CreatedSession here, wait_for_inactivity there.
git merge-tree flags it; both are wanted, so whoever merges second keeps both.

Follow-up once both land. SessionActivity wraps exactly the
(epoch, last_buffer_ms) pair that wait_for_inactivity still takes as two loose
parameters and recomputes idle from. The watchdog should take &SessionActivity and
call .idle(), so there is one definition of session idleness rather than two that
have to agree.

[srperens]

Verdict: Approve — correct mechanism for a real bug, the concurrency reasoning holds up under tracing, and the new tests exercise the changed function directly on a green, non-skipping CI run.

Claims

Claim	Verdict	Evidence
Watchdog cleanup and takeover cleanup share the same single-fire flag	CONFIRMED	takeover: `if !candidate.cleanup_sent.swap(true, Ordering::SeqCst) {`; watchdog at backend/src/blocks/builtin/whip.rs:799 `if !cleanup_sent_watchdog.swap(true, Ordering::SeqCst) {` — same Arc<AtomicBool>
allocate_slot_or_take_over is the only production path into slot allocation for WHIP ingest	CONFIRMED	grepped backend/src/api/, backend/src/blocks/ for allocate_slot outside tests — sole non-test call is inside allocate_slot_or_take_over itself
Per-buffer touch() stays lock-free, matching CLAUDE.md's hot-path rule	CONFIRMED	SessionActivity::touch — one Instant::elapsed() and one relaxed store, no Mutex, no formatting; called from new_sample (activity_cb.touch();)
CI ran cargo test/clippy/fmt on this diff, not just build/check	CONFIRMED	.github/workflows/ci.yml:113-121: Check (Linux) runs fmt --check, clippy -D warnings, cargo test with STROM_REQUIRE_GST_PLUGINS=1 (skip becomes failure); green on de149ef

Diagnosis — Root cause matches: an abrupt drop leaves webrtcbin at completed until consent-freshness expiry (EXTERNAL, webrtcbin's own state machine, consistent with the symptom, not contradicted here). "Counter moved" beats "idle time" for the stated reason and the tests prove it: a_session_that_died_moments_before_the_post_is_still_displaced asserts no displacement before TAKEOVER_IDLE_THRESHOLD even at zero initial idle — exactly the race a naive idle check loses. Traced the concurrent-rejoin case by hand: allocate_slot's RwLock::write makes the grant atomic, the loser re-enters the loop, sees a freshly-registered (never-delivered-media) session, and correctly falls into "never displaced" rather than evicting the winner.

Not fully exercised — idlest_session's max_by_key((dying, idle)) tie-break only matters for max_sessions > 1; every test uses a 1-slot endpoint, matching the PR's stated deployment (one seat per endpoint). BOUNDED to the case actually deployed.

Radius — LOCAL: three files, one call site, no strom-types/API/WebSocket surface change, and SessionActivity holds no gst::Element/Pipeline/Bin.

Tests & CI — Green, non-skipping, and the four new #[tokio::test]s call allocate_slot_or_take_over on a real WhipSessionManager. I did not run cargo test/clippy/fmt myself (no GStreamer toolchain here) — the CI run is the evidence, not a local repro. The claimed revert-fails-two-of-four behavior I did not execute.

Confidence: HIGH