diff --git a/src-tauri/src/player/lock_order.rs b/src-tauri/src/player/lock_order.rs new file mode 100644 index 000000000..72a74d573 --- /dev/null +++ b/src-tauri/src/player/lock_order.rs @@ -0,0 +1,268 @@ +//! A declared lock hierarchy for [`PlayerController`], and a tripwire that +//! enforces it. +//! +//! The controller carries seventeen separate mutexes, reached from the MPV event +//! loop, JNI callbacks, sleep/autoplay timers, the session poller and every IPC +//! command. Nothing about that arrangement prevents two threads taking the same +//! two locks in opposite orders, which deadlocks the player outright — and this +//! subsystem has already produced one deadlock (a tokio `MutexGuard` held in a +//! `match` scrutinee, which stalled the `AdvanceToNext` arm). +//! +//! Today the code is disciplined: acquisitions are scoped, and `previous()` for +//! instance explicitly drops the backend guard before touching the queue. But +//! that holds by convention, and convention is not checked. [`LOCK_ORDER`] +//! writes the convention down and `every_overlapping_acquisition_respects_the_order` +//! fails the build when a change breaks it. +//! +//! Ordering only matters where one guard is **still held** while another lock is +//! taken. Acquiring two locks one after another, each released before the next, +//! cannot deadlock — so the analysis looks for overlap, not for mere sequence. +//! +//! TRACES: UR-005 | DR-052 + +// This module is a static analysis of `player/mod.rs` plus the hierarchy it +// checks against. Its only caller is its own test module, but `LOCK_ORDER` is +// the documentation of record for how these locks nest, so it stays compiled +// (and rustdoc'd) rather than hidden behind `cfg(test)`. +#![allow(dead_code)] + +/// The order in which `PlayerController`'s locks may be nested. +/// +/// A thread already holding one of these may only acquire a lock that appears +/// **later** in this list. The order is not arbitrary — it follows the nesting +/// the code already relies on: +/// +/// - `repository` and `sleep_timer` are taken by long-running decisions that go +/// on to consult playback state, so they sit outermost. +/// - `backend` outranks `queue`: "what is playing" is read before "what is +/// next", never the reverse. +/// - `event_emitter` is last. Emitting is a leaf — notifying the frontend must +/// never reach back for more player state. +/// +/// TRACES: UR-005 | DR-052 +pub const LOCK_ORDER: &[&str] = &[ + "repository", + "sleep_timer", + "countdown_cancel", + "jellyfin_client", + "backend", + "queue", + "stream_resume", + "end_reason", + "reported_time", + "background_audio_active", + "background_audio_base", + "html5_playing", + "autoplay_settings", + "autoplay_episode_count", + "reports", + "event_emitter", +]; + +/// Rank of `field` in [`LOCK_ORDER`], or `None` if it is not a declared lock. +pub fn rank(field: &str) -> Option { + LOCK_ORDER.iter().position(|f| *f == field) +} + +/// One lock acquired while another is still held. +#[derive(Debug, PartialEq, Eq)] +pub struct Overlap { + /// The lock already held. + pub outer: String, + /// The lock acquired underneath it. + pub inner: String, + /// 1-indexed line of the inner acquisition, for a useful failure message. + pub line: usize, +} + +/// Find every place `src` takes a lock while holding another. +/// +/// Deliberately simple and line-based: it tracks `let … = self.FIELD.lock_safe()` +/// bindings and looks for a different `self.OTHER.lock_safe()` before the +/// binding goes out of scope or is explicitly dropped. A guard that is not bound +/// to a name (`*self.flag.lock_safe() = false;`) is released at the end of its +/// statement and cannot overlap anything, so it is only ever an *inner* +/// acquisition here. +/// +/// TRACES: UR-005 | DR-052 | UT-052 +pub fn overlapping_acquisitions(src: &str) -> Vec { + let lines: Vec<&str> = src.lines().collect(); + let mut found = Vec::new(); + + for (i, line) in lines.iter().enumerate() { + let Some((var, field)) = parse_binding(line) else { + continue; + }; + let indent = line.len() - line.trim_start().len(); + + for (j, later) in lines.iter().enumerate().skip(i + 1) { + if later.contains(&format!("drop({var})")) { + break; + } + let trimmed = later.trim(); + if trimmed.is_empty() { + continue; + } + // Left the block the guard lives in. + let later_indent = later.len() - later.trim_start().len(); + if later_indent < indent && !trimmed.starts_with(['.', ')', '}']) { + break; + } + if trimmed == "}" && later_indent < indent { + break; + } + if let Some(inner) = parse_acquisition(later, field) { + found.push(Overlap { + outer: field.to_string(), + inner, + line: j + 1, + }); + break; + } + } + } + found +} + +/// `let [mut] name = self.field.lock_safe()` → `(name, field)`. +fn parse_binding(line: &str) -> Option<(&str, &str)> { + let rest = line.trim_start().strip_prefix("let ")?; + let rest = rest.strip_prefix("mut ").unwrap_or(rest); + let (name, rest) = rest.split_once(" = self.")?; + let (field, _) = rest.split_once(".lock_safe()")?; + if name.contains(' ') || field.contains('.') { + return None; + } + Some((name, field)) +} + +/// The first `self.other.lock_safe()` on `line` that is not `held`. +fn parse_acquisition(line: &str, held: &str) -> Option { + let mut search = line; + while let Some(at) = search.find("self.") { + let after = &search[at + 5..]; + if let Some((field, _)) = after.split_once(".lock_safe()") { + if !field.contains(['.', '(', ' ']) && field != held { + return Some(field.to_string()); + } + } + search = after; + } + None +} + +// TRACES: UR-005 | DR-052 | UT-052 +#[cfg(test)] +mod tests { + use super::*; + + /// **The tripwire.** Every nested acquisition in the controller must follow + /// [`LOCK_ORDER`]. + /// + /// A violation is a lock-order inversion: two threads taking the same pair + /// in opposite orders deadlock the player, and the symptom is a frozen app + /// with no error anywhere. + /// + /// TRACES: UR-005 | DR-052 | UT-052 + #[test] + fn every_overlapping_acquisition_respects_the_order() { + let src = include_str!("mod.rs"); + let mut violations = Vec::new(); + + for overlap in overlapping_acquisitions(src) { + let (Some(outer), Some(inner)) = (rank(&overlap.outer), rank(&overlap.inner)) else { + violations.push(format!( + "player/mod.rs:{} takes '{}' while holding '{}', and one of them \ + is not declared in LOCK_ORDER", + overlap.line, overlap.inner, overlap.outer + )); + continue; + }; + if outer >= inner { + violations.push(format!( + "player/mod.rs:{} takes '{}' (rank {inner}) while holding '{}' \ + (rank {outer}) — an inversion against LOCK_ORDER", + overlap.line, overlap.inner, overlap.outer + )); + } + } + + assert!( + violations.is_empty(), + "lock-order inversions in PlayerController:\n {}\n\nEither reorder the \ + acquisitions or, if the new order is the correct one, change LOCK_ORDER \ + and re-check every other site.", + violations.join("\n ") + ); + } + + /// The analysis must actually see the nesting the controller does today, + /// or the tripwire above passes by finding nothing. + /// + /// TRACES: UR-005 | DR-052 | UT-052 + #[test] + fn the_analysis_finds_the_nesting_that_exists() { + let found = overlapping_acquisitions(include_str!("mod.rs")); + assert!( + found.len() >= 5, + "expected the controller's known nested acquisitions, found {found:?}" + ); + assert!( + found + .iter() + .any(|o| o.outer == "backend" && o.inner == "queue"), + "the backend->queue nesting in state() should be detected: {found:?}" + ); + } + + /// A guard held across a lock taken in the wrong order must be caught. + /// + /// TRACES: UR-005 | DR-052 | UT-052 + #[test] + fn an_inversion_is_detected() { + let src = " fn bad(&self) {\n\ + \x20 let queue = self.queue.lock_safe();\n\ + \x20 let b = self.backend.lock_safe();\n\ + \x20 }\n"; + let found = overlapping_acquisitions(src); + assert_eq!(found.len(), 1, "{found:?}"); + assert_eq!(found[0].outer, "queue"); + assert_eq!(found[0].inner, "backend"); + assert!(rank("queue").unwrap() > rank("backend").unwrap()); + } + + /// Sequential, non-overlapping acquisitions cannot deadlock and must not be + /// reported — `previous()` drops the backend guard before taking the queue. + /// + /// TRACES: UR-005 | DR-052 | UT-052 + #[test] + fn a_dropped_guard_is_not_an_overlap() { + let src = " fn fine(&self) {\n\ + \x20 let backend = self.backend.lock_safe();\n\ + \x20 drop(backend);\n\ + \x20 let queue = self.queue.lock_safe();\n\ + \x20 }\n"; + assert!(overlapping_acquisitions(src).is_empty()); + } + + /// Every declared lock name must be a real field, or the order documents + /// something that no longer exists. + /// + /// TRACES: UR-005 | DR-052 | UT-052 + #[test] + fn every_declared_lock_is_a_real_field() { + let src = include_str!("mod.rs"); + let decl = src + .split_once("pub struct PlayerController {") + .expect("PlayerController struct") + .1; + let decl = decl.split_once("\n}").expect("end of struct").0; + + for name in LOCK_ORDER { + assert!( + decl.contains(&format!("{name}:")), + "LOCK_ORDER names '{name}', which is not a PlayerController field" + ); + } + } +} diff --git a/src-tauri/src/player/mod.rs b/src-tauri/src/player/mod.rs index 083a515cd..96238936f 100644 --- a/src-tauri/src/player/mod.rs +++ b/src-tauri/src/player/mod.rs @@ -28,6 +28,10 @@ pub mod track_switch; #[cfg(test)] mod mpv_backend_test; +// The declared lock hierarchy for `PlayerController` below, and the tripwire +// that enforces it. See the module docs for why seventeen mutexes need one. +pub mod lock_order; + // Panic containment for the JNI boundary. Not gated on the target: the guard // and its tripwire test are exercised on the host, where `android` never builds. pub mod jni_guard;