From 0d836d14c27f131fc195cbccb159dd8e2a921851 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Mon, 27 Jul 2026 01:24:46 -0400 Subject: [PATCH] =?UTF-8?q?docs:=20round=2018=20=E2=80=94=20repair=20moves?= =?UTF-8?q?=20to=20libpulse;=20two=20reviews,=20three=20reusable=20lessons?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rounds 17c and 4 of the repair review, recorded together because they resolve to one decision: `pactl`'s text output cannot carry the guarantees repair claims, so observation and unloading now go through libpulse introspection over a single verified-local connection. The dependency was taken with the user's sign-off after vetting (details beside the dep in pixelpass Cargo.toml). Three lessons that generalise beyond this phase: - A *prescription* can fail reachability just as a finding can. "Use `pactl -f json list modules`" is sound reasoning against an API that does not exist — those records carry no module index, and `unload-module` accepts only an index. - Auditing my own fixes paid a third time: two of the four fixes applied in round 17a were themselves defective, including a correlation scheme that is unsound whenever module names repeat. - The live field test caught a bug unit tests structurally cannot reach, and it was phase 0b's bug one layer down: fields drop in declaration order, the Pulse context's teardown frees IO events owned by the mainloop, and declaring the mainloop first turned a fully successful repair into SIGABRT and exit 134. Also recorded: the newline defect needed no adversary and was confirmed on the live server, and the remaining namespace hole is left open with its trade stated — an owner token would close it but would make orphans from older builds uncleanable. Co-Authored-By: Claude Opus 5 --- docs/screenshare-audio-exclusion-impl-plan.md | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/docs/screenshare-audio-exclusion-impl-plan.md b/docs/screenshare-audio-exclusion-impl-plan.md index 9ca17ee..c66393a 100644 --- a/docs/screenshare-audio-exclusion-impl-plan.md +++ b/docs/screenshare-audio-exclusion-impl-plan.md @@ -1030,6 +1030,67 @@ light (the `abort()` orphan race, the unbounded join, peerspeak advertising a de | S4 | the `AudioGraphOwner` actor itself, the readiness handshake, and both measured budgets | the only slice that needs new PipeWire mechanism | | S5 | the two live exit gates: two-host ownership, and the never-yet-run Stop Share SIGINT gate | needs S4 on the graph | +**Round 18 (2026-07-26 night) — two more repair review rounds. `--repair` now reads and unloads +through libpulse, and one of the review's own prescriptions had to be replaced after measuring.** + +*Round 17c — the re-review of my round-17a fixes found two more blocking P2s. Two of the four +fixes I had applied were themselves defective; this is the third time the "audit your own fixes" +rule has paid.* + +- **My two-listing correlation was unsound.** Pairing short-listing indices with JSON arguments by + position breaks 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, because correlation "succeeded". +- **My liveness fix still converted invisible-but-alive into dead.** A `/proc/self` preflight + proves nothing: inside a pid namespace — a container, a distrobox — `self` stays visible while + every process in the parent namespace is invisible, and `hidepid` has the same shape. + +*Round 18 (round 4) — the fix for both, and a third defect neither of us had reached.* + +- **Record boundaries in `pactl list short modules` are unprovable, and this needs no adversary.** + A genuine module whose argument contains a newline renders a first line that is byte-exactly one + of our canonical forms, with the rest dropped as an unparseable continuation — no forged index, + so no duplicate-index check can see it. **Field-confirmed on the live server** with + `…latency_msec=20\nremix=false`, `remix` being a real loopback option. A tab in the same position + is worse: it hides a sink reference from the gate that protects a still-referenced sink. +- **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. +- **Resolution: `src/repair/introspect.rs`, one verified-local connection.** `pa_module_info` + carries 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. Bounded + throughout (3 s connect, 3 s per request, non-blocking iteration plus a 2 ms sleep). The layer + holds no policy but "refuse the wrong server" — every decision stays in the pure planner. +- **The dependency was the user's call, taken with sign-off after vetting.** libpulse-binding + 2.30.1: MIT/Apache-2.0, 5.5M downloads, 3 new crates total, a build script that only probes + pkg-config, no network or subprocess use in any source, and all three historical RustSec + advisories (2018-0020/0021, 2019-0038) fixed by 2.6.0. Reasoning recorded beside the dep. +- ⚠️ **REUSABLE — a prescription can fail reachability, not just a finding.** The reviewer's + fidelity fix was "use `pactl -f json list modules`". On pactl 17 those records carry **no module + index at all** (`"index": null`) while `unload-module` accepts only an index, so it can never + stand alone. Measuring first is what caught it. +- ⚠️ **REUSABLE — the field test found a bug no unit test could reach, and it was 0b's bug again.** + The first introspection version did its work correctly and then aborted on the way out: + `Assertion '!e->dead' failed at mainloop.c:207, function mainloop_io_free()` — SIGABRT, core + dumped, **exit 134, so a fully successful repair reported failure to its caller**. Rust drops + fields in declaration order and the context's teardown frees IO events living in the mainloop, + which I had declared first. Fixed, 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. **Field-order drop hazards are not a peerspeak-specific lesson; they recur wherever + one object's teardown reaches into another's.** +- **Still open, deliberately, and recorded rather than guessed:** closing the namespace hole needs + modules to carry an **owner token** (machine/boot identity plus pid-namespace identity), with + token-less modules treated as `Unknown`. That changes what pixelpass writes into the graph *and + how far back `--repair` can clean up* — orphans from any older build would become uncleanable, + which is a regression in the tool's entire purpose. `NSpid > 1` remains a sound negative signal; + `NSpid == 1` is explicitly **not** proof, since its leftmost value is relative to the procfs that + was mounted. +- **Deferred, now cheap to reconsider:** `host/audio.rs` still loads modules via `pactl` and parses + the index off stdout, which is part of why S3's ambiguous-load problem exists. With libpulse in + the tree, `pa_context_load_module` returns the index through an observable operation. + **Round 1 — 13 items, 12 accepted.** Phase reorder (AEC machine before dry-run); typed capture plan (accepted, moved *earlier* than proposed); Phase 3 five-part gate; tag-consumption gating; Phase 6 matrix mandatory; 0b unwind backstop restored **and my mutation test corrected — it