diff --git a/src/host/observer/classify.rs b/src/host/observer/classify.rs index 299d2d2..62e3fd2 100644 --- a/src/host/observer/classify.rs +++ b/src/host/observer/classify.rs @@ -64,6 +64,11 @@ const HARDWARE_PCM_FACTORIES: &[&str] = &[ "api.alsa.pcm.source", ]; +/// The `device.api` every entry in [`HARDWARE_PCM_FACTORIES`] belongs to. +/// A single value rather than a list, because the allowlist is ALSA-only; +/// this constant is the thing to change when that stops being true. +const HARDWARE_PCM_API: &str = "alsa"; + /// ALSA drivers that expose a hardware-PCM `factory.name` but are **not** /// passive terminals — audio written in reappears on their capture side /// through a path the PipeWire Link graph cannot see, so classifying them @@ -199,8 +204,20 @@ pub fn classify(claim: &DeviceClaim, device: Option<&DeviceProps>) -> Classifica .flatten() .any(|d| NON_TERMINAL_ALSA_DRIVERS.contains(&d)); let driver_ok = driver.is_some() && !driver_denied; - let api_present = device.device_api.is_some() || claim.device_api.is_some(); - let is_hardware_pcm = api_present && on_factory_allowlist && driver_ok; + // The API must positively be the one the factory allowlist is written + // for, not merely present (Codex phase-3r review, finding 3). "Present" + // admitted `device.api=v4l2` alongside `factory.name=api.alsa.pcm.sink` + // — a contradiction no truthful configuration produces, which is exactly + // why it should be read as an observation gone wrong rather than as + // corroboration. Disagreement between the two sides fails closed for the + // same reason. ⚠️ Tied to [`HARDWARE_PCM_FACTORIES`] being ALSA-only: + // adding a BlueZ factory means allowing `bluez5` here too. + let api_ok = match (device.device_api.as_deref(), claim.device_api.as_deref()) { + (Some(from_device), Some(from_node)) if from_device != from_node => false, + (Some(api), _) | (None, Some(api)) => api == HARDWARE_PCM_API, + (None, None) => false, + }; + let is_hardware_pcm = api_ok && on_factory_allowlist && driver_ok; if is_hardware_pcm { Classification::SessionDevice } else { diff --git a/src/host/observer/mod.rs b/src/host/observer/mod.rs index 108ba50..87f5a27 100644 --- a/src/host/observer/mod.rs +++ b/src/host/observer/mod.rs @@ -596,21 +596,31 @@ impl RegistryModel { } /// The bound properties of the Device a node claims by global id, or - /// `None` when that claim is unresolved — which covers all three - /// fail-closed cases at once: no such Device observed, its bind still - /// outstanding, or **two live Devices sharing the recycled id**, where - /// there is no way to tell whose properties these are (v3.4 §6.1.3). + /// `None` when that claim is unresolved — which covers every fail-closed + /// case at once: no such Device observed, its bind still outstanding, or + /// **the id claimed by more than one live global**, where there is no way + /// to tell whose properties these are (v3.4 §6.1.3). + /// + /// ⚠️ The ambiguity test is "**exactly one** live global holds this id", + /// not "exactly one live *Device*" (Codex phase-3r review, finding 2). + /// The weaker test looks equivalent and is not: with `[Device, Port]` on + /// one id — a missed removal, the same precondition as every other + /// recycled-id hazard — it keeps answering with the older Device's + /// properties, so a node claiming that id holds a stale + /// `session_device = true`. That flag *removes* the node's owner keys and + /// its fail-closed backstop, so a forwarder wearing it can put its output + /// leg back on the eligible side: echo, from a lookup that was merely + /// looking at the wrong object type. fn device_props(&self, id: GlobalId) -> Option<&DeviceProps> { - let mut found: Option = None; - for slot in self.live_ids.get(&id)? { - if let Slot::Device(serial) = slot { - if found.is_some() { - return None; // Ambiguous ⇒ unresolved ⇒ withheld. - } - found = Some(*serial); - } + let slots = self.live_ids.get(&id)?; + if slots.len() != 1 { + return None; // Ambiguous ⇒ unresolved ⇒ withheld. } - self.devices.get(&found?)?.props.as_ref() + let Slot::Device(serial) = slots.front()? else { + // The id is live, but it is not a Device any more. + return None; + }; + self.devices.get(serial)?.props.as_ref() } /// Classify one node's device claim against the currently resolved diff --git a/src/host/observer/tests.rs b/src/host/observer/tests.rs index 87b4087..3b38e74 100644 --- a/src/host/observer/tests.rs +++ b/src/host/observer/tests.rs @@ -346,6 +346,53 @@ fn classify_device_api_from_either_side_corroborates() { ); } +#[test] +fn classify_contradictory_api_fails_closed() { + // Codex phase-3r review, finding 3. A `device.api` that is merely + // *present* is not corroboration: `v4l2` under an ALSA PCM factory is a + // contradiction, and the safe reading of a contradiction is "an + // observation went wrong", not "close enough". + let claim = DeviceClaim { + device_id: Some(gid(7)), + device_api: None, + factory_name: Some("api.alsa.pcm.sink".to_string()), + alsa_driver_name: Some("snd_hda_intel".to_string()), + }; + assert_eq!( + classify( + &claim, + Some(&device_with(Some("v4l2"), Some("snd_hda_intel"))) + ), + Classification::NotSessionDevice, + "a non-ALSA api under an ALSA factory must not corroborate" + ); + + // The two sides disagreeing fails closed for the same reason. + let disagreeing = DeviceClaim { + device_id: Some(gid(7)), + device_api: Some("bluez5".to_string()), + factory_name: Some("api.alsa.pcm.sink".to_string()), + alsa_driver_name: Some("snd_hda_intel".to_string()), + }; + assert_eq!( + classify(&disagreeing, Some(&alsa_device())), + Classification::NotSessionDevice, + "node and Device naming different APIs must fail closed" + ); + + // An empty value is not a value. + let empty = DeviceClaim { + device_id: Some(gid(7)), + device_api: Some(String::new()), + factory_name: Some("api.alsa.pcm.sink".to_string()), + alsa_driver_name: Some("snd_hda_intel".to_string()), + }; + assert_eq!( + classify(&empty, Some(&device_with(None, Some("snd_hda_intel")))), + Classification::NotSessionDevice + ); +} + #[test] fn classify_allowlist_is_exact_not_substring() { // A factory that merely *contains* an allowlisted name must not pass. @@ -1300,6 +1347,47 @@ fn model_removed_device_withholds_its_nodes_again() { assert!(!m.graph_ready(), "and it is an obligation again"); } +#[test] +fn model_device_id_shared_with_another_object_type_withholds() { + // Codex phase-3r review, finding 2 (certain). The ambiguity test has to + // be "exactly one live global holds this id", not "exactly one live + // *Device*": a Port recycling the id is the same missed-removal + // condition, and answering from the older Device leaves the claiming + // node wearing a stale `session_device = true` — which strips its owner + // keys and backstop, the difference between over-exclusion and echo. + let mut m = model(); + add_device(&mut m, 4200, 42, alsa_device()); + add_device_node( + &mut m, + 100, + 50, + MediaRole::Sink, + hw_claim(42, "alsa", "api.alsa.pcm.sink"), + ); + assert!( + m.project() + .snapshot + .node(ser(100)) + .unwrap() + .props + .session_device + ); + + // A Port appears on the recycled id 42. + m.apply(port(4300, 42, 50, PortDirection::In)); + assert_eq!( + m.project().snapshot.nodes().count(), + 0, + "a contested device id resolves nothing" + ); + assert!(!m.graph_ready(), "and it is an outstanding obligation"); + + // Accounting for the Device's removal leaves the Port holding the id + // alone — still not a Device, so the node stays withheld. + m.apply(RegEvent::Removed { id: gid(42) }); + assert_eq!(m.project().snapshot.nodes().count(), 0); +} + #[test] fn model_ambiguous_device_id_withholds_its_nodes() { // Two live Devices on one recycled id: there is no way to know whose