c2d82acf8262b34dd8a6564b7eab6019810ca835
5
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 mollusk
|
2c2b861516 | feat(screenshare): integrate desktop audio exclusion | ||
 molluskandClaude Opus 5
|
9f06741b99 |
core/teardown: an unconfirmed stop is not a clean stop
Codex's re-review of the branch returned "approve with follow-ups" — no blocking findings, five P3s. All five are applied here rather than carried as debt, since each is a few lines. The one with user-visible consequences: `stop_host` returned a bare "was sharing" bool, so the single case where the availability-first policy gives up (SIGKILL queued, reap never confirmed) still sent `ScreenShareStopped` with nothing else. The UI would say sharing had ended while pixelpass might still be alive and fanning out — a claim the user cannot see through. `shutdown` now returns `StopOutcome`, `stop_host` returns `Option<StopOutcome>`, and an unconfirmed *user-initiated* stop raises a UI error naming the stray process. Session and viewer teardown discard the outcome deliberately: nobody is waiting on an answer there, and the residual risk is already logged. Also: the three failure diagnoses in `shutdown` (the signal never left, the child ignored it, the wait itself broke) were collapsed into one log line and are now distinct — they mean different things to whoever reads the log. The second cancellation gate is the one worth keeping. The review pointed out that all cancellation coverage sat in the *graceful* wait, so a mutant that disarmed the wrapper between the two waits would survive. It was right, with a wrinkle: the naive mutant does not compile, because the child is borrowed from `self` for the whole function — the borrow checker is doing real work here. The restructured form (`self.child.take()` once cooperation has failed) does compile, and the pre-existing mid-wait test passes it. `cancelling_shutdown_after_the_kill_leaves_the_fallback_armed` kills it. Mutation-verified, both new gates: reporting an unconfirmed stop as `Reaped` fails exactly `a_failed_wait_is_not_treated_as_a_confirmed_reap`; disarming between the waits fails the new cancellation test (and the failed-wait test, which also asserts armedness) while leaving the old mid-wait test green — which is the proof the new test is not redundant. The logging split is diagnostics only and has no gate; said plainly rather than dressed up as covered. Docs: the "four mutations" line is now an explicit table naming each target and its test, with 0c's pair counted under 0c; and the aggregate teardown latency is recorded as a deferred item with a trigger (a fourth routine child, or a measured teardown over 5 s) instead of an unwritten known cost. 638 lib tests, clippy clean, fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
molluskandClaude Opus 5
|
d8b8fd79cf |
core/teardown: stay armed across the wait, and never call a failed wait a reap
Codex's review of the two commits below returned "changes requested" with two blocking findings. Both were real. 1. `ReapOnDrop` disarmed itself across the async wait. `shutdown` moved the child out of the wrapper with `take()` before the first `.await`, so if that future was cancelled or unwound mid-wait, the raw child dropped with nothing but `kill_on_drop` (signals, does not reap) while `Drop` found `None` and did nothing — the AEC could then unload over a live child. That is precisely the hole the type exists to close, left open for the duration of every wait. The child now stays owned by `self` across every await and is released only on a *confirmed* reap. 2. A failed wait was silently converted into success, and the hard-kill path was unbounded. `wait_reaped` discarded `io::Result`, so a wait error made the timeout return `Ok` and shutdown returned as though the reap were confirmed; meanwhile a process stuck in uninterruptible sleep after SIGKILL could wedge the core command loop forever. The trait now preserves the result, both waits are bounded, and the conflict case has an explicit written policy: we choose availability, leave the child owned so the bounded Drop retry stays armed, and log the residual risk rather than hiding it. Codex also showed the test double was flattering the implementation in four ways. All four are closed: the fake can now be cancelled mid-wait, can fail its wait, and can take several polls to die, and the grace is pinned independently. That last one caught a flaw in my own gate. The elapsed-time assertion compares against `STOP_GRACE` itself, so setting the constant to zero leaves it vacuously true — both sides move together. `the_grace_is_a_real_interval` pins the constant to a band instead, and now kills that mutation directly. Mutation-verified again, five mutants, each killed by its own gate: disarming the wrapper (cancellation test), treating a wait error as success (failed-wait test, exactly one), a zero grace (the new band test), a single poll instead of the drop loop (delayed-reap test, exactly one), reversed field order (the two ordering tests). Also applies the matrix adjudication, which Codex and I reached independently: teardown moves out of the reliable close arm to ONE unconditional site after the loop, so every `break` is covered structurally — including any added later — instead of duplicating teardown across one live arm and one provably dead one. 637 lib tests, clippy clean, fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
molluskandClaude Opus 5
|
92a64465a4 |
core/teardown: ask pixelpass to stop before killing it
The peerspeak half of phase 0c (design v3.4 §7.4 item 1). Stop Share and session teardown both went straight to `Child::kill()`, i.e. SIGKILL, which skips pixelpass's own cleanup and leaks one null-sink module every time. `ReapOnDrop::shutdown` now asks first: SIGINT, a bounded 2 s wait, then SIGKILL only if the child ignored the request. SIGINT specifically, not SIGTERM — pixelpass installs only a `ctrl_c()` handler, so SIGTERM would take the default disposition and be indistinguishable from SIGKILL. Signalling by pid is safe against pid reuse here: we have not reaped the child, so it is a zombie whose pid the kernel reserves until we wait it, and the pid cannot name a stranger. (Same reasoning that dismissed pixelpass bug #6.) The grace is 2 s because it is awaited inline in the core command loop, so it is also how long a wedged child can delay other commands. A healthy pixelpass never spends it. The drop/unwind path deliberately stays a hard kill: `Drop` cannot await, and there the ordering invariant (§7.1) outranks tidiness. Once the pixelpass half of 0c lands, the capture sink is connection-owned and that path stops leaking by construction. `libc` becomes a direct unix-only dependency, pinned to 0.2.186 — the version already in the tree via alsa/cpal/tokio — so Cargo.lock gains one line and no new code enters the build. Mutation-verified, five mutations, each killing its own gate: no wait (6 fail), reversed field order (2, reap test green), no reap loop (2, ordering test green), no SIGINT (6), no SIGKILL fallback (exactly 1 — the wedged-child test). 633 lib tests, clippy clean, fmt clean. Not yet field-tested: the live Stop Share gate (SIGINT sent, child exits within the bound, no fallback kill on the normal path) still owes a real run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
molluskandClaude Opus 5
|
6ba763774d |
core/teardown: reap the screen-share children before the AEC unloads
Phase 0b, fixes 1-3 of design v3.4 §7.2 (decision D4). The invariant is that the echo-cancel module must not unload while a pixelpass host is alive and fanning out; two paths have to honour it and only one is code we get to run. The explicit path: `ActiveSession::shutdown` now awaits `ScreenshareTeardown::shutdown_children`, and the reliable command channel's close arm tears the session down explicitly instead of letting it drop on the way out of `run_core_loop`. The drop/unwind path: the ordering-critical fields move out of `ActiveSession` into `core::teardown::ScreenshareTeardown`, where `echo_cancel` is the LAST declared field and therefore the last dropped. Previously it was declared first (`:682`, ahead of `screenshare_host` at `:685`), so an unwind unloaded the AEC while the host was still live — and unwind is reachable, the core is full of `unwrap()` and has no `panic=abort` profile. Killing is not enough. `kill_on_drop(true)` only signals: it hands the child to the runtime's orphan queue and returns, which an unwinding runtime may never drain. `ReapOnDrop` blocks on a bounded 250 ms budget until the child is really gone, because a bounded stall beats unloading the AEC out from under a live pixelpass. Everything is generic over a narrow `ChildProcess` trait and over the guard type, so ordering is unit-testable without spawning processes or loading PipeWire modules — the seam idiom already used by `replace_viewer_index`. Mutation-verified, and the plan's demand that mutations 4 and 5 prove *different* defenses holds: reversing the field order fails only the AEC-ordering tests and leaves the reap test green; removing the reap loop fails only the reap tests and leaves the ordering test green. Removing the explicit wait fails the explicit-path tests. 631 lib tests, clippy clean, fmt clean. ⚠️ Mutations 1 and 2 of the pinned matrix do not both exist: the best-effort wake arm is unreachable by construction, twice over. Documented at the site; adjudication owed in the impl plan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |