From 985c63806bf0cef5bd3f2f1a4ffcdc2cd8b886a4 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sat, 25 Jul 2026 23:51:02 -0400 Subject: [PATCH] audio/ownership: state the playlist policy, and gate main's ordering properly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 10 review, findings 2 and 5. Finding 2 — R10-2's rationale for tagging local playlist audio was factually wrong. It claimed a local track is "already being broadcast to peers on the same keypress", but shared listening is opt-in: music_broadcast defaults to false, play_music_index starts local playback unconditionally, and broadcast_track returns immediately when can_broadcast_music is false. So a default-config playlist is not already broadcast. The tag stays, now as an explicit policy with the real reason: the carriers reach rodio through PIPEWIRE_ALSA, which is process-wide, and clip_player and music_player are two ClipPlayer instances in one process — no value of that variable can tag one and not the other. Exempting the playlist means giving it a separately taggable stream, which is a large change for a case with a one-step workaround (play it in any other app). Tagging is not optional for received clips and peer music, which are the far end's own audio. Finding 5 — the ordering test proved only "before run_gui", which a thread::spawn inserted above the tag still satisfies while making the set_var a data race. It now requires the tag to be the first executable statement in main: attributes, `unsafe` and block punctuation are stripped, and any residue fails. Mutation-verified against a spawn, an unrelated statement, and the call deleted. Co-Authored-By: Claude Opus 5 --- src/audio/ownership.rs | 94 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 85 insertions(+), 9 deletions(-) diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index 751a439..fbb1b0d 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -95,15 +95,41 @@ pub const PIPEWIRE_ALSA_ENV: &str = "PIPEWIRE_ALSA"; /// Role of in-process audio played through `rodio`: received chat clips, peer /// music, and locally chosen playlist tracks (round 10, R10-2). /// -/// **All three are tagged, deliberately**, including the local playlist. The -/// case for exempting locally chosen music is that the user picked it and may -/// want it shared; the case against is stronger. A local track played from the -/// playlist is *already being broadcast to peers over the call* on the same -/// keypress (`app::play_local_track` → `broadcast_track`), so sharing it a -/// second time through the screen-share sends the far end two copies of the -/// same audio, offset by the two paths' differing latency. That is not a lost -/// feature, it is a defect. A user who wants music in the share can play it in -/// any other application, which peerspeak never tags. +/// # Policy: peerspeak's own playlist audio is never screen-shareable +/// +/// **All three are tagged, and for the local playlist that is a deliberate +/// policy rather than a consequence** (round 10 review, finding 2). +/// +/// ⚠️ An earlier version of this comment justified it by claiming a local +/// track is "already being broadcast to peers on the same keypress", so +/// sharing it again would send the far end two copies. **That claim is false +/// in the default configuration** and Codex was right to call it: shared +/// listening is opt-in, `Config::music_broadcast` defaults to `false` (pinned +/// by `config::tests::test_input_output_volume_fields`), +/// `app::play_music_index` starts local playback unconditionally, and +/// `app::broadcast_track` returns immediately when +/// `can_broadcast_music` is false. With broadcasting off there is no second +/// copy and no defect — only a track the user chose that they cannot put in +/// the share. +/// +/// The policy stands anyway, on cost rather than on that argument: +/// +/// - The two carriers reach `rodio` through [`PIPEWIRE_ALSA_ENV`], which is +/// **process-wide**. `clip_player` and `music_player` are two `ClipPlayer` +/// instances in one process (`app::AppState`), so no value of that variable +/// can tag one and not the other, and `set_var` cannot be flipped at +/// runtime once threads exist. +/// - Exempting the playlist therefore means giving it a separately taggable +/// stream — a helper process, or rebuilding `ClipPlayer` on peerspeak's own +/// PipeWire backend — for a case with a one-step workaround. +/// - Tagging is **not** optional for the other two roles: received clips and +/// peer music are the far end's own audio, and re-sharing them is the exact +/// echo this feature exists to prevent. +/// +/// So: a user who wants music inside the screen share plays it in any other +/// application, which peerspeak never tags. When shared listening *is* on, +/// the original two-copies argument does apply, and the policy is simply +/// right for that case too. pub const CLIP_ROLE: &str = "clip"; /// Role of the native call-playback stream — the node carrying the far end's @@ -809,6 +835,16 @@ mod tests { /// A source-text assertion is crude. It is also the *only* check available /// short of driving the real GUI binary, and phase 1's miss was precisely /// a call site nobody verified existed. + /// + /// ⚠️ **The check is "first statement in `main`", not "before + /// `run_gui`"** (round 10 review, finding 5). Ordering against `run_gui` + /// proved far less than the safety obligation needs: inserting a + /// long-lived `std::thread::spawn` above the tag still satisfied it, while + /// making the `set_var` a data race — the exact thing the `unsafe` block's + /// SAFETY comment claims cannot happen. `set_var` is sound only while the + /// process is single-threaded, so the property to gate is that **nothing + /// executable precedes the call**, which is checkable and is what `main` + /// actually does today. #[test] fn main_tags_this_process_before_anything_starts() { const MAIN: &str = include_str!("../main.rs"); @@ -829,6 +865,46 @@ mod tests { let call = code .find("tag_this_process_alsa_audio()") .expect("main must call tag_this_process_alsa_audio (round 10, R10-2)"); + + let body = code + .find("fn main()") + .and_then(|start| code[start..].find('{').map(|open| start + open + 1)) + .expect("main has a body"); + // Back up over the module path the call is written with, so the + // statement's own `peerspeak::audio::ownership::` is not mistaken for + // code preceding it. + let call_start = code[..call] + .rfind(|c: char| !(c.is_alphanumeric() || c == '_' || c == ':')) + .map_or(0, |index| index + 1); + assert!( + call_start > body, + "the call must be inside main, not above it" + ); + + // Everything between `main`'s opening brace and the call, with the + // syntax that cannot *run* removed: attributes, the `unsafe` keyword, + // block punctuation, whitespace. Anything left is a statement that + // executes before the tag — which is what must not exist. + let mut before = code[body..call_start].to_string(); + while let Some(start) = before.find("#[") { + let end = before[start..] + .find(']') + .map(|offset| start + offset + 1) + .expect("an attribute closes"); + before.replace_range(start..end, ""); + } + let residue: String = before + .replace("unsafe", "") + .chars() + .filter(|c| !c.is_whitespace() && !matches!(c, '{' | '}')) + .collect(); + assert!( + residue.is_empty(), + "nothing may execute before the tag; found {residue:?} in main" + ); + + // And it must still be the real `main` of a program that starts the + // GUI, or the check above is asserting about the wrong function. let gui = code .find("run_gui") .expect("main runs the GUI; this test's ordering check assumes it");