screenshare: action A23 P2/P3 follow-up audit (capability probe, event guard, pactl kill)
Triage of Codex's a23-p2-audit-2026-06-26 (the strict-audio fix itself verified
sound — no path reloads the desktop loopback). Four findings actioned:
- P2 (F1, silent-from-start): pair pixelpass's new initial `app_audio:lost`
with a reworded warning ("Shared app isn't sending audio — viewers hear
silence until it plays") so an app share that never produced audio is flagged,
not just one that stopped.
- P2 (version skew): probe `pixelpass --help` for `--strict-audio` before
offering per-app capture (pure `help_mentions_strict_audio` + timeout-bounded,
kill-on-drop `supports_strict_audio`). An older pixelpass would have clap
reject the flag and hard-fail the share; now the picker degrades to
whole-desktop only with an upgrade nudge. Never a silent best-effort `--app`
fallback (that reopens A23). AudioAppsListed now carries `app_audio_supported`.
- P3 (pactl child leak): list_audio_apps' enumeration child is now
kill_on_drop, so a wedged pactl is reaped on the 2s timeout.
- P3 (unscoped events): apply ShareAudioActive only while actually sharing a
specific app (`share_audio_app_active`), so a late event from a just-killed
host can't flip the warning after stop or on a whole-desktop share.
+4 tests (probe parse, post-stop/whole-desktop guard, old-pixelpass picker
degrade), updated reset + app_audio tracking tests. 440 lib tests, clippy
--all-targets clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
+143
-17
@@ -589,10 +589,21 @@ pub struct AppState {
|
||||
/// `ScreenShareStarted`, `ScreenShareStopped`, or any `Error`.
|
||||
share_starting: bool,
|
||||
/// While sharing a specific app's audio (A23 strict mode): `true` when that
|
||||
/// app's audio has stopped, so viewers currently hear silence. Drives a
|
||||
/// transient warning. Always `false` for whole-desktop shares (pixelpass
|
||||
/// emits no `app_audio` events then) and when not sharing.
|
||||
/// app's audio has stopped (or hasn't started yet), so viewers currently hear
|
||||
/// silence. Drives a transient warning. Always `false` for whole-desktop
|
||||
/// shares (pixelpass emits no `app_audio` events then) and when not sharing.
|
||||
share_audio_dropped: bool,
|
||||
/// Whether the current share is a specific-app capture (vs whole-desktop).
|
||||
/// Set from the confirmed selection on `ScreenShareStarted`, cleared on
|
||||
/// stop/reset. Gates applying `ShareAudioActive`, so a late event from a
|
||||
/// just-killed host can't flip the warning on a new whole-desktop share or
|
||||
/// after stop (audit P3, unscoped events).
|
||||
share_audio_app_active: bool,
|
||||
/// Whether the resolved pixelpass supports `--strict-audio` (per-app audio).
|
||||
/// `false` ⇒ the picker offers whole-desktop only, because a per-app share
|
||||
/// would pass a flag an older pixelpass rejects (audit P2). Optimistic `true`
|
||||
/// until the core's `AudioAppsListed` reports otherwise.
|
||||
share_app_audio_supported: bool,
|
||||
/// Whether the Chat drawer is open (drawer layout only).
|
||||
drawer_chat_open: bool,
|
||||
/// Raw mic level (normalized RMS, `0.0..=1.0`) for the settings meter.
|
||||
@@ -667,6 +678,8 @@ impl AppState {
|
||||
self.share_audio_selection = None;
|
||||
self.share_starting = false;
|
||||
self.share_audio_dropped = false;
|
||||
self.share_audio_app_active = false;
|
||||
self.share_app_audio_supported = true;
|
||||
}
|
||||
|
||||
fn custom_sound_path(&self, sound: Sound) -> &str {
|
||||
@@ -793,6 +806,8 @@ impl Default for AppState {
|
||||
share_audio_selection: None,
|
||||
share_starting: false,
|
||||
share_audio_dropped: false,
|
||||
share_audio_app_active: false,
|
||||
share_app_audio_supported: true,
|
||||
drawer_chat_open: false,
|
||||
mic_level: 0.0,
|
||||
mic_test_active: false,
|
||||
@@ -1365,24 +1380,35 @@ fn update(state: &mut AppState, message: AppMessage) -> Task<AppMessage> {
|
||||
state.attachments.insert(key, AttachmentState::Failed(error.clone()), None);
|
||||
state.status_message = format!("Attachment failed: {error}");
|
||||
}
|
||||
UiEvent::AudioAppsListed(apps) => {
|
||||
UiEvent::AudioAppsListed { apps, app_audio_supported } => {
|
||||
// Only meaningful while the picker is open; if the user
|
||||
// already cancelled, drop it. Keep the current selection if it
|
||||
// still exists in the refreshed list, else fall back to "All
|
||||
// system audio".
|
||||
// already cancelled, drop it.
|
||||
if state.share_picker_open {
|
||||
if let Some(sel) = &state.share_audio_selection
|
||||
&& !apps.iter().any(|a| a == sel)
|
||||
{
|
||||
state.share_app_audio_supported = app_audio_supported;
|
||||
if app_audio_supported {
|
||||
// Keep the current selection if it still exists in the
|
||||
// refreshed list, else fall back to "All system audio".
|
||||
if let Some(sel) = &state.share_audio_selection
|
||||
&& !apps.iter().any(|a| a == sel)
|
||||
{
|
||||
state.share_audio_selection = None;
|
||||
}
|
||||
state.share_audio_apps = apps;
|
||||
} else {
|
||||
// Older pixelpass: per-app capture would hard-fail
|
||||
// (--strict-audio unknown). Force whole-desktop only.
|
||||
state.share_audio_apps.clear();
|
||||
state.share_audio_selection = None;
|
||||
}
|
||||
state.share_audio_apps = apps;
|
||||
}
|
||||
}
|
||||
UiEvent::ScreenShareStarted => {
|
||||
state.self_sharing = true;
|
||||
state.share_starting = false;
|
||||
state.share_audio_dropped = false;
|
||||
// Remember whether this share captures a specific app, so we
|
||||
// only apply `app_audio` warnings to app shares (P3).
|
||||
state.share_audio_app_active = state.share_audio_selection.is_some();
|
||||
// Defensive: ensure no picker lingers across a successful start.
|
||||
state.share_picker_open = false;
|
||||
state.status_message = "Sharing your screen".to_string();
|
||||
@@ -1391,11 +1417,17 @@ fn update(state: &mut AppState, message: AppMessage) -> Task<AppMessage> {
|
||||
state.self_sharing = false;
|
||||
state.share_starting = false;
|
||||
state.share_audio_dropped = false;
|
||||
state.share_audio_app_active = false;
|
||||
state.status_message = "Screen share stopped".to_string();
|
||||
}
|
||||
UiEvent::ShareAudioActive(active) => {
|
||||
// Per-app audio routed/lost (only while sharing a chosen app).
|
||||
state.share_audio_dropped = !active;
|
||||
// Per-app audio routed/lost. Apply only while we're actually
|
||||
// sharing a specific app: a late event from a just-killed host
|
||||
// must not flip the warning after stop or on a whole-desktop
|
||||
// share (audit P3, unscoped events).
|
||||
if state.self_sharing && state.share_audio_app_active {
|
||||
state.share_audio_dropped = !active;
|
||||
}
|
||||
}
|
||||
UiEvent::IdentityStatus { node_id, persisted, error } => {
|
||||
state.self_node_id = Some(node_id);
|
||||
@@ -4098,7 +4130,7 @@ fn view(state: &AppState) -> Element<'_, AppMessage> {
|
||||
column![
|
||||
badge,
|
||||
text(
|
||||
"⚠ Shared app's audio stopped — viewers hear silence until it plays again"
|
||||
"⚠ Shared app isn't sending audio — viewers hear silence until it plays"
|
||||
)
|
||||
.size(11)
|
||||
.color(color_yellow),
|
||||
@@ -5501,7 +5533,18 @@ fn with_share_picker<'a>(
|
||||
));
|
||||
}
|
||||
|
||||
let list: Element<'_, AppMessage> = if state.share_audio_apps.is_empty() {
|
||||
let list: Element<'_, AppMessage> = if !state.share_app_audio_supported {
|
||||
// Older pixelpass without --strict-audio: per-app capture is unavailable
|
||||
// (it would hard-fail), so only whole-desktop is offered. Nudge to upgrade.
|
||||
column![
|
||||
options,
|
||||
text("Update pixelpass to capture a single app's audio (avoids echoing the call to viewers).")
|
||||
.size(11)
|
||||
.color(yellow),
|
||||
]
|
||||
.spacing(8)
|
||||
.into()
|
||||
} else if state.share_audio_apps.is_empty() {
|
||||
column![
|
||||
options,
|
||||
text("No other apps are playing audio right now.")
|
||||
@@ -6347,6 +6390,8 @@ mod tests {
|
||||
state.share_audio_selection = Some("Firefox".to_string());
|
||||
state.share_starting = true;
|
||||
state.share_audio_dropped = true;
|
||||
state.share_audio_app_active = true;
|
||||
state.share_app_audio_supported = false;
|
||||
state.clip_status.lock().unwrap().playing_id = Some(attachment_id);
|
||||
|
||||
state.reset_room_state();
|
||||
@@ -6372,6 +6417,8 @@ mod tests {
|
||||
assert!(state.share_audio_selection.is_none());
|
||||
assert!(!state.share_starting);
|
||||
assert!(!state.share_audio_dropped);
|
||||
assert!(!state.share_audio_app_active);
|
||||
assert!(state.share_app_audio_supported, "reset is optimistic by default");
|
||||
|
||||
for _ in 0..50 {
|
||||
if crate::audio::clip_player::status_snapshot(&state.clip_status).playing_id.is_none() {
|
||||
@@ -6444,14 +6491,19 @@ mod tests {
|
||||
fn share_audio_dropped_tracks_app_audio_events() {
|
||||
// While sharing a specific app, app_audio lost/routed toggles the warning
|
||||
// flag; start and stop both reset it so it can't linger across sessions.
|
||||
let mut state = AppState::default();
|
||||
// A specific app was chosen in the picker, so the share is app-specific.
|
||||
let mut state = AppState {
|
||||
share_audio_selection: Some("mpv".to_string()),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
// Start sharing — flag is clear.
|
||||
// Start sharing — flag is clear, and the share is marked app-specific.
|
||||
let _ = update(
|
||||
&mut state,
|
||||
AppMessage::UiEventReceived(UiEvent::ScreenShareStarted),
|
||||
);
|
||||
assert!(!state.share_audio_dropped);
|
||||
assert!(state.share_audio_app_active);
|
||||
|
||||
// The chosen app's audio stops → warning on.
|
||||
let _ = update(
|
||||
@@ -6478,6 +6530,80 @@ mod tests {
|
||||
AppMessage::UiEventReceived(UiEvent::ScreenShareStopped),
|
||||
);
|
||||
assert!(!state.share_audio_dropped);
|
||||
assert!(!state.share_audio_app_active);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn share_audio_active_ignored_unless_app_sharing() {
|
||||
// P3 (unscoped events): a late app_audio event from a just-killed host
|
||||
// must not flip the warning when we're not sharing a specific app —
|
||||
// neither after stop nor on a whole-desktop share.
|
||||
|
||||
// (a) Whole-desktop share (no app selected): events are ignored.
|
||||
let mut state = AppState::default();
|
||||
let _ = update(
|
||||
&mut state,
|
||||
AppMessage::UiEventReceived(UiEvent::ScreenShareStarted),
|
||||
);
|
||||
assert!(!state.share_audio_app_active, "no app selected ⇒ not app-specific");
|
||||
let _ = update(
|
||||
&mut state,
|
||||
AppMessage::UiEventReceived(UiEvent::ShareAudioActive(false)),
|
||||
);
|
||||
assert!(!state.share_audio_dropped, "whole-desktop share ignores app_audio");
|
||||
|
||||
// (b) After stop: a straggling event can't resurrect the warning.
|
||||
let mut state = AppState {
|
||||
share_audio_selection: Some("mpv".to_string()),
|
||||
..Default::default()
|
||||
};
|
||||
let _ = update(
|
||||
&mut state,
|
||||
AppMessage::UiEventReceived(UiEvent::ScreenShareStarted),
|
||||
);
|
||||
let _ = update(
|
||||
&mut state,
|
||||
AppMessage::UiEventReceived(UiEvent::ScreenShareStopped),
|
||||
);
|
||||
let _ = update(
|
||||
&mut state,
|
||||
AppMessage::UiEventReceived(UiEvent::ShareAudioActive(false)),
|
||||
);
|
||||
assert!(!state.share_audio_dropped, "post-stop event is ignored");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn old_pixelpass_picker_offers_whole_desktop_only() {
|
||||
// P2 (version skew): when the resolved pixelpass lacks --strict-audio, the
|
||||
// picker must drop all per-app options and force the whole-desktop choice,
|
||||
// so a per-app share (which would pass the unknown flag) can't be started.
|
||||
let mut state = AppState {
|
||||
share_picker_open: true,
|
||||
share_audio_selection: Some("Firefox".to_string()),
|
||||
share_audio_apps: vec!["Firefox".to_string(), "mpv".to_string()],
|
||||
..Default::default()
|
||||
};
|
||||
let _ = update(
|
||||
&mut state,
|
||||
AppMessage::UiEventReceived(UiEvent::AudioAppsListed {
|
||||
apps: vec!["Firefox".to_string(), "mpv".to_string()],
|
||||
app_audio_supported: false,
|
||||
}),
|
||||
);
|
||||
assert!(!state.share_app_audio_supported);
|
||||
assert!(state.share_audio_apps.is_empty(), "no per-app rows offered");
|
||||
assert!(state.share_audio_selection.is_none(), "forced to whole-desktop");
|
||||
|
||||
// A supported pixelpass keeps the app list and a valid selection.
|
||||
let _ = update(
|
||||
&mut state,
|
||||
AppMessage::UiEventReceived(UiEvent::AudioAppsListed {
|
||||
apps: vec!["Firefox".to_string(), "mpv".to_string()],
|
||||
app_audio_supported: true,
|
||||
}),
|
||||
);
|
||||
assert!(state.share_app_audio_supported);
|
||||
assert_eq!(state.share_audio_apps.len(), 2);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -149,8 +149,11 @@ pub enum UiEvent {
|
||||
AttachmentFailed { from: EndpointId, id: crate::files::AttachmentId, error: String },
|
||||
/// The apps currently producing audio, for the screen-share audio picker
|
||||
/// (A23). Sorted, deduplicated `application.name`s; empty when nothing is
|
||||
/// playing or enumeration isn't available.
|
||||
AudioAppsListed(Vec<String>),
|
||||
/// playing or enumeration isn't available. `app_audio_supported` reports
|
||||
/// whether the resolved pixelpass understands `--strict-audio`: when `false`
|
||||
/// (an older pixelpass) the picker must offer whole-desktop audio only, since
|
||||
/// a per-app share would pass a flag that older binary rejects (audit P2).
|
||||
AudioAppsListed { apps: Vec<String>, app_audio_supported: bool },
|
||||
/// Our own screen share started; the UI flips the Share button to "Stop".
|
||||
ScreenShareStarted,
|
||||
/// Our own screen share stopped (or failed to start).
|
||||
|
||||
+19
-2
@@ -2635,8 +2635,25 @@ async fn run_core_loop(
|
||||
}
|
||||
|
||||
CoreCommand::ListAudioApps => {
|
||||
let apps = crate::screenshare::list_audio_apps().await;
|
||||
let _ = ui_tx.send(UiEvent::AudioAppsListed(apps)).await;
|
||||
// Probe whether this pixelpass supports `--strict-audio` before
|
||||
// offering per-app capture: an older binary would reject the flag
|
||||
// and hard-fail the share (audit P2). When unsupported (or
|
||||
// pixelpass is missing), skip enumeration and let the picker show
|
||||
// whole-desktop audio only — never a best-effort `--app` that
|
||||
// would reopen the A23 echo.
|
||||
let app_audio_supported =
|
||||
match crate::screenshare::pixelpass_path(pixelpass_override.as_deref()) {
|
||||
Some(bin) => crate::screenshare::supports_strict_audio(&bin).await,
|
||||
None => false,
|
||||
};
|
||||
let apps = if app_audio_supported {
|
||||
crate::screenshare::list_audio_apps().await
|
||||
} else {
|
||||
Vec::new()
|
||||
};
|
||||
let _ = ui_tx
|
||||
.send(UiEvent::AudioAppsListed { apps, app_audio_supported })
|
||||
.await;
|
||||
}
|
||||
|
||||
CoreCommand::StartScreenShare { audio_app } => {
|
||||
|
||||
@@ -183,6 +183,10 @@ pub async fn list_audio_apps() -> Vec<String> {
|
||||
.stdin(Stdio::null())
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::null())
|
||||
// On [`LIST_APPS_TIMEOUT`] the `output()` future is dropped, which drops
|
||||
// the child — `kill_on_drop(true)` then SIGKILLs and reaps it so a wedged
|
||||
// `pactl` can't linger/accumulate across picker opens (audit P3).
|
||||
.kill_on_drop(true)
|
||||
.output();
|
||||
match tokio::time::timeout(LIST_APPS_TIMEOUT, run).await {
|
||||
Ok(Ok(o)) if o.status.success() => parse_audio_apps(&o.stdout),
|
||||
@@ -190,6 +194,41 @@ pub async fn list_audio_apps() -> Vec<String> {
|
||||
}
|
||||
}
|
||||
|
||||
/// Hard cap on the capability probe (`pixelpass --help`). Conservative: a slow or
|
||||
/// hung pixelpass degrades to "strict audio unsupported" → whole-desktop-only
|
||||
/// picker (safe), never a stalled core loop.
|
||||
const HELP_PROBE_TIMEOUT: Duration = Duration::from_secs(2);
|
||||
|
||||
/// Whether the resolved pixelpass understands `--strict-audio` (added in pixelpass
|
||||
/// `85fdebe`). peerspeak only offers per-app audio capture when it does: a per-app
|
||||
/// share always appends `--strict-audio`, and an **older** pixelpass would have
|
||||
/// clap reject the unknown flag → the host spawn hard-fails and the share is
|
||||
/// broken (audit P2, version skew). When unsupported the picker degrades to
|
||||
/// whole-desktop audio only — we never silently drop to best-effort `--app`, which
|
||||
/// would reintroduce the call echo (A23).
|
||||
///
|
||||
/// Any probe failure/timeout returns `false` (degrade to the safe path). The
|
||||
/// `--help` child is `kill_on_drop` so a hung pixelpass can't linger.
|
||||
pub async fn supports_strict_audio(bin: &Path) -> bool {
|
||||
let run = Command::new(bin)
|
||||
.arg("--help")
|
||||
.stdin(Stdio::null())
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::null())
|
||||
.kill_on_drop(true)
|
||||
.output();
|
||||
match tokio::time::timeout(HELP_PROBE_TIMEOUT, run).await {
|
||||
Ok(Ok(o)) => help_mentions_strict_audio(&o.stdout),
|
||||
_ => false,
|
||||
}
|
||||
}
|
||||
|
||||
/// Pure check: does `pixelpass --help` advertise `--strict-audio`? Matches the
|
||||
/// flag token rather than a whole line, since clap may wrap/realign help text.
|
||||
pub fn help_mentions_strict_audio(help_stdout: &[u8]) -> bool {
|
||||
String::from_utf8_lossy(help_stdout).contains("--strict-audio")
|
||||
}
|
||||
|
||||
/// Parse `pactl -f json list sink-inputs` stdout into a sorted, deduplicated list
|
||||
/// of `application.name`s. Pure: no I/O. Unparseable input yields an empty list.
|
||||
/// Each name is passed through [`sanitize_app_name`] so the picker only ever
|
||||
@@ -594,6 +633,19 @@ mod tests {
|
||||
assert_eq!(parse_audio_apps(stdout.as_bytes()), vec!["mpv".to_string()]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn help_probe_detects_strict_audio_flag() {
|
||||
// A new pixelpass advertises the flag; an old one doesn't. The probe must
|
||||
// match the token even when clap wraps the option onto its own line.
|
||||
let new_help = b"Options:\n --app <APP>\n --strict-audio\n With --app, never fall back...";
|
||||
assert!(help_mentions_strict_audio(new_help));
|
||||
let old_help = b"Options:\n --app <APP>\n --output <OUTPUT>\n -h, --help";
|
||||
assert!(!help_mentions_strict_audio(old_help));
|
||||
// Garbage / empty output degrades to "unsupported" (safe path).
|
||||
assert!(!help_mentions_strict_audio(b""));
|
||||
assert!(!help_mentions_strict_audio(&[0xff, 0xfe, 0x00]));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sanitize_ticket_accepts_pixelpass_endpoint_ticket_shape() {
|
||||
let ticket = "endpointaabwxjexzensznfvuudiapn5tyzws3angd2merarm";
|
||||
|
||||
Reference in New Issue
Block a user