diff --git a/crates/pdfs-core/src/opener.rs b/crates/pdfs-core/src/opener.rs index f1e356b..6795f86 100644 --- a/crates/pdfs-core/src/opener.rs +++ b/crates/pdfs-core/src/opener.rs @@ -189,11 +189,21 @@ fn spawn(argv: &[String], path: &Path) { .stderr(Stdio::null()) .process_group(0) .spawn(); - if let Err(e) = spawned { - tracing::error!(command = ?argv, "open failed: {e}"); + match spawned { + Ok(child) => reap(child), + Err(e) => tracing::error!(command = ?argv, "open failed: {e}"), } } +/// Wait for `child` on a thread of its own. A child nobody waits for stays a +/// zombie for as long as its parent lives, which for the app and the tray is +/// the whole session. +pub fn reap(mut child: std::process::Child) { + std::thread::spawn(move || { + let _ = child.wait(); + }); +} + /// [`open`] with the policy from the user's `config.json`, read once per /// process. For callers that open the occasional path and have no reason to /// carry a policy around; long-lived windows should hold their own copy. @@ -472,6 +482,21 @@ fn glob_match(pattern: &str, name: &str) -> bool { mod tests { use super::*; + #[test] + fn a_reaped_child_does_not_stay_a_zombie() { + let child = std::process::Command::new("true").spawn().unwrap(); + let proc_entry = std::path::PathBuf::from(format!("/proc/{}", child.id())); + reap(child); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while proc_entry.exists() && std::time::Instant::now() < deadline { + std::thread::sleep(std::time::Duration::from_millis(10)); + } + assert!( + !proc_entry.exists(), + "the child is still in the process table" + ); + } + fn rule(patterns: &[&str], command: &[&str], terminal: bool) -> OpenRule { OpenRule { patterns: patterns.iter().map(|p| (*p).to_string()).collect(), diff --git a/crates/pdfs-gui/src/activation.rs b/crates/pdfs-gui/src/activation.rs index b697134..8da2fe1 100644 --- a/crates/pdfs-gui/src/activation.rs +++ b/crates/pdfs-gui/src/activation.rs @@ -10,9 +10,14 @@ //! `dead_code` allowances below are for — not code nobody uses. use std::path::{Path, PathBuf}; +use std::time::Duration; use pdfs_core::control::{PhotoKind, SearchHit}; +/// How long a `stat` of the mount may take before the file counts as not +/// there. A frozen mount never answers, and every caller has a fallback. +const STAT_TIMEOUT: Duration = Duration::from_secs(2); + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum DriveActivation { Folder, @@ -55,18 +60,32 @@ pub(crate) fn mounted_target(mountpoint: &Path, hit: &SearchHit) -> Option Option { - present(mounted_path(mountpoint, relative_path)) -} - fn present(path: PathBuf) -> Option { path.symlink_metadata().is_ok().then_some(path) } +/// Whether anything is at `path`, asked on a thread of its own and given up +/// on after [`STAT_TIMEOUT`]. `None` means "not there" or "no answer". +/// +/// For the GTK binaries, whose main thread must not `stat` the mount: a +/// mount that hangs hung the whole window with it. The thread of a `stat` +/// that never returns is left behind; there is no way to cancel one. +#[allow(dead_code)] // GTK binaries only +pub(crate) async fn present_within(path: PathBuf) -> Option { + let (tx, rx) = async_channel::bounded(1); + std::thread::spawn(move || { + let _ = tx.send_blocking(present(path)); + }); + match gtk4::glib::future_with_timeout(STAT_TIMEOUT, rx.recv()).await { + Ok(Ok(target)) => target, + Ok(Err(_)) => None, + Err(_) => { + tracing::warn!("the mount did not answer in time"); + None + } + } +} + /// The mount path for a hit, whether or not anything is there — for callers /// that have no fallback (a folder cannot be materialised) or already know the /// entry came from a mount listing. diff --git a/crates/pdfs-gui/src/app/main.rs b/crates/pdfs-gui/src/app/main.rs index 277d6f1..ba820ae 100644 --- a/crates/pdfs-gui/src/app/main.rs +++ b/crates/pdfs-gui/src/app/main.rs @@ -297,7 +297,10 @@ fn present_window(app: &adw::Application) { /// Spawn the tray icon process in the background. pub(crate) fn spawn_tray() { match Command::new("pdfs-tray").spawn() { - Ok(_) => tracing::info!("spawned `pdfs-tray`"), + Ok(child) => { + tracing::info!("spawned `pdfs-tray`"); + pdfs_core::opener::reap(child); + } Err(e) => tracing::error!("failed to spawn `pdfs-tray`: {e}"), } } @@ -1713,9 +1716,12 @@ pub(crate) fn flash_copied(button: >k4::Button, icon: &'static str) { /// Open a local path with the handler the user configured — `xdg-open` unless /// `open_with` in `config.json` overrides it for this kind of file. +/// +/// On a thread of its own: telling a folder from a file is a `stat`, and the +/// path is often in the mount, which may hang. fn open_path(path: &str) { - let path = Path::new(path); - pdfs_core::opener::open_default(path, path.is_dir()); + let path = PathBuf::from(path); + std::thread::spawn(move || pdfs_core::opener::open_default(&path, path.is_dir())); } /// [`open_path`] for a file the daemon materialised into the content cache, diff --git a/crates/pdfs-gui/src/app/pages/browser.rs b/crates/pdfs-gui/src/app/pages/browser.rs index dd39886..1d6ced8 100644 --- a/crates/pdfs-gui/src/app/pages/browser.rs +++ b/crates/pdfs-gui/src/app/pages/browser.rs @@ -1,4 +1,4 @@ -use crate::activation::{DriveActivation, drive_activation, mounted_path, mounted_target_rel}; +use crate::activation::{DriveActivation, drive_activation, mounted_path, present_within}; use crate::*; pub(crate) struct BrowserState { @@ -983,28 +983,27 @@ pub(crate) fn background_context_menu(ui: &Rc) -> ActionMenu { pub(crate) fn open_entry_with(ui: &Rc, entry: &DirEntry) { let rel = entry_rel(ui, entry); let mountpoint = ui.dirs.resolved_mountpoint(&ui.dirs.load_config()); - let Some(path) = mounted_target_rel(&mountpoint, &rel) else { - toast_error( - ui, - &gettext("Couldn't open file"), - &gettext("Open With needs the Proton Drive service to be running."), - ); - return; - }; - let launcher = gtk4::FileLauncher::new(Some(&gio::File::for_path(path))); - launcher.set_always_ask(true); let ui = ui.clone(); - launcher.launch( - ui_window(&ui).as_ref(), - gio::Cancellable::NONE, - move |result| { + glib::spawn_future_local(async move { + let Some(path) = present_within(mounted_path(&mountpoint, &rel)).await else { + toast_error( + &ui, + &gettext("Couldn't open file"), + &gettext("Open With needs the Proton Drive service to be running."), + ); + return; + }; + let launcher = gtk4::FileLauncher::new(Some(&gio::File::for_path(path))); + launcher.set_always_ask(true); + let window = ui_window(&ui); + launcher.launch(window.as_ref(), gio::Cancellable::NONE, move |result| { if let Err(e) = result && !e.matches(gtk4::DialogError::Dismissed) { toast_error(&ui, &gettext("Couldn't open file"), &e.to_string()); } - }, - ); + }); + }); } /// Copy the entry's public link, when it has one this client can read the @@ -1112,27 +1111,29 @@ pub(crate) fn activate_entry(ui: &Rc, entry: &DirEntry) { /// [`mounted_target`](crate::activation::mounted_target). pub(crate) fn download_and_open(ui: &Rc, entry: &DirEntry) { let rel = entry_rel(ui, entry); - let mountpoint = ui.dirs.resolved_mountpoint(&ui.dirs.load_config()); - if let Some(path) = mounted_target_rel(&mountpoint, &rel) { - open_path(&path.to_string_lossy()); - return; - } - // Ignore a repeat activation of a file already downloading, so an impatient - // double-click doesn't kick off a second round-trip. + // Ignore a repeat activation of a file already opening, so an impatient + // double-click doesn't open it twice or kick off a second round-trip. if !ui.opening.borrow_mut().insert(rel.clone()) { return; } - ui.busy_begin(); - let name = entry.name.clone(); - let rx = spawn_request( - ui.dirs.control_socket(), - Request::OpenFile { - path: rel.clone(), - uid: Some(entry.uid.clone()), - }, - ); + let mountpoint = ui.dirs.resolved_mountpoint(&ui.dirs.load_config()); + let (name, uid) = (entry.name.clone(), entry.uid.clone()); let ui = ui.clone(); glib::spawn_future_local(async move { + // Asked off the main thread: a mount that hangs used to hang the app. + if let Some(path) = present_within(mounted_path(&mountpoint, &rel)).await { + ui.opening.borrow_mut().remove(&rel); + open_path(&path.to_string_lossy()); + return; + } + ui.busy_begin(); + let rx = spawn_request( + ui.dirs.control_socket(), + Request::OpenFile { + path: rel.clone(), + uid: Some(uid), + }, + ); let result = rx.recv().await; ui.busy_end(); ui.opening.borrow_mut().remove(&rel); diff --git a/crates/pdfs-gui/src/app/pages/photos.rs b/crates/pdfs-gui/src/app/pages/photos.rs index 2e9feb2..3015e9b 100644 --- a/crates/pdfs-gui/src/app/pages/photos.rs +++ b/crates/pdfs-gui/src/app/pages/photos.rs @@ -3481,10 +3481,10 @@ pub(crate) fn gallery_unreachable(ui: &Rc) { /// right tool for the HEVC `.mkv`s this is aimed at — and falls back to the /// user's default handler when mpv isn't installed. pub(crate) fn play_external(path: &str) { - if Command::new("mpv").arg(path).spawn().is_ok() { - return; + match Command::new("mpv").arg(path).spawn() { + Ok(child) => pdfs_core::opener::reap(child), + Err(_) => open_path(path), } - open_path(path); } #[cfg(test)] diff --git a/crates/pdfs-gui/src/hud/mod.rs b/crates/pdfs-gui/src/hud/mod.rs index c282a39..04287c3 100644 --- a/crates/pdfs-gui/src/hud/mod.rs +++ b/crates/pdfs-gui/src/hud/mod.rs @@ -15,7 +15,6 @@ mod thumbs; use std::cell::{Cell, RefCell}; use std::path::{Path, PathBuf}; use std::rc::Rc; -use std::time::Duration; use adw::prelude::*; use gtk4::{gdk, gio, glib}; @@ -24,7 +23,7 @@ use pdfs_core::config::AppDirs; use pdfs_core::control::{Request, Response, SearchFilters, SearchHit, SearchSource}; use pdfs_core::opener::{self, OpenWith}; -use crate::activation::{DriveActivation, drive_activation, mounted_or_relative, mounted_target}; +use crate::activation::{DriveActivation, drive_activation, mounted_or_relative, present_within}; use crate::compat::{Spinner, spinner}; use crate::theme::set_proton_theme; use crate::{ @@ -38,10 +37,6 @@ use thumbs::Thumbs; /// Recent files shown above the pinned ones while the query is empty. const RECENT_LIMIT: usize = 8; -/// How long opening waits for the mount to answer whether it has a file. A -/// frozen mount must not freeze the launcher; the daemon is asked instead. -const STAT_TIMEOUT: Duration = Duration::from_secs(2); - /// The titled groups of the result list. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum Section { @@ -516,22 +511,11 @@ pub(crate) fn build_window(app: &adw::Application) -> Option> { Some(ui) } -/// Whether the mount has `hit`, asked on a worker thread and given up on -/// after [`STAT_TIMEOUT`]. `None` means "not there" or "no answer"; either -/// way the daemon is the fallback. +/// Whether the mount has `hit`, without blocking the window +/// ([`present_within`]). `None` means "not there" or "no answer"; either way +/// the daemon is the fallback. async fn mounted_target_within(mountpoint: PathBuf, hit: SearchHit) -> Option { - let (tx, rx) = async_channel::bounded(1); - std::thread::spawn(move || { - let _ = tx.send_blocking(mounted_target(&mountpoint, &hit)); - }); - match glib::future_with_timeout(STAT_TIMEOUT, rx.recv()).await { - Ok(Ok(target)) => target, - Ok(Err(_)) => None, - Err(_) => { - tracing::warn!("the mount did not answer in time; opening through the daemon"); - None - } - } + present_within(mounted_or_relative(&mountpoint, &hit)).await } impl Ui { diff --git a/crates/pdfs-gui/src/main.rs b/crates/pdfs-gui/src/main.rs index 8accc27..d177c80 100644 --- a/crates/pdfs-gui/src/main.rs +++ b/crates/pdfs-gui/src/main.rs @@ -361,8 +361,9 @@ fn open_manager(args: &[&str]) { .and_then(|p| p.parent().map(|d| d.join("pdfs-app"))) .filter(|p| p.exists()) .unwrap_or_else(|| PathBuf::from("pdfs-app")); - if let Err(e) = Command::new(&exe).args(args).spawn() { - tracing::error!("failed to launch {}: {e}", exe.display()); + match Command::new(&exe).args(args).spawn() { + Ok(child) => pdfs_core::opener::reap(child), + Err(e) => tracing::error!("failed to launch {}: {e}", exe.display()), } } diff --git a/docs/BUGS.md b/docs/BUGS.md index 784f9a9..ca7f099 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -100,6 +100,7 @@ a live account since. | Entry | Title | |---|---| +| [B243](#b243--a-hung-mount-freezes-the-app-when-a-file-is-opened) | A hung mount freezes the app when a file is opened | | [B242](#b242--opening-a-large-photo-freezes-the-app) | Opening a large photo freezes the app | | [B241](#b241--every-closed-photo-viewer-stays-in-memory-with-its-photo) | Every closed photo viewer stays in memory with its photo | | [B240](#b240--pdfs-mount-can-be-core-dumped-with-the-keys-in-memory) | pdfs mount can be core-dumped with the keys in memory | @@ -345,6 +346,34 @@ a live account since. Newest first. +### B243 — A hung mount freezes the app when a file is opened + +**Status:** Fixed, not verified live +**Found:** 2026-10-16, architecture audit (`FINDINGS.md` 2.5.3, and the unreaped child processes +in section 3). +**Where:** `present_within` in `crates/pdfs-gui/src/activation.rs`; `download_and_open` and +`open_entry_with` in `crates/pdfs-gui/src/app/pages/browser.rs`; `open_path` in +`crates/pdfs-gui/src/app/main.rs`; `reap` in `crates/pdfs-core/src/opener.rs` + +**Repro:** Let the mount hang (a stalled link with a request stuck in the kernel), then +double-click a file in the app's browser, or open the Proton Drive location. Separately, open a +few files from the app and look for `` children of `pdfs-app` in `ps`. + +**Cause.** Opening a file asked the mount whether it had the file with a `stat` on the GTK main +thread, and opening a location asked whether it was a folder the same way. A hung mount never +answers, so the whole app hung with it. The search launcher already asked on a worker with a +2 s limit. Separately, nothing waited for the applications the app, the tray and the player +button started, so each one stayed a zombie until the app quit. + +**Fix.** The browser's Open and Open With ask through `present_within`, which the launcher now +shares: a thread of its own, given up on after 2 s, with the daemon as the fallback. Opening a +location runs on a thread. Every child the GUI starts is waited for on a thread of its own. + +**Test:** `opener::tests::a_reaped_child_does_not_stay_a_zombie`; the mount checks have no +automated test, as the GUI tests run without a display. + +--- + ### B242 — Opening a large photo freezes the app **Status:** Fixed, not verified live