docs: round 18 — repair moves to libpulse; two reviews, three reusable lessons
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user