diff --git a/docs/screenshare-audio-exclusion-impl-plan.md b/docs/screenshare-audio-exclusion-impl-plan.md index 406ac81..f4a661c 100644 --- a/docs/screenshare-audio-exclusion-impl-plan.md +++ b/docs/screenshare-audio-exclusion-impl-plan.md @@ -832,10 +832,21 @@ mutation is retired as vacuous.** Reached independently by both reviewers, then - **Rejected: an integration test built to preserve the number five.** It would pay for a full iroh core plus test-only observability and prove only that a method was called — not the ordering invariant, which is the thing that actually breaks. -- The 0b gate is therefore **four mutations** (no wait · reversed field order · no reap loop · - plus the 0c pair below), each killed by its own named test, with 4-vs-5 verified separated: - reversing the field order leaves the reap test green and removing the reap loop leaves the - ordering test green. +- The 0b gate is therefore **four mutations**, enumerated exactly (round 16 P3-5 — the earlier + wording said "three plus the 0c pair", which reads as five and blurred what 0b owns): + + | # | mutation | killed by | status | + |---|----------|-----------|--------| + | 1 (old gate 3) | remove the wait after the host kill | `explicit_shutdown_reaps_the_host_before_the_aec_can_unload` | killed now | + | 2 (old gate 4) | reverse `ScreenshareTeardown`'s field declaration order | `the_aec_unloads_after_the_children_on_the_drop_path` | killed now | + | 3 (old gate 5) | remove the reap loop from `ReapOnDrop::drop` | `dropping_a_guard_kills_and_then_reaps_the_child` | killed now | + | 4 (old 1) | remove the teardown at the hoisted post-loop call site | — | **deferred to the phase-9 row** *drop the controller / close the command channel while sharing* | + + 4-vs-5 separation verified: reversing the field order leaves the reap test green, and + removing the reap loop leaves the ordering test green. **0c's own pair (no SIGINT · no + SIGKILL fallback) is counted under 0c, not here**, along with the round-15/16 additions + (disarm the wrapper at entry · disarm it between the waits · treat a wait error as a reap · + report an unconfirmed stop as clean · zero the grace). **Round 15 (2026-07-26) — review of the 0b/0c-peerspeak implementation returned two blocking findings, both accepted.** Recorded because both are the same shape: a defence that existed @@ -854,6 +865,32 @@ but was disarmed exactly when it was needed. elapsed-time assertion compared against `STOP_GRACE` itself, so zeroing the constant left it trivially true. `the_grace_is_a_real_interval` now pins the constant to a band. +**Round 16 (2026-07-26) — the re-review of the 0b/0c-peerspeak fixes returned *approve with +follow-ups*: no blocking findings, five P3s, all five applied before the merge.** The two that +carry design content: + +- **An unconfirmed stop was reported to the user as a clean one.** `stop_host` returned a bare + "was sharing" bool, so the one case where availability-first gives up (SIGKILL queued, reap + never confirmed) still emitted `ScreenShareStopped` with no warning — the UI would say + sharing ended while pixelpass might still be fanning out. `ReapOnDrop::shutdown` now returns + `StopOutcome`, `stop_host` returns `Option`, and an `Unconfirmed` user-initiated + stop raises a UI error naming the stray process. Session/viewer teardown discards the outcome + on purpose: no user is waiting on an answer there and the risk is already logged. +- **Cancellation coverage only reached the graceful wait.** The mid-wait test could not kill a + mutant that disarmed the wrapper *between* the two waits. Verified: the naive form of that + mutant does not compile (the child is borrowed from `self`), but the restructured form — + `self.child.take()` once cooperation has failed — compiles, and the pre-existing test passes + it. `cancelling_shutdown_after_the_kill_leaves_the_fallback_armed` kills it. + +**Deferred item — aggregate teardown latency (round 16 P3-4).** Bounds are per child, not per +teardown. Sequential drain gives `2 × STOP_GRACE` per unconfirmed child inline (≈4 s), plus +`REAP_BUDGET` (250 ms) per child on the `Drop` path: three wedged children ≈6 s of command-loop +stall, ≈12.75 s worst case including drop retries. Accepted as-is for 0b — one host plus one or +two viewers is the real shape, and concurrency here would mean detaching children from the +session that owns the AEC's lifetime. **Trigger to revisit: a fourth tracked child becomes +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 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 diff --git a/src/core/mod.rs b/src/core/mod.rs index 1f1fa46..6a3c78e 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -3494,8 +3494,24 @@ async fn run_core_loop( CoreCommand::StopScreenShare => { current_sharing = None; if let Some(session) = &mut active_session { - if session.teardown.stop_host().await { - crate::log_msg("Screen share host stopped"); + match session.teardown.stop_host().await { + None => {} + Some(teardown::StopOutcome::Reaped) => { + crate::log_msg("Screen share host stopped"); + } + // We gave up waiting rather than freeze the client, so + // pixelpass may still be alive and serving. Saying + // "stopped" and nothing else would be a lie the user + // cannot see through (round-16 review, P3-2). + Some(teardown::StopOutcome::Unconfirmed) => { + let _ = ui_tx + .send(UiEvent::Error( + "Couldn't confirm the screen-share process exited — \ + it may still be sharing. Check for a stray pixelpass." + .into(), + )) + .await; + } } let self_state = presence.to_state( is_muted.load(Ordering::Relaxed), diff --git a/src/core/teardown.rs b/src/core/teardown.rs index 216f073..2a1caa3 100644 --- a/src/core/teardown.rs +++ b/src/core/teardown.rs @@ -126,6 +126,21 @@ impl ChildProcess for tokio::process::Child { } } +/// Did the explicit stop path actually confirm the child was reaped? +/// +/// The distinction is not cosmetic: on [`Unconfirmed`](Self::Unconfirmed) we +/// deliberately stopped waiting (see [`ReapOnDrop::shutdown`]), so pixelpass may +/// still be alive and fanning out. A user-initiated Stop Share must not report +/// that as a clean stop. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[must_use = "an unconfirmed stop means the child may still be sharing"] +pub(super) enum StopOutcome { + /// The child is gone and has been reaped. + Reaped, + /// We could not confirm the reap within the bound and gave up waiting. + Unconfirmed, +} + /// A child that is killed **and reaped** when it is dropped. /// /// The explicit path calls [`shutdown`](Self::shutdown), which releases the @@ -180,23 +195,37 @@ impl ReapOnDrop { /// this future at that moment and the raw child would drop with nothing but /// `kill_on_drop` (which signals without reaping) while `Drop` below found /// `None` and did nothing — the precise hole this type exists to close. - pub(super) async fn shutdown(&mut self) { + pub(super) async fn shutdown(&mut self) -> StopOutcome { let Some(child) = self.child.as_mut() else { - return; + return StopOutcome::Reaped; }; - let _ = child.request_stop(); - if let Ok(Ok(())) = tokio::time::timeout(STOP_GRACE, child.wait_reaped()).await { - self.child = None; - return; + // Three different things can go wrong here and they want three + // different operator diagnoses: the signal never left (a runtime or + // permission fault), the child ignored it (a wedged pixelpass), or the + // wait itself broke (we no longer know anything about the child). + // Collapsing them into one line was P3-1 of the round-16 review. + if let Err(e) = child.request_stop() { + crate::log_msg(&format!( + "teardown: could not ask {} to stop: {e}", + self.label + )); + } + match tokio::time::timeout(STOP_GRACE, child.wait_reaped()).await { + Ok(Ok(())) => { + self.child = None; + return StopOutcome::Reaped; + } + Ok(Err(e)) => crate::log_msg(&format!( + "teardown: waiting for {} failed ({e}); killing it", + self.label + )), + Err(_) => crate::log_msg(&format!( + "teardown: {} ignored the graceful stop within {STOP_GRACE:?}; killing it", + self.label + )), } - // Either the grace expired or the wait itself failed. A failed wait is - // not a reap, so both land here. - crate::log_msg(&format!( - "teardown: {} did not stop gracefully; killing it", - self.label - )); if let Err(e) = child.start_kill() { crate::log_msg(&format!( "teardown: {} could not be killed: {e}", @@ -209,19 +238,22 @@ impl ReapOnDrop { // permanently frozen app is a worse failure than the risk below. if let Ok(Ok(())) = tokio::time::timeout(STOP_GRACE, child.wait_reaped()).await { self.child = None; - return; + return StopOutcome::Reaped; } // Explicit policy for the one case where the two guarantees conflict: // we could not confirm the reap and will NOT block indefinitely, so we // give up availability-first and leave the child owned — `Drop`'s // bounded retry stays armed, and the AEC may unload over a child that - // is still somehow alive. That residual risk is logged, not silent. + // is still somehow alive. That residual risk is logged, not silent — + // and, for a user-initiated stop, reported to the caller rather than + // dressed up as success. crate::log_msg(&format!( "teardown: {} could not be confirmed dead; the echo-cancel module \ may unload while it lives", self.label )); + StopOutcome::Unconfirmed } /// Is the `Drop` fallback still armed? Test-only: the arming rule is the @@ -297,13 +329,13 @@ impl ScreenshareTeardown { self.host = Some(ReapOnDrop::new(child, "screen-share host")); } - /// Stop sharing: kill the host and wait for it to be reaped. - pub(super) async fn stop_host(&mut self) -> bool { - let Some(mut host) = self.host.take() else { - return false; - }; - host.shutdown().await; - true + /// Stop sharing: kill the host and wait for it to be reaped. `None` if we + /// were not sharing; otherwise whether the reap was actually confirmed — + /// the caller owns telling the user, since an unconfirmed stop may leave + /// pixelpass fanning out after the UI says sharing ended. + pub(super) async fn stop_host(&mut self) -> Option { + let mut host = self.host.take()?; + Some(host.shutdown().await) } /// Drop viewers whose player window has already closed, so the list only @@ -319,7 +351,10 @@ impl ScreenshareTeardown { return false; }; let (_, mut old) = self.viewers.remove(pos); - old.shutdown().await; + // A viewer is our own player window, not the thing peers are watching: + // an unconfirmed reap is already logged, and there is no user decision + // riding on it the way there is for Stop Share. + let _ = old.shutdown().await; true } @@ -335,12 +370,16 @@ impl ScreenshareTeardown { /// the drop/unwind path nothing calls it and field order carries the /// invariant instead. pub(super) async fn shutdown_children(&mut self) { + // Outcomes are discarded on purpose: this runs on the session/teardown + // path, where the policy is already availability-first and the residual + // risk is logged by `shutdown` itself. There is no user still waiting + // on an answer here, unlike `stop_host`. if let Some(host) = &mut self.host { - host.shutdown().await; + let _ = host.shutdown().await; } self.host = None; for (_, viewer) in self.viewers.iter_mut() { - viewer.shutdown().await; + let _ = viewer.shutdown().await; } self.viewers.clear(); } @@ -348,7 +387,7 @@ impl ScreenshareTeardown { #[cfg(test)] mod tests { - use super::{ChildProcess, ReapOnDrop, STOP_GRACE, ScreenshareTeardown}; + use super::{ChildProcess, ReapOnDrop, STOP_GRACE, ScreenshareTeardown, StopOutcome}; use std::future::Future; use std::sync::{Arc, Mutex}; use std::time::Duration; @@ -423,6 +462,17 @@ mod tests { } } + /// A child that ignores SIGINT *and* does not die the instant it is + /// killed — the only shape that lets a test reach the post-SIGKILL + /// wait and still be reaped by the `Drop` poll loop afterwards. + fn wedged_then_dies_after_polls(log: &Log, label: &'static str, polls: u32) -> Self { + Self { + honours_interrupt: false, + polls_before_death: polls, + ..Self::new(log, label) + } + } + /// A child whose `wait` fails. A failed wait is not a confirmed reap, /// so it must not be reported as one. fn wait_fails(log: &Log, label: &'static str) -> Self { @@ -588,7 +638,7 @@ mod tests { let mut t = teardown(&log); t.set_host(FakeChild::new(&log, "host")); - assert!(t.stop_host().await); + assert_eq!(t.stop_host().await, Some(StopOutcome::Reaped)); assert_eq!(entries(&log), vec!["host:sigint", "host:reap"]); assert!( @@ -664,6 +714,49 @@ mod tests { assert_eq!(entries(&log), vec!["host:sigint", "host:kill", "host:reap"]); } + /// The test above only ever cancels during the *graceful* wait, so a + /// mutation that disarmed the wrapper between the two waits would survive + /// it (round-16 review, P3-3). This one cancels during the post-SIGKILL + /// wait — the window where we have already given up on cooperation and the + /// `Drop` fallback is the only thing left. + #[tokio::test(start_paused = true)] + async fn cancelling_shutdown_after_the_kill_leaves_the_fallback_armed() { + let log = log(); + // Ignores SIGINT, so the grace expires and we reach the kill; then + // survives three polls, so the second wait is still pending when we + // cancel, and the drop loop still gets to reap it. + let mut guard = ReapOnDrop::new( + FakeChild::wedged_then_dies_after_polls(&log, "host", 3), + "host", + ); + + assert!( + tokio::time::timeout(STOP_GRACE + STOP_GRACE / 4, guard.shutdown()) + .await + .is_err(), + "we should have been cancelled inside the post-kill wait" + ); + assert_eq!( + entries(&log), + vec!["host:sigint", "host:kill"], + "the graceful stop must have expired and escalated before we cancelled" + ); + assert!( + guard.is_armed(), + "cancelling after the kill must not disarm the drop fallback either" + ); + + drop(guard); + // The fake's `start_kill` is idempotent, so `Drop` re-signalling an + // already-killed child adds no entry; the *reap* is what proves the + // fallback ran to completion after we abandoned the wait. + assert_eq!( + entries(&log), + vec!["host:sigint", "host:kill", "host:reap"], + "Drop must poll until the child is actually gone" + ); + } + /// A failed wait is not a reap. Reporting it as one is how the AEC ends up /// unloading over a child that is still alive. #[tokio::test(start_paused = true)] @@ -671,7 +764,11 @@ mod tests { let log = log(); let mut guard = ReapOnDrop::new(FakeChild::wait_fails(&log, "host"), "host"); - guard.shutdown().await; + assert_eq!( + guard.shutdown().await, + StopOutcome::Unconfirmed, + "a stop we could not confirm must not be reported as a clean one" + ); assert!( !entries(&log).contains(&"host:reap".to_string()), @@ -746,11 +843,11 @@ mod tests { let log = log(); let mut t = teardown(&log); assert!(!t.is_sharing()); - assert!(!t.stop_host().await, "not sharing: nothing to stop"); + assert_eq!(t.stop_host().await, None, "not sharing: nothing to stop"); t.set_host(FakeChild::new(&log, "host")); assert!(t.is_sharing()); - assert!(t.stop_host().await); + assert_eq!(t.stop_host().await, Some(StopOutcome::Reaped)); assert!(!t.is_sharing()); assert_eq!(entries(&log), vec!["host:sigint", "host:reap"]); }