From 76c1a13e111fe6da64b3244016dc2e6c6440e84d Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sun, 26 Jul 2026 21:33:57 -0400 Subject: [PATCH] =?UTF-8?q?docs:=20round=2017=20=E2=80=94=20the=20repair?= =?UTF-8?q?=20review,=20the=200c=20actor=20review,=20and=200c's=20slicing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two reviews in one round. The repair planner returned changes-requested (no P1s, four reachable P2s, all applied in pixelpass `9145b2a`); the 0c actor design returned four blocking issues, all accepted, plus a concession on epoch. Recorded because three of them generalise: - A prescribed fix was not implementable as written. "Use `pactl -f json list modules`" is sound reasoning against an API that does not exist: on pactl 17 those records carry no module index, and `unload-module` takes only an index. The reachability rule now applies to prescriptions, not just findings. - The actor's bounded join would have disarmed `Drop` by moving the thread handle into `spawn_blocking` — the same defect shape as round 15's, a defence disarmed exactly when needed. - Epoch was over-specified and my vacuity instinct was right: serial equality is the entire identity guarantee, so epoch is diagnostic and explicitly not a gate. Measured on the live graph rather than argued: recorded module arguments are byte-exact with `@DEFAULT_SINK@` unresolved (both load-bearing for exact-form matching), and two sinks may share one `node.name` with capture attaching to the OLDER one in 3 of 3 trials — so a surviving wedged owner silently steals the next session's capture instead of merely risking a collision. 0c step 2 is sliced into S2–S5 so each lands reviewed. Nothing reopens D6: the connection-owned-sink design is unchanged, and a material part of the growth is pre-existing debt 0c forced into the light. Co-Authored-By: Claude Opus 5 --- docs/screenshare-audio-exclusion-impl-plan.md | 139 ++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/docs/screenshare-audio-exclusion-impl-plan.md b/docs/screenshare-audio-exclusion-impl-plan.md index f4a661c..9ca17ee 100644 --- a/docs/screenshare-audio-exclusion-impl-plan.md +++ b/docs/screenshare-audio-exclusion-impl-plan.md @@ -891,6 +891,145 @@ session that owns the AEC's lifetime. **Trigger to revisit: a fourth tracked chi routine, or a measured teardown exceeds 5 s.** The fix, when triggered, is to drain viewers concurrently while still owned by `shutdown_children` — not to detach them. +**Round 17 (2026-07-26 night) — two reviews: the repair planner (changes-requested, all applied) +and the 0c actor design (four blocking issues, all accepted).** 0c is now sliced, because the +fault-handling surface — not the design — is what grew. + +*The repair planner: no P1s, four reachable P2s and a P3, all applied in `9145b2a`.* + +- **Only the canonical forms are ours.** `classify` recognised any loopback with one + pixelpass-looking endpoint, so a third party's `module-loopback source=some_mic + sink=pixelpass_capture_4242` was ours to unload once that pid died; and a `sink=` token nested + inside a quoted `sink_input_properties` value could be read as a top-level argument. The whole + recorded argument string must now equal what pixelpass itself writes. +- **The matcher's templates are generated from the loader's own renderers.** Hard-coding + `latency_msec=20` beside a matcher means a loader change silently blinds repair to every module + the new build loads — the fail-closed-and-silent class this project has now been bitten by + three times (F2, F13-1, the sticky-uncertainty inversion). `host/audio.rs` loads through the + same renderers, so drift is a compile-time question. Blindness is also *reported*: + `unrecognised_pixelpass_modules` names anything matching `pixelpass_capture_*` that no + canonical form recognises, so a newer pixelpass's shapes cannot make an older `--repair` + quietly clean up nothing. +- **Ordering is not a licence either.** Planning loopbacks before the sink is necessary and + insufficient: an unload can fail or be skipped, and a loopback can appear after planning. The + sink unload is now gated on `sink_still_referenced` against the fresh snapshot — any other + module naming that sink blocks it, ours or not, because the question is what would break rather + than who owns it. +- **Undecidable is not dead.** `Path::exists()` maps a permission error, a missing `/proc` and a + foreign pid namespace all to `false`, which read here as "dead, unload it". Liveness is now + `Alive | Dead | Unknown` via `try_exists()` behind a `/proc/self/stat` preflight, `Unknown` + behaves exactly like `Alive`, and it is reported separately so holding back is visible. +- **⚠️ One prescribed fix was not implementable as written, and measuring first is what caught + it.** The reviewer's fix for fingerprint fidelity was "use `pactl -f json list modules` and + deserialize the complete `argument`". **On pactl 17.0 those records carry no module index at + all** (`"index": null`), and `unload-module` accepts only an index — JSON alone cannot drive + repair. Replacement: **two listings, correlated positionally and checked** (ids and names from + the short listing, exact arguments from JSON; equal counts and equal names at every position or + the run refuses, with retries for a concurrent load). Verified on this host: both listings + return the same 17 modules in an identical name sequence, from 41 physical lines. The check + also turns the reviewer's fabricated-row attack from exploitable into harmless — a crafted + short-listing line has no JSON counterpart, so the sequences misalign and repair stops instead + of unloading an index inferred from text. *This is the reachability rule applied to a + prescription rather than a finding: the chain was valid, the API it assumed did not exist.* +- **Measured before relying on it (pactl 17.0, live server):** recorded arguments come back + byte-for-byte as passed, joined with single spaces, in order, and **`@DEFAULT_SINK@` is not + resolved** to the concrete device. Both facts are load-bearing for exact matching — had either + been false, the P2 fix would itself have been a silent blinding — so both carry a test. +- Normalisation deleted (P3): within one invocation every snapshot comes from one server, so + re-rendering does not happen and normalising only made different arguments compare equal. The + residual ABA window (planned module vanishes, a byte-identical one takes its index) cannot be + closed through an index-only unload API, and is now stated as a limitation in `Fingerprint`'s + own doc comment instead of implied away. +- Five vacuity gaps closed: a raw-pactl-text-to-plan test (the whole planner suite survived a + parser that dropped every argument), per-pid liveness counters over two pids, non-canonical and + nested-quote cases, and a reference-gate test. **One gap deliberately left open and declared:** + a comparator using only `id + args` cannot be killed by a non-vacuous test, because the module + *name* determines which argument grammar can match at all — that field is enforced structurally + by `classify`, and a test appearing to cover it would be the self-satisfying kind. +- **Field-verified twice on the live graph:** the A/B orphan test still removes exactly the two + orphans with the module table otherwise byte-identical, and a new fixture — a dead pid's legacy + sink plus a *non-canonical* loopback naming it — unloads nothing, reports the unrecognised + module, and reports the sink as still referenced. + +*The 0c actor design: four blocking issues, all accepted; the epoch requirement conceded.* + +- **A bounded join must not move the OS handle into `spawn_blocking`.** My ladder would have + taken the thread handle out of the guard to poll it; if the close future is then cancelled or + unwinds, `Drop` finds no handle and can neither poison nor fail-stop, while the blocking task + stays wedged forever and can pin runtime shutdown. This is the **same defect shape as round + 15's** — a defence disarmed exactly when needed. The handle stays owned across every await; + `is_finished()` is polled and `join()` called only once it reports finished. Same rule for the + event task's handle (`await` through `&mut JoinHandle`). +- **`Commit::UnloadNow(id)` cannot forget the id.** An immediate unload can time out or be + cancelled, and a ledger that never recorded the module cannot retry or reconcile it. Slots + become a state machine — `Vacant | Loading { token, expected } | Loaded { fp } | Unloading + { fp }` — with **affine** permits carrying a unique token, so two permitted loads for one slot + cannot both commit. +- **`kill_on_drop` does not roll back a server-side mutation.** A bounded `pactl load-module` + killed after the server created the module but before its id was read leaves a module with no + id anywhere. So an ambiguous load requires **bounded reconciliation by fingerprint** — reusing + repair's classification idea inside the live session, never its dead-pid policy — before any + further capture may start. Related: cancellation must never be `select!`ed against + `Command::output()`, or a completed load's id is dropped on the floor. +- **`_exit` is right, but the pre-exit sequence must not be able to block.** Event emission, + stdio flushing and tracing all take locks a wedged thread may hold, so the watchdog able to + `_exit` past a stalled diagnostic has to be **armed before** the wedge is detected, not created + in response to it. And `_exit` skips `CaptureHandle::Drop`, so `gst-launch-1.0` and any + in-flight `pactl` need parent-death/process-group containment or they outlive the host that + reported its own death — with gst still holding screen-capture resources. +- **Epoch conceded, and my vacuity instinct was right.** `object.serial` is unique and never + reused while global ids are, so "the object at this id still has the serial I recorded" is + complete proof of identity; there is no same-core interleaving that serial equality misses. + Epoch is carried for diagnostics and explicitly **not** a gate. It would only become + load-bearing across a daemon incarnation or an actor reconnect, and the design makes core + failure terminal with no reconnect — if that changes, the right answer is a core-incarnation + nonce, not a "something churned" counter that invalidates observations on unrelated traffic. +- **"Unjoinability, not slowness" is not literally implementable** and the wording is corrected: + no bounded observation distinguishes "returns one millisecond later" from "never returns", so + the death condition is *failure to terminate within the post-cancellation policy deadline*. + Two budgets, not one — a running MainLoop quitting is a different question from an + initialisation call returning after cancellation, and the second is normally longer. +- **`GraphCmd::Route(Vec)` is deleted rather than fixed.** Matching and routing stay inside + the actor's registry callback, where removals are already ordered against routes in-thread, so + the privacy race is not introduced at all. For phase 6 the rule is structural: the only + addressable type is an `ObservedNode { global_id, serial, epoch }` constructible solely from + the actor's own observation, kept private and non-`Copy`, revalidated on serial immediately + before any mutation. A bare id is not addressable. +- **An unacked `ClearRoutes` is not a wedge** (agreed), with one qualification taken: a stream + setting `node.dont-reconnect`/`node.dont-fallback` may be left silent rather than moved back to + the default, so the outcome is surfaced as `ClearRoutesUnconfirmed` rather than treated as + benign. Separately, blindly clearing `target.object` can erase a target the user set manually — + the prior value must be recorded and restored only while it is still pixelpass-owned. +- **One terminal fault needs a coordinator, not an emitter.** If the actor emits `CoreError` + immediately and the subsequent teardown then fails to join, peerspeak never learns the process + is fail-stopping. Actor faults are internal *candidates*; the tokio-side coordinator emits + exactly one final fault, and `Wedged` overrides any earlier candidate. Because a callback panic + can cross `extern "C"` and abort before any event is produced, **peerspeak must treat + unexpected stdout EOF as a synthetic terminal fault** rather than trusting that a JSON line + arrives. + +*Measured for the actor argument (3 of 3 trials, live graph):* pipewire-pulse accepts **two sinks +with an identical `node.name`** — no rename, no suffix, no refusal, both visible as `` and +`.monitor` — and `pulsesrc device=.monitor` attached to the **older** one every time. +So a surviving wedged owner does not merely risk a collision: it **silently steals the next +session's capture** while the loopbacks feed the new sink. That retires "detach and carry on" as +an option, and it is the evidence behind rejecting session-unique sink names (which would trade a +fail-stop ownership fault for silent accumulation, and re-open the discovery grammar 0c step 1 +just closed and field-proved). + +**0c step 2 is therefore sliced, and the slices land and are reviewed independently.** Nothing +here reopens D6 — the connection-owned-sink design is unchanged; what grew is the process- +lifecycle and fault surface, and a material part of it is pre-existing debt 0c forced into the +light (the `abort()` orphan race, the unbounded join, peerspeak advertising a dead share): + +| slice | scope | why it can land alone | +|-------|-------|-----------------------| +| S1 | repair planner (`919d5bd` + `9145b2a`) | done; awaiting re-review, then merge | +| S2 | peerspeak host-fault path: always-on notice channel, EOF synthesis, session-scoped fault, clear `is_sharing` + presence ticket, `ScreenShareStopped` then error | fixes a defect **today** — a dead share stays advertised — and is independent of the actor | +| S3 | pixelpass ledger transactions + ambiguous-load reconciliation + child containment + pre-armed watchdog + poison state machine + supervisor health arm | fixes the `abort()` orphan race **today**; no libpipewire work | +| 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 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