Breadway/breadcast

Fork 0

Stale DLNA SessionEnded from an untracked bus-watcher thread can kill (or wrongly tear down) the next cast #4

New issue

Closed

opened 2026-09-05 01:59:27 +08:00 by Breadway · 1 comment

Breadway commented

2026-09-05 01:59:27 +08:00

Owner

What's broken

breadcastd/src/dlna_mirror.rs's DLNA session start spawns a bare std::thread to watch the GStreamer bus (around line 190-200):

std::thread::spawn(move || {
    match run_until_eos_or_error(&pipeline_watch) {
        Ok(outcome) => tracing::debug!(?outcome, "DLNA encode pipeline bus watcher ended"),
        Err(e) => tracing::error!(error = ?e, "DLNA encode pipeline error"),
    }
    let _ = daemon_tx.blocking_send(DaemonCommand::SessionEnded { generation });
});

This JoinHandle is discarded — it's not stored on DlnaMirrorSession, and DlnaMirrorSession::stop() (lines 216-241) never joins or bounds it. stop() sets the pipeline to Null, stops the renderer, tears down HTTP/portal capture, and returns — with no guarantee the bus-watcher thread has noticed the Null transition and sent (or finished sending) its SessionEnded yet. run_until_eos_or_error polls the bus roughly every 500ms (breadcast-core/src/pipeline/mod.rs:544-556), so that message can land an unbounded (if short) time after stop() has already returned to the caller.

Compare CastMirrorSession: its equivalent notification comes from the frame_pump thread, which is stored on the struct and explicitly joined (bounded 5s, cast_mirror.rs:287) inside stop() before stop() returns. DLNA has no equivalent guarantee.

Compounding this, daemon.rs's StopCast handler (lines 151-165) only bumps start_generation when a start is still in flight (self.starting):

DaemonCommand::StopCast { reply } => {
    if self.starting {
        self.starting = false;
        self.start_generation = self.start_generation.wrapping_add(1);
    }
    if let Some(session) = self.active_session.take() {
        session.stop().await;
        ...
    }
    ...
}

When it stops an already-installed session (the normal "user clicks Stop" case), start_generation is left unchanged. start_cast (line 249) then reads the current start_generation for the next start without bumping it first either. So a stop→start sequence on an installed DLNA session keeps the exact same generation number throughout, and SessionEnded's staleness check (daemon.rs:171, if generation != self.start_generation { return; }) can't tell the old thread's message apart from a legitimate one for the new session.

Why it matters — concrete failure scenario

User stops a DLNA cast, then starts a new one (same or a different device) within a few seconds — an entirely ordinary UI interaction. Negotiation for the new start (portal picker + wait_for_playlist_segments waiting for 2 HLS segments at ~1s each) easily takes several seconds, comfortably longer than the old session's bus-watcher thread needs to notice Null and fire its stale SessionEnded{generation}. Two live failure modes, both reachable because the generation is unchanged:

Stale message arrives while the new start is in flight: daemon.rs:200-204 treats it as "the in-flight session died," setting self.starting = false and bumping the generation — so the legitimate upcoming StartFinished for the new session gets rejected as "start cancelled" and the working new session is torn down, even though negotiation actually succeeded.
Stale message arrives after the new session is already installed: daemon.rs:180-187 matches the (unchanged/reused) generation, active_session.take()s the brand-new, actively-casting session, and tears it down — the TV/renderer that had just started successfully mirroring drops out seconds later with no user action, right after a working stop→start.

Suggested fix

Either (or both):

(a) Store the DLNA bus-watcher thread's JoinHandle on DlnaMirrorSession and join/bound it inside stop() the same way CastMirrorSession::stop() bounds frame_pump via its existing join pattern, so the notification's enqueue-ordering relative to stop() completion becomes deterministic (same fix shape already proven for the Cast path).
(b) Make StopCast unconditionally bump start_generation whenever it takes an installed active_session, mirroring what the SessionEnded handler itself already does at daemon.rs:183 when it tears down a session that ended on its own — so a generation number can never be reused across any stop→start sequence, regardless of which protocol (Cast/DLNA) is involved.

(b) is the smaller, more robust fix since it closes the gap for DLNA without relying on thread-join timing, and doesn't regress if a future third mirroring backend makes the same untracked-thread choice DLNA did.

