host/observer: address the Codex phase-3r review (2 fixes, both verified)

Codex's adversarial review of the pure core found no *certain* P1. Two
findings taken, both mutation-verified (the fix reverted, the intended test
dies, nothing else moves):

**F2, certain, P2 — `device_props` tested the wrong kind of ambiguity.** It
required exactly one live *Device* on the claimed id rather than exactly one
live *global*. With `[Device, Port]` on one id — a missed removal, the same
precondition as every other recycled-id hazard — it kept answering from the
older Device, so a node claiming that id held a stale `session_device = true`.
That flag strips 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. Now:
one slot total, and it must be the Device.

**F3, worth checking, P3 — `device.api` was corroborating by presence.**
`device.api=v4l2` under `factory.name=api.alsa.pcm.sink` satisfied the
positive classifier. No truthful configuration produces that pair, which is
the argument for reading it as an observation gone wrong rather than as
corroboration. The API must now equal the one the factory allowlist is
written for, an empty value is not a value, and the two sides disagreeing
fails closed. Tied to the allowlist being ALSA-only via a named constant.

Two findings NOT fixed here, both pre-existing and neither introduced by
round 8 — raised to the design doc instead:

- **P1, worth checking: hardware playback-to-capture paths** (Stereo Mix,
  Digital Loopback) on a card whose driver is an ordinary `snd_hda_intel`.
  Both its sink and source classify `session_device`, taint cannot cross the
  hardware hop, and a capture app reading that source can re-emit the call.
  This is `snd_aloop` again in a form the driver name cannot detect;
  distinguishing it needs ALSA control inspection, which is a design change
  and a new I/O surface, not a local fix.
- **P3: the 2 s readiness budget** can in principle never see an
  obligation-free instant under sustained startup churn, and `TimedOut` is
  sticky by design, so the process would be silent for its lifetime.
  Measured here: readiness at ~3 ms with 19 binds, so the margin is three
  orders of magnitude — but it wants a calibration argument, not a guess.

197 unit + 3 live green, clippy -D warnings and fmt clean.
This commit is contained in:
2026-07-25 18:48:53 -04:00
parent 306b601490
commit 471b8221ff
3 changed files with 130 additions and 15 deletions
+19 -2
View File
@@ -64,6 +64,11 @@ const HARDWARE_PCM_FACTORIES: &[&str] = &[
"api.alsa.pcm.source", "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** /// ALSA drivers that expose a hardware-PCM `factory.name` but are **not**
/// passive terminals — audio written in reappears on their capture side /// passive terminals — audio written in reappears on their capture side
/// through a path the PipeWire Link graph cannot see, so classifying them /// 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() .flatten()
.any(|d| NON_TERMINAL_ALSA_DRIVERS.contains(&d)); .any(|d| NON_TERMINAL_ALSA_DRIVERS.contains(&d));
let driver_ok = driver.is_some() && !driver_denied; let driver_ok = driver.is_some() && !driver_denied;
let api_present = device.device_api.is_some() || claim.device_api.is_some(); // The API must positively be the one the factory allowlist is written
let is_hardware_pcm = api_present && on_factory_allowlist && driver_ok; // 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 { if is_hardware_pcm {
Classification::SessionDevice Classification::SessionDevice
} else { } else {
+23 -13
View File
@@ -596,21 +596,31 @@ impl RegistryModel {
} }
/// The bound properties of the Device a node claims by global id, or /// The bound properties of the Device a node claims by global id, or
/// `None` when that claim is unresolved — which covers all three /// `None` when that claim is unresolved — which covers every fail-closed
/// fail-closed cases at once: no such Device observed, its bind still /// case at once: no such Device observed, its bind still outstanding, or
/// outstanding, or **two live Devices sharing the recycled id**, where /// **the id claimed by more than one live global**, where there is no way
/// there is no way to tell whose properties these are (v3.4 §6.1.3). /// 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> { fn device_props(&self, id: GlobalId) -> Option<&DeviceProps> {
let mut found: Option<Serial> = None; let slots = self.live_ids.get(&id)?;
for slot in self.live_ids.get(&id)? { if slots.len() != 1 {
if let Slot::Device(serial) = slot { return None; // Ambiguous ⇒ unresolved ⇒ withheld.
if found.is_some() {
return None; // Ambiguous ⇒ unresolved ⇒ withheld.
}
found = Some(*serial);
}
} }
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 /// Classify one node's device claim against the currently resolved
+88
View File
@@ -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] #[test]
fn classify_allowlist_is_exact_not_substring() { fn classify_allowlist_is_exact_not_substring() {
// A factory that merely *contains* an allowlisted name must not pass. // 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"); 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] #[test]
fn model_ambiguous_device_id_withholds_its_nodes() { fn model_ambiguous_device_id_withholds_its_nodes() {
// Two live Devices on one recycled id: there is no way to know whose // Two live Devices on one recycled id: there is no way to know whose