repair: read and unload Pulse modules through libpulse, not pactl text

Third review round found three more blocking P2s, and they shared one root cause:
`pactl`'s text output cannot carry the guarantees repair was claiming. All three
are closed by talking to the protocol instead. New dependency taken with the
user's explicit sign-off after vetting.

**Record boundaries were unprovable.** `pactl list short modules` prints a
module's argument raw into a tab/newline-delimited format with no escaping. A
*genuine* module whose argument contains a newline renders a first line that reads
byte-exactly like one of our canonical forms, with the remainder dropped as an
unparseable continuation — no forged index, so the duplicate-index check could not
see it. Repair would have classified and unloaded a module it never saw in full.
**Field-confirmed on the live server**, because this needed no adversary: loading a
loopback whose argument is canonical-then-newline-then-`remix=false` (a real
loopback option) produces exactly that listing. A tab in the same position is
worse: it hid a sink reference from the gate that protects a still-referenced sink.

**Index and argument could be mis-paired.** The listing carrying exact arguments
(`-f json`) carries no index at all on pactl 17; the one carrying the index cannot
carry the argument faithfully. Correlating them by position — which the previous
commit did — is unsound whenever module names repeat: another client loading one
module and unloading another between the two calls leaves counts and names aligned
while every argument has shifted by one, so a foreign module inherits a canonical
fingerprint. The name check cannot see it and the retry never fires.

**Locality was a guess.** `PULSE_SERVER` is a fallback *list*, so
`unix:/missing tcp:remote:4713` passes any "starts with unix:" test and then
connects to another machine, where local pids mean nothing and a live remote host's
modules look dead. A remote server can also be selected by client config with the
variable unset entirely.

New `repair/introspect.rs` owns one verified-local connection: `pa_module_info`
gives index, name and exact argument in a single record, `pa_context_is_local()`
answers locality about the connection actually established, and unloading goes back
through that same connection so listing and destruction cannot disagree about which
server they mean. It holds no policy beyond refusing the wrong server; every
decision stays in the pure planner.

⚠️ **The field test caught a real bug that no unit test could have.** The first
version did its work correctly and then aborted on the way out:

    Assertion '!e->dead' failed at ../pulseaudio/src/pulse/mainloop.c:207,
    function mainloop_io_free(). Aborting.

SIGABRT, core dumped, exit 134 — a fully successful repair reporting failure to its
caller. Cause: Rust drops fields in declaration order and the context's teardown
frees IO events living in the mainloop, which I had declared first. **This is
exactly the invariant phase 0b exists for, met again one layer down.** Fixed, and
then hardened past the fix: `Drop` explicitly takes and destroys the context before
the mainloop, so the ordering no longer depends on where the fields are written.

Liveness keeps its `NSpid`/container checks but the claim is corrected: `NSpid > 1`
means "definitely nested", while `NSpid == 1` is NOT proof of the initial namespace
— its leftmost value is relative to the procfs that was mounted, so a nested
namespace with its own `/proc` reports one entry legitimately. These are negative
signals that fail closed, not a proof of trustworthy pids. Closing that properly
needs modules to carry an owner token (machine/boot plus pid-namespace identity),
which changes what pixelpass writes into the graph and how far back `--repair` can
clean up: recorded as a design decision, not guessed at.

libpulse-binding 2.30.1 vetted before use: MIT/Apache-2.0, 5.5M downloads, 3 new
crates total, build script does nothing but probe pkg-config, no network or
subprocess use anywhere in the sources, and all three historical RustSec advisories
(2018-0020, 2018-0021, 2019-0038) were fixed by 2.6.0. The reasoning is recorded in
Cargo.toml beside the dependency.

247 tests, clippy clean, fmt clean apart from the pre-existing taint/tests.rs:2683.
The text parser's tests are gone with the parser; the liveness probe keeps its own,
and the live field gates — A/B orphan removal, the reference/unrecognised fixture,
and the newline fixture — all pass with exit 0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-07-27 01:23:23 -04:00
co-authored by Claude Opus 5
parent 01c582427b
commit 45464b4af5
5 changed files with 405 additions and 277 deletions
+5 -28
View File
@@ -97,24 +97,16 @@ pub fn recorded_argument(args: &[String]) -> String {
/// One module exactly as the server reported it.
///
/// `args` is the **exact** recorded argument string, not a normalised one. Within
/// a single repair invocation every snapshot comes from the same server, so
/// re-rendering is not a thing that happens, and normalising would only make two
/// genuinely different arguments compare equal (whitespace inside a quoted
/// property value is not layout).
/// `args` is the **exact** argument string from `pa_module_info`, not a normalised
/// one, and not a reconstruction from a text listing. Normalising would only make
/// two genuinely different arguments compare equal — whitespace inside a quoted
/// property value is not layout — and every snapshot within one invocation comes
/// from the same connection, so there is no re-rendering to absorb.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ModuleObservation {
pub id: u32,
pub name: String,
pub args: String,
/// False when the observation format could not carry the whole argument (the
/// short listing is tab-delimited, and an argument may itself contain a tab).
///
/// A truncated argument is never ours — [`classify`] refuses it outright,
/// because a truncation could otherwise coincide with a canonical form. It is
/// still kept in the snapshot: [`sink_still_referenced`] must be able to see
/// that *something* names a sink even when it cannot read the whole argument.
pub args_complete: bool,
}
impl ModuleObservation {
@@ -123,15 +115,6 @@ impl ModuleObservation {
id,
name: name.to_string(),
args: args.to_string(),
args_complete: true,
}
}
/// An observation whose argument the format could not fully carry.
pub fn truncated(id: u32, name: &str, args: &str) -> Self {
Self {
args_complete: false,
..Self::new(id, name, args)
}
}
}
@@ -357,12 +340,6 @@ impl Plan {
/// grows an argument, changes a latency, or repeats the pid cannot leave a matcher
/// quietly accepting the old form.
pub fn classify(obs: &ModuleObservation) -> Option<Fingerprint> {
// An argument we could not read in full is never ours: a truncation could
// coincide with a canonical form, and unloading on a coincidence is the one
// outcome none of these rules tolerate.
if !obs.args_complete {
return None;
}
for shape in ALL_SHAPES {
let template = shape.template();
if obs.name != template.module_name {