mollusk/pixelpass
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

repair: exact-form matching, tri-state liveness, and a reference-gated sink

Browse Source 
Codex's review of the repair planner returned changes-requested: no P1s, four
reachable P2s plus a P3, all in the observation and execution layers rather than
in the discovery fix itself. All applied.

**Only the canonical forms are ours** (P2, plan.rs). `classify` recognised any
loopback with one pixelpass-looking endpoint, so a third party's
`module-loopback source=some_mic sink=pixelpass_capture_4242` was ours to unload
once that pid died — and a `sink=` token nested inside a quoted
`sink_input_properties` value could be mistaken for a top-level argument. The
whole recorded argument string must now equal what pixelpass itself would have
written.

The matcher's templates are **generated from the loader's own renderers**, not
written out beside them: hard-coding `latency_msec=20` in a matcher means a
loader change silently blinds repair to every module the new build loads, which
is the fail-closed-and-silent failure this project has been bitten by three
times. `host/audio.rs` now loads through those same renderers, so the two cannot
drift. Blindness is also reported rather than assumed impossible —
`unrecognised_pixelpass_modules` finds modules that name our sinks but match no
canonical form, and `--repair` says so loudly.

Measured on the live server before relying on it (pactl 17.0): arguments come
back byte-for-byte as passed, joined with single spaces, with `@DEFAULT_SINK@`
NOT resolved. Both facts are load-bearing for exact matching and both have a
test.

**Ordering is not a licence either** (P2, mod.rs). The plan put loopbacks before
the sink, but an unload can fail or be skipped and a loopback can appear after
planning, so the executor could still destroy a sink that something was attached
to. The sink unload is now gated on `sink_still_referenced` against the fresh
snapshot — any other module naming that sink blocks it, ours or not, because the
question is what would break, not who owns it.

**Undecidable is not dead** (P2, mod.rs). `Path::exists()` maps permission
errors, a missing `/proc` and a foreign pid namespace all to `false`, which here
read as "dead, go ahead and unload". Liveness is now `Alive | Dead | Unknown`
via `try_exists()` behind a `/proc/self/stat` preflight, and `Unknown` is
treated exactly like alive and reported separately.

**The short listing cannot carry a fingerprint** (P2, mod.rs). Its arguments are
tab-delimited text that a module argument may itself contain, and a continuation
line beginning with a digit could fabricate a row. Observations now come from two
listings: the short one for the module index, and `pactl -f json list modules`
for the exact argument. Codex proposed JSON alone; on pactl 17 its records carry
`"index": null`, so it cannot be used on its own — verified, hence the
correlation. The pairing is positional and *checked* (same count, same name at
every position, else refuse), which is also what makes a fabricated row harmless
instead of exploitable: it has no JSON counterpart, so the sequences misalign.

Normalisation is gone (P3). Within one invocation every snapshot comes from the
same server, so re-rendering does not happen, and normalising only made
genuinely different arguments compare equal. The residual ABA window — planned
module vanishes, byte-identical one takes its index — cannot be closed through an
unload API whose only argument is an index; that is now said plainly in the
fingerprint's own doc comment rather than implied away.

Five vacuity gaps Codex found in the tests, closed: a raw-pactl-output-to-plan
test (the planner suite survived a parser that dropped every argument), the
liveness-once test now uses two pids with per-pid counters, non-canonical and
nested-quoted arguments have their own cases, and the reference gate has one.

Field-verified on the live graph, both new rules: the A/B orphan test still
removes exactly the two orphans with the module table otherwise byte-identical,
and a fixture of a dead pid's legacy sink plus a non-canonical loopback naming it
leaves both alone and reports why.

251 tests (+9), clippy clean, fmt clean apart from the pre-existing
taint/tests.rs:2683.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
mollusk
2026-07-26 21:31:30 -04:00
co-authored by Claude Opus 5
parent 919d5bdef2
commit 9145b2a726
3 changed files with 881 additions and 203 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/host/audio.rs
+17 -22
View File
@@ -39,6 +39,7 @@ use std::sync::{Arc, Mutex};
use std::thread::JoinHandle;
use crate::cli::HostOpts;
use crate::repair::plan as repair_plan;
/// Owns the pactl-loaded modules plus, when filtering is active, the
/// libpipewire stream-router thread. Drop unloads modules as a backstop;
@@ -64,9 +65,9 @@ impl Routing {
/// also spawn the libpipewire thread that reroutes matching streams.
pub async fn start(opts: &HostOpts) -> Result<Self> {
let pid = std::process::id();
let sink_name = format!("pixelpass_capture_{pid}");
let sink_name = repair_plan::sink_name_for(pid);
let sink_module = load_module(&["module-null-sink", &format!("sink_name={sink_name}")])
let sink_module = load_module("module-null-sink", &repair_plan::null_sink_args(pid))
.context("failed to load module-null-sink")?;
// In strict per-app mode we never mirror the default sink: the viewer
@@ -82,13 +83,8 @@ impl Routing {
None
} else {
Some(
load_module(&[
"module-loopback",
"source=@DEFAULT_SINK@.monitor",
&format!("sink={sink_name}"),
"latency_msec=20",
])
.context("failed to load module-loopback (null-sink cleaned up on Drop)")?,
load_module("module-loopback", &repair_plan::mirror_args(pid))
.context("failed to load module-loopback (null-sink cleaned up on Drop)")?,
)
};
@@ -115,7 +111,6 @@ impl Routing {
let (router, mut event_rx) = StreamRouter::spawn(app.clone(), sink_name.clone())?;
let loopback_for_task = Arc::clone(&loopback_arc);
let local_monitor_for_task = Arc::clone(&local_monitor_arc);
let sink_name_for_task = sink_name.clone();
let strict = opts.strict_audio;
let event_task = tokio::spawn(async move {
use crate::common::output::{self, AppAudioState};
@@ -137,12 +132,10 @@ impl Routing {
// only, never the desktop/call — so it can't echo into
// the capture.
if local_monitor_for_task.lock().unwrap().is_none() {
match load_module(&[
match load_module(
"module-loopback",
&format!("source={sink_name_for_task}.monitor"),
"sink=@DEFAULT_SINK@",
"latency_msec=20",
]) {
&repair_plan::local_monitor_args(pid),
) {
Ok(id) => {
tracing::info!(
module = id,
@@ -195,12 +188,7 @@ impl Routing {
tracing::info!(
"audio routing: last routed stream gone → restoring default-sink loopback"
);
match load_module(&[
"module-loopback",
"source=@DEFAULT_SINK@.monitor",
&format!("sink={sink_name_for_task}"),
"latency_msec=20",
]) {
match load_module("module-loopback", &repair_plan::mirror_args(pid)) {
Ok(id) => {
*loopback_for_task.lock().unwrap() = Some(id);
}
@@ -349,9 +337,16 @@ struct SinkInputProperties {
// pactl module helpers
// ──────────────────────────────────────────────────────────────────────
fn load_module(args: &[&str]) -> Result<u32> {
/// Load one Pulse module and return its index.
///
/// `args` comes from the renderers in [`crate::repair::plan`] rather than being
/// written out here, so that `--repair`'s exact-form matcher and this loader are
/// one source of truth. A latency or argument change that only moved one of them
/// would leave repair silently unable to recognise the modules this build loads.
fn load_module(module: &str, args: &[String]) -> Result<u32> {
let output = Command::new("pactl")
.arg("load-module")
.arg(module)
.args(args)
.output()
.context("failed to run pactl load-module")?;