From 47c9b4a99b01c53f76f2537f5d3ec656b33fd896 Mon Sep 17 00:00:00 2001
From: FredApps <153173854+FredApps@users.noreply.github.com>
Date: Mon, 9 Nov 2026 20:58:45 +0100
Subject: [PATCH] #3357: finish crash reporter after accepted macOS teardown

---
 apps/effectcraft/src/crash_report.rs | 80 +++++++++++++++++++++++++++-
 apps/effectcraft/src/main.rs         | 42 +++++++++------
 docs/testing.md                      |  5 +-
 3 files changed, 109 insertions(+), 18 deletions(-)

diff --git a/apps/effectcraft/src/crash_report.rs b/apps/effectcraft/src/crash_report.rs
index 1dd25c571..837161811 100644
--- a/apps/effectcraft/src/crash_report.rs
+++ b/apps/effectcraft/src/crash_report.rs
@@ -60,6 +60,32 @@ fn finish_child(child: &mut Child, grace: Duration) -> io::Result<()> {
     Err(io::Error::other("crash reporter termination could not be confirmed"))
 }
 
+/// Shared ownership lets macOS finish in on_exit, while main retains the startup-error path.
+#[derive(Clone, Default)]
+pub struct Shutdown(std::sync::Arc<std::sync::Mutex<Option<Reporter>>>);
+
+impl Shutdown {
+    /// Install the one reporter created for this desktop launch.
+    pub fn install(&self, reporter: Reporter) -> io::Result<()> {
+        let mut slot = self.0.lock().map_err(|_| io::Error::other("crash reporter ownership was poisoned"))?;
+        *slot = Some(reporter);
+        Ok(())
+    }
+
+    /// Consume the reporter exactly once, after the applicable platform teardown boundary.
+    pub fn finish(&self) -> io::Result<()> {
+        let reporter = self.0.lock().map_err(|_| io::Error::other("crash reporter ownership was poisoned"))?.take();
+        reporter.map_or(Ok(()), Reporter::finish)
+    }
+}
+
+/// A panic during teardown must leave the reporter armed. macOS terminates after this hook
+/// without returning to main; other platforms pass None and finish after run_native returns.
+pub fn after_teardown(reporter: Option<&Shutdown>, teardown: impl FnOnce()) -> io::Result<()> {
+    teardown();
+    reporter.map_or(Ok(()), Shutdown::finish)
+}
+
 /// Each process/run has its own report, including simultaneous launches.
 pub fn run_id() -> String {
     let nanos = SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default().as_nanos();
@@ -363,6 +389,51 @@ mod tests {
         }
     }
 
+    #[test]
+    fn accepted_native_close_finishes_before_process_termination() {
+        let dir = folder();
+        let status = Command::new(std::env::current_exe().unwrap())
+            .args(["--exact", &test_name("aborting_parent"), "--ignored", "--nocapture"])
+            .env(ROLE, "native-close")
+            .env("EC_REPORT_DIR", &dir)
+            .stdout(Stdio::null())
+            .status()
+            .unwrap();
+        assert!(status.success());
+        wait_file(&dir.join("ready-8-1"));
+        assert!(!dir.join("crash-8-1.txt").exists());
+        std::fs::remove_dir_all(dir).unwrap();
+    }
+
+    #[test]
+    fn a_teardown_panic_does_not_mark_the_reporter_clean() {
+        let dir = folder();
+        let reporter = Shutdown::default();
+        reporter.install(helper(&dir, "8-2")).unwrap();
+        let result = std::panic::catch_unwind(|| {
+            let _ = after_teardown(Some(&reporter), || panic!("simulated teardown failure"));
+        });
+        assert!(result.is_err());
+        drop(reporter);
+        wait_file(&dir.join("ready-8-2"));
+        assert!(dir.join("crash-8-2.txt").exists());
+        std::fs::remove_dir_all(dir).unwrap();
+    }
+
+    #[test]
+    fn platforms_returning_to_main_keep_the_reporter_armed_until_then() {
+        let dir = folder();
+        let reporter = Shutdown::default();
+        reporter.install(helper(&dir, "8-3")).unwrap();
+        after_teardown(None, || {}).unwrap();
+        assert!(reporter.0.lock().unwrap().is_some());
+        reporter.clone().finish().unwrap();
+        reporter.finish().unwrap();
+        wait_file(&dir.join("ready-8-3"));
+        assert!(!dir.join("crash-8-3.txt").exists());
+        std::fs::remove_dir_all(dir).unwrap();
+    }
+
     #[test]
     fn an_aborting_parent_leaves_one_diagnostic_report() {
         let dir = folder();
@@ -475,8 +546,15 @@ mod tests {
     #[test]
     #[ignore]
     fn aborting_parent() {
-        assert_eq!(std::env::var(ROLE).unwrap(), "abort");
+        let role = std::env::var(ROLE).unwrap();
         let dir = PathBuf::from(std::env::var_os("EC_REPORT_DIR").unwrap());
+        if role == "native-close" {
+            let reporter = Shutdown::default();
+            reporter.install(helper(&dir, "8-1")).unwrap();
+            after_teardown(Some(&reporter.clone()), || {}).unwrap();
+            std::process::exit(0); // Models macOS not returning from its native event loop.
+        }
+        assert_eq!(role, "abort");
         let _reporter = helper(&dir, "4-1");
         std::process::abort();
     }
diff --git a/apps/effectcraft/src/main.rs b/apps/effectcraft/src/main.rs
index ce4bcdd84..3d9455c81 100644
--- a/apps/effectcraft/src/main.rs
+++ b/apps/effectcraft/src/main.rs
@@ -187,19 +187,17 @@ fn main() -> eframe::Result {
     }) {
         return Ok(());
     }
-    let mut reporter = None;
-    let result = run_desktop(&mut reporter);
+    let reporter = crash_report::Shutdown::default();
+    let result = run_desktop(&reporter);
     // run_desktop has dropped the app and handled any startup error with its existing dialog.
     // An unwinding panic or abort never reaches this marker.
-    if let Some(reporter) = reporter
-        && let Err(error) = reporter.finish()
-    {
+    if let Err(error) = reporter.finish() {
         log::warn!("could not mark crash reporter clean: {error}");
     }
     result
 }
 
-fn run_desktop(reporter: &mut Option<crash_report::Reporter>) -> eframe::Result {
+fn run_desktop(reporter: &crash_report::Shutdown) -> eframe::Result {
     // Windows: the update helper is a copy of this program started with `--finish-update`.
     #[cfg(windows)]
     if let Some(result) = craft_update::self_update::finish(&effectcraft_ui_egui::UPDATE_APP, &std::env::args_os().skip(1).collect::<Vec<_>>()) {
@@ -235,7 +233,11 @@ fn run_desktop(reporter: &mut Option<crash_report::Reporter>) -> eframe::Result
     }
     let logs = effectcraft_engine::logging::log_dir(effectcraft_host::config_dir().as_deref());
     match crash_report::start(&run_id, effectcraft_engine::logging::session_file().as_deref(), &logs.join("Crash Reports"), control_port.is_none()) {
-        Ok(started) => *reporter = Some(started),
+        Ok(started) => {
+            if let Err(error) = reporter.install(started) {
+                log::warn!("crash reporter could not be retained: {error}");
+            }
+        }
         Err(error) => log::warn!("crash reporter could not start: {error}"),
     }
     // The font menus list the installed fonts: read their names while the window opens.
@@ -272,6 +274,7 @@ fn run_desktop(reporter: &mut Option<crash_report::Reporter>) -> eframe::Result
     let apple_events = apple_events::AppleEvents::install();
     #[cfg(target_os = "macos")]
     let apple_events = &apple_events;
+    let desktop_reporter = reporter.clone();
     let result = eframe::run_native(
         "EffectCraft",
         options,
@@ -425,6 +428,7 @@ fn run_desktop(reporter: &mut Option<crash_report::Reporter>) -> eframe::Result
                 window: window::Tracker::new(opened, saved_window, control_port.is_none()),
                 launch,
                 drawn: false,
+                reporter: desktop_reporter,
             }))
         }),
     );
