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>
890 lines
34 KiB
Rust
890 lines
34 KiB
Rust
//! Destruction-order guarantees for the screen-share children and the
|
|
//! echo-cancel module (phase 0b of the screenshare audio-exclusion plan;
|
|
//! design v3.4 §7.1–§7.2, decision D4).
|
|
//!
|
|
//! # The invariant
|
|
//!
|
|
//! > **The echo-cancel module must not unload while a pixelpass host is alive
|
|
//! > and fanning out.**
|
|
//!
|
|
//! If it does, the AEC's virtual nodes vanish from under a live pixelpass that
|
|
//! still holds link proxies and a stale module index. Phase 6 makes this sharp
|
|
//! — it is the first phase whose objects live only as long as pixelpass does —
|
|
//! so the ordering guarantee has to exist *before* it.
|
|
//!
|
|
//! Two paths have to honour it, and only one of them is code we get to run:
|
|
//!
|
|
//! 1. **The explicit path** — [`ScreenshareTeardown::shutdown_children`], awaited
|
|
//! by `ActiveSession::shutdown` before the guard is dropped.
|
|
//! 2. **The drop/unwind path** — nobody calls anything. The core has numerous
|
|
//! `unwrap()` sites and no `panic=abort` profile, so unwind is reachable, and
|
|
//! on that path the only thing standing between us and a violated invariant
|
|
//! is *field declaration order* plus [`ReapOnDrop`].
|
|
//!
|
|
//! Hence the two structural rules enforced here:
|
|
//!
|
|
//! - `echo_cancel` is the **last declared field** of [`ScreenshareTeardown`].
|
|
//! Rust drops fields in declaration order, so last-declared is last-dropped.
|
|
//! This is not a style choice; reversing it reintroduces the bug.
|
|
//! - Killing is not enough — a child must be **reaped**. `kill_on_drop(true)`
|
|
//! only *signals*; it hands the child to the runtime's orphan queue and
|
|
//! returns, which on an unwinding runtime may never be drained. [`ReapOnDrop`]
|
|
//! therefore blocks, briefly and boundedly, until the child is actually gone.
|
|
//!
|
|
//! Everything here is generic over [`ChildProcess`] and over the guard type so
|
|
//! the ordering is unit-testable without spawning processes or loading PipeWire
|
|
//! modules — the same seam idiom as `replace_viewer_index` and
|
|
//! `rebuild_with_fallback` in the parent module.
|
|
|
|
use std::future::Future;
|
|
use std::time::{Duration, Instant};
|
|
|
|
/// How long [`ReapOnDrop::drop`] will block waiting for a killed child to be
|
|
/// reaped before giving up and logging. This runs on the unwind path, so it is
|
|
/// a deliberate trade: a bounded stall is preferable to unloading the AEC out
|
|
/// from under a live pixelpass, and unbounded blocking in a `Drop` is not.
|
|
const REAP_BUDGET: Duration = Duration::from_millis(250);
|
|
|
|
/// Poll interval while waiting out [`REAP_BUDGET`].
|
|
const REAP_POLL: Duration = Duration::from_millis(5);
|
|
|
|
/// How long a child gets to honour the graceful stop before it is killed.
|
|
///
|
|
/// A healthy pixelpass exits in well under this, so the normal path never
|
|
/// spends it; only a wedged child does. It is awaited inline in the core
|
|
/// command loop, so it is also how long a wedged child can delay other
|
|
/// commands — hence seconds, not tens of seconds.
|
|
const STOP_GRACE: Duration = Duration::from_secs(2);
|
|
|
|
/// The child-process operations the teardown ordering actually depends on.
|
|
///
|
|
/// Deliberately narrow, and deliberately not `ExitStatus`-shaped: the ordering
|
|
/// rules care only about *whether* a child has been signalled and *whether* it
|
|
/// has been reaped, so the test double is a few lines instead of a fabricated
|
|
/// exit status.
|
|
pub(super) trait ChildProcess {
|
|
/// Ask the child to exit **gracefully**, so it can run its own cleanup.
|
|
/// Does **not** wait, and is not guaranteed to be honoured.
|
|
fn request_stop(&mut self) -> std::io::Result<()>;
|
|
|
|
/// Signal the child to die. Does **not** wait.
|
|
fn start_kill(&mut self) -> std::io::Result<()>;
|
|
|
|
/// Poll once. `true` once the child has exited **and been reaped**.
|
|
fn try_reap(&mut self) -> bool;
|
|
|
|
/// Wait until the child has exited and been reaped.
|
|
///
|
|
/// The `io::Result` is load-bearing and must not be discarded by callers:
|
|
/// a failed wait is *not* a confirmed reap, and treating it as one is how
|
|
/// the AEC ends up unloading over a live child.
|
|
fn wait_reaped(&mut self) -> impl Future<Output = std::io::Result<()>> + Send;
|
|
}
|
|
|
|
impl ChildProcess for tokio::process::Child {
|
|
/// **SIGINT, not SIGTERM.** pixelpass installs only a `tokio::signal::ctrl_c()`
|
|
/// handler (`pixelpass/src/common/signal.rs`), so SIGTERM would be the default
|
|
/// disposition — instant death, no cleanup — which is indistinguishable from
|
|
/// SIGKILL for our purposes.
|
|
///
|
|
/// Signalling by pid is safe against pid reuse here because we have not
|
|
/// reaped this child: an exited-but-unreaped child is a zombie whose pid the
|
|
/// kernel reserves until we `wait` it, so the pid cannot name a stranger.
|
|
#[cfg(unix)]
|
|
fn request_stop(&mut self) -> std::io::Result<()> {
|
|
let Some(pid) = self.id() else {
|
|
// Already reaped — nothing to signal.
|
|
return Ok(());
|
|
};
|
|
// SAFETY: `kill` is async-signal-safe and takes no pointers; the pid is
|
|
// this process's own unreaped child (see above).
|
|
if unsafe { libc::kill(pid as libc::pid_t, libc::SIGINT) } == 0 {
|
|
Ok(())
|
|
} else {
|
|
Err(std::io::Error::last_os_error())
|
|
}
|
|
}
|
|
|
|
/// Windows has no SIGINT to send to another process without attaching to its
|
|
/// console, so the graceful request degrades to the hard kill and the
|
|
/// bounded wait below simply returns early.
|
|
#[cfg(not(unix))]
|
|
fn request_stop(&mut self) -> std::io::Result<()> {
|
|
tokio::process::Child::start_kill(self)
|
|
}
|
|
|
|
fn start_kill(&mut self) -> std::io::Result<()> {
|
|
tokio::process::Child::start_kill(self)
|
|
}
|
|
|
|
fn try_reap(&mut self) -> bool {
|
|
matches!(self.try_wait(), Ok(Some(_)))
|
|
}
|
|
|
|
async fn wait_reaped(&mut self) -> std::io::Result<()> {
|
|
self.wait().await.map(|_| ())
|
|
}
|
|
}
|
|
|
|
/// 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
|
|
/// child only once its reap is *confirmed*, so the `Drop` below is a no-op
|
|
/// afterwards but stays armed through every await until then. `Drop` is the
|
|
/// last-ditch protection for the panic/unwind/cancellation paths.
|
|
pub(super) struct ReapOnDrop<C: ChildProcess> {
|
|
/// `None` once the child has been reaped through the explicit path.
|
|
child: Option<C>,
|
|
/// Names the child in the reap-timeout log line.
|
|
label: &'static str,
|
|
}
|
|
|
|
impl<C: ChildProcess> ReapOnDrop<C> {
|
|
pub(super) fn new(child: C, label: &'static str) -> Self {
|
|
Self {
|
|
child: Some(child),
|
|
label,
|
|
}
|
|
}
|
|
|
|
/// Poll once, without killing. `true` if the child has exited on its own —
|
|
/// used to sweep player windows the user has already closed.
|
|
pub(super) fn has_exited(&mut self) -> bool {
|
|
match &mut self.child {
|
|
Some(child) => {
|
|
if child.try_reap() {
|
|
self.child = None;
|
|
true
|
|
} else {
|
|
false
|
|
}
|
|
}
|
|
// Already reaped through the explicit path.
|
|
None => true,
|
|
}
|
|
}
|
|
|
|
/// Stop the child gracefully if it will go, and by force if it will not.
|
|
/// Waits for it to be reaped either way. Idempotent.
|
|
///
|
|
/// Ask, then insist (design v3.4 §7.4): a pixelpass host that gets SIGINT
|
|
/// unloads its capture sink on the way out, whereas SIGKILL skips that and
|
|
/// leaks a null-sink module on every Stop Share.
|
|
///
|
|
/// The wait is the point: returning after signalling would let the caller
|
|
/// proceed to unload the AEC while the child is still running.
|
|
///
|
|
/// ⚠️ The child stays owned by `self` across every `.await`, and is released
|
|
/// **only after a confirmed reap**. Taking it out first would disarm the
|
|
/// `Drop` fallback for exactly as long as the wait lasts: cancel or unwind
|
|
/// 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) -> StopOutcome {
|
|
let Some(child) = self.child.as_mut() else {
|
|
return StopOutcome::Reaped;
|
|
};
|
|
|
|
// 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
|
|
)),
|
|
}
|
|
|
|
if let Err(e) = child.start_kill() {
|
|
crate::log_msg(&format!(
|
|
"teardown: {} could not be killed: {e}",
|
|
self.label
|
|
));
|
|
}
|
|
|
|
// The second wait is bounded too. An unbounded one lets a process stuck
|
|
// in uninterruptible sleep wedge the core command loop forever, and a
|
|
// 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 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 —
|
|
// 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
|
|
/// whole point of holding the child across the waits.
|
|
#[cfg(test)]
|
|
fn is_armed(&self) -> bool {
|
|
self.child.is_some()
|
|
}
|
|
}
|
|
|
|
impl<C: ChildProcess> Drop for ReapOnDrop<C> {
|
|
fn drop(&mut self) {
|
|
let Some(child) = self.child.as_mut() else {
|
|
return;
|
|
};
|
|
let _ = child.start_kill();
|
|
// `Drop` cannot await, so poll on a bounded budget. See `REAP_BUDGET`.
|
|
let deadline = Instant::now() + REAP_BUDGET;
|
|
loop {
|
|
if child.try_reap() {
|
|
return;
|
|
}
|
|
if Instant::now() >= deadline {
|
|
crate::log_msg(&format!(
|
|
"teardown: {} did not exit within the reap budget; \
|
|
continuing (the echo-cancel module may unload while it lives)",
|
|
self.label
|
|
));
|
|
return;
|
|
}
|
|
std::thread::sleep(REAP_POLL);
|
|
}
|
|
}
|
|
}
|
|
|
|
/// Everything in an `ActiveSession` whose **destruction order** is load-bearing.
|
|
///
|
|
/// ⚠️ Field order below **is** the invariant. `echo_cancel` is declared last so
|
|
/// it is dropped last, after every screen-share child has been killed and
|
|
/// reaped. Do not reorder these fields.
|
|
pub(super) struct ScreenshareTeardown<C: ChildProcess, G> {
|
|
/// Our pixelpass screen-share host child while sharing.
|
|
host: Option<ReapOnDrop<C>>,
|
|
/// pixelpass viewer children we spawned to watch peers' shares, each paired
|
|
/// with the share ticket it is viewing so a re-watch of the same share can
|
|
/// replace (not stack) its player.
|
|
viewers: Vec<(String, ReapOnDrop<C>)>,
|
|
/// Loaded PipeWire echo-cancel module (if enabled); unloads on drop.
|
|
///
|
|
/// ⚠️ **LAST FIELD ON PURPOSE** — see the module docs and the struct note.
|
|
///
|
|
/// Never read, and that is the design: the guard is held only so that its
|
|
/// `Drop` runs, and only so that it runs *here*, last. `dead_code` is right
|
|
/// that nothing reads it and wrong that it does nothing.
|
|
#[allow(dead_code)]
|
|
echo_cancel: Option<G>,
|
|
}
|
|
|
|
impl<C: ChildProcess, G> ScreenshareTeardown<C, G> {
|
|
pub(super) fn new(echo_cancel: Option<G>) -> Self {
|
|
Self {
|
|
host: None,
|
|
viewers: Vec::new(),
|
|
echo_cancel,
|
|
}
|
|
}
|
|
|
|
pub(super) fn is_sharing(&self) -> bool {
|
|
self.host.is_some()
|
|
}
|
|
|
|
pub(super) fn set_host(&mut self, child: C) {
|
|
self.host = Some(ReapOnDrop::new(child, "screen-share host"));
|
|
}
|
|
|
|
/// 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
|
|
/// tracks live players.
|
|
pub(super) fn sweep_exited_viewers(&mut self) {
|
|
self.viewers.retain_mut(|(_, child)| !child.has_exited());
|
|
}
|
|
|
|
/// Kill and reap the viewer already showing `ticket`, if any, so a re-watch
|
|
/// replaces its player instead of stacking a second one.
|
|
pub(super) async fn replace_viewer(&mut self, ticket: &str) -> bool {
|
|
let Some(pos) = super::replace_viewer_index(&self.viewers, ticket) else {
|
|
return false;
|
|
};
|
|
let (_, mut old) = self.viewers.remove(pos);
|
|
// 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
|
|
}
|
|
|
|
pub(super) fn push_viewer(&mut self, ticket: String, child: C) {
|
|
self.viewers
|
|
.push((ticket, ReapOnDrop::new(child, "screen-share viewer")));
|
|
}
|
|
|
|
/// Kill and reap **every** screen-share child, host first so viewers see the
|
|
/// stream end promptly.
|
|
///
|
|
/// The caller must await this before the echo-cancel guard is dropped. On
|
|
/// 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 {
|
|
let _ = host.shutdown().await;
|
|
}
|
|
self.host = None;
|
|
for (_, viewer) in self.viewers.iter_mut() {
|
|
let _ = viewer.shutdown().await;
|
|
}
|
|
self.viewers.clear();
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use super::{ChildProcess, ReapOnDrop, STOP_GRACE, ScreenshareTeardown, StopOutcome};
|
|
use std::future::Future;
|
|
use std::sync::{Arc, Mutex};
|
|
use std::time::Duration;
|
|
|
|
type Log = Arc<Mutex<Vec<String>>>;
|
|
|
|
fn log() -> Log {
|
|
Arc::new(Mutex::new(Vec::new()))
|
|
}
|
|
|
|
fn entries(log: &Log) -> Vec<String> {
|
|
log.lock().unwrap().clone()
|
|
}
|
|
|
|
fn position(log: &Log, entry: &str) -> Option<usize> {
|
|
entries(log).iter().position(|e| e == entry)
|
|
}
|
|
|
|
/// Records the events the ordering rules turn on. Death is gated on an
|
|
/// actual signal, so the double cannot report a reap that nothing caused.
|
|
struct FakeChild {
|
|
log: Log,
|
|
label: &'static str,
|
|
interrupted: bool,
|
|
killed: bool,
|
|
reaped: bool,
|
|
/// A well-behaved child exits on SIGINT. A wedged one ignores it and
|
|
/// dies only to SIGKILL.
|
|
honours_interrupt: bool,
|
|
/// When true the child is already dead before anyone signals it — the
|
|
/// closed-player-window case that `sweep_exited_viewers` looks for.
|
|
exited_on_its_own: bool,
|
|
/// Death is not instantaneous: `try_reap` reports the child alive this
|
|
/// many more times before it goes.
|
|
polls_before_death: u32,
|
|
/// `wait` reports an error instead of a reap.
|
|
wait_fails: bool,
|
|
}
|
|
|
|
impl FakeChild {
|
|
/// A well-behaved child: exits when asked.
|
|
fn new(log: &Log, label: &'static str) -> Self {
|
|
Self {
|
|
log: log.clone(),
|
|
label,
|
|
interrupted: false,
|
|
killed: false,
|
|
reaped: false,
|
|
honours_interrupt: true,
|
|
exited_on_its_own: false,
|
|
polls_before_death: 0,
|
|
wait_fails: false,
|
|
}
|
|
}
|
|
|
|
/// A child that ignores the graceful stop entirely.
|
|
fn wedged(log: &Log, label: &'static str) -> Self {
|
|
Self {
|
|
honours_interrupt: false,
|
|
..Self::new(log, label)
|
|
}
|
|
}
|
|
|
|
/// A child that does not die the instant it is signalled: `try_reap`
|
|
/// reports it alive for `polls` calls first. Without this the `Drop`
|
|
/// polling loop could be replaced by a single `try_reap` and no test
|
|
/// would notice.
|
|
fn reaps_after_polls(log: &Log, label: &'static str, polls: u32) -> Self {
|
|
Self {
|
|
polls_before_death: polls,
|
|
..Self::new(log, label)
|
|
}
|
|
}
|
|
|
|
/// 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 {
|
|
Self {
|
|
wait_fails: true,
|
|
..Self::new(log, label)
|
|
}
|
|
}
|
|
|
|
fn already_exited(log: &Log, label: &'static str) -> Self {
|
|
Self {
|
|
exited_on_its_own: true,
|
|
..Self::new(log, label)
|
|
}
|
|
}
|
|
|
|
/// Has anything actually made this child exit yet? A signalled child
|
|
/// still has to burn through `polls_before_death` first.
|
|
fn is_dead(&self) -> bool {
|
|
let signalled = self.killed
|
|
|| self.exited_on_its_own
|
|
|| (self.interrupted && self.honours_interrupt);
|
|
signalled && self.polls_before_death == 0
|
|
}
|
|
|
|
/// One observation of a dying-but-not-yet-dead child.
|
|
fn tick(&mut self) {
|
|
self.polls_before_death = self.polls_before_death.saturating_sub(1);
|
|
}
|
|
|
|
fn record(&self, event: &str) {
|
|
self.log
|
|
.lock()
|
|
.unwrap()
|
|
.push(format!("{}:{event}", self.label));
|
|
}
|
|
|
|
fn mark_reaped(&mut self) {
|
|
if !self.reaped {
|
|
self.reaped = true;
|
|
self.record("reap");
|
|
}
|
|
}
|
|
}
|
|
|
|
impl ChildProcess for FakeChild {
|
|
fn request_stop(&mut self) -> std::io::Result<()> {
|
|
if !self.interrupted {
|
|
self.interrupted = true;
|
|
self.record("sigint");
|
|
}
|
|
Ok(())
|
|
}
|
|
|
|
fn start_kill(&mut self) -> std::io::Result<()> {
|
|
if !self.killed {
|
|
self.killed = true;
|
|
self.record("kill");
|
|
}
|
|
Ok(())
|
|
}
|
|
|
|
fn try_reap(&mut self) -> bool {
|
|
if self.is_dead() {
|
|
self.mark_reaped();
|
|
return true;
|
|
}
|
|
self.tick();
|
|
false
|
|
}
|
|
|
|
/// Pending until something actually kills the child, so a wedged child
|
|
/// really does make the caller wait out `STOP_GRACE`. No waker is
|
|
/// registered: under `start_paused` the runtime auto-advances its clock
|
|
/// when every task is idle, which is exactly what fires the timeout.
|
|
fn wait_reaped(&mut self) -> impl Future<Output = std::io::Result<()>> + Send {
|
|
std::future::poll_fn(move |_cx| {
|
|
if self.wait_fails {
|
|
return std::task::Poll::Ready(Err(std::io::Error::other("wait failed")));
|
|
}
|
|
if self.is_dead() {
|
|
self.mark_reaped();
|
|
std::task::Poll::Ready(Ok(()))
|
|
} else {
|
|
std::task::Poll::Pending
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
/// Stands in for `EchoCancelGuard`, whose real `Drop` runs `pactl unload`.
|
|
struct FakeAec(Log);
|
|
|
|
impl Drop for FakeAec {
|
|
fn drop(&mut self) {
|
|
self.0.lock().unwrap().push("aec:unload".to_string());
|
|
}
|
|
}
|
|
|
|
fn teardown(log: &Log) -> ScreenshareTeardown<FakeChild, FakeAec> {
|
|
ScreenshareTeardown::new(Some(FakeAec(log.clone())))
|
|
}
|
|
|
|
// --- The drop/unwind path: field order + ReapOnDrop carry the invariant ---
|
|
|
|
/// Mutation gate #5 (remove the reap loop from `ReapOnDrop::drop`).
|
|
///
|
|
/// Asserts only that dropping a guard reaps, and reaps *after* killing —
|
|
/// deliberately says nothing about the AEC, so reversing the struct's field
|
|
/// order leaves this test green and only the ordering test below fails.
|
|
#[test]
|
|
fn dropping_a_guard_kills_and_then_reaps_the_child() {
|
|
let log = log();
|
|
drop(ReapOnDrop::new(FakeChild::new(&log, "host"), "host"));
|
|
assert_eq!(entries(&log), vec!["host:kill", "host:reap"]);
|
|
}
|
|
|
|
/// Mutation gate #4 (reverse the field order of `ScreenshareTeardown`).
|
|
///
|
|
/// Asserts only kill-before-unload, so removing the reap loop leaves this
|
|
/// test green and only the reap test above fails.
|
|
#[test]
|
|
fn the_aec_unloads_after_the_children_on_the_drop_path() {
|
|
let log = log();
|
|
let mut t = teardown(&log);
|
|
t.set_host(FakeChild::new(&log, "host"));
|
|
t.push_viewer("ticket-A".to_string(), FakeChild::new(&log, "viewer"));
|
|
drop(t);
|
|
|
|
let unload = position(&log, "aec:unload").expect("the AEC guard must be dropped");
|
|
let host_kill = position(&log, "host:kill").expect("the host must be killed");
|
|
let viewer_kill = position(&log, "viewer:kill").expect("the viewer must be killed");
|
|
assert!(
|
|
host_kill < unload,
|
|
"the AEC unloaded while the host was alive: {:?}",
|
|
entries(&log)
|
|
);
|
|
assert!(
|
|
viewer_kill < unload,
|
|
"the AEC unloaded while a viewer was alive: {:?}",
|
|
entries(&log)
|
|
);
|
|
}
|
|
|
|
/// The whole invariant in one sequence, as documentation.
|
|
#[test]
|
|
fn the_drop_path_reaps_every_child_before_unloading_the_aec() {
|
|
let log = log();
|
|
let mut t = teardown(&log);
|
|
t.set_host(FakeChild::new(&log, "host"));
|
|
drop(t);
|
|
assert_eq!(entries(&log), vec!["host:kill", "host:reap", "aec:unload"]);
|
|
}
|
|
|
|
// --- The explicit path: ask, then insist ---
|
|
|
|
/// A healthy child must be *asked*, never killed. If Stop Share went
|
|
/// straight to SIGKILL, pixelpass would skip its own cleanup and leak a
|
|
/// null-sink module every time (design v3.4 §7.4).
|
|
#[tokio::test]
|
|
async fn a_healthy_child_is_asked_to_stop_and_never_killed() {
|
|
let log = log();
|
|
let mut t = teardown(&log);
|
|
t.set_host(FakeChild::new(&log, "host"));
|
|
|
|
assert_eq!(t.stop_host().await, Some(StopOutcome::Reaped));
|
|
|
|
assert_eq!(entries(&log), vec!["host:sigint", "host:reap"]);
|
|
assert!(
|
|
!entries(&log).contains(&"host:kill".to_string()),
|
|
"a child that honoured the graceful stop must not be killed: {:?}",
|
|
entries(&log)
|
|
);
|
|
}
|
|
|
|
/// ...but a child that ignores the request must not be able to hold the
|
|
/// session open forever: the grace is bounded and SIGKILL follows.
|
|
#[tokio::test(start_paused = true)]
|
|
async fn a_wedged_child_is_killed_once_the_grace_expires() {
|
|
let log = log();
|
|
let mut t = teardown(&log);
|
|
t.set_host(FakeChild::wedged(&log, "host"));
|
|
|
|
// The outer bound turns "the fallback was removed" into a failure
|
|
// rather than a hung test. Under `start_paused` no real time passes.
|
|
let start = tokio::time::Instant::now();
|
|
tokio::time::timeout(Duration::from_secs(60), t.stop_host())
|
|
.await
|
|
.expect("a wedged child must not block teardown indefinitely");
|
|
|
|
assert_eq!(entries(&log), vec!["host:sigint", "host:kill", "host:reap"]);
|
|
assert!(
|
|
start.elapsed() >= STOP_GRACE,
|
|
"the child must actually be given the grace period, waited {:?}",
|
|
start.elapsed()
|
|
);
|
|
}
|
|
|
|
/// The assertion above compares elapsed time against `STOP_GRACE` itself,
|
|
/// so it stays vacuously true if the constant is set to zero — both sides
|
|
/// move together. Pin the constant independently: the whole point of the
|
|
/// graceful stop is that pixelpass gets a real interval in which to unload
|
|
/// its capture sink, and zero is not one.
|
|
#[test]
|
|
fn the_grace_is_a_real_interval() {
|
|
assert!(
|
|
STOP_GRACE >= Duration::from_millis(500),
|
|
"too short to let pixelpass tear its pipeline down: {STOP_GRACE:?}"
|
|
);
|
|
// ...and short enough that a wedged child cannot visibly stall the core
|
|
// command loop, which awaits this inline.
|
|
assert!(
|
|
STOP_GRACE <= Duration::from_secs(5),
|
|
"long enough to freeze the UI's command handling: {STOP_GRACE:?}"
|
|
);
|
|
}
|
|
|
|
/// The hole the whole type exists to close, and the one place the old
|
|
/// implementation left open: if `shutdown` is cancelled while waiting, the
|
|
/// child must still be owned, so dropping the guard still kills and reaps.
|
|
#[tokio::test(start_paused = true)]
|
|
async fn cancelling_shutdown_mid_wait_leaves_the_fallback_armed() {
|
|
let log = log();
|
|
let mut guard = ReapOnDrop::new(FakeChild::wedged(&log, "host"), "host");
|
|
|
|
// Cancel well inside the grace, while it is still waiting.
|
|
assert!(
|
|
tokio::time::timeout(STOP_GRACE / 4, guard.shutdown())
|
|
.await
|
|
.is_err(),
|
|
"the wedged child should still have been waiting when we cancelled"
|
|
);
|
|
assert!(
|
|
guard.is_armed(),
|
|
"a cancelled shutdown must not disarm the drop fallback"
|
|
);
|
|
|
|
drop(guard);
|
|
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)]
|
|
async fn a_failed_wait_is_not_treated_as_a_confirmed_reap() {
|
|
let log = log();
|
|
let mut guard = ReapOnDrop::new(FakeChild::wait_fails(&log, "host"), "host");
|
|
|
|
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()),
|
|
"nothing confirmed the reap: {:?}",
|
|
entries(&log)
|
|
);
|
|
assert!(
|
|
entries(&log).contains(&"host:kill".to_string()),
|
|
"a child that would not stop must still be escalated: {:?}",
|
|
entries(&log)
|
|
);
|
|
assert!(
|
|
guard.is_armed(),
|
|
"an unconfirmed reap must leave the drop fallback armed"
|
|
);
|
|
}
|
|
|
|
/// Death is not instantaneous, so the drop path has to keep polling. A
|
|
/// single `try_reap` in place of the loop must not pass.
|
|
#[test]
|
|
fn the_drop_path_polls_until_the_child_is_actually_gone() {
|
|
let log = log();
|
|
drop(ReapOnDrop::new(
|
|
FakeChild::reaps_after_polls(&log, "host", 3),
|
|
"host",
|
|
));
|
|
assert_eq!(entries(&log), vec!["host:kill", "host:reap"]);
|
|
}
|
|
|
|
/// Mutation gate #3 (remove the wait after the host kill).
|
|
#[tokio::test]
|
|
async fn explicit_shutdown_reaps_the_host_before_the_aec_can_unload() {
|
|
let log = log();
|
|
let mut t = teardown(&log);
|
|
t.set_host(FakeChild::new(&log, "host"));
|
|
t.push_viewer("ticket-A".to_string(), FakeChild::new(&log, "viewer"));
|
|
|
|
t.shutdown_children().await;
|
|
|
|
// Reaped by the explicit path — before the guard is anywhere near dropped.
|
|
assert_eq!(
|
|
entries(&log),
|
|
vec!["host:sigint", "host:reap", "viewer:sigint", "viewer:reap"],
|
|
"children must be stopped and reaped by the explicit path"
|
|
);
|
|
|
|
drop(t);
|
|
let unload = position(&log, "aec:unload").expect("the AEC guard must be dropped");
|
|
let host_reap = position(&log, "host:reap").expect("the host must be reaped");
|
|
assert!(host_reap < unload);
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn explicit_shutdown_is_idempotent_with_the_drop_path() {
|
|
let log = log();
|
|
let mut t = teardown(&log);
|
|
t.set_host(FakeChild::new(&log, "host"));
|
|
t.shutdown_children().await;
|
|
drop(t);
|
|
// Exactly one stop and one reap: the drop path must not re-signal a
|
|
// child the explicit path already took.
|
|
assert_eq!(
|
|
entries(&log),
|
|
vec!["host:sigint", "host:reap", "aec:unload"]
|
|
);
|
|
}
|
|
|
|
// --- Host/viewer bookkeeping ---
|
|
|
|
#[tokio::test]
|
|
async fn stop_host_reports_whether_it_was_sharing() {
|
|
let log = log();
|
|
let mut t = teardown(&log);
|
|
assert!(!t.is_sharing());
|
|
assert_eq!(t.stop_host().await, None, "not sharing: nothing to stop");
|
|
|
|
t.set_host(FakeChild::new(&log, "host"));
|
|
assert!(t.is_sharing());
|
|
assert_eq!(t.stop_host().await, Some(StopOutcome::Reaped));
|
|
assert!(!t.is_sharing());
|
|
assert_eq!(entries(&log), vec!["host:sigint", "host:reap"]);
|
|
}
|
|
|
|
#[test]
|
|
fn sweeping_drops_only_the_players_that_already_closed() {
|
|
let log = log();
|
|
let mut t = teardown(&log);
|
|
t.push_viewer(
|
|
"closed".to_string(),
|
|
FakeChild::already_exited(&log, "closed"),
|
|
);
|
|
t.push_viewer("live".to_string(), FakeChild::new(&log, "live"));
|
|
|
|
t.sweep_exited_viewers();
|
|
|
|
// The live player survives the sweep; only the closed one is dropped,
|
|
// and dropping it must not kill anything (it was already gone).
|
|
assert_eq!(t.viewers.len(), 1);
|
|
assert_eq!(t.viewers[0].0, "live");
|
|
assert_eq!(entries(&log), vec!["closed:reap"]);
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn re_watching_a_share_replaces_that_player_only() {
|
|
let log = log();
|
|
let mut t = teardown(&log);
|
|
t.push_viewer("ticket-A".to_string(), FakeChild::new(&log, "a"));
|
|
t.push_viewer("ticket-B".to_string(), FakeChild::new(&log, "b"));
|
|
|
|
assert!(t.replace_viewer("ticket-A").await);
|
|
assert_eq!(entries(&log), vec!["a:sigint", "a:reap"]);
|
|
assert_eq!(t.viewers.len(), 1);
|
|
assert_eq!(t.viewers[0].0, "ticket-B");
|
|
|
|
// A share we are not watching has nothing to replace.
|
|
assert!(!t.replace_viewer("ticket-C").await);
|
|
}
|
|
}
|