## What's broken `breadcastd/src/dlna_mirror.rs`'s DLNA session start spawns a bare `std::thread` to watch the GStreamer bus (around line 190-200): ```rust std::thread::spawn(move || { match run_until_eos_or_error(&pipeline_watch) { Ok(outcome) => tracing::debug!(?outcome, "DLNA encode pipeline bus watcher ended"), Err(e) => tracing::error!(error = ?e, "DLNA encode pipeline error"), } let _ = daemon_tx.blocking_send(DaemonCommand::SessionEnded { generation }); }); ``` This `JoinHandle` is discarded — it's not stored on `DlnaMirrorSession`, and `DlnaMirrorSession::stop()` (lines 216-241) never joins or bounds it. `stop()` sets the pipeline to `Null`, stops the renderer, tears down HTTP/portal capture, and returns — with no guarantee the bus-watcher thread has noticed the `Null` transition and sent (or finished sending) its `SessionEnded` yet. `run_until_eos_or_error` polls the bus roughly every 500ms (`breadcast-core/src/pipeline/mod.rs:544-556`), so that message can land an unbounded (if short) time after `stop()` has already returned to the caller. Compare `CastMirrorSession`: its equivalent notification comes from the `frame_pump` thread, which **is** stored on the struct and explicitly joined (bounded 5s, `cast_mirror.rs:287`) inside `stop()` before `stop()` returns. DLNA has no equivalent guarantee. Compounding this, `daemon.rs`'s `StopCast` handler (lines 151-165) only bumps `start_generation` when a start is still *in flight* (`self.starting`): ```rust DaemonCommand::StopCast { reply } => { if self.starting { self.starting = false; self.start_generation = self.start_generation.wrapping_add(1); } if let Some(session) = self.active_session.take() { session.stop().await; ... } ... } ``` When it stops an already-installed session (the normal "user clicks Stop" case), `start_generation` is left unchanged. `start_cast` (line 249) then reads the current `start_generation` for the *next* start without bumping it first either. So a stop→start sequence on an installed DLNA session keeps the exact same generation number throughout, and `SessionEnded`'s staleness check (`daemon.rs:171`, `if generation != self.start_generation { return; }`) can't tell the old thread's message apart from a legitimate one for the new session. ## Why it matters — concrete failure scenario User stops a DLNA cast, then starts a new one (same or a different device) within a few seconds — an entirely ordinary UI interaction. Negotiation for the new start (portal picker + `wait_for_playlist_segments` waiting for 2 HLS segments at ~1s each) easily takes several seconds, comfortably longer than the old session's bus-watcher thread needs to notice `Null` and fire its stale `SessionEnded{generation}`. Two live failure modes, both reachable because the generation is unchanged: 1. **Stale message arrives while the new start is in flight:** `daemon.rs:200-204` treats it as "the in-flight session died," setting `self.starting = false` and bumping the generation — so the *legitimate* upcoming `StartFinished` for the new session gets rejected as "start cancelled" and the working new session is torn down, even though negotiation actually succeeded. 2. **Stale message arrives after the new session is already installed:** `daemon.rs:180-187` matches the (unchanged/reused) generation, `active_session.take()`s the **brand-new, actively-casting session**, and tears it down — the TV/renderer that had just started successfully mirroring drops out seconds later with no user action, right after a working stop→start. ## Suggested fix Either (or both): - (a) Store the DLNA bus-watcher thread's `JoinHandle` on `DlnaMirrorSession` and join/bound it inside `stop()` the same way `CastMirrorSession::stop()` bounds `frame_pump` via its existing join pattern, so the notification's enqueue-ordering relative to `stop()` completion becomes deterministic (same fix shape already proven for the Cast path). - (b) Make `StopCast` unconditionally bump `start_generation` whenever it takes an installed `active_session`, mirroring what the `SessionEnded` handler itself already does at `daemon.rs:183` when it tears down a session that ended on its own — so a generation number can never be reused across any stop→start sequence, regardless of which protocol (Cast/DLNA) is involved. (b) is the smaller, more robust fix since it closes the gap for DLNA without relying on thread-join timing, and doesn't regress if a future third mirroring backend makes the same untracked-thread choice DLNA did.

Breadway added the

bug

label

2026-09-05 01:59:28 +08:00

Breadway commented

2026-09-05 02:19:29 +08:00

Author

Owner

Confirmed root cause against real code and fixed. PR: #5 (branch fix/cast-4-dlna-stale-session-ended)

Root cause (both bugs confirmed):

