mollusk/peerspeak
Public Access
1
0
Fork 0
Code Issues Pull Requests 3 Actions 1 Packages Projects Releases 10 Wiki Activity

core/teardown: an unconfirmed stop is not a clean stop

Browse Source 
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>
This commit is contained in:
mollusk
2026-07-26 19:43:04 -04:00
co-authored by Claude Opus 5
parent 3aa768af52
commit 9f06741b99
3 changed files with 185 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/screenshare-audio-exclusion-impl-plan.md
+41 -4
View File
@@ -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<StopOutcome>`, 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
src/core/mod.rs
+18 -2
View File
@@ -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),
src/core/teardown.rs
+126 -29
View File
@@ -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<C: ChildProcess> ReapOnDrop<C> {
/// 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<C: ChildProcess> ReapOnDrop<C> {
// 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<C: ChildProcess, G> ScreenshareTeardown<C, G> {
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<StopOutcome> {
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<C: ChildProcess, G> ScreenshareTeardown<C, G> {
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<C: ChildProcess, G> ScreenshareTeardown<C, G> {
/// 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<C: ChildProcess, G> ScreenshareTeardown<C, G> {
#[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"]);
}