diff --git a/docs/screenshare-audio-exclusion-plan.md b/docs/screenshare-audio-exclusion-plan.md index 044a0f5..12c9a93 100644 --- a/docs/screenshare-audio-exclusion-plan.md +++ b/docs/screenshare-audio-exclusion-plan.md @@ -1,7 +1,7 @@ # Design v3: whole-desktop screen-share audio without self-echo -**Status:** design complete, **implementation-planning phase — not approved for merge.** -**Date:** 2026-07-21 (v1: 2026-07-19 · v2: 2026-07-20 · Option C adopted 2026-07-20) +**Status:** v3.1 — round-4 findings applied; **still BLOCKED pending round-5 re-review** (§14). +**Date:** 2026-07-21 (v1: 07-19 · v2: 07-20 · Option C adopted 07-20 · v3.1 round-4 revision) **Origin:** Joe's suggestion — "whitelist all audio except audio coming from peerspeak." **Scope:** a new capture mode in pixelpass (`src/host/pipeline.rs`, `src/host/audio.rs`), playback tagging + AEC-identity export + teardown-ordering invariants in peerspeak. @@ -132,13 +132,22 @@ it is also what makes the daemon-restart case (§12) untested rather than free. Every peerspeak-owned playback path is either a stream peerspeak itself creates, a `Command::new(…)` spawn, or a pactl-loaded module. Three mechanisms cover all three. -### 5.1 Env-inherited tag, for spawned children — ✅ MEASURED WORKING +### 5.1 Env-inherited tag, for spawned children — mechanism ✅ MEASURED, not yet built -| Owner | Mechanism | -| --- | --- | -| native call playback (`src/audio/pipewire_impl.rs:374-388`) | set the tag directly in the stream's property dict | -| mpv / VLC (`src/screenshare/mod.rs:761`) | `.env("PULSE_PROP", …)` / `PIPEWIRE_PROPS` on the `Command` | -| `pw-play` / `paplay` / `aplay` (`src/notify.rs:265`) | same | +⚠️ **This table lists IMPLEMENTATION SITES, not current behaviour.** None of these sites +tags anything today: `pipewire_impl.rs:374-388` sets no `peerspeak.owned` prop, and +neither the mpv/VLC spawn (`screenshare/mod.rs:768-775`) nor the notification spawn +(`notify.rs:265-272`) sets any env. The *mechanism* is measured working (below); the +*wiring* is work. + +| Owner | Site | Mechanism to add | +| --- | --- | --- | +| native call playback | `src/audio/pipewire_impl.rs:374-388` | add the tag to the stream's property dict | +| mpv / VLC | `src/screenshare/mod.rs:768-775` | `.env("PULSE_PROP", …)` + `PIPEWIRE_PROPS` on the `Command` | +| `pw-play` / `paplay` / `aplay` | `src/notify.rs:265-272` | same | + +Exact strings to be pinned at implementation time (§11), and each site needs a test: +native props asserted on the constructed dict, child sites asserted on the `Command` env. Measured 2026-07-20 on this box (PipeWire 1.6.8): `peerspeak.owned=1` reached the graph node for **paplay ✅, mpv ✅, VLC ✅**. `PULSE_PROP` and `PULSE_PROP_OVERRIDE` are both @@ -219,16 +228,38 @@ so they fit u32 — but this sits right next to the `object.serial` u32-truncati ### 5.3 Fail closed — validation and revocation Because the identity is an observed correlation rather than a contract, pixelpass must -**verify it at runtime and refuse to run the mode when it cannot**: +**verify it at runtime and refuse to run the mode when it cannot.** A one-shot +"enumerate at start" check is *not* good enough — it races, in both directions: -- **At start:** given `--exclude-pulse-module `, enumerate the graph and confirm at - least one node carries that `pulse.module.id`. If peerspeak said the AEC is on and no - such node exists, **do not start fan-out** — report a capability failure and fall back - to a mode with no echo risk (no audio, or an explicit user override). -- **On revocation:** if the validated AEC leg **disappears** while sharing, **stop - fan-out immediately.** Do not keep the numeric index and hope. Disappearance means - either the AEC unloaded (index may be recycled onto something else) or our model of the - graph is wrong; both are fail-closed conditions. +- peerspeak's `enable()` returns once `wait_for_nodes` sees the virtual **source and + sink** by name (`echo_cancel.rs:132-158`, polling `pactl list sources/sinks short`). + It does **not** wait for the playback `Stream/Output/Audio` hazard leg, which is the + node we actually need to exclude. +- pixelpass's capture spawns **lazily, on first viewer** (`pixelpass/src/host/mod.rs:300-304`), + so enumeration happens at a moment peerspeak does not control, and a refusal surfaces + *after* the ticket has been handed out. + +**So validation is a bounded state machine, not a check:** + +``` +NotConfigured ──(--aec=off)──────────────► fan-out proceeds, no AEC exclusion +Validating ──(--aec=pulse-module:)─► enumerate after a registry sync barrier; + wait up to a bounded deadline for ≥1 node + with pulse.module.id == i +Validated ──► fan-out permitted, excluding that identity transitively (§6.1) +Failed ──► deadline expired, identity never observed ⇒ NO FAN-OUT +Revoked ──► the live module identity disappeared ⇒ STOP FAN-OUT NOW +``` + +- **No fan-out occurs in `Validating`.** Silence is the safe direction; echo is not. +- **`Failed` is fail-closed**: report a capability failure, do not silently share. + Fall back to a mode with no echo risk, or an explicit user override. +- **Revocation is loss of the live *module* identity** — *all* nodes bearing the index + gone — not the transient absence of one playback leg, which can cork or relink. Getting + this wrong turns a normal cork into a spurious share-wide audio stop. +- On `Revoked`, **drop the link proxies**; do not keep the numeric index and hope. The + index is reused (§5.2 correction 3), so a retained stale index can alias onto an + unrelated future module. - **Never** infer exclusion from `node.name == "echo-cancel-playback"` alone — it is a fixed, non-unique, trivially spoofable string. Usable only as belt-and-braces *after* the exact match, or as foreign-instance hazard detection. @@ -250,7 +281,45 @@ allowlist is out. But "fan out everything" is still wrong: it recreates self-ech recreates call echo, it can build cycles, and a second link participates in format/buffer negotiation. -Agreed predicate: +### 6.1 ⚠️ Eligibility is a GRAPH property, not a node property + +**This is the single biggest correction in round 4, and it is reachable on the target +machine, not hypothetical.** + +A node-local property check is unsound, because **exclusion does not propagate downstream.** +If any node re-emits audio it received, the re-emitting node is a fresh, *untagged* +`Stream/Output/Audio` carrying the mix — including peerspeak's playback and the AEC leg, +both of which were correctly excluded one hop earlier. + +The canonical instance: **EasyEffects.** When it is running, applications play into +`ee_sink` and EasyEffects emits a single combined output stream to the hardware. That +output leg has no `peerspeak.owned` tag and no `pulse.module.id` matching our AEC, so +**both §5.1 and §5.2 pass it**, and fanning it out re-injects the entire call into the +share at full level — the exact bug this feature exists to prevent, arriving through the +front door. ⚠️ **`easyeffects` is installed on this machine** (`pacman -Qq easyeffects`), +and `libpipewire-module-filter-chain.so` is present. It happened not to be running during +the spike, which is *why* the spike did not catch this. Same shape: combine-sinks, +`module-loopback`, virtual-sink forwarders, tunnel/RTP sinks, DSP bridges, and any +monitor-sourced stream. + +**So the predicate is transitive upstream reachability over the live graph:** + +> A node is eligible only if **no** path through Node/Port/Link globals reaches a +> peerspeak-owned node, the live AEC identity, any pixelpass-owned object (ours *or* +> another host's `pixelpass_capture_*`), or a monitor/capture source that cannot be +> proven clean. + +This means pixelpass must **track Node, Port and Link globals** — not just node +global-add, which is all the existing router does (`pixelpass/src/host/audio.rs:523-584` +subscribes to global-add/global-remove and never binds links). That is a materially +larger component than v3.0 implied. + +Ancestry that cannot be resolved (unknown forwarder shape, unresolvable monitor source) +is **not** eligible. That is a deliberate reversal of v3.0's "unrecognized ⇒ fan out": +E2 makes mis-selection cheap *for the user's audio graph*, but it is not cheap for +**echo**, and echo is the whole feature. + +### 6.2 The predicate **Start from all `Stream/Output/Audio` nodes, then exclude:** @@ -258,21 +327,32 @@ Agreed predicate: | --- | --- | | `peerspeak.owned` present | §5.1 env tag | | `pulse.module.id` == the live AEC index | §5.2, exact equality only | -| pixelpass-owned objects, and any node with capture-sink ancestry | cycle prevention | +| **any upstream ancestry reaching an excluded node** | §6.1 — the load-bearing one | +| pixelpass-owned objects, incl. **other hosts'** `pixelpass_capture_*` ancestry | cycle prevention | +| unresolvable / unknown ancestry | fail closed | | `port.exclusive` ports, encoded/passthrough streams | fan-out will refuse or corrupt | | links we already own for that node | idempotence | -Notes: +- `node.dont-move` **drops out entirely** — fan-out is not a metadata move. +- Links are created **per port**; channel-count and layout mismatches are per-port. -- `node.dont-move` **drops out of the predicate entirely** — fan-out is not a metadata - move, so a node's move policy is irrelevant. -- Links are created **per port**, not per node; channel-count and layout mismatches are - a per-port concern. -- Anything unrecognized is **fanned out** (copy is cheap) *unless* it hits an exclusion — - the inverse of v2's posture, and a direct consequence of E2. -- Dynamic nodes: the selector must run on registry global-add for the life of the share, - not once at start. Nodes created *after* enumeration are exactly the case the spike rig - was blind to. +### 6.3 Dynamic graph handling — revalidate, don't fire-and-forget + +A node can appear with incomplete ancestry, pass the predicate, get fanned out, and only +*then* receive an inbound link from a filter input or `pixelpass_capture_.monitor`. +An add-only listener never sees it. Required: + +1. An **initial registry sync barrier** — enumerate to a `core sync`/`done` before + deciding anything; a partially-populated registry is not a graph. +2. Candidates stay **pending** until their ports *and* current inbound links are known. +3. **Revalidate immediately before creating each Link**, not just at selection time. +4. On any later link add/remove that makes ancestry unsafe, **drop the owned link + proxies** for the affected node — retention (§4.2) is what makes revocation possible. + +## 6.5 Foreign/hazard nodes + +Treat an unvalidated `echo-cancel-*` output node as an excluded hazard rather than an +eligible stream (§5.4). ## 7. Lifecycle and teardown invariants @@ -304,11 +384,31 @@ ordering the invariant forbids. Verified in source. `echo_cancel` at `:730` — but only as an emergent property of statement order, which any refactor can silently invert. -**Fix (implementation phase):** move `echo_cancel` to be the **last** declared field, add -a comment naming the invariant, and add a regression guard. A drop-order unit test is -awkward in safe Rust; the cheap version is a compile-time/`#[test]` assertion over field -order via a doc-tested constructor, or a `Drop` impl on a wrapper that records ordering -into a test-visible slot. **Open decision D4 (§13).** +**⚠️ Reordering the fields is NOT sufficient** (round-4 correction). `screenshare_host` +is spawned with `kill_on_drop(true)` (`peerspeak/src/screenshare/mod.rs:402`). Tokio's +`Drop` for such a `Child` *sends* the kill and hands the process to a best-effort orphan +reaper — it does **not** wait, and gives no promptness guarantee, especially if the +runtime is itself shutting down. Explicit `child.kill().await` is SIGKILL **plus wait**; +implicit drop is SIGKILL and move on. So even with `echo_cancel` declared last, the +`pactl unload` can still run while pixelpass is briefly alive. + +**Fix, in order of importance:** + +1. **Eliminate the implicit path.** At both channel-close `break` sites + (`core/mod.rs:1516` and `:1532`), explicitly `take()` the session and + `shutdown().await` it rather than letting it drop. The ordered teardown should be the + *only* teardown. +2. **Move `echo_cancel` to the last declared field** anyway, with a comment naming the + invariant — defence in depth for paths 1 does not cover (panics, unwinds). +3. **Last-ditch ordering in the drop path**: a wrapper whose `Drop` does `start_kill` + + a bounded `try_wait` loop on the host before the AEC guard unloads. + +**D4 regression guard** (concrete, replacing v3.0's vague "field-order assertion"): a +test using a **fake host** and **fake echo-cancel guard** that each record a timestamped +event into a shared slot on teardown, asserting `host kill+wait completed` strictly +precedes `echo unload` — plus a core-loop test that closes the command channel and proves +`shutdown()` actually ran. A field-order assertion alone does not test the behaviour that +matters. ### 7.3 Construction paths @@ -326,7 +426,7 @@ Anyone adding a second `enable` site, or any hot-reload, re-opens that requireme ### 7.4 Graceful stop is still owed -**Stop Share is currently SIGKILL** (`peerspeak/src/core/mod.rs:698` + `:3479`, Tokio +**Stop Share is currently SIGKILL** (`peerspeak/src/core/mod.rs:699` + `:3480`, Tokio `Child::kill()`). Under Option C this no longer strands the user's desktop audio — the links die with the connection, which is the point. But it still leaks: @@ -476,18 +576,43 @@ link exists in the graph** before measuring; reproduce over the transport produc actually uses (`parec -d .monitor`, not `pw-record --target `, which reads −91 dB silence). -## 13. Open decisions for the reviewer +## 13. Decisions — resolved in round 4 -- **D1** — Is the fail-closed posture of §5.3 (refuse the mode when AEC identity cannot be - validated; stop fan-out on revocation) the right trade against "share anyway, warn"? -- **D2** — Capability negotiation: detection today is a `pixelpass --help` substring probe - (`src/screenshare/mod.rs:245-278`, invoked at `src/core/mod.rs:3368`). This mode must +Both reviewers agree on all seven. Recorded as decided; reopen only with new evidence. + +- **D1 — fail closed, but only after a bounded validation wait** (§5.3). Never "share + anyway, warn" as the default. ✅ +- **D2 — a versioned machine-readable capability response or bitset.** The + `pixelpass --help` substring probe (`src/screenshare/mod.rs:245-278`, invoked at + `src/core/mod.rs:3368`) stays only as a compatibility fallback, and this mode must **not** overload `app_audio_supported: bool` — strict per-app and desktop-excluding are - independent capabilities. Enum, bitset, or a version handshake? -- **D3** — Foreign/second AEC policy (§5.4): fail closed, or warn + exclude all - `echo-cancel-*`? -- **D4** — What is the actual regression guard for drop order (§7.2)? -- **D5** — Explicit `--aec off` vs flag-absent (§8)? -- **D6** — Do the §10 prerequisites all land first, or only 1, 4 and 5? -- **D7** — Is there a materially simpler design meeting Joe's ask that three rounds have - missed? + independent capabilities. ✅ +- **D3 — warn and exclude foreign `echo-cancel-*` nodes** for the first implementation; + never fan them out. ✅ +- **D4 — explicit async shutdown + a fake-resource ordering test** (§7.2). A field-order + assertion alone is inadequate. ✅ +- **D5 — `--aec=off|pulse-module:`, always passed.** Absence of the flag is not a + protocol state (§8). ✅ +- **D6 — land §10 items 1, 4 and 5 first**, plus the new graph-ancestry predicate (§6.1). + Items 2 and 3 are real per-app debt but do not block Option C unless the new mode reuses + those code paths. ✅ +- **D7 — no materially simpler design exists** that still meets Joe's ask. The available + simplification is to *narrow v1 scope*, not to change architecture. ✅ + +## 14. Readiness — 🔴 BLOCKED (round 4), unblock set + +v3.0 was reviewed and blocked; v3.1 (this revision) applies the three required rewrites. +The minimum unblock set was: + +1. **§6 rewritten around transitive graph eligibility + dynamic link revalidation** — + applied in §6.1–6.3. This is the round's biggest finding: eligibility is a graph + property, and **EasyEffects is installed on the target machine**, so the leak is + reachable, not theoretical. +2. **§5.3 rewritten as a bounded AEC validation state machine with explicit `--aec`** — + applied. +3. **§7.2's field-order fix replaced with explicit shutdown + a real regression guard** — + applied. + +**Next step: a fifth round re-reviewing v3.1**, focused on whether the graph-ancestry +predicate in §6.1 is actually implementable against the `pipewire` crate's Link/Port +globals, and on the cost of the sync barrier in §6.3. Nothing here is approved for merge.