diff --git a/src/audio/ownership.rs b/src/audio/ownership.rs index ffe5c4a..11e0a7f 100644 --- a/src/audio/ownership.rs +++ b/src/audio/ownership.rs @@ -173,16 +173,22 @@ fn pipewire_props_value(node_name: &str) -> String { /// ALSA output. Setting the ones the child ignores costs nothing. /// /// An inherited value is **merged** rather than replaced (round 10, R10-5). -/// Both variables can legitimately carry a user's own routing policy — +/// All three can legitimately carry a user's own routing policy — /// `media.role`, a target sink — and clobbering it changes where the user's /// audio goes as a side effect of a tagging mechanism that is supposed to be -/// behaviourally invisible. Our pairs go **last**, so they win any duplicate -/// key. An inherited value that does not match the expected shape is logged -/// and overwritten: a half-merged string that fails to parse would drop the -/// tag silently, which is the one outcome worse than losing the user's -/// routing preference. +/// behaviourally invisible. An inherited value that does not match the +/// expected shape is logged and overwritten: a half-merged string that fails +/// to parse would drop the tag silently, which is the one outcome worse than +/// losing the user's routing preference. /// -/// ⚠️ Reachability, measured 2026-07-25: neither variable is set anywhere in +/// ✅ **Our pairs go last, and last wins — measured 2026-07-25**, not assumed. +/// `PIPEWIRE_PROPS='{ "node.name" = "theirs_first", "media.role" = "music", +/// "node.name" = "ours_last" }'` on `pw-play` produced `node.name=ours_last` +/// with `media.role` preserved, and the `PULSE_PROP` equivalent on `paplay` +/// did the same. So a user who already sets `node.name` cannot silently +/// untag us, and their other keys survive. +/// +/// ⚠️ Reachability, measured 2026-07-25: none of the three is set anywhere in /// this user's environment or configuration, so this is a correctness /// property with no live consumer today. pub fn tag_child(command: &mut Command, role: &str) { @@ -191,13 +197,23 @@ pub fn tag_child(command: &mut Command, role: &str) { PULSE_PROP_ENV, merge_pulse_prop(inherited(PULSE_PROP_ENV).as_deref(), &node_name), ); - let pipewire_props = merge_pipewire_props(inherited(PIPEWIRE_PROPS_ENV).as_deref(), &node_name); + command.env( + PIPEWIRE_PROPS_ENV, + merge_pipewire_props(inherited(PIPEWIRE_PROPS_ENV).as_deref(), &node_name), + ); // The ALSA carrier takes the same SPA-JSON grammar and is set here too, so // a child configured for ALSA output — which neither of the other two - // variables reaches — is tagged with *its own* role rather than inheriting - // this process's `clip` tag from `tag_this_process_alsa_audio`. - command.env(PIPEWIRE_ALSA_ENV, &pipewire_props); - command.env(PIPEWIRE_PROPS_ENV, pipewire_props); + // variables reaches — is tagged at all, and with *its own* role. + // + // Merged from the inherited `PIPEWIRE_ALSA`, which by now holds this + // process's own `clip` tag from [`tag_this_process_alsa_audio`] plus + // whatever the user set. The duplicate `node.name` that produces is + // resolved in our favour by the measured last-wins rule above, so the + // child gets its own role rather than inheriting `clip`. + command.env( + PIPEWIRE_ALSA_ENV, + merge_pipewire_props(inherited(PIPEWIRE_ALSA_ENV).as_deref(), &node_name), + ); } /// This process's value for `key`, which is what a child would inherit. @@ -301,18 +317,20 @@ fn merge_pipewire_props(inherited: Option<&str>, node_name: &str) -> String { /// of `main`, before anything is spawned; that is also correct on the merits, /// since the plugin reads the variable when a stream is opened. /// -/// Inherited values are **replaced**. Unlike the `Command` carriers (R10-5), -/// there is nothing to preserve: no user sets `PIPEWIRE_ALSA` to route -/// peerspeak's own clip playback, and honouring one would defeat the tag. +/// An inherited value is **merged**, on the same reasoning as [`tag_child`]'s +/// (R10-5): a user's `PIPEWIRE_ALSA` can carry routing policy, and this +/// function is not entitled to move their audio as a side effect of tagging +/// ours. Ours goes last and last wins (measured — see [`tag_child`]). /// /// # Safety /// /// The caller must guarantee no other thread exists in this process. pub unsafe fn tag_this_process_alsa_audio() { let node_name = owned_node_name(CLIP_ROLE); + let value = merge_pipewire_props(inherited(PIPEWIRE_ALSA_ENV).as_deref(), &node_name); // SAFETY: the caller's obligation, discharged by calling this at the top // of `main` before any thread is spawned. - unsafe { std::env::set_var(PIPEWIRE_ALSA_ENV, pipewire_props_value(&node_name)) }; + unsafe { std::env::set_var(PIPEWIRE_ALSA_ENV, value) }; } /// Live-test support for the phase-1 exit gate, shared by the two modules @@ -717,12 +735,39 @@ mod tests { let alsa = env.get(PIPEWIRE_ALSA_ENV).expect("PIPEWIRE_ALSA set"); assert!( alsa.contains(&format!("\"node.name\" = \"{}\"", owned_node_name("mpv"))), - "the child's own role, not this process's clip role: {alsa}" + "the child's own role: {alsa}" ); - assert!(!alsa.contains(&format!("_{CLIP_ROLE}_")), "{alsa}"); assert!(alsa.contains(&format!("\"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\""))); } + /// ⚠️ In production `main` has already set `PIPEWIRE_ALSA` to this + /// process's `clip` tag by the time a player is spawned, so `tag_child` + /// merges into *that*, not into nothing. The test binary has no such + /// value, so the row above never exercises the real shape — this one does, + /// on the merge function directly. + /// + /// The child's `node.name` must come last, because last wins (measured): + /// otherwise a spawned mpv would report itself as `clip` and the audit + /// could not tell the incoming screenshare's audio from a chat clip. + #[test] + fn a_childs_role_overrides_the_inherited_clip_tag_in_the_alsa_carrier() { + let clip = pipewire_props_value(&owned_node_name(CLIP_ROLE)); + let child = owned_node_name("mpv"); + let merged = merge_pipewire_props(Some(&clip), &child); + + let last_name = merged + .rfind("\"node.name\" = ") + .expect("a node.name pair in the merged value"); + assert!( + merged[last_name..].contains(&child), + "the child's role must be the last node.name: {merged}" + ); + // Both are still peerspeak-owned, so the duplicate is harmless. + assert!(merged.contains(&format!("\"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\""))); + assert!(merged.starts_with('{') && merged.ends_with('}'), "{merged}"); + assert_eq!(merged.matches('{').count(), 1, "one object only: {merged}"); + } + #[test] fn every_generated_name_matches_the_prefix_pixelpass_looks_for() { for role in ["call", "mpv", "vlc", "notify"] {