From f550ab7bea81f35e24ef98196f0616bb647a8430 Mon Sep 17 00:00:00 2001 From: Michael Date: Tue, 20 Oct 2026 11:22:51 +0800 Subject: [PATCH] Wait for recovery cleanup before the app exits (#3140) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A save makes the document clean, which queues discarding this session's now-redundant recovery copy — but that's only queued, on a background worker thread, the same as a print job. Closing the app right after Save (the very next frame, with no regular recovery_tick in between to notice and queue the discard, or with the discard queued but not yet finished by the worker) could race the process exiting before the copy was actually removed, so the next launch wrongly offered to recover a document that was in fact saved cleanly. Added Recovery::finish and Session::finish_recovery, mirroring the print spooler's own finish_pending_jobs, and call it from the same places: on_exit and the macOS hard-quit path. Co-Authored-By: Claude Sonnet 5 --- apps/wordcraft/src/main.rs | 6 ++- crates/engine/src/recovery.rs | 15 ++++++++ crates/engine/src/recovery/desktop.rs | 53 +++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 2 deletions(-) diff --git a/apps/wordcraft/src/main.rs b/apps/wordcraft/src/main.rs index db5b2488a..a0ed03249 100644 --- a/apps/wordcraft/src/main.rs +++ b/apps/wordcraft/src/main.rs @@ -77,7 +77,7 @@ impl eframe::App for App { { ctx.send_viewport_cmd(egui::ViewportCommand::CancelClose); self.0.quit_requested = false; - schedule_macos_terminate(&self.0, self.2.as_ref()); + schedule_macos_terminate(&mut self.0, self.2.as_ref()); } #[cfg(not(target_os = "macos"))] if self.0.quit_requested { @@ -94,6 +94,7 @@ impl eframe::App for App { } fn on_exit(&mut self) { print::finish_pending_jobs(); + self.0.session.finish_recovery(); save_prefs(&self.0); if let Some(key_file) = &self.2 { key_file.remove(); @@ -109,7 +110,7 @@ impl eframe::App for App { /// removed first, as `on_exit` does, because a fallback [`std::process::exit`] /// skips `on_exit`. A second close while a quit is already scheduled is ignored. #[cfg(target_os = "macos")] -fn schedule_macos_terminate(app: &WordApp, key_file: Option<&control_server::KeyFile>) { +fn schedule_macos_terminate(app: &mut WordApp, key_file: Option<&control_server::KeyFile>) { use std::sync::atomic::{AtomicBool, Ordering}; if app.printing.busy() { return; @@ -121,6 +122,7 @@ fn schedule_macos_terminate(app: &WordApp, key_file: Option<&control_server::Key log::info!("window close: quitting"); // This path can bypass on_exit. Join the owned workers before termination and cleanup. print::finish_pending_jobs(); + app.session.finish_recovery(); save_prefs(app); if let Some(key_file) = key_file { key_file.remove(); diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index cf03a45a4..3306a9d61 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -88,6 +88,21 @@ impl Session { } None } + /// Waits for recovery to finish anything already under way, discarding this session's own + /// copy first if it no longer needs one (the document was saved, say). Desktop hosts call + /// this right before the process exits, the same as they wait out pending print jobs: + /// otherwise a save just before close can race the process exiting before a now-redundant + /// recovery copy actually gets removed, so the next launch wrongly offers to recover a + /// document that was in fact saved cleanly. + pub fn finish_recovery(&mut self) { + #[cfg(not(target_arch = "wasm32"))] + if let Some(mut r) = self.recovery.take() { + if let Err(e) = r.finish(self) { + log::warn!("recovery: {e}"); + } + self.recovery = Some(r); + } + } /// Only an explicit protect/save action waits for the worker and removes plaintext copies. /// A failure prevents applying the password; typing never waits. pub(crate) fn clear_recovery_before_password(&mut self) -> Result<(), CmdError> { diff --git a/crates/engine/src/recovery/desktop.rs b/crates/engine/src/recovery/desktop.rs index 676a6ed0c..040a24138 100644 --- a/crates/engine/src/recovery/desktop.rs +++ b/crates/engine/src/recovery/desktop.rs @@ -549,6 +549,34 @@ impl Recovery { self.revision = None; Ok(()) } + /// Waits out any job already in flight (an autosave queued just before shutdown, say), then, + /// if the document turns out to need no recovery copy kept (not dirty, or password-protected + /// — [`tick`](Self::tick)'s own condition), discards it and waits for that too. Desktop hosts + /// call this before the process exits, the same as the print spooler's own pending-job wait + /// (`finish_pending_jobs`): otherwise a save that lands right before close can race the + /// process exiting before the now-redundant recovery copy is actually removed, so the *next* + /// launch wrongly offers to recover a document that was in fact saved cleanly. + pub fn finish(&mut self, s: &Session) -> Result<(), String> { + if self.busy { + let e = self.rx.recv().map_err(|e| e.to_string())?; + self.documents = e.documents; + self.busy = false; + self.ready = true; + e.outcome?; + } + if let Some(id) = self.id.clone() + && (!s.dirty || s.password.is_some()) + { + self.queue(Job::Discard(id))?; + let e = self.rx.recv().map_err(|e| e.to_string())?; + self.busy = false; + self.documents = e.documents; + e.outcome?; + self.id = None; + self.revision = None; + } + Ok(()) + } } #[cfg(test)] @@ -628,6 +656,31 @@ mod tests { settle(s); } + /// #3140: a save that lands right before the app closes can race the process exiting before + /// the now-redundant recovery copy is actually removed, so the next launch wrongly offers to + /// recover a document that was in fact saved cleanly. `finish_recovery` (desktop hosts call it + /// on exit, the same as they wait for pending print jobs) must discard it synchronously + /// instead, with no regular tick in between — simulating closing right after Save, not after + /// however long it'd take the next normal autosave tick to notice and clean up. + #[test] + fn finish_recovery_discards_its_own_copy_for_a_document_saved_right_before_exit() { + let profile = Temp::new(); + let mut s = session(&profile.0); + s.run("document.setText", &json!({"text": "About to be saved"})).unwrap(); + save_now(&mut s); + assert_eq!(s.recovery_documents().len(), 1, "a recovery copy exists for the dirty document"); + // A real Save does this, then the app can close on the very next frame, with no regular + // recovery_tick() in between to notice `!s.dirty` and queue the discard itself. + s.dirty = false; + assert_eq!( + s.recovery_documents().len(), + 1, + "confirms the race: setting dirty=false alone (what Save does) doesn't discard anything by itself" + ); + s.finish_recovery(); + assert!(s.recovery_documents().is_empty(), "finish_recovery should have discarded the now-redundant copy before returning"); + } + #[test] fn recovery_commands_only_address_known_profile_records() { use crate::file_access::{AutomationRoots, confine};