@@ -518,6 +522,7 @@ struct Desktop {
     launch: launch_guard::Launch,
     /// The first frame was drawn (logged once).
     drawn: bool,
+    reporter: crash_report::Shutdown,
 }
 
 impl eframe::App for Desktop {
@@ -552,15 +557,20 @@ impl eframe::App for Desktop {
     }
 
     fn on_exit(&mut self) {
-        // On macOS the app quits through `-[NSApplication terminate:]`, and the process ends
-        // before `eframe::run_native` returns (#341).
-        log::info!("the window is closing");
-        self.launch.exited();
-        self.window.save(self.app.session.config.as_deref());
-        self.app.on_exit();
-        effectcraft_host::stop_helpers();
-        // Scripts in Scripts/Shutdown run with the project closed.
-        let _ = self.app.session.execute("file.runShutdownScripts", json!({}));
+        // macOS terminates after this hook without returning from run_native (#341).
+        // Other platforms keep the reporter armed until main sees complete event-loop teardown.
+        let reporter = cfg!(target_os = "macos").then(|| self.reporter.clone());
+        if let Err(error) = crash_report::after_teardown(reporter.as_ref(), || {
+            log::info!("the window is closing");
+            self.launch.exited();
+            self.window.save(self.app.session.config.as_deref());
+            self.app.on_exit();
+            effectcraft_host::stop_helpers();
+            // Scripts in Scripts/Shutdown run with the project closed.
+            let _ = self.app.session.execute("file.runShutdownScripts", json!({}));
+        }) {
+            log::warn!("could not mark crash reporter clean after teardown: {error}");
+        }
     }
 
     fn raw_input_hook(&mut self, ctx: &egui::Context, raw_input: &mut egui::RawInput) {
diff --git a/docs/testing.md b/docs/testing.md
index 6c3d91626..54152b9dc 100644
--- a/docs/testing.md
+++ b/docs/testing.md
@@ -117,7 +117,10 @@ missing coverage, empty results and wall-time-only measurements cannot pass the
 
 The desktop starts a separate, lightweight reporter before loading fonts, projects or the GPU.
 A normal exit (including a handled startup error) sends a clean marker only after desktop
-teardown. An abort or other unexpected process termination closes the pipe without that marker.
+teardown. macOS finishes at the end of its successful exit hook because its native event loop
+terminates without returning to main; other platforms finish after the event loop returns.
+A teardown panic leaves reporting armed. An abort or other unexpected process termination
+closes the pipe without that marker.
 The helper saves a local report under the session log folder's `Crash Reports` directory and
 shows a native error dialog. Control-server launches save the report without a modal dialog.
 Reports contain version/platform and at most the last 64 KiB of the matching session's log.