docs: v3.1 audio-exclusion — apply Codex round-4 findings

Round 4 (review-2026-07-21-design-v3-round4.md) returned 5 findings, 3 of
them blocking. All claims re-verified against source before acceptance.

Biggest correction: eligibility is a GRAPH property, not a node property.
Exclusion does not propagate downstream — a filter-chain/loopback/combine-sink
re-emits the mix as a fresh untagged Stream/Output/Audio that passes both the
peerspeak.owned and pulse.module.id checks, re-injecting the whole call into
the share. Reachability confirmed: easyeffects IS installed on this machine
(it merely wasn't running during the fan-out spike, which is why the spike
missed it). §6 rewritten around transitive upstream reachability, tracking
Node/Port/Link globals, with a registry sync barrier and revalidation
immediately before each link creation.

Also applied:
- §5.3 is now a bounded validation state machine, not a one-shot check.
  wait_for_nodes only waits for the virtual source/sink, never the playback
  hazard leg, and pixelpass capture spawns lazily on first viewer, so the
  one-shot check raced in both directions. Revocation redefined as loss of
  the module identity, not transient absence of one leg.
- §7.2: reordering ActiveSession fields is NOT sufficient — kill_on_drop
  sends SIGKILL without waiting, so AEC can still unload while pixelpass
  lives. Fix is explicit shutdown().await at both channel-close breaks,
  field order as defence in depth, plus a fake-resource ordering test.
- §5.1 relabelled implementation sites; none of them tag anything today.
- Stop Share citation corrected to :699/:3480.
- D1-D7 resolved; readiness section added.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

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.**
+**Status:** v3.1 — round-4 findings applied; **still BLOCKED pending round-5 re-review** (§14).
-**Date:** 2026-07-21 (v1: 2026-07-19 · v2: 2026-07-20 · Option C adopted 2026-07-20)
+**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 |
+⚠️ **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
-| native call playback (`src/audio/pipewire_impl.rs:374-388`) | set the tag directly in the stream's property dict |
+neither the mpv/VLC spawn (`screenshare/mod.rs:768-775`) nor the notification spawn
-| mpv / VLC (`src/screenshare/mod.rs:761`) | `.env("PULSE_PROP", …)` / `PIPEWIRE_PROPS` on the `Command` |
+(`notify.rs:265-272`) sets any env. The *mechanism* is measured working (below); the
-| `pw-play` / `paplay` / `aplay` (`src/notify.rs:265`) | same |
+*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 <idx>`, enumerate the graph and confirm at
+- peerspeak's `enable()` returns once `wait_for_nodes` sees the virtual **source and
-  least one node carries that `pulse.module.id`. If peerspeak said the AEC is on and no
+  sink** by name (`echo_cancel.rs:132-158`, polling `pactl list sources/sinks short`).
-  such node exists, **do not start fan-out** — report a capability failure and fall back
+  It does **not** wait for the playback `Stream/Output/Audio` hazard leg, which is the
-  to a mode with no echo risk (no audio, or an explicit user override).
+  node we actually need to exclude.
-- **On revocation:** if the validated AEC leg **disappears** while sharing, **stop
+- pixelpass's capture spawns **lazily, on first viewer** (`pixelpass/src/host/mod.rs:300-304`),
-  fan-out immediately.** Do not keep the numeric index and hope. Disappearance means
+  so enumeration happens at a moment peerspeak does not control, and a refusal surfaces
-  either the AEC unloaded (index may be recycled onto something else) or our model of the
+  *after* the ticket has been handed out.
-  graph is wrong; both are fail-closed conditions.
+
+**So validation is a bounded state machine, not a check:**
+
+```
+NotConfigured  ──(--aec=off)──────────────► fan-out proceeds, no AEC exclusion
+Validating     ──(--aec=pulse-module:<i>)─► 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
+### 6.3 Dynamic graph handling — revalidate, don't fire-and-forget
-  move, so a node's move policy is irrelevant.
+
-- Links are created **per port**, not per node; channel-count and layout mismatches are
+A node can appear with incomplete ancestry, pass the predicate, get fanned out, and only
-  a per-port concern.
+*then* receive an inbound link from a filter input or `pixelpass_capture_<pid>.monitor`.
-- Anything unrecognized is **fanned out** (copy is cheap) *unless* it hits an exclusion —
+An add-only listener never sees it. Required:
-  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,
+1. An **initial registry sync barrier** — enumerate to a `core sync`/`done` before
-  not once at start. Nodes created *after* enumeration are exactly the case the spike rig
+   deciding anything; a partially-populated registry is not a graph.
-  was blind to.
+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
+**⚠️ Reordering the fields is NOT sufficient** (round-4 correction). `screenshare_host`
-a comment naming the invariant, and add a regression guard. A drop-order unit test is
+is spawned with `kill_on_drop(true)` (`peerspeak/src/screenshare/mod.rs:402`). Tokio's
-awkward in safe Rust; the cheap version is a compile-time/`#[test]` assertion over field
+`Drop` for such a `Child` *sends* the kill and hands the process to a best-effort orphan
-order via a doc-tested constructor, or a `Drop` impl on a wrapper that records ordering
+reaper — it does **not** wait, and gives no promptness guarantee, especially if the
-into a test-visible slot. **Open decision D4 (§13).**
+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 <sink>.monitor`, not `pw-record --target <sink-node-id>`, 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
+Both reviewers agree on all seven. Recorded as decided; reopen only with new evidence.
-  validated; stop fan-out on revocation) the right trade against "share anyway, warn"?
+
-- **D2** — Capability negotiation: detection today is a `pixelpass --help` substring probe
+- **D1 — fail closed, but only after a bounded validation wait** (§5.3). Never "share
-  (`src/screenshare/mod.rs:245-278`, invoked at `src/core/mod.rs:3368`). This mode must
+  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?
+  independent capabilities. ✅
-- **D3** — Foreign/second AEC policy (§5.4): fail closed, or warn + exclude all
+- **D3 — warn and exclude foreign `echo-cancel-*` nodes** for the first implementation;
-  `echo-cancel-*`?
+  never fan them out. ✅
-- **D4** — What is the actual regression guard for drop order (§7.2)?
+- **D4 — explicit async shutdown + a fake-resource ordering test** (§7.2). A field-order
-- **D5** — Explicit `--aec off` vs flag-absent (§8)?
+  assertion alone is inadequate. ✅
-- **D6** — Do the §10 prerequisites all land first, or only 1, 4 and 5?
+- **D5 — `--aec=off|pulse-module:<idx>`, always passed.** Absence of the flag is not a
-- **D7** — Is there a materially simpler design meeting Joe's ask that three rounds have
+  protocol state (§8). ✅
-  missed?
+- **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.