breadcastd/src/daemon.rs's StopCast handler only bumped start_generation when cancelling an in-flight start (self.starting); it left the generation unchanged when tearing down an already-installed session (the ordinary "user clicks Stop" path). A stop → start on an installed session therefore kept the same generation throughout.
breadcastd/src/dlna_mirror.rs's bus-watcher thread (start(), then around lines 190-200) spawned via bare std::thread::spawn with its JoinHandle discarded — never stored on DlnaMirrorSession, never joined in stop(). stop() could return before the thread noticed the pipeline hit Null and sent its SessionEnded, unlike CastMirrorSession's frame_pump, which is stored and joined (bounded 5s) in its stop().

Fix (both parts, as the issue suggested):

daemon.rs: StopCast now unconditionally bumps start_generation whenever it takes an installed active_session, mirroring what the SessionEnded handler itself already does when a session ends on its own (daemon.rs:183). Since the generation is captured by value in the watcher thread at spawn time, this alone makes the staleness check correctly reject any late message from a superseded session, regardless of timing.
dlna_mirror.rs: the bus-watcher thread's JoinHandle is now stored on DlnaMirrorSession and joined (bounded 5s) in stop(), right after pipeline.set_state(Null) unblocks it — same pattern as CastMirrorSession::stop()'s frame_pump join. Brings DLNA to parity with Cast and closes the thread-leak/non-determinism gap.

Test: added a deterministic regression test in daemon.rs (stop_cast_on_an_installed_session_bumps_generation_so_a_stale_watcher_thread_cannot_kill_the_next_session) that drives stop → install-new-session → stale-SessionEnded directly through Daemon::handle via a #[cfg(test)]-only ActiveSession::Stub (a real session needs GStreamer/portal, can't be built in a unit test). No sleeps, no real threads.

cargo build --release --workspace, cargo test --release --workspace (17 passed / 0 failed), and cargo clippy --release --workspace --all-targets -- -D warnings all clean.

Confirmed root cause against real code and fixed. PR: https://git.breadway.dev/Breadway/breadcast/pulls/5 (branch `fix/cast-4-dlna-stale-session-ended`) **Root cause (both bugs confirmed):** 1. `breadcastd/src/daemon.rs`'s `StopCast` handler only bumped `start_generation` when cancelling an in-flight start (`self.starting`); it left the generation unchanged when tearing down an already-*installed* session (the ordinary "user clicks Stop" path). A stop → start on an installed session therefore kept the same generation throughout. 2. `breadcastd/src/dlna_mirror.rs`'s bus-watcher thread (`start()`, then around lines 190-200) spawned via bare `std::thread::spawn` with its `JoinHandle` discarded — never stored on `DlnaMirrorSession`, never joined in `stop()`. `stop()` could return before the thread noticed the pipeline hit `Null` and sent its `SessionEnded`, unlike `CastMirrorSession`'s `frame_pump`, which is stored and joined (bounded 5s) in its `stop()`. **Fix (both parts, as the issue suggested):** - `daemon.rs`: `StopCast` now unconditionally bumps `start_generation` whenever it takes an installed `active_session`, mirroring what the `SessionEnded` handler itself already does when a session ends on its own (`daemon.rs:183`). Since the generation is captured by value in the watcher thread at spawn time, this alone makes the staleness check correctly reject any late message from a superseded session, regardless of timing. - `dlna_mirror.rs`: the bus-watcher thread's `JoinHandle` is now stored on `DlnaMirrorSession` and joined (bounded 5s) in `stop()`, right after `pipeline.set_state(Null)` unblocks it — same pattern as `CastMirrorSession::stop()`'s `frame_pump` join. Brings DLNA to parity with Cast and closes the thread-leak/non-determinism gap. **Test:** added a deterministic regression test in `daemon.rs` (`stop_cast_on_an_installed_session_bumps_generation_so_a_stale_watcher_thread_cannot_kill_the_next_session`) that drives stop → install-new-session → stale-`SessionEnded` directly through `Daemon::handle` via a `#[cfg(test)]`-only `ActiveSession::Stub` (a real session needs GStreamer/portal, can't be built in a unit test). No sleeps, no real threads. `cargo build --release --workspace`, `cargo test --release --workspace` (17 passed / 0 failed), and `cargo clippy --release --workspace --all-targets -- -D warnings` all clean.

Breadway closed this issue

2026-09-05 09:24:05 +08:00

No labels

bug

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Breadway/breadcast#4

No description provided.

Rows
Columns