[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. A clean leave sends the DELETE and frees the slot immediately.

The ICE callback does not cover it: an abruptly killed peer leaves webrtcbin at completed until consent freshness expires.

What this does

When every slot is taken, whip_post 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.

• Counter moves between two polls: a publisher is still sending. 503 after one poll (~100 ms); the session is not touched.
• Counter frozen past 2 s, and the session has delivered audio: dead transport. The session goes to the ordinary cleanup path (SessionCleanupRequest, claimed via cleanup_sent.swap(true), so it is torn down once and its watchdog stops) and the new client takes the slot it releases.
• No audio delivered, or nothing at all: never displaced, 503 at once. A session with no media may be negotiating through a TURN relay. On a multi-slot endpoint, sessions with audio are considered first, so a quieter video-only session cannot hide a dead one.

Once a session is marked finished, by any teardown path, its bridge stops pushing into the slot's appsrc. Takeover frees a slot whose publisher may still be sending, and without that check both sessions would feed one appsrc with different timestamp offsets until the old pipeline reaches NULL.

Why the counter moving, not idle time. A single idle reading cannot separate a healthy session from one that died a moment ago, and the reconnect that matters arrives a few hundred milliseconds after the drop. On the rig, an idle-time rule refused that rejoin and the watchdog reclaimed the slot 12 s later, as before the change.

Why audio is required. Two seconds of silence only means "dead" when audio is present, at ~20 ms per packet. A video-only publisher of static content, such as a screen share nobody is touching, goes seconds between frames on a healthy transport. The gate is per session, so a client that negotiates video-only against an audio_video endpoint is covered too. Video-only sessions are left to the watchdog.

Video-only measurement without the audio gate

Video-only endpoint, max_sessions: 1, publisher running throughout, second client POSTing:

Publisher framerate	Second client	Sitting session
30/1	503 in 141 ms	kept
1/1	503 in 137 ms	kept
1/5	slot granted	displaced at 3100 ms (3040 ms on a second run)

The 10 s watchdog never fired for the 1/5 publisher, so it is viable on main and only takeover would have killed it.

Verification

Headless backend, one builtin.whip_input block into fakesinks, publisher a whipclientsink fed by videotestsrc/audiotestsrc with H264 pinned. Media flow confirmed from Pad video_0/Pad audio_0 in the server log, not from ICE state.

Arm	Result
Audio+video publisher SIGKILLed, immediate rejoin	displaced at 2005 ms, slot granted
Audio+video publisher healthy, second client	503 in 143 ms, kept
Video-only 1/5 healthy, second client	503 in 2003 ms, kept ¹
Video-only 30/1 healthy, second client	503 in 141 ms, kept

¹ Measured before undisplaceable sessions were refused at once; the rig was not re-run for that. The unit test asserts the 503 now arrives in under 2 s.

Clean leave unchanged: SIGINT sends the DELETE and the slot is released immediately. Flow stop logs no pipeline-survival ERROR.

Tests

Six #[tokio::test]s in whip_session_manager.rs call the real allocate_slot_or_take_over with a running cleanup task (core fakesrc only, so they run in CI):

• a_dead_session_gives_its_slot_to_a_new_client
• a_session_that_died_moments_before_the_post_is_still_displaced: frozen counter at zero idle
• a_live_session_is_never_displaced: counter stamped every 20 ms; 503 in under the idle threshold
• a_session_that_has_not_delivered_media_yet_is_not_displaced: also asserts the 503 is immediate
• a_sparse_video_only_session_is_not_displaced: same timing assertion
• a_dead_session_is_displaced_past_a_quieter_video_only_one: two slots

a_finished_session_stops_feeding_its_slot in whip.rs calls forward_sample_to_slot on a real appsrc ! appsink and asserts the first sample out is the current session's, not the finished one's.

Reverts, each run separately; each failed only the tests listed:

Reverted	Fails
takeover (plain allocate_slot)	the first two
audio gate	a_sparse_video_only_session_is_not_displaced
immediate refusal	the two timing assertions
audio-first sort key	the two-slot test
finished-session check	a_finished_session_stops_feeding_its_slot

cargo test green locally (583 lib tests plus every integration suite); clippy --all-targets -- -D warnings and fmt --check clean. Two tests ignored, both unrelated: test_jitterbuffer_stalls_without_drop_on_latency (GStreamer-version dependent) and server_is_not_demoted_to_background_qos (needs STROM_TEST_APP_NAP=1).

Risks

A session whose audio stops for good, such as a muted or failed microphone, while its video continues at gaps over 2 s can still be displaced.

A wrongly displaced publisher is not told. It learns through its own consent-freshness timeout, and its UI reads Connected until then. Endpoints are one seat per participant (p1…p5), so a POST to a full endpoint is almost always that participant returning. On a shared endpoint the trade would be worse.

Liveness is stamped at the session appsink, so a session blocked pushing into a stalled main pipeline looks frozen while its publisher is fine. #756 moves the judgement to media that reaches the flow's consumers.

The bundled ingest page does not reach this window yet. Its first re-POST lands ~20 s after a drop, past the watchdog, and whipclientsink exits on a 503 without retrying. Tightening the page's retry timers is a follow-up that needs browser verification.

🤖 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

[srperens]

Housekeeping note plus one substantive question — not a new review. The head is unchanged at de149ef7, so my standing Approve is not being revisited on its own merits; what changed is the base under it.

This branch is now CONFLICTING / DIRTY. #752 and #749 both landed in backend/src/blocks/builtin/whip.rs since this was approved. A test merge against current main reports exactly one conflicting file — that one — and whip_session_manager.rs merges clean despite #749 having touched it. The approval will be dismissed by the merge-forward, so this needs a fresh look regardless.

The interaction I expected to be a problem is not one. #749 made WhipEndpointConfig::allocate_slot unlock each slot's decodebin (they are now built with locked state, so a slot that is claimed without that call would decode nothing). Takeover reaches the slot through that same function rather than writing the assignment itself:

backend/src/whip_session_manager.rs:478 — if let Some(slot) = config.allocate_slot(resource_id) {

So the unlock happens on the takeover path too, and a freshly allocated session cannot be displaced by the next POST either, since idle is None until it delivers a buffer. Nothing to change here — recording it because the combination is new and no review of either PR could have covered it.

The question: video-only ingest. The threshold's reasoning is

backend/src/whip_session_manager.rs:239 — /// A connected WebRTC publisher delivers audio every ~20 ms and video every

which holds only while there is an audio stream. StreamMode has a video-only variant:

types/src/block.rs:30 — Video,

A video-only publisher of static content — a screen share that is not changing — can plausibly go past TAKEOVER_IDLE_THRESHOLD without a buffer while its transport is perfectly healthy, and then the counter is frozen for the same reason a dead one is. With audio in the stream this cannot happen. Two seconds is also short enough that a low-framerate video-only ingest is not an exotic case.

Either an argument for why the counter cannot freeze that long on a video-only session, or making the threshold depend on StreamMode, would close it. I have not tested a video-only WHIP ingest, so this is reasoning from the mode enum rather than an observed failure.

When you resolve the conflict, the thing worth a second look is that the resolved appsink callback still stamps through SessionActivity and keeps #749's locked-decodebin setup intact — the two changes sit in the same region of whip.rs, so the merge is textually one file but semantically two mechanisms meeting. The rest of the resolution should be placement.

For the record on the design: the "counter moved" predicate rather than a single idle reading, Option::None sorting a never-fed session out of displacement range, and reusing cleanup_sent.swap(true) so the ordinary teardown path runs exactly once all still read as correct to me. The per-buffer cost is unchanged too — touch() wraps the same Arc<AtomicU64> the watchdog already stamped, so it stays one relaxed store.

[wagenet]

(from Claude, on behalf of @wagenet)

Rebased onto current main and fixed the video-only case. Head is now 0bcf6dc.

The conflict. One file, as you found. #752 extracted the watchdog loop into wait_for_inactivity and this branch added CreatedSession, both immediately after wait_until_deadline_or_stop. Both kept. The watchdog body itself merged clean because the change here was purely additive, so the loop is #752's per-poll version. One compile break in a test helper: #749 added slot_decodebins to WhipEndpointConfig.

Your video-only question was right, and it reproduces. Video-only endpoint, max_sessions: 1, publisher running throughout, second client POSTing to the full endpoint:

Publisher framerate	Inter-frame gap	Second client	Sitting session
30/1	33 ms	503 in 141 ms	kept
1/1	1 s	503 in 137 ms	kept
1/5	5 s	slot granted	displaced at 3100 ms without media

Reproduced at 3040 ms on a second run. The 10 s inactivity watchdog never fired in those runs, so the sparse session is viable on main today and only takeover killed it. Breakage introduced, not accelerated.

Why the fix is not a longer threshold. There is no value that is both safe for video-only and useful. To survive a genuinely static screen share it would have to exceed the watchdog's 10 s, at which point takeover never fires. A transport-level signal does not rescue it either: consent freshness is the slow signal that made takeover necessary in the first place.

So the rule is that fast takeover is only sound when audio is present, because audio's ~20 ms cadence is the only thing that makes a 2 s gap mean anything. SessionActivity now stamps audio buffers into a counter of their own, and a session that has never delivered audio is never displaced — alongside the existing rule for one that has never delivered anything. Video-only sessions are left to the watchdog. This is per-session rather than per-endpoint mode, so a client negotiating video-only against an audio_video endpoint is covered too.

After the fix, all four rig arms:

Arm	Result
Audio+video SIGKILLed, immediate rejoin	displaced at 2005 ms, slot granted
Audio+video healthy	503 in 143 ms, kept
Video-only 1/5 healthy	503 in 2003 ms, kept
Video-only 30/1 healthy	503 in 141 ms, kept

Residual, in the body as a known limitation. A session whose audio stops for good — muted or failed microphone — while its video continues at gaps wider than 2 s can still be displaced. Narrower than the video-only case, which was a configuration rather than a runtime coincidence. Not closed.

The merge region you flagged. Confirmed: the appsink callback still stamps liveness before pulling the sample, now with the pad's media type, and #749's prepare_idle_decodebin and slot_decodebins wiring is untouched. Takeover reaches the slot through allocate_slot, so the decodebin unlock happens on that path too, as you traced.

Tests. New a_sparse_video_only_session_is_not_displaced, five now in total. Reverting the audio gate fails that one alone and leaves the other four passing; I ran that revert rather than reasoning about it. Locally on the rebased base: cargo test fully green (581 lib tests plus every integration suite), cargo clippy --all-targets and cargo fmt --check clean, nothing skipped.

One thing I am not doing here. My earlier note suggested folding wait_for_inactivity's loose (epoch, last_buffer_ms) pair onto SessionActivity. That would be wrong once #756 lands: it splits the type into ingress and output stamps and its idle() returns the staler of the two, while the watchdog must fire on ingress alone. Give it the combined value and it starts tearing down sessions whose output stalled below the decoder, which is the case where the publisher is healthy and the flow is wedged. The shared definition belongs in #756.

[srperens]

Verdict: Comment — the mechanism is right and "counter moved" is the correct discriminator; my dismissed review at de149ef7 is answered and the video-only regression it would have caused is closed. One ordering gap is left.

Requested changes

1. Gate the slot push on the teardown flag — backend/src/blocks/builtin/whip.rs, in the new_sample callback, read cleanup_sent and return Ok(gst::FlowSuccess::Ok) before appsrc.push_sample. It is already in scope in create_whipserversrc_for_session, and one relaxed load per buffer stays inside the hot-path budget the rest of this diff respects. Why, below.

Claims

Claim	Verdict	Evidence
The displaced slot frees fast enough for the remaining ~1 s of polls to claim it	CONFIRMED	backend/src/whip_session_manager.rs:412 — config.release_slot(slot); runs before the teardown handed off at backend/src/whip_session_manager.rs:423 — ` tokio::task::spawn_blocking(move
All five new tests executed in CI rather than skipping	CONFIRMED	Check (Linux) job 102740835862 at 0bcf6dc2: all five whip_session_manager::tests::a_* lines ... ok, a_sparse_video_only_session_is_not_displaced among them
Stamping liveness stays within the per-buffer budget	CONFIRMED	backend/src/blocks/builtin/whip.rs:1041 — activity_cb.touch(pad_is_audio); — media type resolved once at pad setup; touch is Instant::elapsed plus one or two relaxed stores. No lock, no allocation, no SystemTime

Diagnosis — right layer. What the body does not cost is who pays for a wrong displacement. It frames that cost as falling on the victim, through its own consent timeout. It also falls on the new session: nothing stops a displaced-but-alive publisher from feeding the slot it no longer owns. backend/src/blocks/builtin/whip.rs:1095 — let _ = appsrc.push_sample(&sample); is reached with no check on cleanup_sent, the slot's appsrc belongs to whoever holds the slot, and ts_offset is per session. Between release_slot and the old pipeline reaching NULL, two sessions can push into one appsrc with two different offsets.

On main this cannot bite: every path that frees a slot frees a session the watchdog already found silent for its whole timeout, so nothing is pushing. Takeover is the first path that frees a session whose media still flows — exactly the muted-microphone residual the body discloses. Whether the window ever opens depends on a teardown outlasting a full ICE/DTLS handshake, which I cannot settle from this repository. The guard closes it either way.

Smaller: a POST refused because the sitting session never delivered a buffer is held the full backend/src/whip_session_manager.rs:333 — const TAKEOVER_WAIT: Duration = Duration::from_secs(3);, since a counter stuck at 0 never trips the moving-counter return. A 3 s 503 where main answered at once — worth a line under Known limitations.

Radius — SHARED: the WHIP Input subsystem only, but all of it — the whip_post request path, the manager's slot accounting, and the appsink callback every session installs. No block, codec or platform outside WHIP Input reaches the new path.

Tests & CI — six checks green at 0bcf6dc2 (run 34435913759). Build (Windows) and Build (macOS) are skipping, which is fine here rather than an unverified row: no target_os code in the diff, so Linux-green plus the post-merge main run covers it.

Confidence: HIGH

[wagenet]

(from Claude, on behalf of @wagenet)

Both points addressed in 20007d5.

The slot push is gated. The bridge body is now forward_sample_to_slot in whip.rs, and it returns before touching the buffer once cleanup_sent is set: one relaxed load per buffer. The sample is still pulled first, so the appsink queue drains. The gate covers every teardown path, not only takeover, since they all set the same flag.

New test a_finished_session_stops_feeding_its_slot calls that function on a real appsrc ! appsink: first with a finished session, then with the current one. It asserts that the first sample out of the slot belongs to the current session. Removing the check fails it.

The 3 s hold is gone rather than documented. A sitting session with no audio delivered, including one with nothing delivered, can't be displaced during the wait. If it starts sending, its counter moves and that already refuses. So the POST now gets its 503 on the first poll. This moves the video-only case too: the 2003 ms 503 on the rig becomes immediate.

That exposed the multi-slot ordering you called bounded. With max_by_key((dying, idle)), a video-only session that had been idle longer would be picked over a dead audio session, and the early 503 would then refuse a takeover that should succeed. The key is now (dying, has_audio, idle). a_dead_session_is_displaced_past_a_quieter_video_only_one covers it on a two-slot endpoint.

Verified. Each change was reverted separately and the WHIP tests run each time. Each revert failed only its own test: the gate test, the two new timing assertions on the no-media and video-only tests, and the two-slot test. cargo test is fully green (583 lib tests plus integration suites). clippy --all-targets -D warnings and fmt --check are clean. The two ignored tests are env-gated and unrelated to WHIP. I did not re-run the rig for the early 503; the body marks that row as measured before it.

The review's open question stands: whether a teardown can outlast a full ICE/DTLS handshake. The gate makes that moot, and I have not measured it.

[srperens]

Verdict: Approve — supersedes my review of 2026-09-14 (at 0bcf6dc2), which was Comment. Both standing items are closed. The branch was force-pushed, so I re-read the mechanism rather than diffing the old head.

What closed the two items

1. The ordering gap. I asked for one relaxed load per buffer before pushing into the slot appsrc, so two sessions with different ts_offsets cannot feed one appsrc. That is what landed: backend/src/blocks/builtin/whip.rs:1121 — if session_finished.load(Ordering::Relaxed) {. The order is right at the other end too: the flag is won with a swap before the cleanup request is sent, so the displaced session stops pushing at the moment of the decision, not when teardown eventually runs.

2. The 3-second hold on a refused POST. backend/src/whip_session_manager.rs:583 — } else if !candidate.has_audio { answers immediately instead of watching a counter that was never going to move. That is the case I raised as a regression against main, and it is gone.

Claims

Claim	Verdict	Evidence
The guard test would fail if the guard were removed	CONFIRMED	backend/src/blocks/builtin/whip.rs:2095 — fn a_finished_session_stops_feeding_its_slot() { calls the real forward_sample_to_slot rather than rebuilding it, and asserts the first sample out of the slot has the current session's PTS; without the check the displaced sample arrives first and the assertion fails
"Sessions with audio sort first, so no session on this endpoint can be displaced"	CONFIRMED	backend/src/whip_session_manager.rs:651 — ` .max_by_key(
The per-buffer cost stays within the CLAUDE.md hot-path rules	CONFIRMED	backend/src/whip_session_manager.rs:190 — pub fn touch(&self, is_audio: bool) { is one Instant::elapsed and one or two relaxed stores; the media type is resolved once at backend/src/blocks/builtin/whip.rs:1036 — let pad_is_audio = media_type == "audio";, not per buffer
New manager tests execute on CI	CONFIRMED	Check (Linux) job 104072334089: a_sparse_video_only_session_is_not_displaced, a_session_that_has_not_delivered_media_yet_is_not_displaced, a_dead_session_is_displaced_past_a_quieter_video_only_one and a_finished_session_stops_feeding_its_slot all ... ok

Diagnosis — The added audio precondition is the more interesting change and it is the right call: a video-only publisher can legitimately go longer than TAKEOVER_IDLE_THRESHOLD between frames, so idle time alone is not evidence of a dead transport there. Coverage is BOUNDED, and the bound is named in the code itself — a session whose audio died for good while sparse video continues can still be displaced.

The relaxed load can lag the SeqCst store by a hair, so a sample or two may still cross. That is harmless and deliberate: the slot is not handed over until the cleanup task runs, which is orders of magnitude later, and a stronger ordering on the per-buffer path would cost more than it buys.

Radius — SHARED: every WHIP Input seat plus the ingest handler. Argued for approval because the diff only ever narrows when a session is displaced — a sitting session that survived before still survives.

Tests & CI — Run 34872718857 at 20007d57: Check (Linux), both Build (Linux) jobs, Check & Build (WASM) and API Contract Check pass; macOS and Windows skipping.

Merge order — #759 edits this same callback. Land #759 first; re-applying this one is the smaller move.

Confidence: HIGH