From 1cfa932fbe3234fc8290689fa05e29e91d487eeb Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sun, 26 Jul 2026 19:11:06 -0400 Subject: [PATCH] docs: record the 0c mechanism probe, and revise 0b's mutation matrix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 0c mechanism probe was run on this host before any structural work, because one unverified assumption could have invalidated the whole approach: whether a hand-created `adapter` node is visible to pipewire-pulse under the name the capture path depends on. It is. Five gates green, including the two that mattered — `.monitor` is exposed as a Pulse source, and SIGKILL of the owning connection removes both Pulse-visible names with zero graph residue. No null-sink module is involved at any point. Every O1 stop condition for 0c is retired, and the default sink never moved, so the probe is safe on a live desktop. The probe also settled the native-sink scope question: it applies to every mode that owns a capture sink, not only `DesktopExcluding`. That makes the `--repair` rework load-bearing rather than defensive — repair derives dead PIDs only from `module-null-sink` entries, so once the sink is native its loopbacks become undiscoverable orphans. §10 gains rounds 14 and 15: the 0b matrix drops to four mutations because the best-effort wake arm is unreachable by construction (the loop owns a sender, and the biased select would win anyway), and teardown is hoisted to one unconditional post-loop site instead of being duplicated across one live arm and one dead one. Round 15 records the two blocking implementation-review findings and the vacuous gate of my own that the review's test-double critique exposed. Co-Authored-By: Claude Opus 5 --- docs/screenshare-audio-exclusion-impl-plan.md | 98 ++++++++++++++++++- 1 file changed, 97 insertions(+), 1 deletion(-) diff --git a/docs/screenshare-audio-exclusion-impl-plan.md b/docs/screenshare-audio-exclusion-impl-plan.md index 5d0bc3f..6b9839e 100644 --- a/docs/screenshare-audio-exclusion-impl-plan.md +++ b/docs/screenshare-audio-exclusion-impl-plan.md @@ -151,7 +151,19 @@ v3.4 §7.2, decision D4. All **three** of v3.4's fixes: AEC guard unloads. This is the *only* protection on the panic/unwind path, and unwind is reachable — the core has numerous `unwrap()` sites and no `panic=abort` profile. -⚠️ **Mutation testing: five mutations, each independently breaking a named test.** v1 demanded +> ✅ **0b IMPLEMENTED 2026-07-26** (peerspeak branch `phase-0b-teardown`). The ordering +> defect was live: `echo_cancel` was declared *ahead* of `screenshare_host`, so any unwind +> unloaded the AEC while the host was still fanning out. Fields moved into +> `src/core/teardown.rs` with `echo_cancel` declared last, and `ReapOnDrop` added because +> `kill_on_drop(true)` only *signals* — it hands the child to the runtime's orphan queue, +> which an unwinding runtime may never drain. Matrix revised to four mutations; see §10 +> round 14, and round 15 for the two blocking review findings that followed. +> +> **Owed to phase 9:** an explicit lifecycle row — *drop the controller / close the command +> channel while sharing* — which is the live proof for the hoisted teardown call site. + +⚠️ **Mutation testing: five mutations, each independently breaking a named test.** +⚠️ **SUPERSEDED by §10 round 14 — mutation 2 is vacuous and the matrix is now four.** v1 demanded a mutation that targeted the wrong defense; v2 fixed that but bundled two defenses into one combined mutant, which proves neither. Final form: @@ -167,6 +179,45 @@ Both channel-close arms get their own test; a single "closes the command channel exercise one arm and leave the other unsafe. ### 0c. Graceful stop + connection-owned capture sink — both + +> ✅ **MECHANISM PROBE PASSED on this host, 2026-07-26** (PipeWire 1.6.8). Run *before* any +> structural work, on the reviewer's insistence, because a single unverified assumption could +> have invalidated the entire approach: whether a hand-created adapter is visible to +> pipewire-pulse under the name pixelpass's capture path depends on. It is. +> +> ``` +> pw-cli> create-node adapter factory.name=support.null-audio-sink \ +> node.name=pixelpass_probe_ media.class=Audio/Sink \ +> audio.channels=2 audio.position=[FL,FR] node.virtual=true \ +> monitor.channel-volumes=true object.linger=false +> ``` +> +> Five gates, all green: +> 1. `pactl list short sinks` shows the sink under the **exact** `node.name`. +> 2. `pactl list short sources` shows **`.monitor`** — the derived monitor name is +> a pipewire-pulse contract, not a property of Pulse-created sinks. This was the one that +> could have sunk the approach. +> 3. `gst-launch-1.0 pulsesrc device=.monitor num-buffers=40 ! fakesink` pulled its +> buffers and exited clean, and a real recording stream attached — so the pixelpass capture +> path works against it unchanged. +> 4. **No null-sink module was loaded** (`pactl list short modules | grep -c null-sink` stayed +> at its baseline of 3). It is genuinely not a Pulse module. +> 5. **SIGKILL of the owning connection removed both Pulse-visible names**, with zero residue +> anywhere in `pw-dump`. That is the entire point of 0c, demonstrated on the real graph. +> +> The default sink never moved, so this is also safe to run on a live desktop. +> **Every O1 stop condition listed for 0c is retired.** `object.linger=false` is load-bearing: +> the bundled pipewire-rs example sets `linger=1` for the opposite behaviour. +> +> ⚠️ **`--repair`'s job does not shrink — it BREAKS.** Discovery derives dead host PIDs +> **only** from `module-null-sink` entries (`pixelpass/src/repair.rs`), and only then matches +> loopbacks against that PID set. The native sink is scoped to **every mode that owns a +> capture sink**, not just `DesktopExcluding`, so legacy Pulse loopbacks will coexist with a +> connection-owned sink; when that host dies the sink vanishes automatically and its loopbacks +> become **undiscoverable orphans**. Candidate PIDs must be derived independently from all +> three module shapes (`null-sink sink_name=`, `loopback sink=`, `loopback source=…monitor`), +> with a liveness recheck immediately before each destructive unload. This makes the repair +> rework **load-bearing, not defensive**. v3.4 §7.4. The **largest hidden cost in Phase 0**: moving the null sink off `pactl load-module` (`pixelpass/src/host/audio.rs:69`, cleaned up only in `Routing::cleanup` at `:259-260`, which SIGKILL skips) onto a connection-owned PipeWire object. @@ -758,6 +809,51 @@ mid-share load stays a **synthetic** test until a second `enable` site or hot re ## 10. Adjudication record +**Round 14 (2026-07-26) — 0b's five-mutation matrix is revised to four, and one pinned +mutation is retired as vacuous.** Reached independently by both reviewers, then agreed. + +- **Mutation 2 cannot be killed by any test, because its site cannot execute.** The + best-effort wake arm (`core/mod.rs`, the `besteffort_wake_rx` close arm) is unreachable + **by construction, twice over**: (i) `run_core_loop` owns a clone of `besteffort_wake_tx` + — created at `CoreController::new` and used for the `has_more` re-arm inside the loop — + and a tokio `Receiver::recv()` yields `None` only once *every* sender is dropped; (ii) + even without that clone, both `CoreController` and `CoreCommandSender` hold `reliable_tx` + alongside the wake sender, and the `select!` is `biased` with the reliable arm first, so + the reliable arm always wins the race to exit. Writing teardown there would be code that + provably never runs, dressed as a tested path. +- **Mutation 1's site is reachable but not unit-testable.** It sits inside `run_core_loop`, + which builds a real iroh endpoint and loads identity; no unit test can drive it. +- **Decision: (b) + (c).** Teardown is **hoisted to one unconditional site after the loop**, + so every `break` is covered structurally, including any added later — strictly better than + duplicating teardown across one live arm and one dead one. The **seam-level mutation gates + are the real ordering proof**, and the call site's live proof is owed to **phase 9**, which + gains an explicit row: *drop the controller / close the command channel while sharing*. + "UI crash" is not precise enough to serve as that row. +- **Rejected: an integration test built to preserve the number five.** It would pay for a + full iroh core plus test-only observability and prove only that a method was called — not + the ordering invariant, which is the thing that actually breaks. +- The 0b gate is therefore **four mutations** (no wait · reversed field order · no reap loop · + plus the 0c pair below), each killed by its own named test, with 4-vs-5 verified separated: + reversing the field order leaves the reap test green and removing the reap loop leaves the + ordering test green. + +**Round 15 (2026-07-26) — review of the 0b/0c-peerspeak implementation returned two blocking +findings, both accepted.** Recorded because both are the same shape: a defence that existed +but was disarmed exactly when it was needed. + +- **The drop fallback was disarmed across its own wait.** `shutdown` took the child out of + the wrapper before the first `.await`; a cancellation or unwind during the wait left the + raw child to drop with `kill_on_drop` (which signals without reaping) while `Drop` found + `None`. The child now stays owned until the reap is **confirmed**. +- **A failed wait was reported as a reap, and the hard-kill wait was unbounded.** The + `io::Result` was discarded, so a wait error returned "reaped"; and a process in + uninterruptible sleep after SIGKILL could wedge the core loop forever. Both waits are now + bounded and the conflict case has a written policy: availability wins, the child stays + owned so the bounded `Drop` retry stays armed, and the residual risk is logged. +- **A gate of mine was vacuous and the review's fourth test-double point caught it.** The + elapsed-time assertion compared against `STOP_GRACE` itself, so zeroing the constant left + it trivially true. `the_grace_is_a_real_interval` now pins the constant to a band. + **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