From ec69edad5d4f7757649e017b5d23d130eb480e49 Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 2 Jul 2026 02:40:07 +0200 Subject: [PATCH] refactor(natives): enabled disk logging for background task panics - Downgraded `blocking_task_panic_scope` panics from silent to logged recoverable. - Persists panic reports of caught worker task panics to the disk crash log while keeping stderr silent. - Consolidated panic payload message extraction by reusing `crash_handler::panic_payload` in the task runner. - Promoted the `SilenceHook` test helper to the shared testing module for use across multiple test suites. --- crates/pi-natives/src/crash_handler.rs | 51 ++++++++++++---------- crates/pi-natives/src/task.rs | 60 +++----------------------- crates/pi-natives/src/testing.rs | 35 +++++++++++++++ packages/natives/CHANGELOG.md | 2 +- 4 files changed, 72 insertions(+), 76 deletions(-) diff --git a/crates/pi-natives/src/crash_handler.rs b/crates/pi-natives/src/crash_handler.rs index be7cdc819..eae7cf60e 100644 --- a/crates/pi-natives/src/crash_handler.rs +++ b/crates/pi-natives/src/crash_handler.rs @@ -10,9 +10,11 @@ //! see issue #2211 ("Windows crash: Rust allocator failure after tasklist.exe //! popup"). The cdylib builds with `panic = "unwind"`, so a panic in vendored //! uutils code unwinds to the shell boundary and is recovered as a failed -//! command; such recoverable panics are logged to disk only, while fatal -//! crashes (allocation failure, or panics with no active uutils scope) still -//! get the stderr dump + process exit. Either way the record stays diagnosable. +//! command, and a panic in a `task::blocking` worker is caught at the napi +//! boundary and surfaces as a rejected JS Promise; such recoverable panics are +//! logged to disk only, while fatal crashes (allocation failure, or panics +//! with no active recovery scope) still get the stderr dump + process exit. +//! Either way the record stays diagnosable. //! //! Notes: //! - Backtraces are captured via [`Backtrace::force_capture`], so they work @@ -66,9 +68,13 @@ thread_local! { #[derive(Clone, Copy, Debug, Eq, PartialEq)] enum PanicDisposition { + /// No recovery boundary is active: persist the report, echo it to stderr, + /// and chain to the default hook (which ends the process). Fatal, + /// The panic will be caught and mapped to a failed command / rejected + /// Promise: persist the report to the crash log for diagnosis, but keep + /// stderr quiet and do not chain to the default hook. LoggedRecoverable, - SilentRecoverable, } /// Install the panic and allocation-error hooks. Idempotent. @@ -76,7 +82,6 @@ pub fn install() { INSTALL.call_once(|| { let prev_panic = std::panic::take_hook(); std::panic::set_hook(Box::new(move |info| match panic_disposition() { - PanicDisposition::SilentRecoverable => {}, PanicDisposition::LoggedRecoverable => { let report = format_panic_report(info); persist(&report, CrashKind::Panic, false); @@ -108,8 +113,10 @@ pub fn install() { /// /// The global panic hook checks this thread-local scope before reporting a /// panic. When a blocking worker closure panics, [`std::panic::catch_unwind`] -/// will turn it into a rejected JS Promise, so the hook must not emit a crash -/// report or chain to the default hook while this scope is active. +/// will turn it into a rejected JS Promise, so the hook downgrades the panic +/// to [`PanicDisposition::LoggedRecoverable`]: the report (location + +/// backtrace) is still persisted to the crash log, but nothing is echoed to +/// stderr and the default hook is not chained. pub(crate) fn blocking_task_panic_scope(f: impl FnOnce() -> R) -> R { struct Guard; @@ -129,9 +136,7 @@ fn blocking_task_panic_scope_active() -> bool { } fn panic_disposition() -> PanicDisposition { - if blocking_task_panic_scope_active() { - PanicDisposition::SilentRecoverable - } else if pi_uutils_ctx::is_active() { + if blocking_task_panic_scope_active() || pi_uutils_ctx::is_active() { PanicDisposition::LoggedRecoverable } else { PanicDisposition::Fatal @@ -209,7 +214,12 @@ fn write_alloc_failure_line(mut out: impl std::io::Write, size: usize) { let _ = out.write_all(b" bytes failed\n"); } -fn panic_payload(payload: &(dyn std::any::Any + Send)) -> String { +/// Extract a printable message from a panic payload captured by +/// [`std::panic::catch_unwind`] or handed to the panic hook. Handles the two +/// shapes `panic!` produces — `&'static str` (literal) and `String` +/// (formatted) — and degrades to a sentinel for arbitrary +/// [`panic_any`](std::panic::panic_any) payloads. +pub(crate) fn panic_payload(payload: &(dyn std::any::Any + Send)) -> String { if let Some(s) = payload.downcast_ref::<&'static str>() { (*s).to_owned() } else if let Some(s) = payload.downcast_ref::() { @@ -221,8 +231,9 @@ fn panic_payload(payload: &(dyn std::any::Any + Send)) -> String { fn persist(report: &str, kind: CrashKind, echo_stderr: bool) { // Echo to stderr so the user sees something even when the file write fails - // (read-only home, missing $HOME, …). Suppressed for recoverable uutils - // panics, which surface as a failed command instead of a crash. + // (read-only home, missing $HOME, …). Suppressed for recoverable panics + // (uutils shell boundary, `task::blocking` workers), which surface as a + // failed command / rejected Promise instead of a crash. if echo_stderr { let _ = writeln!(std::io::stderr(), "{report}"); } @@ -379,24 +390,20 @@ mod tests { use super::*; #[test] - fn blocking_task_panic_scope_silences_crash_reporting() { + fn blocking_task_panic_scope_downgrades_to_logged_recoverable() { assert_eq!(panic_disposition(), PanicDisposition::Fatal); blocking_task_panic_scope(|| { - assert_eq!(panic_disposition(), PanicDisposition::SilentRecoverable); + assert_eq!(panic_disposition(), PanicDisposition::LoggedRecoverable); }); assert_eq!(panic_disposition(), PanicDisposition::Fatal); } #[test] fn blocking_task_panic_scope_restores_after_unwind() { - // `std::panic::set_hook` is process-global; serialize the swap with - // every other hook-mutating test so a parallel take/set cannot pin the - // noop hook as the global default (see `crate::testing`). - let _hook_guard = crate::testing::lock_panic_hook(); - let prev = std::panic::take_hook(); - std::panic::set_hook(Box::new(|_| {})); + // Silence the process-global hook for the injected panic (and serialize + // the swap with every other hook-mutating test — see `crate::testing`). + let _silence = crate::testing::SilenceHook::new(); let unwound = std::panic::catch_unwind(|| blocking_task_panic_scope(|| panic!("boom"))); - std::panic::set_hook(prev); assert!(unwound.is_err(), "panic propagated to catch_unwind"); assert_eq!(panic_disposition(), PanicDisposition::Fatal); diff --git a/crates/pi-natives/src/task.rs b/crates/pi-natives/src/task.rs index 227921c45..15bc0ea4e 100644 --- a/crates/pi-natives/src/task.rs +++ b/crates/pi-natives/src/task.rs @@ -183,7 +183,8 @@ where // edge and force-abort the host under Rust's stabilized C-unwind rules // (RFC 2945, stable since 1.81). The crash handler scope tells the // global panic hook this panic is about to be caught and mapped to a - // `GenericFailure`, so it must not emit a native crash report. + // `GenericFailure`, so it downgrades the report to a disk-only crash + // log — no stderr dump, no default-hook chaining. match catch_unwind(AssertUnwindSafe(move || { crate::crash_handler::blocking_task_panic_scope(move || work(cancel_token)) })) { @@ -191,7 +192,7 @@ where Err(payload) => { // Extract the message BEFORE touching the payload's destructor: // disposal is the one remaining step that can panic again. - let message = panic_payload_message(&*payload); + let message = crate::crash_handler::panic_payload(&*payload); dispose_panic_payload(payload); Err(Error::new( Status::GenericFailure, @@ -206,20 +207,6 @@ where } } -/// Extract a printable message from a panic payload captured by -/// [`std::panic::catch_unwind`]. Handles the two shapes `panic!` produces — -/// `&'static str` (literal) and `String` (formatted) — and degrades to a -/// sentinel for arbitrary `panic_any` payloads. -fn panic_payload_message(payload: &(dyn std::any::Any + Send)) -> String { - if let Some(s) = payload.downcast_ref::<&'static str>() { - (*s).to_owned() - } else if let Some(s) = payload.downcast_ref::() { - s.clone() - } else { - String::from("") - } -} - /// Dispose of a caught panic payload without any possibility of a second /// unwind escaping this frame. /// @@ -229,10 +216,10 @@ fn panic_payload_message(payload: &(dyn std::any::Any + Send)) -> String { /// cross the same non-`C-unwind` FFI edge the surrounding [`catch_unwind`] /// exists to guard — force-aborting the host and defeating the recovery. The /// drop is therefore attempted under its own [`catch_unwind`], inside a -/// crash-handler scope so the global hook stays silent for a panic we are -/// about to swallow. +/// crash-handler scope so the global hook records at most a disk-only crash +/// log for a panic we are about to swallow. /// -/// SAFETY: if the destructor panics, the *secondary* payload is +/// Leak rationale: if the destructor panics, the *secondary* payload is /// [`std::mem::forget`]-ten instead of dropped — dropping it could panic /// again, unwinding out of this frame after the guard already fired once. /// Leaking one payload on this pathological path is a bounded, acceptable @@ -334,40 +321,7 @@ mod tests { //! past this method. use super::*; - - /// Boxed panic hook signature, factored out so the [`SilenceHook`] wrapper - /// stays readable — matches [`std::panic::take_hook`]'s return type. - type PanicHook = Box) + Sync + Send + 'static>; - - /// Suppress the default panic hook for a single `catch_unwind`, so injected - /// panic tests don't dump backtraces onto the test output. - /// - /// [`std::panic::set_hook`] is process-global, so `SilenceHook` holds - /// [`crate::testing::lock_panic_hook`] for the entire take → set → run → - /// restore window. Without that lock, two parallel tests could interleave - /// their hook swaps and permanently install the noop hook, muting crash - /// diagnostics for every later test in the crate. - struct SilenceHook { - prev: Option, - _guard: std::sync::MutexGuard<'static, ()>, - } - - impl SilenceHook { - fn new() -> Self { - let guard = crate::testing::lock_panic_hook(); - let prev = std::panic::take_hook(); - std::panic::set_hook(Box::new(|_| {})); - Self { prev: Some(prev), _guard: guard } - } - } - - impl Drop for SilenceHook { - fn drop(&mut self) { - if let Some(prev) = self.prev.take() { - std::panic::set_hook(prev); - } - } - } + use crate::testing::SilenceHook; fn blocking_task(tag: &'static str, work: F) -> Blocking where diff --git a/crates/pi-natives/src/testing.rs b/crates/pi-natives/src/testing.rs index 7f76ee0da..bc0c9717e 100644 --- a/crates/pi-natives/src/testing.rs +++ b/crates/pi-natives/src/testing.rs @@ -32,3 +32,38 @@ pub fn lock_panic_hook() -> MutexGuard<'static, ()> { .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()) } + +/// Boxed panic hook signature, factored out so the [`SilenceHook`] wrapper +/// stays readable — matches [`std::panic::take_hook`]'s return type. +type PanicHook = Box) + Sync + Send + 'static>; + +/// Suppress the global panic hook for the guard's lifetime, so injected panic +/// tests don't dump backtraces (or persist crash reports) onto the test run. +/// +/// [`std::panic::set_hook`] is process-global, so `SilenceHook` holds +/// [`lock_panic_hook`] for the entire take → set → run → restore window. +/// Without that lock, two parallel tests could interleave their hook swaps and +/// permanently install the noop hook, muting crash diagnostics for every later +/// test in the crate. +pub struct SilenceHook { + prev: Option, + _guard: MutexGuard<'static, ()>, +} + +impl SilenceHook { + #[allow(clippy::new_without_default, reason = "Default acquiring a global lock would surprise")] + pub fn new() -> Self { + let guard = lock_panic_hook(); + let prev = std::panic::take_hook(); + std::panic::set_hook(Box::new(|_| {})); + Self { prev: Some(prev), _guard: guard } + } +} + +impl Drop for SilenceHook { + fn drop(&mut self) { + if let Some(prev) = self.prev.take() { + std::panic::set_hook(prev); + } + } +} diff --git a/packages/natives/CHANGELOG.md b/packages/natives/CHANGELOG.md index 0ebd1397c..ed6ab0ef8 100644 --- a/packages/natives/CHANGELOG.md +++ b/packages/natives/CHANGELOG.md @@ -8,7 +8,7 @@ ### Fixed -- Fixed an issue where panics in native worker tasks (such as grep, AST parsing, globbing, workspace listing, HTML-to-markdown conversion, fuzzy finding, and clipboard image reading) would abort the host process instead of properly rejecting the returned JavaScript Promise. +- Fixed an issue where panics in native worker tasks (such as grep, AST parsing, globbing, workspace listing, HTML-to-markdown conversion, fuzzy finding, and clipboard image reading) would abort the host process instead of properly rejecting the returned JavaScript Promise. Panics recovered this way are recorded in the native crash log (disk only, no stderr noise) so real native bugs still leave a diagnostic artifact. - Fixed the blocking-task panic recovery itself aborting the host when a panic payload's own destructor panics; the message is extracted first and the payload disposed without unwinding across the FFI boundary. - Fixed a crash on Windows under low memory/commit charge conditions when spawning worker threads for token counting or sorting operations.