docs: round 17 — the repair review, the 0c actor review, and 0c's slicing
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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<u32>)` 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 `<name>` and
|
||||
`<name>.monitor` — and `pulsesrc device=<name>.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
|
||||
|
||||
Reference in New Issue
Block a user