diff --git a/app/src-tauri/src/audio.rs b/app/src-tauri/src/audio.rs index 851317af..e301a7db 100644 --- a/app/src-tauri/src/audio.rs +++ b/app/src-tauri/src/audio.rs @@ -197,7 +197,9 @@ pub(crate) async fn blocking( #[tauri::command] pub async fn audio_assets_dir(bridge: State<'_, CoreBridge>, page: String) -> IpcResult { - let dir = bridge.with(|bridge| assets_dir(bridge, &page))?; + let dir = bridge + .run(move |bridge| bridge.with(|bridge| assets_dir(bridge, &page))) + .await?; Ok(dir.to_string_lossy().into_owned()) } @@ -217,7 +219,10 @@ pub async fn audio_prepare( bridge: State<'_, CoreBridge>, mut request: StartRequest, ) -> IpcResult { - let (dir, _) = bridge.with(|bridge| checked_dir(bridge, &request.assets_dir.to_string_lossy()))?; + let assets_dir = request.assets_dir.to_string_lossy().into_owned(); + let (dir, _) = bridge + .run(move |bridge| bridge.with(|bridge| checked_dir(bridge, &assets_dir))) + .await?; request.assets_dir = dir; blocking(&state, move |state| state.lock().prepare(request).map_err(audio_error)).await } @@ -232,12 +237,16 @@ pub async fn audio_adopt_tracks( entry: RecordingEntry, growing: bool, ) -> IpcResult<()> { - let (dir, page) = bridge.with(|bridge| checked_dir(bridge, &assets_dir))?; + let (dir, page) = bridge + .run(move |bridge| bridge.with(|bridge| checked_dir(bridge, &assets_dir))) + .await?; let assets = blocking(&state, move |state| { track_assets(&state.0.services, &dir, &entry, growing) }) .await?; - bridge.with(|bridge| adopt(bridge, page, assets)) + bridge + .run(move |bridge| bridge.with(|bridge| adopt(bridge, page, assets))) + .await } #[tauri::command] @@ -291,7 +300,9 @@ pub async fn audio_recover( assets_dir: String, entry: RecordingEntry, ) -> IpcResult { - let (dir, _) = bridge.with(|bridge| checked_dir(bridge, &assets_dir))?; + let (dir, _) = bridge + .run(move |bridge| bridge.with(|bridge| checked_dir(bridge, &assets_dir))) + .await?; blocking(&state, move |state| { state.lock().recover(&dir, &entry).map_err(audio_error) }) @@ -306,7 +317,9 @@ pub async fn audio_open_playback( entry: RecordingEntry, device: Option, ) -> IpcResult { - let (dir, _) = bridge.with(|bridge| checked_dir(bridge, &assets_dir))?; + let (dir, _) = bridge + .run(move |bridge| bridge.with(|bridge| checked_dir(bridge, &assets_dir))) + .await?; blocking(&state, move |state| { state .lock() @@ -381,7 +394,9 @@ pub async fn audio_trim_silence( assets_dir: String, entry: RecordingEntry, ) -> IpcResult> { - let (dir, _) = bridge.with(|bridge| checked_dir(bridge, &assets_dir))?; + let (dir, _) = bridge + .run(move |bridge| bridge.with(|bridge| checked_dir(bridge, &assets_dir))) + .await?; blocking(&state, move |state| { let services = &state.0.services; let summary = entry.summary().map_err(audio_error)?; @@ -402,7 +417,9 @@ pub async fn audio_remove_part( start_ns: u64, end_ns: u64, ) -> IpcResult { - let (dir, _) = bridge.with(|bridge| checked_dir(bridge, &assets_dir))?; + let (dir, _) = bridge + .run(move |bridge| bridge.with(|bridge| checked_dir(bridge, &assets_dir))) + .await?; if end_ns <= start_ns { return Err(IpcError::invalid("endNs", "The part to remove ends before it starts.")); } @@ -426,7 +443,9 @@ pub async fn audio_delete_files( assets_dir: String, entry: RecordingEntry, ) -> IpcResult { - let (dir, _) = bridge.with(|bridge| checked_dir(bridge, &assets_dir))?; + let (dir, _) = bridge + .run(move |bridge| bridge.with(|bridge| checked_dir(bridge, &assets_dir))) + .await?; blocking(&state, move |_| { let summary = entry.summary().map_err(audio_error)?; storage::delete_audio(&dir, &summary).map_err(audio_error) diff --git a/app/src-tauri/src/audio_more/mod.rs b/app/src-tauri/src/audio_more/mod.rs index 699bdc08..bdc2cbfe 100644 --- a/app/src-tauri/src/audio_more/mod.rs +++ b/app/src-tauri/src/audio_more/mod.rs @@ -65,7 +65,9 @@ pub async fn audio_split( entry: RecordingEntry, at_ns: u64, ) -> IpcResult { - let (dir, _) = bridge.with(|bridge| checked_dir(bridge, &assets_dir))?; + let (dir, _) = bridge + .run(move |bridge| bridge.with(|bridge| checked_dir(bridge, &assets_dir))) + .await?; blocking(&state, move |state| { let services = state.services(); let summary = entry.summary().map_err(audio_error)?; @@ -90,7 +92,9 @@ pub async fn audio_enhance( assets_dir: String, entry: RecordingEntry, ) -> IpcResult { - let (dir, _) = bridge.with(|bridge| checked_dir(bridge, &assets_dir))?; + let (dir, _) = bridge + .run(move |bridge| bridge.with(|bridge| checked_dir(bridge, &assets_dir))) + .await?; blocking(&state, move |state| { let services = state.services(); let summary = entry.summary().map_err(audio_error)?; @@ -118,7 +122,9 @@ pub async fn audio_compress( entry: RecordingEntry, quality: String, ) -> IpcResult { - let (dir, _) = bridge.with(|bridge| checked_dir(bridge, &assets_dir))?; + let (dir, _) = bridge + .run(move |bridge| bridge.with(|bridge| checked_dir(bridge, &assets_dir))) + .await?; blocking(&state, move |state| { let services = state.services(); let summary = entry.summary().map_err(audio_error)?; @@ -150,7 +156,7 @@ pub struct Stored { /// What every recording of every open notebook takes, largest first. Pages in locked sections are left out. #[tauri::command] pub async fn audio_storage_scan(state: State<'_, AudioState>, bridge: State<'_, CoreBridge>) -> IpcResult> { - let notebooks = bridge.notebooks(); + let notebooks = bridge.run(|bridge| Ok(bridge.notebooks())).await?; blocking(&state, move |_| { let mut found = Vec::new(); for notebook in notebooks { @@ -212,13 +218,17 @@ fn recordings_of(page_dir: &Path) -> Vec { #[tauri::command] pub async fn audio_purge_history(bridge: State<'_, CoreBridge>, page: String) -> IpcResult { let id = PageId::parse(&page).map_err(|_| IpcError::invalid("page", "That isn't a page ID."))?; - let notebook = bridge.with(|bridge| { - bridge - .core - .find_node(id.0) - .map(|(notebook, _)| notebook) - .ok_or_else(|| IpcError::new("notFound", "That page isn't in a notebook.")) - })?; + let notebook = bridge + .run(move |bridge| { + bridge.with(|bridge| { + bridge + .core + .find_node(id.0) + .map(|(notebook, _)| notebook) + .ok_or_else(|| IpcError::new("notFound", "That page isn't in a notebook.")) + }) + }) + .await?; tauri::async_runtime::spawn_blocking(move || { notebook .delete_history(HistoryScope::Page(id), false) @@ -241,7 +251,9 @@ pub async fn audio_export( dest: String, format: String, ) -> IpcResult { - let (dir, _) = bridge.with(|bridge| checked_dir(bridge, &assets_dir))?; + let (dir, _) = bridge + .run(move |bridge| bridge.with(|bridge| checked_dir(bridge, &assets_dir))) + .await?; let dest = PathBuf::from(dest); if !grants.take(&dest) { return Err(IpcError::invalid("dest", "Choose where to save before exporting.")); @@ -311,7 +323,10 @@ pub async fn audio_import_file( InvokeBody::Raw(bytes) => bytes.clone(), InvokeBody::Json(_) => return Err(IpcError::invalid("body", "The file must come as raw bytes.")), }; - let (dir, _) = bridge.with(|bridge| checked_dir(bridge, &header.assets_dir))?; + let assets_dir = header.assets_dir.clone(); + let (dir, _) = bridge + .run(move |bridge| bridge.with(|bridge| checked_dir(bridge, &assets_dir))) + .await?; blocking(&state, move |state| { let services = state.services(); let extension: String = Path::new(&header.name) diff --git a/app/src-tauri/src/core_bridge.rs b/app/src-tauri/src/core_bridge.rs index 3aa88223..c0239304 100644 --- a/app/src-tauri/src/core_bridge.rs +++ b/app/src-tauri/src/core_bridge.rs @@ -10,11 +10,13 @@ //! (see [`crate::notes::migrate`]). use std::{ + borrow::Cow, collections::HashMap, fs, + panic::Location, path::{Path, PathBuf}, sync::{mpsc, Arc, Mutex, OnceLock, PoisonError}, - time::Duration, + time::{Duration, Instant}, }; use opennote_core::{ @@ -42,8 +44,12 @@ use crate::{ }; // Phase 8: search and linking over this core. One command carries every method. +mod held; pub mod search; +use held::{called_from, Door, Held, Holding, COMMAND_WAIT, HELD_WARNING}; +pub use held::{BUSY, RESPONSIVE_EVENT, STALLED_EVENT}; + /// How long the exit flush may take before the journals keep the rest for the next start (core plan 9.3). const EXIT_FLUSH: Duration = Duration::from_secs(5); @@ -109,14 +115,27 @@ impl EventSink for AppEvents { } } -/// The managed state behind the notes and page commands. +/// The managed state behind the notes and page commands. Cloning shares it, so a command can take it onto a +/// blocking thread (see [`CoreBridge::run`]). +#[derive(Clone)] pub struct CoreBridge { /// This device's files, `%LOCALAPPDATA%\OpenNote`: the core's own data and a beta 1 profile's files. root: PathBuf, /// The started bridge. A start that failed leaves it empty, so the next command tries again. state: Arc>>, + /// Where the commands wait for the core; see [`CoreBridge::take_core_within`]. + door: Door, relay: Arc, search: Arc, + /// The command that holds the core now, for the watchdog and the exit. + held: Held, + /// After how long a held core is reported: [`HELD_WARNING`], shorter in tests. + warn_after: Duration, + /// How long a command waits for the core: [`COMMAND_WAIT`], shorter in tests. + command_wait: Duration, + /// Set once the watchdog runs. It starts with the first command, before the core does, so a start that + /// never returns is reported too. + watched: Arc>, } /// The started core with what the commands keep beside it. @@ -268,11 +287,46 @@ impl CoreBridge { CoreBridge { root, state: Arc::new(Mutex::new(None)), + door: Arc::default(), relay: Arc::default(), search: Arc::default(), + held: Arc::default(), + warn_after: HELD_WARNING, + command_wait: COMMAND_WAIT, + watched: Arc::default(), } } + /// The same bridge, reporting a held core after `warn_after`. For tests. + #[cfg(test)] + pub fn warning_after(mut self, warn_after: Duration) -> CoreBridge { + self.warn_after = warn_after; + self + } + + /// The same bridge, with commands giving up on a held core after `command_wait`. For tests. + #[cfg(test)] + pub fn waiting_at_most(mut self, command_wait: Duration) -> CoreBridge { + self.command_wait = command_wait; + self + } + + /// Runs `work` on a blocking thread and answers when it is done. Every command that touches the core goes + /// through it. The core serves one command at a time, so a command waits for the one before it. That wait + /// used to sit on one of the async runtime's few worker threads. One command that never returned then took + /// every worker in turn, and nothing the interface asked for came back, core or not (beta 4's T2-3). On a + /// blocking thread a wait costs nothing else: unrelated commands keep answering, and the watchdog says what + /// the core is busy with. + pub async fn run( + &self, + work: impl FnOnce(&CoreBridge) -> IpcResult + Send + 'static, + ) -> IpcResult { + let bridge = self.clone(); + tauri::async_runtime::spawn_blocking(move || work(&bridge)) + .await + .map_err(|error| IpcError::new(codes::INTERNAL, error.to_string()))? + } + /// Sends the core's events through `emit`. The first call wins. pub fn listen(&self, emit: Emit) { let _ = self.relay.emit.set(emit); @@ -292,9 +346,30 @@ impl CoreBridge { } /// Runs `work` on the bridge, starting the core first if it hasn't started. A start that fails is logged and - /// tried again by the next command. + /// tried again by the next command. The watchdog knows the command by where it was called from. + #[track_caller] pub fn with(&self, work: impl FnOnce(&mut Bridge) -> IpcResult) -> IpcResult { - let mut state = self.state.lock().unwrap_or_else(PoisonError::into_inner); + self.with_named(called_from(Location::caller()), work) + } + + /// [`CoreBridge::with`], with the command's name for the watchdog should it hold the core too long. + pub fn with_named( + &self, + what: impl Into>, + work: impl FnOnce(&mut Bridge) -> IpcResult, + ) -> IpcResult { + self.watched.get_or_init(|| self.watch_held()); + let what = what.into(); + let Some(mut state) = self.take_core_within(self.command_wait) else { + let holder = self.holder(); + ::log::error!("{what} gave up waiting for the core: {holder}"); + return Err(IpcError::new( + BUSY, + format!("OpenNote's core is busy ({holder}). Try again in a moment."), + )); + }; + *self.held.lock().unwrap_or_else(PoisonError::into_inner) = Some((Instant::now(), what)); + let _holding = Holding(self.held.clone()); if state.is_none() { match Bridge::start(&self.root, self.relay.clone(), &self.search) { Ok(bridge) => { @@ -315,8 +390,19 @@ impl CoreBridge { /// Runs a notes command in the notes folder `folder`, after the folder's notebooks are open and a beta 1 /// profile is migrated, and sends the notes events for what it changed. + #[track_caller] pub fn notes(&self, folder: Option, work: impl FnOnce(&mut Bridge) -> IpcResult) -> IpcResult { - self.with(|bridge| { + self.notes_named(called_from(Location::caller()), folder, work) + } + + /// [`CoreBridge::notes`], with the command's name for the watchdog. + pub fn notes_named( + &self, + what: impl Into>, + folder: Option, + work: impl FnOnce(&mut Bridge) -> IpcResult, + ) -> IpcResult { + self.with_named(what, |bridge| { bridge.use_folder(folder); let result = work(bridge); bridge.drop_stale_handles(); @@ -365,8 +451,22 @@ impl CoreBridge { /// Saves every page and stops the core. The app calls it on exit. pub fn shutdown(&self) { + self.shutdown_within(EXIT_FLUSH); + } + + /// [`CoreBridge::shutdown`], waiting at most `timeout` for a command that holds the core. A command that + /// never returns must not keep the window open with nothing to show for it: the exit goes on without the + /// final save, and the journals keep every edit for the next start (spec 20). + pub fn shutdown_within(&self, timeout: Duration) { *self.relay.trees.lock().unwrap_or_else(PoisonError::into_inner) = None; - let state = self.state.lock().unwrap_or_else(PoisonError::into_inner); + let Some(state) = self.take_core_within(timeout) else { + ::log::error!( + "The core is still busy at exit ({}), so OpenNote exits without a final save. \ + The journals keep the edits for the next start.", + self.holder() + ); + return; + }; if let Some(bridge) = state.as_ref() { if let Err(error) = bridge.core.flush_all(EXIT_FLUSH) { ::log::error!("Couldn't save the open pages: {error}"); @@ -392,13 +492,24 @@ pub(crate) fn notes_folder(app: &AppHandle) -> Option { } /// Runs a notes command for the app: its events go to the window, in the notes folder from settings. +#[track_caller] pub(crate) fn run_notes( app: &AppHandle, bridge: &CoreBridge, work: impl FnOnce(&mut Bridge) -> IpcResult, +) -> IpcResult { + run_notes_named(app, bridge, called_from(Location::caller()), work) +} + +/// [`run_notes`], with the command's name for the watchdog. +pub(crate) fn run_notes_named( + app: &AppHandle, + bridge: &CoreBridge, + what: impl Into>, + work: impl FnOnce(&mut Bridge) -> IpcResult, ) -> IpcResult { bridge.listen_app(app); - bridge.notes(notes_folder(app), work) + bridge.notes_named(what, notes_folder(app), work) } /// An open page of the core, for Phase 4's image commands (P3-8). @@ -433,11 +544,15 @@ pub async fn page_open( client: String, viewport: Option, ) -> IpcResult { - run_notes(&app, &bridge, |bridge| { - let handle = bridge.handle(&page, &client)?; - let envelope = handle.envelope(viewport).map_err(internal)?; - Ok(Response::new(envelope.bytes)) - }) + bridge + .run(move |bridge| { + run_notes_named(&app, bridge, "page_open", |bridge| { + let handle = bridge.handle(&page, &client)?; + let envelope = handle.envelope(viewport).map_err(internal)?; + Ok(Response::new(envelope.bytes)) + }) + }) + .await } /// One transaction from the interface, in the client's sequence (core plan 11.4). @@ -451,70 +566,96 @@ pub async fn page_apply( ui: Option, edits: Vec, ) -> IpcResult { - bridge.with(|bridge| { - let handle = bridge.open_handle(&page, &client)?; - let request = TxnRequest { - page: handle.id(), - client: handle.client().clone(), - client_seq, - coalesce, - ui, - edits, - }; - handle.apply(request).map_err(edit_error) - }) + bridge + .run(move |bridge| { + bridge.with_named("page_apply", |bridge| { + let handle = bridge.open_handle(&page, &client)?; + let request = TxnRequest { + page: handle.id(), + client: handle.client().clone(), + client_seq, + coalesce, + ui, + edits, + }; + handle.apply(request).map_err(edit_error) + }) + }) + .await } /// The applied-changes frame of the undone step, or no bytes when there is nothing to undo. #[tauri::command] pub async fn page_undo(bridge: State<'_, CoreBridge>, page: String, client: String) -> IpcResult { - bridge.with(|bridge| { - let handle = bridge.open_handle(&page, &client)?; - let frame = handle.undo(handle.client()).map_err(edit_error)?; - Ok(Response::new(frame.map(|frame| frame.bytes).unwrap_or_default())) - }) + bridge + .run(move |bridge| { + bridge.with_named("page_undo", |bridge| { + let handle = bridge.open_handle(&page, &client)?; + let frame = handle.undo(handle.client()).map_err(edit_error)?; + Ok(Response::new(frame.map(|frame| frame.bytes).unwrap_or_default())) + }) + }) + .await } #[tauri::command] pub async fn page_redo(bridge: State<'_, CoreBridge>, page: String, client: String) -> IpcResult { - bridge.with(|bridge| { - let handle = bridge.open_handle(&page, &client)?; - let frame = handle.redo(handle.client()).map_err(edit_error)?; - Ok(Response::new(frame.map(|frame| frame.bytes).unwrap_or_default())) - }) + bridge + .run(move |bridge| { + bridge.with_named("page_redo", |bridge| { + let handle = bridge.open_handle(&page, &client)?; + let frame = handle.redo(handle.client()).map_err(edit_error)?; + Ok(Response::new(frame.map(|frame| frame.bytes).unwrap_or_default())) + }) + }) + .await } /// Saves the page now, as Ctrl+S does. #[tauri::command] pub async fn page_save_now(bridge: State<'_, CoreBridge>, page: String) -> IpcResult<()> { - bridge.with(|bridge| { - for handle in bridge.open_handles(&page) { - handle.save_now().map_err(internal)?; - } - Ok(()) - }) + bridge + .run(move |bridge| { + bridge.with_named("page_save_now", |bridge| { + for handle in bridge.open_handles(&page) { + handle.save_now().map_err(internal)?; + } + Ok(()) + }) + }) + .await } /// Closes the client's session of the page. The core saves it in the background. #[tauri::command] pub async fn page_close(bridge: State<'_, CoreBridge>, page: String, client: String) -> IpcResult<()> { - bridge.with(|bridge| { - let Ok(id) = PageId::parse(&page) else { - return Ok(()); - }; - let client = ClientId::parse(&client).map_err(|error| invalid("client", error))?; - bridge.homes.remove(&(id, client.clone())); - if let Some(handle) = bridge.open.remove(&(id, client.clone())) { - handle.close(&client).map_err(internal)?; - } - Ok(()) - }) + bridge + .run(move |bridge| { + bridge.with_named("page_close", |bridge| { + let Ok(id) = PageId::parse(&page) else { + return Ok(()); + }; + let client = ClientId::parse(&client).map_err(|error| invalid("client", error))?; + bridge.homes.remove(&(id, client.clone())); + if let Some(handle) = bridge.open.remove(&(id, client.clone())) { + handle.close(&client).map_err(internal)?; + } + Ok(()) + }) + }) + .await } /// The page's saved versions, newest first (core plan 8). #[tauri::command] pub async fn history_list(bridge: State<'_, CoreBridge>, page: String, client: String) -> IpcResult> { - bridge.with(|bridge| bridge.open_handle(&page, &client)?.history().map_err(core_error)) + bridge + .run(move |bridge| { + bridge.with_named("history_list", |bridge| { + bridge.open_handle(&page, &client)?.history().map_err(core_error) + }) + }) + .await } /// A saved version as a read-only page envelope. @@ -525,11 +666,15 @@ pub async fn history_open( client: String, revision: String, ) -> IpcResult { - bridge.with(|bridge| { - let handle = bridge.open_handle(&page, &client)?; - let envelope = handle.open_version(revision_id(&revision)?).map_err(core_error)?; - Ok(Response::new(envelope.bytes)) - }) + bridge + .run(move |bridge| { + bridge.with_named("history_open", |bridge| { + let handle = bridge.open_handle(&page, &client)?; + let envelope = handle.open_version(revision_id(&revision)?).map_err(core_error)?; + Ok(Response::new(envelope.bytes)) + }) + }) + .await } /// Restores a version in place, or as a new page, which the notes events then add to the tree. @@ -542,12 +687,16 @@ pub async fn history_restore( revision: String, as_copy: bool, ) -> IpcResult { - run_notes(&app, &bridge, |bridge| { - let handle = bridge.open_handle(&page, &client)?; - handle - .restore_version(revision_id(&revision)?, as_copy) - .map_err(core_error) - }) + bridge + .run(move |bridge| { + run_notes_named(&app, bridge, "history_restore", |bridge| { + let handle = bridge.open_handle(&page, &client)?; + handle + .restore_version(revision_id(&revision)?, as_copy) + .map_err(core_error) + }) + }) + .await } /// Brings blocks back from a version as one transaction of the client (P3-7). @@ -560,12 +709,16 @@ pub async fn history_restore_blocks( revision: String, blocks: Vec, ) -> IpcResult { - bridge.with(|bridge| { - let handle = bridge.open_handle(&page, &client)?; - handle - .restore_blocks(client_seq, revision_id(&revision)?, &blocks) - .map_err(edit_error) - }) + bridge + .run(move |bridge| { + bridge.with_named("history_restore_blocks", |bridge| { + let handle = bridge.open_handle(&page, &client)?; + handle + .restore_blocks(client_seq, revision_id(&revision)?, &blocks) + .map_err(edit_error) + }) + }) + .await } /// Names a version, or marks it to keep forever. @@ -578,12 +731,16 @@ pub async fn history_name( name: Option, keep: bool, ) -> IpcResult<()> { - bridge.with(|bridge| { - let handle = bridge.open_handle(&page, &client)?; - handle - .name_version(revision_id(&revision)?, name, keep) - .map_err(core_error) - }) + bridge + .run(move |bridge| { + bridge.with_named("history_name", |bridge| { + let handle = bridge.open_handle(&page, &client)?; + handle + .name_version(revision_id(&revision)?, name, keep) + .map_err(core_error) + }) + }) + .await } #[cfg(test)] diff --git a/app/src-tauri/src/core_bridge/held.rs b/app/src-tauri/src/core_bridge/held.rs new file mode 100644 index 00000000..bb2f9a8c --- /dev/null +++ b/app/src-tauri/src/core_bridge/held.rs @@ -0,0 +1,179 @@ +//! How a command takes the core and how long it waits for one that holds it, and the watchdog that names a +//! command holding it too long. The core serves one command at a time; beta 4's T2-3 was one that never +//! returned, with nothing to say so. This module bounds every wait for the core and reports the holder. + +use std::{ + borrow::Cow, + panic::Location, + sync::{Arc, Condvar, Mutex, MutexGuard, PoisonError}, + time::{Duration, Instant}, +}; + +use super::{Bridge, CoreBridge}; + +/// How long one command may hold the core before the log and the interface hear which one, and how often they +/// hear it again while it goes on. The core serves one command at a time, so a command that never returns stops +/// every later one; beta 4's T2-3 was such a stop, and nothing said what the core was doing. Now the watchdog +/// names the holder, the interface shows that the core isn't responding, and the exit goes on without it. +pub(super) const HELD_WARNING: Duration = Duration::from_secs(10); + +/// How long a command waits for the core before it fails as [`BUSY`] instead of joining the queue behind a +/// command that never returns. The interface sends its commands over the webview's IPC channel, which carries +/// only a handful at a time: once that many wait on the core, nothing else gets through, not even the window's +/// Close or a log line (beta 4's T2-3). A command that fails after this wait keeps the channel open, and the +/// interface says the core isn't responding instead of saying "Saving" for the rest of the session. It is longer +/// than the core's journal `OPEN_TIMEOUT` (10 s, journal_thread.rs), the one bounded wait a command makes on +/// another thread while it holds the core, so a stuck journal open gives up before the commands behind it do. +/// +/// The channel's limit stays: the interface keeps one edit in flight per page, but with many pages open their +/// edits and the tree's and search's commands can still fill the channel for one such wait while the core is +/// held. The watchdog reports the holder after [`HELD_WARNING`] all the same. +pub(super) const COMMAND_WAIT: Duration = Duration::from_secs(15); + +/// The error code of a command that gave up waiting for the core. The message names what holds it. +pub const BUSY: &str = "coreBusy"; + +/// The event that says a command has held the core for too long, with `what` holds it and for how many +/// `seconds`, and the one that says the core answers again. +pub const STALLED_EVENT: &str = "core:stalled"; +pub const RESPONSIVE_EVENT: &str = "core:responsive"; + +/// What holds the core now: since when, and which command. +pub(super) type Held = Arc)>>>; + +/// The door the commands queue at: whether the core is taken, and the condition the waiters sleep on until +/// the holder leaves. Serializing the commands here, not on the core's own lock, is what lets a command give up +/// after a bounded wait without polling. +pub(super) type Door = Arc<(Mutex, Condvar)>; + +/// The taken core: the core's state, given back through the door when dropped (after the state's own guard). +pub(super) struct CoreGuard<'a> { + state: MutexGuard<'a, Option>, + _turn: Turn<'a>, +} + +impl std::ops::Deref for CoreGuard<'_> { + type Target = Option; + fn deref(&self) -> &Option { + &self.state + } +} + +impl std::ops::DerefMut for CoreGuard<'_> { + fn deref_mut(&mut self) -> &mut Option { + &mut self.state + } +} + +/// Frees the door and wakes the next waiter when a command returns, or unwinds. +struct Turn<'a>(&'a Door); + +impl Drop for Turn<'_> { + fn drop(&mut self) { + let (taken, freed) = &**self.0; + *taken.lock().unwrap_or_else(PoisonError::into_inner) = false; + freed.notify_one(); + } +} + +/// Clears the holder when the command returns, or unwinds. +pub(super) struct Holding(pub(super) Held); + +impl Drop for Holding { + fn drop(&mut self) { + *self.0.lock().unwrap_or_else(PoisonError::into_inner) = None; + } +} + +/// Where a command was called from, for the watchdog: the file and line, with the workspace path trimmed. +pub(super) fn called_from(location: &Location<'_>) -> String { + let file = location.file().replace('\\', "/"); + let file = file.rsplit_once("/src/").map_or(file.as_str(), |(_, rest)| rest); + format!("the command at {file}:{}", location.line()) +} + +impl CoreBridge { + /// Takes the core, waiting at most `timeout` for the command that holds it. `None` when it is still held + /// after that. Commands queue at the door: a waiter sleeps on its condition variable until the holder + /// leaves, not on a poll, so a contended command starts the moment the one before it ends. + pub(super) fn take_core_within(&self, timeout: Duration) -> Option> { + let deadline = Instant::now().checked_add(timeout); + let (taken, freed) = &*self.door; + let mut taken = taken.lock().unwrap_or_else(PoisonError::into_inner); + while *taken { + let left = deadline.map_or(Duration::MAX, |d| d.saturating_duration_since(Instant::now())); + if left.is_zero() { + return None; + } + taken = freed + .wait_timeout(taken, left) + .unwrap_or_else(PoisonError::into_inner) + .0; + } + *taken = true; + drop(taken); + let state = self.state.lock().unwrap_or_else(PoisonError::into_inner); + Some(CoreGuard { + state, + _turn: Turn(&self.door), + }) + } + + /// What holds the core now, for the log. + pub(super) fn holder(&self) -> String { + let holder = self.held.lock().unwrap_or_else(PoisonError::into_inner); + holder + .as_ref() + .map_or("nothing holds it now".to_owned(), |(since, what)| { + format!("{what} has held it for {} s", since.elapsed().as_secs()) + }) + } + + /// Starts the watchdog that reports a command holding the core for longer than `warn_after`: an error in + /// the log, and [`STALLED_EVENT`] to the interface, again every `warn_after` while it goes on, and then + /// [`RESPONSIVE_EVENT`] once the command returns. It can't free the core, but the log then says what to + /// look at, and the title bar says the core isn't responding, where a silent hang said "Saving" forever. + pub(super) fn watch_held(&self) { + let held = Arc::downgrade(&self.held); + let relay = Arc::downgrade(&self.relay); + let warn_after = self.warn_after; + let spawned = std::thread::Builder::new() + .name("opennote-core-watchdog".into()) + .spawn(move || { + // The hold last reported, by its start, and when it was reported. + let mut reported: Option<(Instant, Instant)> = None; + loop { + std::thread::sleep(warn_after / 4); + let Some(held) = held.upgrade() else { return }; + let now = held.lock().unwrap_or_else(PoisonError::into_inner).clone(); + let send = |name, payload| { + if let Some(relay) = relay.upgrade() { + relay.send(name, payload); + } + }; + let Some((since, what)) = now else { + if reported.take().is_some() { + ::log::info!("The core answers again"); + send(RESPONSIVE_EVENT, serde_json::json!({})); + } + continue; + }; + let elapsed = since.elapsed(); + let due = match reported { + Some((hold, at)) if hold == since => at.elapsed() >= warn_after, + _ => elapsed >= warn_after, + }; + if !due { + continue; + } + reported = Some((since, Instant::now())); + let seconds = elapsed.as_secs(); + ::log::error!("{what} has held the core for {seconds} s, and every other command waits for it"); + send(STALLED_EVENT, serde_json::json!({ "what": what, "seconds": seconds })); + } + }); + if let Err(error) = spawned { + ::log::warn!("Couldn't start the core watchdog: {error}"); + } + } +} diff --git a/app/src-tauri/src/core_bridge/search.rs b/app/src-tauri/src/core_bridge/search.rs index 91793669..91a2fbaa 100644 --- a/app/src-tauri/src/core_bridge/search.rs +++ b/app/src-tauri/src/core_bridge/search.rs @@ -197,7 +197,9 @@ pub async fn search_call( ) -> IpcResult { bridge.listen_app(&app); // The first call starts the core, because the index follows it. - bridge.with(|_| Ok(()))?; + bridge + .run(|bridge| bridge.with_named("search_call", |_| Ok(()))) + .await?; let hub = bridge.search.clone(); hub.call(&method, args) } diff --git a/app/src-tauri/src/core_bridge_tests.rs b/app/src-tauri/src/core_bridge_tests.rs index af64390b..144ab832 100644 --- a/app/src-tauri/src/core_bridge_tests.rs +++ b/app/src-tauri/src/core_bridge_tests.rs @@ -18,8 +18,13 @@ fn text_of(handle: &PageHandle) -> String { } fn insert(handle: &PageHandle, seq: u64, markdown: &str) { + insert_block(handle, seq, "01k6f00000000000000000b001", markdown); +} + +/// Inserts a text block whose ID is made from `n`, so a page can take many. +fn insert_block(handle: &PageHandle, seq: u64, id: &str, markdown: &str) { let block: opennote_core::ops::resolve::NewBlock = serde_json::from_value(json!({ - "id": "01k6f00000000000000000b001", "type": "text", "data": { "markdown": markdown } + "id": id, "type": "text", "data": { "markdown": markdown } })) .expect("a new block"); let request = TxnRequest { @@ -37,8 +42,18 @@ fn insert(handle: &PageHandle, seq: u64, markdown: &str) { handle.apply(request).expect("applies"); } +/// The ID of the `n`th block a test inserts. +fn block_id(n: u64) -> String { + format!("01k6f0000000000000000{n:05}") +} + /// A notebook with a section and a page in `notes`; returns the page's ID. fn a_page(bridge: &CoreBridge, notes: &Path) -> String { + pages(bridge, notes, 1).remove(0) +} + +/// A notebook with a section and `count` pages in `notes`; returns the pages' IDs. +fn pages(bridge: &CoreBridge, notes: &Path, count: usize) -> Vec { bridge .notes(Some(notes.to_path_buf()), |bridge| { let create = |bridge: &mut Bridge, kind: &str, parent: Option| { @@ -48,9 +63,11 @@ fn a_page(bridge: &CoreBridge, notes: &Path) -> String { }; let notebook = create(bridge, "notebook", None)?; let section = create(bridge, "section", Some(notebook))?; - create(bridge, "page", Some(section)) + (0..count) + .map(|_| create(bridge, "page", Some(section.clone()))) + .collect() }) - .expect("a page") + .expect("pages") } #[test] @@ -74,6 +91,267 @@ fn typed_text_survives_a_restart_of_the_core() { assert_eq!(text, "Cells divide"); } +/// A command that holds the core for too long is reported, to the log and to the interface, with its name and +/// for how long; and the interface hears when the core answers again. Beta 4's T2-3 hung with "Saving" in the +/// title bar and nothing in the log, and nobody could tell what the core was doing. +#[test] +fn a_command_that_holds_the_core_too_long_is_reported_and_the_core_answers_again_after() { + let dir = tempfile::tempdir().expect("a temp folder"); + let notes = dir.path().join("Notes"); + let bridge = CoreBridge::at(dir.path().join("local")).warning_after(Duration::from_millis(100)); + let heard: Arc>> = Arc::default(); + let seen = heard.clone(); + bridge.listen(Box::new(move |name, payload| { + seen.lock().expect("the events").push((name, payload)); + })); + a_page(&bridge, ¬es); + // The first command started the core, which may itself have been reported: the events are looked for + // after the ones heard so far, by what they say. + let event = |name: &str, after: usize, what: Option<&str>| -> Option<(usize, Value)> { + let deadline = std::time::Instant::now() + Duration::from_secs(5); + while std::time::Instant::now() < deadline { + let found = heard + .lock() + .expect("the events") + .iter() + .enumerate() + .skip(after) + .find_map(|(i, (n, p))| { + (*n == name && what.is_none_or(|what| p["what"] == what)).then(|| (i, p.clone())) + }); + if found.is_some() { + return found; + } + std::thread::sleep(Duration::from_millis(10)); + } + None + }; + let mut seen_until = 0; + let (started, ready) = std::sync::mpsc::channel(); + let (release, released) = std::sync::mpsc::channel::<()>(); + std::thread::scope(|scope| { + let stuck = &bridge; + scope.spawn(move || { + let _ = stuck.with_named("a stuck command", |_| { + let _ = started.send(()); + let _ = released.recv_timeout(Duration::from_secs(5)); + Ok(()) + }); + }); + ready.recv().expect("the command runs"); + let (at, stalled) = + event(STALLED_EVENT, 0, Some("a stuck command")).expect("the interface hears that the core stalled"); + seen_until = at + 1; + assert!(stalled["seconds"].is_u64(), "{stalled}"); + assert!( + bridge.holder().starts_with("a stuck command has held it for"), + "{}", + bridge.holder() + ); + let _ = release.send(()); + }); + event(RESPONSIVE_EVENT, seen_until, None).expect("the interface hears that the core answers again"); + assert_eq!(bridge.holder(), "nothing holds it now"); + bridge + .with_named("a command after it", |_| Ok(())) + .expect("the core is free"); + bridge.shutdown(); +} + +/// Every command through `with` is known to the watchdog by where it was called from, so the log names the +/// holder without each caller naming itself. +#[test] +fn the_watchdog_knows_a_command_by_where_it_was_called_from() { + let dir = tempfile::tempdir().expect("a temp folder"); + let bridge = CoreBridge::at(dir.path().join("local")); + let holder = bridge.with(|_| Ok(bridge.holder())).expect("runs"); + assert!(holder.starts_with("the command at core_bridge_tests.rs:"), "{holder}"); + let holder = bridge.notes(None, |_| Ok(bridge.holder())).expect("runs"); + assert!(holder.starts_with("the command at core_bridge_tests.rs:"), "{holder}"); + bridge.shutdown(); +} + +/// Closes a client's session of a page, as `page_close` does. +fn close_page(bridge: &CoreBridge, page: &str, client: &str) { + bridge + .with(|bridge| { + let id = PageId::parse(page).expect("a page ID"); + let client = ClientId::parse(client).expect("a client ID"); + bridge.homes.remove(&(id, client.clone())); + if let Some(handle) = bridge.open.remove(&(id, client.clone())) { + handle.close(&client).map_err(internal)?; + } + Ok(()) + }) + .expect("closes"); +} + +/// The lock order of the core, under the load beta 4's T2-3 session had: typing, undo, saves, renames, pages +/// opening and closing, and a page going to Trash and back, all at once with the saver and maintenance threads. +/// A command that waits for another forever would hang this test, so a watchdog of its own ends the process +/// with the holder's name after a minute. +#[test] +fn concurrent_edits_saves_renames_and_closes_never_wait_for_each_other_forever() { + let dir = tempfile::tempdir().expect("a temp folder"); + let notes = dir.path().join("Notes"); + let bridge = CoreBridge::at(dir.path().join("local")); + let ids = pages(&bridge, ¬es, 3); + let (a, b, c) = (ids[0].clone(), ids[1].clone(), ids[2].clone()); + let open = |page: &str, client: &str| { + bridge + .notes(Some(notes.clone()), |bridge| bridge.handle(page, client)) + .expect("opens") + }; + let typing = open(&a, "typing"); + let saving = open(&a, "saving"); + let done = Arc::new(std::sync::atomic::AtomicBool::new(false)); + { + let (done, bridge) = (done.clone(), bridge.clone()); + std::thread::spawn(move || { + for _ in 0..60 { + std::thread::sleep(Duration::from_secs(1)); + if done.load(std::sync::atomic::Ordering::SeqCst) { + return; + } + } + eprintln!("The core deadlocked: {}", bridge.holder()); + std::process::abort(); + }); + } + let until = std::time::Instant::now() + Duration::from_millis(1500); + let running = || std::time::Instant::now() < until; + std::thread::scope(|scope| { + scope.spawn(|| { + let (mut seq, mut n) = (0, 0); + while running() { + seq += 1; + n += 1; + insert_block(&typing, seq, &block_id(n), "typed"); + if n % 3 == 0 { + typing.undo(typing.client()).expect("undoes"); + } + } + }); + scope.spawn(|| { + while running() { + saving.save_now().expect("saves"); + } + }); + scope.spawn(|| { + let mut n = 0; + while running() { + n += 1; + let title = format!("Title {n}"); + bridge + .notes(Some(notes.clone()), |bridge| { + bridge.dispatch("notes_rename", &json!({ "id": a, "title": title })) + }) + .expect("renames"); + } + }); + scope.spawn(|| { + let mut n = 1_000; + while running() { + n += 1; + let handle = open(&b, "window"); + insert_block(&handle, 1, &block_id(n), "in a window"); + close_page(&bridge, &b, "window"); + } + }); + scope.spawn(|| { + while running() { + let receipt = bridge + .notes(Some(notes.clone()), |bridge| { + bridge.dispatch("notes_trash", &json!({ "ids": [c] })) + }) + .expect("trashes"); + let receipt = receipt["id"].as_str().unwrap_or_default().to_owned(); + bridge + .notes(Some(notes.clone()), |bridge| { + bridge.dispatch("notes_restore", &json!({ "receiptId": receipt })) + }) + .expect("restores"); + } + }); + }); + done.store(true, std::sync::atomic::Ordering::SeqCst); + close_page(&bridge, &a, "typing"); + close_page(&bridge, &a, "saving"); + bridge.shutdown(); +} + +/// A command behind one that never returns fails as busy after its wait, naming the holder, instead of joining +/// the queue. The webview's IPC channel carries only a few commands at once, so a queue of them froze every +/// command. Close and the log froze with them (beta 4's T2-3). The core serves the next command once the holder +/// returns. +#[test] +fn a_command_gives_up_on_a_held_core_and_names_the_holder() { + let dir = tempfile::tempdir().expect("a temp folder"); + let notes = dir.path().join("Notes"); + let bridge = CoreBridge::at(dir.path().join("local")).waiting_at_most(Duration::from_millis(100)); + a_page(&bridge, ¬es); + let (started, ready) = std::sync::mpsc::channel(); + let (release, released) = std::sync::mpsc::channel::<()>(); + std::thread::scope(|scope| { + let stuck = &bridge; + scope.spawn(move || { + let _ = stuck.with_named("a stuck command", |_| { + let _ = started.send(()); + let _ = released.recv_timeout(Duration::from_secs(5)); + Ok(()) + }); + }); + ready.recv().expect("the command runs"); + let at = std::time::Instant::now(); + let error = bridge + .with_named("the next command", |_| Ok(())) + .expect_err("the next command gives up"); + assert!(at.elapsed() < Duration::from_secs(2), "it waited {:?}", at.elapsed()); + assert_eq!(error.code, BUSY); + assert!( + error.message.contains("a stuck command"), + "the message names the holder: {}", + error.message + ); + release.send(()).expect("the holder is released"); + }); + bridge + .with_named("after", |_| Ok(())) + .expect("the core answers once the holder returns"); + bridge.shutdown(); +} + +#[test] +fn the_exit_gives_up_on_a_command_that_holds_the_core_too_long() { + let dir = tempfile::tempdir().expect("a temp folder"); + let notes = dir.path().join("Notes"); + let bridge = CoreBridge::at(dir.path().join("local")); + a_page(&bridge, ¬es); + let (started, ready) = std::sync::mpsc::channel(); + let took = std::thread::scope(|scope| { + scope.spawn(|| { + let _ = bridge.with_named("a stuck command", |_| { + let _ = started.send(()); + std::thread::sleep(Duration::from_millis(900)); + Ok(()) + }); + }); + ready.recv().expect("the command runs"); + let at = std::time::Instant::now(); + bridge.shutdown_within(Duration::from_millis(150)); + at.elapsed() + }); + assert!( + took < Duration::from_millis(800), + "the exit waited {took:?} for the command" + ); + assert!( + bridge.held.lock().expect("the holder").is_none(), + "the holder is cleared once the command returns" + ); + bridge.shutdown(); +} + #[test] fn only_pages_of_open_notebooks_open_and_page_ids_are_checked() { let dir = tempfile::tempdir().expect("a temp folder"); diff --git a/app/src-tauri/src/ink_bridge.rs b/app/src-tauri/src/ink_bridge.rs index 6e79dc6d..dff26d7f 100644 --- a/app/src-tauri/src/ink_bridge.rs +++ b/app/src-tauri/src/ink_bridge.rs @@ -81,15 +81,26 @@ pub async fn page_add_strokes(bridge: State<'_, CoreBridge>, request: tauri::ipc return Err(IpcError::invalid("body", "The strokes must come as raw bytes.")); }; let (header, records) = split_body(body)?; - let handle = bridge.with(|bridge| bridge.open_handle(&header.page, &header.client))?; - add_strokes(&handle, header, records) + let records = records.to_vec(); + bridge + .run(move |bridge| { + let handle = bridge.with_named("page_add_strokes", |bridge| { + bridge.open_handle(&header.page, &header.client) + })?; + add_strokes(&handle, header, &records) + }) + .await } /// Every live stroke of the page as ink records, in drawing order. #[tauri::command] pub async fn page_read_strokes(bridge: State<'_, CoreBridge>, page: String, client: String) -> IpcResult { - let handle = bridge.with(|bridge| bridge.open_handle(&page, &client))?; - Ok(Response::new(handle.read_strokes(None, None).records)) + bridge + .run(move |bridge| { + let handle = bridge.with_named("page_read_strokes", |bridge| bridge.open_handle(&page, &client))?; + Ok(Response::new(handle.read_strokes(None, None).records)) + }) + .await } #[cfg(test)] diff --git a/app/src-tauri/src/interop/commands.rs b/app/src-tauri/src/interop/commands.rs index af878edc..163f334e 100644 --- a/app/src-tauri/src/interop/commands.rs +++ b/app/src-tauri/src/interop/commands.rs @@ -24,7 +24,7 @@ use super::{ restore::{self, ImportedTree}, }; use crate::{ - core_bridge::{notes_folder, run_notes, CoreBridge}, + core_bridge::{notes_folder, run_notes_named, CoreBridge}, ipc::{codes, IpcError, IpcResult}, }; @@ -184,7 +184,9 @@ pub async fn interop_preview( path: String, choices: Option, ) -> IpcResult> { - let device = bridge.with(|bridge| Ok(bridge.core.device()))?; + let device = bridge + .run(|bridge| bridge.with(|bridge| Ok(bridge.core.device()))) + .await?; let (guard, control) = Job::start(&job, emitter(&app)); let options = choices.unwrap_or_default().options(); let result = blocking(move || { @@ -243,7 +245,9 @@ pub async fn interop_import( let Some(parent) = notes_folder(&app) else { return Err(IpcError::invalid("folder", "Choose a notes folder first.")); }; - let device = bridge.with(|bridge| Ok(bridge.core.device()))?; + let device = bridge + .run(|bridge| bridge.with(|bridge| Ok(bridge.core.device()))) + .await?; let (guard, control) = Job::start(&job, emitter(&app)); let options = choices.unwrap_or_default().options(); let folder = parent.clone(); @@ -253,20 +257,29 @@ pub async fn interop_import( run_import(&folder, Path::new(&path), &options, device, control) }) .await?; - outcome(result, |(report, dir)| { - let tree = run_notes(&app, &bridge, |bridge| { - let handle = bridge.core.open_notebook(&dir).map_err(internal)?; - Ok(restore::tree_of(&handle)) - })?; - let (lost_pages, skipped) = report.loss_counts(); - jobs::remember_report(&job, report.to_markdown()); - Ok(ImportDone { + let (report, dir) = match result { + Ok(made) => made, + Err(InteropError::Canceled) => return Ok(Outcome::Canceled), + Err(error) => return Err(failure(error)), + }; + let tree = bridge + .run(move |bridge| { + run_notes_named(&app, bridge, "interop_import", |bridge| { + let handle = bridge.core.open_notebook(&dir).map_err(internal)?; + Ok(restore::tree_of(&handle)) + }) + }) + .await?; + let (lost_pages, skipped) = report.loss_counts(); + jobs::remember_report(&job, report.to_markdown()); + Ok(Outcome::Done { + result: ImportDone { tree, pages: report.pages.len(), losses: report.loss_groups(), lost_pages, skipped, - }) + }, }) } @@ -320,11 +333,16 @@ pub async fn interop_export( if !Path::new(&request.folder).is_dir() { return Err(IpcError::invalid("folder", "Choose a folder that exists.")); } - let source = bridge.with(|bridge| { - // The export reads the page files, so the open pages are saved first. - bridge.core.flush_all(SAVE_OPEN_PAGES).map_err(internal)?; - TreeSource::build(bridge, &request).map_err(failure) - })?; + let (source, request) = bridge + .run(move |bridge| { + bridge.with(|bridge| { + // The export reads the page files, so the open pages are saved first. + bridge.core.flush_all(SAVE_OPEN_PAGES).map_err(internal)?; + let source = TreeSource::build(bridge, &request).map_err(failure)?; + Ok((source, request)) + }) + }) + .await?; let (guard, control) = Job::start(&job, emitter(&app)); let result = blocking(move || { let _guard = guard; diff --git a/app/src-tauri/src/notes/mod.rs b/app/src-tauri/src/notes/mod.rs index 7a7e1086..d0423562 100644 --- a/app/src-tauri/src/notes/mod.rs +++ b/app/src-tauri/src/notes/mod.rs @@ -315,12 +315,16 @@ pub async fn notes_load_initial( bridge: State<'_, CoreBridge>, path: Vec, ) -> IpcResult { - run_notes(&app, &bridge, |bridge| bridge.load_initial(&path)) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| bridge.load_initial(&path))) + .await } #[tauri::command] pub async fn notes_list_notebooks(app: AppHandle, bridge: State<'_, CoreBridge>) -> IpcResult> { - run_notes(&app, &bridge, |bridge| Ok(bridge.list_notebooks())) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| Ok(bridge.list_notebooks()))) + .await } #[tauri::command] @@ -329,17 +333,23 @@ pub async fn notes_list_children( bridge: State<'_, CoreBridge>, parent_id: String, ) -> IpcResult> { - run_notes(&app, &bridge, |bridge| bridge.list_children(&parent_id)) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| bridge.list_children(&parent_id))) + .await } #[tauri::command] pub async fn notes_get(app: AppHandle, bridge: State<'_, CoreBridge>, id: String) -> IpcResult> { - run_notes(&app, &bridge, |bridge| Ok(bridge.get_node(&id))) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| Ok(bridge.get_node(&id)))) + .await } #[tauri::command] pub async fn notes_create(app: AppHandle, bridge: State<'_, CoreBridge>, input: CreateInput) -> IpcResult { - run_notes(&app, &bridge, |bridge| bridge.create(input)) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| bridge.create(input))) + .await } /// Renames a node. A page's title is the one in its `page.json`, which its heading shows too. @@ -350,7 +360,9 @@ pub async fn notes_rename( id: String, title: String, ) -> IpcResult { - run_notes(&app, &bridge, |bridge| bridge.rename(&id, &title)) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| bridge.rename(&id, &title))) + .await } #[tauri::command] @@ -360,7 +372,9 @@ pub async fn notes_set_color( id: String, color: Option, ) -> IpcResult { - run_notes(&app, &bridge, |bridge| bridge.set_color(&id, color)) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| bridge.set_color(&id, color))) + .await } #[tauri::command] @@ -370,7 +384,9 @@ pub async fn notes_move( ids: Vec, placement: Placement, ) -> IpcResult<()> { - run_notes(&app, &bridge, |bridge| bridge.move_nodes(&ids, &placement)) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| bridge.move_nodes(&ids, &placement))) + .await } #[tauri::command] @@ -380,12 +396,16 @@ pub async fn notes_set_page_level( ids: Vec, level: u8, ) -> IpcResult<()> { - run_notes(&app, &bridge, |bridge| bridge.set_page_level(&ids, level)) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| bridge.set_page_level(&ids, level))) + .await } #[tauri::command] pub async fn notes_trash(app: AppHandle, bridge: State<'_, CoreBridge>, ids: Vec) -> IpcResult { - run_notes(&app, &bridge, |bridge| bridge.trash(&ids)) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| bridge.trash(&ids))) + .await } #[tauri::command] @@ -394,12 +414,16 @@ pub async fn notes_restore( bridge: State<'_, CoreBridge>, receipt_id: String, ) -> IpcResult> { - run_notes(&app, &bridge, |bridge| bridge.restore(&receipt_id)) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| bridge.restore(&receipt_id))) + .await } #[tauri::command] pub async fn notes_list_trash(app: AppHandle, bridge: State<'_, CoreBridge>) -> IpcResult> { - run_notes(&app, &bridge, |bridge| bridge.list_trash()) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| bridge.list_trash())) + .await } #[tauri::command] @@ -408,21 +432,29 @@ pub async fn notes_restore_from_trash( bridge: State<'_, CoreBridge>, ids: Vec, ) -> IpcResult> { - run_notes(&app, &bridge, |bridge| bridge.restore_from_trash(&ids)) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| bridge.restore_from_trash(&ids))) + .await } /// Deletes Trash items for good. A notebook in Trash keeps its folder, which File Explorer can delete. #[tauri::command] pub async fn notes_purge(app: AppHandle, bridge: State<'_, CoreBridge>, ids: Vec) -> IpcResult<()> { - run_notes(&app, &bridge, |bridge| bridge.purge(&ids)) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| bridge.purge(&ids))) + .await } #[tauri::command] pub async fn notes_status(app: AppHandle, bridge: State<'_, CoreBridge>) -> IpcResult { - run_notes(&app, &bridge, |bridge| Ok(bridge.notes_status())) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| Ok(bridge.notes_status()))) + .await } #[tauri::command] pub async fn notes_flush(app: AppHandle, bridge: State<'_, CoreBridge>) -> IpcResult<()> { - run_notes(&app, &bridge, |bridge| bridge.notes_flush()) + bridge + .run(move |bridge| run_notes(&app, bridge, |bridge| bridge.notes_flush())) + .await } diff --git a/app/src-tauri/src/shellqol/mod.rs b/app/src-tauri/src/shellqol/mod.rs index fc6f507d..994e70a8 100644 --- a/app/src-tauri/src/shellqol/mod.rs +++ b/app/src-tauri/src/shellqol/mod.rs @@ -90,22 +90,28 @@ pub async fn shellqol_call( if name == "library.rebuildIndex" { return crate::core_bridge::search::search_call(app, bridge, "rebuild".to_owned(), json!({})).await; } - let (group, _) = name.split_once('.').unwrap_or((name.as_str(), "")); - match group { - "notes" => notes_ops::call(&app, &bridge, &name, &args), - "window" => windows_ops::call(&app, &name, &args), - "dock" => dock::call(&app, &name, &args), - "power" => power::call(&app, &name), - "cloud" => cloud::call(&app, &name, &args), - "conflict" => conflicts::call(&app, &name, &args), - "backup" => backup::call(&app, &bridge, &name, &args), - "library" => library::call(&app, &bridge, &name, &args), - "quick" => quick::call(&app, &bridge, &name, &args), - "shortcut" => shortcuts::call(&app, &bridge, &name, &args), - "external" => external::call(&app, &name, &args), - "prefs" => prefs::call(&app, &name, &args), - _ => Err(IpcError::invalid("name", &format!("{name} isn't a shell call"))), - } + // Every call runs on a blocking thread: the ones on the core may wait for it (see `CoreBridge::run`), and + // the others open files and dialogs. + bridge + .run(move |bridge| { + let (group, _) = name.split_once('.').unwrap_or((name.as_str(), "")); + match group { + "notes" => notes_ops::call(&app, bridge, &name, &args), + "window" => windows_ops::call(&app, &name, &args), + "dock" => dock::call(&app, &name, &args), + "power" => power::call(&app, &name), + "cloud" => cloud::call(&app, &name, &args), + "conflict" => conflicts::call(&app, &name, &args), + "backup" => backup::call(&app, bridge, &name, &args), + "library" => library::call(&app, bridge, &name, &args), + "quick" => quick::call(&app, bridge, &name, &args), + "shortcut" => shortcuts::call(&app, bridge, &name, &args), + "external" => external::call(&app, &name, &args), + "prefs" => prefs::call(&app, &name, &args), + _ => Err(IpcError::invalid("name", &format!("{name} isn't a shell call"))), + } + }) + .await } /// A small `{ ok: true }` answer. diff --git a/app/src-tauri/src/shellqol/notes_ops.rs b/app/src-tauri/src/shellqol/notes_ops.rs index e3ee5331..6ad121c8 100644 --- a/app/src-tauri/src/shellqol/notes_ops.rs +++ b/app/src-tauri/src/shellqol/notes_ops.rs @@ -10,13 +10,13 @@ use tauri::AppHandle; use super::{arg, out}; use crate::{ - core_bridge::{run_notes, Bridge, CoreBridge}, + core_bridge::{run_notes_named, Bridge, CoreBridge}, ipc::{IpcError, IpcResult}, notes::{from_core, invalid_move, not_found, tree::Found, CreateInput, NodeSummary, Placement}, }; pub fn call(app: &AppHandle, bridge: &CoreBridge, name: &str, args: &Value) -> IpcResult { - run_notes(app, bridge, |b| match name { + run_notes_named(app, bridge, name.to_owned(), |b| match name { "notes.setPinned" => out(b.set_pinned(&arg::(args, "id")?, arg(args, "pinned")?)?), "notes.setArchived" => out(b.set_archived(&arg::(args, "id")?, arg(args, "archived")?)?), "notes.duplicate" => out(b.duplicate_node(&arg::(args, "id")?)?), diff --git a/app/src/features/page/sync/textSync.test.ts b/app/src/features/page/sync/textSync.test.ts index 82920e1e..798534eb 100644 --- a/app/src/features/page/sync/textSync.test.ts +++ b/app/src/features/page/sync/textSync.test.ts @@ -4,6 +4,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { META_AUTO_CHANGE, META_COMMAND } from '../../../editor/meta'; import { PageServiceError } from '../../../services/pages/types'; +import { t } from '../../../strings/t'; +import { showToast } from '../../../ui/toast'; import { exitHook } from './queue'; import { blockId, caretAtEnd, syncPage, syncRig } from './testSync'; import type { SyncRig } from './testSync'; @@ -229,6 +231,32 @@ describe('a change the core refuses', () => { expect(held.blocks.map((block) => [block.id, block.data.markdown])).toEqual([[A, 'Onex']]); }); + it('keeps the text and its riders when the core is busy, says so, and sends them with the next flush', async () => { + // A command that gave up waiting for the core (`coreBusy`) is not a resync: nothing typed is lost, the person + // hears that the change wasn't kept (the title bar says the core isn't responding), and the next flush sends it. + const rig = await syncRig(syncPage(['One', 'Two'])); + caretAtEnd(rig.editors.get(A)!); + vi.mocked(showToast).mockClear(); + const send = rig.page.send.bind(rig.page); + let refused = false; + vi.spyOn(rig.page, 'send').mockImplementation((batch) => { + if (refused) return send(batch); + refused = true; + return Promise.reject(new PageServiceError('coreBusy', 'The core is busy (page_open has held it for 15 s).')); + }); + rig.type(A, 'x'); + rig.queue.appendToNextBatch(A, [{ edit: 'deleteBlocks', blocks: [B] }]); + await tick(150); + expect(rig.queue.hasUnsent()).toBe(true); + expect(showToast).toHaveBeenCalledWith( + expect.objectContaining({ id: 'page-sync-not-kept', message: t('pageSync.notKept.other'), tone: 'danger' }), + ); + await rig.queue.flushAll('exit'); + const held = await rig.held(); + expect(held.blocks.map((block) => [block.id, block.data.markdown])).toEqual([[A, 'Onex']]); + expect(rig.queue.hasUnsent()).toBe(false); + }); + it('keeps the window open on exit while a refused change is unsent, until Close anyway', async () => { const rig = await syncRig(syncPage(['Hello'])); caretAtEnd(rig.editors.get(A)!); diff --git a/app/src/features/tree/SaveStatus.test.tsx b/app/src/features/tree/SaveStatus.test.tsx new file mode 100644 index 00000000..964efea1 --- /dev/null +++ b/app/src/features/tree/SaveStatus.test.tsx @@ -0,0 +1,37 @@ +// The title bar's save status tells the truth about pages too: a page whose save failed shows "Couldn't save" +// until the core saves it, however the tree's own commands fare. + +import { act, cleanup, screen } from '@testing-library/react'; +import { afterEach, describe, expect, it } from 'vitest'; +import { pageSaveFailed, pageSaved, setCoreStalled } from '../../services/pages/saveHealth'; +import { announcements, renderUi } from '../../test'; +import { SaveStatus } from './SaveStatus'; +import { treeStore } from './store'; + +afterEach(cleanup); + +describe('the save status', () => { + it('shows a failed page save and says so, then Saved again once the page saves', () => { + renderUi(); + expect(screen.getByText('Saved')).toBeTruthy(); + act(() => pageSaveFailed('p1')); + expect(screen.getByText("Couldn't save")).toBeTruthy(); + expect(announcements()).toContain("Couldn't save your notes. OpenNote keeps the changes and tries again."); + act(() => treeStore.set((state) => ({ ...state, saveStatus: 'saving' }))); + expect(screen.getByText('Saving')).toBeTruthy(); + act(() => treeStore.set((state) => ({ ...state, saveStatus: 'saved' }))); + expect(screen.getByText("Couldn't save")).toBeTruthy(); + act(() => pageSaved('p1')); + expect(screen.getByText('Saved')).toBeTruthy(); + }); + + it('says the core is not responding while a command holds it, whatever the tree says', () => { + renderUi(); + act(() => treeStore.set((state) => ({ ...state, saveStatus: 'saving' }))); + act(() => setCoreStalled(true)); + expect(screen.getByText('Not responding')).toBeTruthy(); + expect(announcements().some((said) => said.startsWith("OpenNote's core isn't responding"))).toBe(true); + act(() => setCoreStalled(false)); + expect(screen.getByText('Saving')).toBeTruthy(); + }); +}); diff --git a/app/src/features/tree/SaveStatus.tsx b/app/src/features/tree/SaveStatus.tsx index dbe31fef..e778b828 100644 --- a/app/src/features/tree/SaveStatus.tsx +++ b/app/src/features/tree/SaveStatus.tsx @@ -1,14 +1,16 @@ -// The save status in the title bar (ARCHITECTURE.md section 13): "Saved", "Saving", "Offline", or "Couldn't save". -// Only the trouble is announced, because the rest would chatter. In the compact layout it is an icon whose name -// is its text. +// The save status in the title bar (ARCHITECTURE.md section 13): "Saved", "Saving", "Offline", "Couldn't save", +// or "Not responding" while a command holds the core. Only the trouble is announced, because the rest would +// chatter. In the compact layout it is an icon whose name is its text. import { CloudCheckIcon } from '@phosphor-icons/react/dist/csr/CloudCheck'; import { CloudSlashIcon } from '@phosphor-icons/react/dist/csr/CloudSlash'; import { CloudArrowUpIcon } from '@phosphor-icons/react/dist/csr/CloudArrowUp'; import { CloudWarningIcon } from '@phosphor-icons/react/dist/csr/CloudWarning'; +import { CloudXIcon } from '@phosphor-icons/react/dist/csr/CloudX'; import { useEffect } from 'react'; import type { ComponentType } from 'react'; import type { SaveStatus as Status } from '../../services/notes'; +import { anySaveFailing, combinedStatus, coreStalled, saveHealthStore } from '../../services/pages/saveHealth'; import { useStore } from '../../state/store'; import { t } from '../../strings/t'; import { announce } from '../../ui'; @@ -21,12 +23,17 @@ const ICONS: Record> = { saving: CloudArrowUpIcon, offline: CloudSlashIcon, error: CloudWarningIcon, + stalled: CloudXIcon, }; export function SaveStatus({ presentation }: { presentation: 'full' | 'icon' | 'menuItem' }) { - const status = useStore(treeStore, (state) => state.saveStatus); + const notes = useStore(treeStore, (state) => state.saveStatus); + const pageFailing = useStore(saveHealthStore, anySaveFailing); + const stalled = useStore(saveHealthStore, coreStalled); + const status = combinedStatus(notes, pageFailing, stalled); useEffect(() => { if (status === 'error') announce(t('tree.save.errorAnnounce'), 'assertive'); + if (status === 'stalled') announce(t('tree.save.stalledAnnounce'), 'assertive'); }, [status]); const Icon = ICONS[status]; const text = t(`tree.save.${status}`); diff --git a/app/src/services/notes/types.ts b/app/src/services/notes/types.ts index d619211e..261c2e92 100644 --- a/app/src/services/notes/types.ts +++ b/app/src/services/notes/types.ts @@ -18,7 +18,7 @@ export type NodeKind = 'notebook' | 'sectionGroup' | 'section' | 'page'; export type ChipColor = 'ink' | 'indigo' | 'brick' | 'fern' | 'plum' | 'amber' | 'walnut'; /** Subpage depth, as in OneNote. */ export type PageLevel = 0 | 1 | 2; -export type SaveStatus = 'saved' | 'saving' | 'offline' | 'error'; +export type SaveStatus = 'saved' | 'saving' | 'offline' | 'error' | 'stalled'; /** Every chip color, in the tokens' order, for menus and for checking stored values. */ // checks-disable-next-line brand-consistency: the same pen names as ChipColor, listed for use at run time diff --git a/app/src/services/pages/saveHealth.test.ts b/app/src/services/pages/saveHealth.test.ts new file mode 100644 index 00000000..67c43cc8 --- /dev/null +++ b/app/src/services/pages/saveHealth.test.ts @@ -0,0 +1,94 @@ +// The pages whose last save failed, from the core's save events, and what the title bar makes of them. +import { beforeEach, describe, expect, it } from 'vitest'; +import { resetStores } from '../../state/store'; +import { + anySaveFailing, + combinedStatus, + coreBusy, + coreStalled, + pageClosed, + pageSaveFailed, + pageSaved, + saveHealthStore, + setCoreStalled, + watchSaveHealth, +} from './saveHealth'; + +function fakeClient() { + const handlers = new Map void>(); + return { + onEvent: (event: string, handler: (payload: unknown) => void) => { + handlers.set(event, handler); + return () => void handlers.delete(event); + }, + emit: (event: string, payload: unknown) => handlers.get(event)?.(payload), + listening: () => [...handlers.keys()], + }; +} + +beforeEach(() => resetStores()); + +describe('save health', () => { + it('keeps a page failing from its first failed save until the core saves it', () => { + expect(anySaveFailing(saveHealthStore.get())).toBe(false); + pageSaveFailed('p1'); + pageSaveFailed('p1'); + pageSaveFailed('p2'); + expect([...saveHealthStore.get().failing]).toEqual(['p1', 'p2']); + pageSaved('p1'); + expect([...saveHealthStore.get().failing]).toEqual(['p2']); + pageSaved('p2'); + expect(anySaveFailing(saveHealthStore.get())).toBe(false); + }); + + it('forgets a page that closed, since no save will be reported for it', () => { + pageSaveFailed('p1'); + pageSaveFailed('p2'); + pageClosed('p1'); + expect([...saveHealthStore.get().failing]).toEqual(['p2']); + pageClosed('p3'); + expect([...saveHealthStore.get().failing]).toEqual(['p2']); + }); + + it('says the core is stalled from core:stalled until core:responsive', () => { + const client = fakeClient(); + const stop = watchSaveHealth(client); + expect(coreStalled(saveHealthStore.get())).toBe(false); + client.emit('core:stalled', { what: 'page_apply', seconds: 10 }); + expect(coreStalled(saveHealthStore.get())).toBe(true); + client.emit('core:stalled', { what: 'page_apply', seconds: 20 }); + expect(coreStalled(saveHealthStore.get())).toBe(true); + client.emit('core:responsive', {}); + expect(coreStalled(saveHealthStore.get())).toBe(false); + setCoreStalled(true); + expect(combinedStatus('saving', false, coreStalled(saveHealthStore.get()))).toBe('stalled'); + client.emit('core:responsive', {}); + coreBusy(); + expect(coreStalled(saveHealthStore.get())).toBe(true); + stop(); + }); + + it('hears core:save-failed and core:saved, and nothing without a page', () => { + const client = fakeClient(); + const stop = watchSaveHealth(client); + expect(client.listening()).toEqual(['core:save-failed', 'core:saved', 'core:stalled', 'core:responsive']); + client.emit('core:save-failed', { page: 'p1', kind: 'io', message: 'blocks differs after reading back' }); + client.emit('core:save-failed', { kind: 'io' }); + client.emit('core:save-failed', null); + expect([...saveHealthStore.get().failing]).toEqual(['p1']); + client.emit('core:saved', { page: 'p1', revision: 'r2' }); + expect(anySaveFailing(saveHealthStore.get())).toBe(false); + stop(); + expect(client.listening()).toEqual([]); + }); + + it('turns "Saved" into "Couldn\'t save" while a page fails, and leaves the rest to the notes bridge', () => { + expect(combinedStatus('saved', true)).toBe('error'); + expect(combinedStatus('saved', false)).toBe('saved'); + expect(combinedStatus('saving', true)).toBe('saving'); + expect(combinedStatus('offline', true)).toBe('offline'); + expect(combinedStatus('error', false)).toBe('error'); + expect(combinedStatus('saved', false, true)).toBe('stalled'); + expect(combinedStatus('saving', true, true)).toBe('stalled'); + }); +}); diff --git a/app/src/services/pages/saveHealth.ts b/app/src/services/pages/saveHealth.ts new file mode 100644 index 00000000..d21ced92 --- /dev/null +++ b/app/src/services/pages/saveHealth.ts @@ -0,0 +1,92 @@ +// Whether the core could save each page, and whether it answers at all. The title bar's status came from the +// notes bridge alone, which knows only the tree's own commands: a page whose every save failed still showed +// "Saved" (beta 4's T2-1), and a core that stopped answering showed "Saving" for as long as the app ran (T2-3). +// The core says `core:save-failed` after each failed try and `core:saved` after a save, and this store keeps the +// pages whose last try failed, for the status to show "Couldn't save" until the page saves. The core's watchdog +// says `core:stalled` while one command has held the core for too long, and `core:responsive` once it answers +// again; the status says "Not responding" in between. + +import { createStore } from '../../state/store'; +import type { SaveStatus } from '../notes'; + +export interface SaveHealth { + /** The pages whose last save failed, by ID. */ + readonly failing: ReadonlySet; + /** Whether a command has held the core for too long, so nothing reaches the disk until it returns. */ + readonly stalled: boolean; +} + +export const saveHealthStore = createStore({ failing: new Set(), stalled: false }, 'saveHealth'); + +/** Whether any page's last save failed. */ +export const anySaveFailing = (health: SaveHealth): boolean => health.failing.size > 0; + +/** Whether the core has stopped answering. */ +export const coreStalled = (health: SaveHealth): boolean => health.stalled; + +/** + * The status to show: "Not responding" while the core is stalled, else the notes bridge's, unless a page's last + * save failed while nothing is on its way. + */ +export function combinedStatus(notes: SaveStatus, pageFailing: boolean, stalled = false): SaveStatus { + if (stalled) return 'stalled'; + return notes === 'saved' && pageFailing ? 'error' : notes; +} + +export function setCoreStalled(stalled: boolean): void { + saveHealthStore.set((state) => (state.stalled === stalled ? state : { ...state, stalled })); +} + +/** + * A command gave up waiting for the core (the `coreBusy` error): the core is stalled, even before its watchdog's + * event arrives. `core:responsive` clears it once the holder returns. + */ +export function coreBusy(): void { + setCoreStalled(true); +} + +export function pageSaveFailed(page: string): void { + saveHealthStore.set((state) => { + if (state.failing.has(page)) return state; + return { ...state, failing: new Set([...state.failing, page]) }; + }); +} + +export function pageSaved(page: string): void { + saveHealthStore.set((state) => { + if (!state.failing.has(page)) return state; + const failing = new Set(state.failing); + failing.delete(page); + return { ...state, failing }; + }); +} + +/** + * A page closed, so no `core:saved` will come for it: it stops counting as failing. A page that still can't + * save counts again at its next try, which the core reports as before. + */ +export const pageClosed = pageSaved; + +const pageOf = (payload: unknown): string | null => { + const page = (payload as { page?: unknown } | null)?.page; + return typeof page === 'string' ? page : null; +}; + +/** Keeps the store in step with the core's save events. The returned function stops listening. */ +export function watchSaveHealth(client: { + onEvent(event: string, handler: (payload: unknown) => void): () => void; +}): () => void { + const stops = [ + client.onEvent('core:save-failed', (payload) => { + const page = pageOf(payload); + if (page) pageSaveFailed(page); + }), + client.onEvent('core:saved', (payload) => { + const page = pageOf(payload); + if (page) pageSaved(page); + }), + client.onEvent('core:stalled', () => setCoreStalled(true)), + client.onEvent('core:responsive', () => setCoreStalled(false)), + ]; + return () => stops.forEach((stop) => stop()); +} diff --git a/app/src/services/pages/tauri.test.ts b/app/src/services/pages/tauri.test.ts index 3273a398..c163fb6a 100644 --- a/app/src/services/pages/tauri.test.ts +++ b/app/src/services/pages/tauri.test.ts @@ -3,6 +3,8 @@ import { describe, expect, it, vi } from 'vitest'; import type { CoreClient, DecodedEnvelope } from '../../core/client'; import type { ImagesClient } from '../../platform/types'; +import { resetStores } from '../../state/store'; +import { coreStalled, saveHealthStore } from './saveHealth'; import { createTauriPageService, readOnlyInfo, toVersionInfo } from './tauri'; const AT = '2026-10-01T09:00:00.000Z'; @@ -112,6 +114,17 @@ describe('the Tauri page service’s requests', () => { expect(seqs).toEqual([4, 4, 5]); }); + it('says the core is stalled when an edit gives up waiting for it, before the watchdog has', async () => { + resetStores(); + const { core, calls } = fakeCore([envelope([])]); + calls.pageApply.mockImplementationOnce(() => + Promise.reject({ code: 'coreBusy', message: 'The core is busy (page_open has held it for 15 s).' }), + ); + const page = await createTauriPageService(core, images).open('p1', { viewport: null }); + await expect(page.send({ edits: [] })).rejects.toMatchObject({ code: 'coreBusy', resync: false }); + expect(coreStalled(saveHealthStore.get())).toBe(true); + }); + it('builds asset addresses that WebView2 routes, with each ID kept to one path segment', async () => { const { core } = fakeCore([envelope([])]); const page = await createTauriPageService(core, images).open('p1', { viewport: null }); diff --git a/app/src/services/pages/tauri.ts b/app/src/services/pages/tauri.ts index fe741724..748ce14f 100644 --- a/app/src/services/pages/tauri.ts +++ b/app/src/services/pages/tauri.ts @@ -2,6 +2,7 @@ // one onto a core command through the client in app/src/core/, which keeps one request in flight per page. The // core's `core:*` events become frames, external changes, and read-only notices for the page they name. import type { CoreClient, DecodedEnvelope, FrameInfo } from '../../core/client'; +import { coreBusy, pageClosed, watchSaveHealth } from './saveHealth'; import type { ImagesClient, IpcError } from '../../platform/types'; import { inkListeners } from './ink'; import { PageServiceError } from './types'; @@ -24,6 +25,7 @@ import type { function pageError(error: unknown): PageServiceError { if (error instanceof PageServiceError) return error; const ipc = error as Partial; + if (ipc.code === 'coreBusy') coreBusy(); return new PageServiceError((ipc.code ?? 'invalid') as PageErrorCode, ipc.message ?? String(error)); } @@ -245,6 +247,8 @@ function events(session: Session, frames: ReturnType { const name = `main-${++clients}`; @@ -314,6 +318,7 @@ export function createTauriPageService(client: CoreClient, images: ImagesClient) onReadOnly: heard.onReadOnly, close: () => { heard.stop(); + pageClosed(pageId); return turn(() => client.pageClose(pageId, name)).catch(rejectAs); }, }; diff --git a/app/src/services/pages/types.ts b/app/src/services/pages/types.ts index 08f895f9..b8090ab2 100644 --- a/app/src/services/pages/types.ts +++ b/app/src/services/pages/types.ts @@ -208,7 +208,9 @@ export type PageErrorCode = | 'tooLarge' | 'downloadFailed' | 'staleToken' - | 'notImplemented'; + | 'notImplemented' + /** The command gave up waiting for the core, which another command has held for too long. */ + | 'coreBusy'; /** `resync` says the page is out of date, so the view reopens it and remounts its editors. */ export class PageServiceError extends Error { diff --git a/app/src/strings/en/tree.ts b/app/src/strings/en/tree.ts index afb57683..de9ff956 100644 --- a/app/src/strings/en/tree.ts +++ b/app/src/strings/en/tree.ts @@ -161,6 +161,9 @@ export const tree = { offline: 'Offline', error: "Couldn't save", errorAnnounce: "Couldn't save your notes. OpenNote keeps the changes and tries again.", + stalled: 'Not responding', + stalledAnnounce: + "OpenNote's core isn't responding. Your changes are kept in its journal, and come back when you open OpenNote again.", }, exit: { unsaved: "Some notes aren't saved yet. Try closing again in a moment.", diff --git a/app/vitest.config.ts b/app/vitest.config.ts index 445bb9a8..b6cc7cf0 100644 --- a/app/vitest.config.ts +++ b/app/vitest.config.ts @@ -32,6 +32,9 @@ export default defineConfig((env) => name: 'components', include: ['src/**/*.test.tsx'], setupFiles: ['src/test/setup.ts'], + // The browser project's server port, so parallel runs (agents on one machine) each take their own: + // Vitest's own default when unset, and the CLI's --api doesn't reach an inline project. + api: { port: Number(process.env.OPENNOTE_VITEST_PORT) || 63315 }, browser: { enabled: true, headless: true, diff --git a/crates/core/src/format/json.rs b/crates/core/src/format/json.rs index 92641964..b56e33c9 100644 --- a/crates/core/src/format/json.rs +++ b/crates/core/src/format/json.rs @@ -13,7 +13,7 @@ use crate::error::{FormatError, FormatErrorKind}; use crate::limits::Limits; pub use fields::{expect_object, Fields}; -pub use write::{fixed, write_document, write_string, Json, Obj}; +pub use write::{fixed, fixed_value, geometry, write_document, write_string, Json, Obj}; /// The byte order mark that readers skip (spec 2.2). const BOM: &[u8] = b"\xEF\xBB\xBF"; diff --git a/crates/core/src/format/json/fields.rs b/crates/core/src/format/json/fields.rs index 657b7995..76a57c4b 100644 --- a/crates/core/src/format/json/fields.rs +++ b/crates/core/src/format/json/fields.rs @@ -166,6 +166,16 @@ impl Fields { self.take(key).map(|v| self.as_f64(key, &v)).transpose() } + /// An optional geometry value, rounded to 0.01 as the file writes it. + pub fn opt_geometry(&mut self, key: &str) -> Result, FormatError> { + Ok(self.opt_f64(key)?.map(super::geometry)) + } + + /// A geometry value found under `key`, rounded to 0.01 as the file writes it. + pub fn as_geometry(&self, key: &str, value: &Value) -> Result { + self.as_f64(key, value).map(super::geometry) + } + /// A number found under `key`. pub fn as_f64(&self, key: &str, value: &Value) -> Result { value diff --git a/crates/core/src/format/json/write.rs b/crates/core/src/format/json/write.rs index 9d70776e..3d15e3ef 100644 --- a/crates/core/src/format/json/write.rs +++ b/crates/core/src/format/json/write.rs @@ -262,6 +262,23 @@ fn push_escape(out: &mut String, c: char) { } } +/// A geometry value as the canonical file holds it: rounded to 0.01, the way [`Json::Geometry`] writes it +/// (spec 2.3). Every reader of a geometry value applies it, from a file and from an edit alike, so the page in +/// memory is the page its bytes read back as. Without that, a save's read-back check (spec 17.7, step S5) fails +/// forever for a value with a third decimal, such as the offset of ink drawn on text. +pub fn geometry(value: f64) -> f64 { + fixed_value(value, 2) +} + +/// A number rounded to `decimals` decimals, as [`fixed`] writes it. A value that is not finite stays as it is, +/// for validation to refuse. +pub fn fixed_value(value: f64, decimals: u32) -> f64 { + if !value.is_finite() { + return value; + } + fixed(value, decimals).parse().unwrap_or(value) +} + /// Formats a number rounded to `decimals` decimals, in the shortest form: no exponent, no trailing zeros, /// and `0` instead of `-0`. Values that are not finite are written as `0`, which validation never lets /// through. diff --git a/crates/core/src/format/page_json/blocks.rs b/crates/core/src/format/page_json/blocks.rs index 7addd2f7..660717ca 100644 --- a/crates/core/src/format/page_json/blocks.rs +++ b/crates/core/src/format/page_json/blocks.rs @@ -51,11 +51,11 @@ fn read_block(value: Value, index: usize) -> Result { fn read_frame(value: Value) -> Result { let mut fields = Fields::new(value, "frame")?; Ok(Frame { - x: fields.opt_f64("x")?, - y: fields.opt_f64("y")?, - w: fields.opt_f64("w")?, - h: fields.opt_f64("h")?, - rotate: fields.opt_f64("rotate")?, + x: fields.opt_geometry("x")?, + y: fields.opt_geometry("y")?, + w: fields.opt_geometry("w")?, + h: fields.opt_geometry("h")?, + rotate: fields.opt_geometry("rotate")?, extra: fields.rest(), }) } diff --git a/crates/core/src/format/page_json/data.rs b/crates/core/src/format/page_json/data.rs index d59e5377..4a3cbb71 100644 --- a/crates/core/src/format/page_json/data.rs +++ b/crates/core/src/format/page_json/data.rs @@ -8,7 +8,7 @@ use std::collections::BTreeMap; use serde_json::Value; use super::view::named; -use crate::format::json::{Json, Obj}; +use crate::format::json::{fixed_value, geometry, Json, Obj}; use crate::id::{ElementId, Id}; use crate::model::{ AnchorQuote, BlockData, Crop, FileData, ImageData, InkAnchor, InkBlockData, JsonMap, Named, NamedValue, OtherData, @@ -98,6 +98,11 @@ impl<'a> View<'a> { } } + /// A geometry value, rounded to 0.01 as the file writes it. + fn geometry(&mut self, key: &'static str) -> Result, String> { + Ok(self.number(key)?.map(geometry)) + } + fn named(&mut self, key: &'static str) -> Result>, String> { match self.get(key) { None => Ok(None), @@ -223,8 +228,8 @@ fn read_anchor(map: &JsonMap) -> Result { para, at, quote, - dx: view.number("dx")?.unwrap_or(0.0), - dy: view.number("dy")?.unwrap_or(0.0), + dx: view.geometry("dx")?.unwrap_or(0.0), + dy: view.geometry("dy")?.unwrap_or(0.0), extra: view.rest(), }) } @@ -253,7 +258,12 @@ fn read_image(map: &JsonMap) -> Result { fn read_crop(map: &JsonMap) -> Result { let mut view = View::new(map); - let mut part = |key| view.number(key)?.ok_or(format!("crop.{key}: missing")); + // Crops are written with 6 decimals. + let mut part = |key| { + view.number(key) + .map(|v| v.map(|v| fixed_value(v, 6)))? + .ok_or(format!("crop.{key}: missing")) + }; Ok(Crop { x: part("x")?, y: part("y")?, @@ -295,7 +305,7 @@ fn read_column(value: &Value) -> Result { let mut view = View::new(value.as_object().ok_or("columns: expected objects")?); Ok(TableColumn { id: view.id("id")?, - width: view.number("width")?, + width: view.geometry("width")?, extra: view.rest(), }) } diff --git a/crates/core/src/format/page_json/tests.rs b/crates/core/src/format/page_json/tests.rs index 9e064cfe..54d32dc4 100644 --- a/crates/core/src/format/page_json/tests.rs +++ b/crates/core/src/format/page_json/tests.rs @@ -211,6 +211,53 @@ fn defaults_are_left_out_and_values_that_differ_are_written() { assert!(!text.contains("\"height\"")); } +#[test] +fn geometry_is_read_as_the_file_writes_it_so_a_page_reads_back_as_itself() { + let mut value = minimal(); + value["view"] = json!({"contentWidth": 600.004, "paper": {"width": 793.7001, "margins": [48.005, 72, 48, 72]}}); + value["blocks"] = json!([ + { + "id": "01m3sa14y9zszek1wdk3snddsn", "order": "a0", "type": "text", + "frame": {"x": 96.001, "y": 120.005, "w": 624.4999}, + "created": "2026-09-30T14:03:25.001Z", "modified": "2026-09-30T14:05:40.020Z", + "data": {"markdown": "First"} + }, + { + "id": "01m3sa1242ayy4avvsz3yx5gxj", "order": "a1", "type": "ink", + "frame": {"x": 0, "y": 0}, + "created": "2026-09-30T14:03:25.001Z", "modified": "2026-09-30T14:05:40.020Z", + "data": { + "role": "anchored", "strokeCount": 0, + "anchor": {"block": "01m3sa14y9zszek1wdk3snddsn", "at": 0, "quote": {"exact": ""}, "dx": 12.345678, "dy": -0.001} + } + } + ]); + let page = read(&value).unwrap().page; + let text = page + .blocks + .iter() + .find(|b| matches!(b.data, BlockData::Text(_))) + .unwrap(); + let frame = text.frame.as_ref().unwrap(); + assert_eq!((frame.x, frame.y, frame.w), (Some(96.0), Some(120.01), Some(624.5))); + let ink = page + .blocks + .iter() + .find(|b| matches!(b.data, BlockData::Ink(_))) + .unwrap(); + let BlockData::Ink(data) = &ink.data else { panic!() }; + let anchor = data.anchor.as_ref().unwrap(); + assert_eq!((anchor.dx, anchor.dy), (12.35, 0.0)); + assert_eq!(page.view.content_width, Some(600.0)); + assert_eq!(page.view.paper.width, 793.7); + assert_eq!(page.view.paper.margins, [48.01, 72.0, 48.0, 72.0]); + // What was read writes, and reads back as the same page: the check every save makes (spec 17.7, S5). + let bytes = write_page(&page); + let mut again = read_page(&bytes, &limits()).unwrap().page; + again.format = page.format.clone(); + assert_eq!(again, page); +} + #[test] fn reading_order_keeps_only_blocks_once() { let mut page = sample_page(); diff --git a/crates/core/src/format/page_json/view.rs b/crates/core/src/format/page_json/view.rs index 9f3fc25c..075a2e0e 100644 --- a/crates/core/src/format/page_json/view.rs +++ b/crates/core/src/format/page_json/view.rs @@ -21,7 +21,7 @@ pub fn read_view(value: Value) -> Result { Ok(PageView { layout: fields.named("layout")?.unwrap_or(defaults.layout), mode: fields.named("mode")?.unwrap_or(defaults.mode), - content_width: fields.opt_f64("contentWidth")?, + content_width: fields.opt_geometry("contentWidth")?, reading_order: fields.ids("readingOrder")?, paper, background, @@ -37,7 +37,7 @@ fn read_paper(value: Value) -> Result { Some(Value::Array(items)) if items.len() == 4 => { let mut margins = [0.0; 4]; for (slot, item) in margins.iter_mut().zip(&items) { - *slot = fields.as_f64("margins", item)?; + *slot = fields.as_geometry("margins", item)?; } margins } @@ -46,8 +46,8 @@ fn read_paper(value: Value) -> Result { Ok(Paper { size: fields.named("size")?.unwrap_or(defaults.size), orientation: fields.named("orientation")?.unwrap_or(defaults.orientation), - width: fields.opt_f64("width")?.unwrap_or(defaults.width), - height: fields.opt_f64("height")?.unwrap_or(defaults.height), + width: fields.opt_geometry("width")?.unwrap_or(defaults.width), + height: fields.opt_geometry("height")?.unwrap_or(defaults.height), margins, extra: fields.rest(), }) @@ -58,7 +58,7 @@ fn read_background(value: Value) -> Result { let defaults = Background::default(); Ok(Background { pattern: fields.named("pattern")?.unwrap_or(defaults.pattern), - spacing: fields.opt_f64("spacing")?.unwrap_or(defaults.spacing), + spacing: fields.opt_geometry("spacing")?.unwrap_or(defaults.spacing), color: fields.color("color")?.unwrap_or(defaults.color), margin_line: fields.bool_or("marginLine", defaults.margin_line)?, template: fields.opt_id("template")?, diff --git a/crates/core/src/format/tree_json/styles.rs b/crates/core/src/format/tree_json/styles.rs index d3697fa0..09f67785 100644 --- a/crates/core/src/format/tree_json/styles.rs +++ b/crates/core/src/format/tree_json/styles.rs @@ -3,7 +3,7 @@ use serde_json::Value; use crate::error::FormatError; -use crate::format::json::{expect_object, Fields, Json, Obj}; +use crate::format::json::{expect_object, fixed_value, Fields, Json, Obj}; use crate::model::{NotebookStyles, StyleSpec, STYLE_NAMES}; /// Reads `styles`. Style names and keys a version 1 reader doesn't know are kept (spec 2.9). @@ -17,15 +17,19 @@ pub(super) fn read_styles(value: Value) -> Result { .collect() } +/// Reads one style. Each number is rounded as [`write_style`] writes it (sizes and spacing to 0.01, the line +/// height to two decimals). A style in memory is then the style its bytes read back as, whether it came from +/// the file or from an edit. [`crate::store::notebook_store::NotebookStore::set_notebook_styles`] compares the +/// two to tell a change from a repeat. fn read_style(value: Value, context: &str) -> Result { let mut fields = Fields::new(value, context)?; Ok(StyleSpec { font: fields.opt_str("font")?, - size: fields.opt_f64("size")?, + size: fields.opt_geometry("size")?, color: fields.color("color")?, - space_before: fields.opt_f64("spaceBefore")?, - space_after: fields.opt_f64("spaceAfter")?, - line_height: fields.opt_f64("lineHeight")?, + space_before: fields.opt_geometry("spaceBefore")?, + space_after: fields.opt_geometry("spaceAfter")?, + line_height: fields.opt_f64("lineHeight")?.map(|v| fixed_value(v, 2)), extra: fields.rest(), }) } diff --git a/crates/core/src/format/tree_json/tests.rs b/crates/core/src/format/tree_json/tests.rs index 9d66a233..38e18b19 100644 --- a/crates/core/src/format/tree_json/tests.rs +++ b/crates/core/src/format/tree_json/tests.rs @@ -119,3 +119,21 @@ fn merged_copies_take_the_styles_of_the_copy_changed_later() { ours.changed = Timestamp::parse("2026-09-30T16:00:00.000Z").unwrap(); assert_eq!(merge_notebooks(&theirs, &ours).styles, ours.styles); } + +/// Styles are written rounded (sizes and spacing to 0.01, the line height to two decimals) and read back the +/// same way. A style in memory is then the style its bytes read back as. Setting the same style twice is a +/// repeat, not a change, however many decimals the interface sent (the T2-1 pattern, in `notebook.json`). +#[test] +fn styles_read_as_rounded_as_they_are_written() { + let file = read(¬ebook_with(json!({ + "body": { "size": 12.345, "spaceBefore": 1.005, "spaceAfter": 0.001, "lineHeight": 1.2345 } + }))) + .unwrap(); + let body = &file.styles["body"]; + assert_eq!(body.size, Some(12.35)); + assert_eq!(body.space_before, Some(1.0)); + assert_eq!(body.space_after, Some(0.0)); + assert_eq!(body.line_height, Some(1.23)); + let again = read_notebook(&write_notebook(&file), &limits()).unwrap(); + assert_eq!(again.styles, file.styles); +} diff --git a/crates/core/src/ops/merge_patch/view/data.rs b/crates/core/src/ops/merge_patch/view/data.rs index 6b57515b..dbc649e4 100644 --- a/crates/core/src/ops/merge_patch/view/data.rs +++ b/crates/core/src/ops/merge_patch/view/data.rs @@ -150,8 +150,8 @@ fn anchor_from_json(map: &JsonMap) -> Result { para: a.id("para")?, at: a.count("at")?, quote, - dx: a.number("dx")?.unwrap_or(0.0), - dy: a.number("dy")?.unwrap_or(0.0), + dx: a.geometry("dx")?.unwrap_or(0.0), + dy: a.geometry("dy")?.unwrap_or(0.0), extra: a.extra(), }) } @@ -179,11 +179,13 @@ pub fn image_from_json(map: &JsonMap) -> Result { None => None, Some(crop) => { let c = Fields::new(crop, "crop", &["x", "y", "w", "h"]); + // Crops are written with 6 decimals. + let part = |key| c.required_number(key).map(|v| crate::format::json::fixed_value(v, 6)); Some(Crop { - x: c.required_number("x")?, - y: c.required_number("y")?, - w: c.required_number("w")?, - h: c.required_number("h")?, + x: part("x")?, + y: part("y")?, + w: part("w")?, + h: part("h")?, extra: c.extra(), }) } @@ -258,7 +260,7 @@ pub fn table_from_json(map: &JsonMap) -> Result { let c = Fields::new(item_object(value, "columns")?, "column", &["id", "width"]); Ok(TableColumn { id: c.required_id("id")?, - width: c.number("width")?, + width: c.geometry("width")?, extra: c.extra(), }) }) diff --git a/crates/core/src/ops/merge_patch/view/fields.rs b/crates/core/src/ops/merge_patch/view/fields.rs index a0ae9485..8b652170 100644 --- a/crates/core/src/ops/merge_patch/view/fields.rs +++ b/crates/core/src/ops/merge_patch/view/fields.rs @@ -63,6 +63,11 @@ impl<'a> Fields<'a> { } } + /// A finite geometry member, if present, rounded to 0.01 as the file writes it. + pub fn geometry(&self, key: &str) -> Result, String> { + Ok(self.number(key)?.map(crate::format::json::geometry)) + } + /// A finite number member that must be present. pub fn required_number(&self, key: &str) -> Result { self.number(key)?.ok_or_else(|| self.wrong(key, "present")) diff --git a/crates/core/src/ops/resolve/blocks.rs b/crates/core/src/ops/resolve/blocks.rs index 09d5b852..b3dae1cd 100644 --- a/crates/core/src/ops/resolve/blocks.rs +++ b/crates/core/src/ops/resolve/blocks.rs @@ -117,8 +117,18 @@ fn check_element_ids(page: &Page, block: &Block) -> Result<(), EditError> { Ok(()) } -/// A frame with no values and no unknown keys is no frame. -fn normal_frame(frame: Frame) -> Option { +/// A frame with no values and no unknown keys is no frame. Its values are rounded to 0.01, as the file holds +/// them, so the page in memory reads back from its bytes unchanged. +fn normal_frame(mut frame: Frame) -> Option { + for value in [ + &mut frame.x, + &mut frame.y, + &mut frame.w, + &mut frame.h, + &mut frame.rotate, + ] { + *value = value.map(crate::format::json::geometry); + } (frame != Frame::default()).then_some(frame) } diff --git a/crates/core/src/ops/resolve/view.rs b/crates/core/src/ops/resolve/view.rs index 509f3898..0746ba8c 100644 --- a/crates/core/src/ops/resolve/view.rs +++ b/crates/core/src/ops/resolve/view.rs @@ -75,7 +75,7 @@ pub fn view_from_json(value: &Value, limits: &Limits) -> Result bool { fn paper_from_json(map: &JsonMap, limits: &Limits) -> Result { let f = Fields::new(map, "paper", PAPER_KEYS); let defaults = Paper::default(); - let width = f.number("width")?.unwrap_or(defaults.width); - let height = f.number("height")?.unwrap_or(defaults.height); + let width = f.geometry("width")?.unwrap_or(defaults.width); + let height = f.geometry("height")?.unwrap_or(defaults.height); if !size_ok(width, limits) || !size_ok(height, limits) { return Err("the paper size must be positive and within the limit".to_owned()); } let margins = match f.array("margins")? { [] if !map.contains_key("margins") => defaults.margins, [top, right, bottom, left] => { - let read = |v: &Value| v.as_f64().filter(|m| m.is_finite() && m.abs() <= limits.geometry_abs); + let read = |v: &Value| { + v.as_f64() + .filter(|m| m.is_finite() && m.abs() <= limits.geometry_abs) + .map(crate::format::json::geometry) + }; let all = [top, right, bottom, left].map(read); match all { [Some(t), Some(r), Some(b), Some(l)] => [t, r, b, l], @@ -132,7 +136,7 @@ fn paper_from_json(map: &JsonMap, limits: &Limits) -> Result { fn background_from_json(map: &JsonMap, limits: &Limits) -> Result { let f = Fields::new(map, "background", BACKGROUND_KEYS); let defaults = Background::default(); - let spacing = f.number("spacing")?.unwrap_or(defaults.spacing); + let spacing = f.geometry("spacing")?.unwrap_or(defaults.spacing); if !size_ok(spacing, limits) { return Err("background.spacing must be positive and within the limit".to_owned()); } diff --git a/crates/core/src/session/journal_thread.rs b/crates/core/src/session/journal_thread.rs index 98f674b6..bc6fd926 100644 --- a/crates/core/src/session/journal_thread.rs +++ b/crates/core/src/session/journal_thread.rs @@ -59,6 +59,8 @@ pub struct JournalThread { commands: Sender, codec: Arc, thread: Option>, + /// How long an open waits for the thread's answer: [`OPEN_TIMEOUT`], shorter in tests. + open_wait: Duration, } /// A page's journal, held by its page session. @@ -197,6 +199,13 @@ pub enum TreeOp { }, } +/// How long opening a journal waits for the journal thread. The thread answers in milliseconds; a longer wait +/// means it is stuck, and the page then runs without a journal (spec 20.12) instead of holding its lock, and +/// every command behind it, forever. It is shorter than the app's `COMMAND_WAIT` (15 s in core_bridge.rs): an +/// open stuck here holds the core, and it must give up before the commands queued behind it do, so they run +/// instead of failing as busy. +const OPEN_TIMEOUT: Duration = Duration::from_secs(10); + impl JournalThread { /// Starts the thread. pub fn start(config: JournalConfig) -> Result { @@ -212,6 +221,7 @@ impl JournalThread { commands, codec, thread: Some(thread), + open_wait: OPEN_TIMEOUT, }) } @@ -233,7 +243,9 @@ impl JournalThread { reply, }; self.commands.send(command).map_err(|_| JournalError::Closed)?; - let (id, shared) = answer.recv().map_err(|_| JournalError::Closed)??; + let (id, shared) = answer + .recv_timeout(self.open_wait) + .map_err(|_| JournalError::Timeout)??; Ok(JournalHandle { id, commands: self.commands.clone(), @@ -252,7 +264,9 @@ impl JournalThread { reply, }; self.commands.send(command).map_err(|_| JournalError::Closed)?; - let shared = answer.recv().map_err(|_| JournalError::Closed)??; + let shared = answer + .recv_timeout(self.open_wait) + .map_err(|_| JournalError::Timeout)??; Ok(TreeJournal { key: key.clone(), commands: self.commands.clone(), diff --git a/crates/core/src/session/journal_thread/tests.rs b/crates/core/src/session/journal_thread/tests.rs index 5262d2ba..b2282a63 100644 --- a/crates/core/src/session/journal_thread/tests.rs +++ b/crates/core/src/session/journal_thread/tests.rs @@ -335,3 +335,73 @@ fn shutdown_flushes_and_stops() { assert!(handle.wait_durable(1, WAIT).is_ok()); assert!(matches!(handle.wait_durable(2, WAIT), Err(JournalError::Closed))); } + +/// A page journal opened for a caller that stopped waiting (the open timed out, so the page runs journal-less) +/// has no handle, so no `Close` ever comes for it. Left open, every later open of the page found it and failed +/// as busy for the rest of the run (beta 4's T2-3 follow-up). The worker detaches it again when its answer has +/// nobody to go to. +#[test] +fn a_page_journal_nobody_waits_for_is_closed_again_so_the_next_open_of_the_page_works() { + let fs = FailingFs::new(MemFs::new()); + fs.inner.mkdir_all(Path::new("/data")); + let config = JournalConfig { + fs: Arc::new(fs), + codec: Arc::new(RegistryCodec::new()), + root: "/data/journal".into(), + clock: Arc::new(test_clock()), + timings: Timings::default(), + events: Arc::new(CollectingSink::default()), + }; + let mut worker = super::worker::Worker::new(config); + let page = sample_page().id; + let open = |reply| super::worker::Command::OpenPage { + key: key(), + page, + meta: meta("boot-1"), + base: base(0), + reply, + }; + // The caller gave up: its receiver is gone before the worker answers. + let (gone, _) = mpsc::channel(); + worker.handle(open(gone)); + let (reply, answer) = mpsc::channel(); + worker.handle(open(reply)); + let opened = answer.recv_timeout(WAIT).expect("the worker answers"); + assert!(opened.is_ok(), "the page opens again: {:?}", opened.as_ref().err()); + // A tree journal nobody waits for goes the same way. + let (gone, _) = mpsc::channel(); + worker.handle(super::worker::Command::OpenTree { + key: key(), + meta: meta("boot-1"), + reply: gone, + }); + let (reply, answer) = mpsc::channel(); + worker.handle(super::worker::Command::OpenTree { + key: key(), + meta: meta("boot-1"), + reply, + }); + assert!(answer.recv_timeout(WAIT).expect("the worker answers").is_ok()); +} + +/// Opening a journal used to wait for the journal thread's answer with no limit, under the page's lock, and +/// a command that opened a page then held the core (beta 4's T2-3: every later command waited behind it, for +/// as long as the app ran). A thread that doesn't answer in time now fails the open with `Timeout`, and the +/// page runs without a journal (spec 20.12). +#[test] +fn opening_a_journal_gives_up_when_the_journal_thread_does_not_answer() { + // A thread that never serves its commands: the receiver is kept, so sends succeed, and nobody reads it. + let (commands, _unserved) = mpsc::channel(); + let thread = JournalThread { + commands, + codec: Arc::new(RegistryCodec::new()), + thread: None, + open_wait: Duration::from_millis(50), + }; + let started = std::time::Instant::now(); + let page = thread.open_page(&key(), sample_page().id, meta("boot-1"), base(0)); + assert!(matches!(page, Err(JournalError::Timeout)), "{:?}", page.err()); + let tree = thread.open_tree(&key(), meta("boot-1")); + assert!(matches!(tree, Err(JournalError::Timeout)), "{:?}", tree.err()); + assert!(started.elapsed() < WAIT, "the opens waited {:?}", started.elapsed()); +} diff --git a/crates/core/src/session/journal_thread/worker.rs b/crates/core/src/session/journal_thread/worker.rs index f3632f75..c624d619 100644 --- a/crates/core/src/session/journal_thread/worker.rs +++ b/crates/core/src/session/journal_thread/worker.rs @@ -143,13 +143,20 @@ impl Worker { self.trees.clear(); } - fn handle(&mut self, command: Command) { + pub(super) fn handle(&mut self, command: Command) { let Some(command) = self.handle_page(command) else { return; }; match command { Command::OpenTree { key, meta, reply } => { - let _ = reply.send(self.open_tree(key, meta)); + let known = self.trees.contains_key(&key); + if reply.send(self.open_tree(key.clone(), meta)).is_err() && !known { + // Nobody waits for the answer any more (the open timed out): a tree this opened for it has + // no handle, so it is closed again rather than kept open with no one to use it. + if let Some(mut tree) = self.trees.remove(&key) { + tree.sync(&self.config); + } + } } Command::TreeAppend { key, @@ -176,7 +183,17 @@ impl Worker { base, reply, } => { - let _ = reply.send(self.open_page(key, page, meta, base)); + if let Err(unsent) = reply.send(self.open_page(key, page, meta, base)) { + // Nobody waits for the answer any more: the open timed out on the caller's side, and the page + // runs without a journal. The journal this opened for it has no handle, so no `Close` would + // ever come for it, and every later open of the page would find it still open and fail as + // busy. It is detached again now, so the next open of the page gets a journal. + if let Ok((id, _)) = unsent.0 { + if let Some(mut journal) = self.pages.remove(&id) { + journal.detach(&self.config); + } + } + } } Command::Append { id, seq, bytes, edit } => { if let Some(page) = self.pages.get_mut(&id) { diff --git a/crates/core/src/store/page_store.rs b/crates/core/src/store/page_store.rs index 49858e9d..37776804 100644 --- a/crates/core/src/store/page_store.rs +++ b/crates/core/src/store/page_store.rs @@ -164,6 +164,12 @@ impl PageStore { } /// The records of a segment this store wrote, when `entry` lists it exactly as written. + /// Forgets a segment whose save failed, so a later minor compaction doesn't merge from it. + fn forget(&self, entry: &SegmentRef) { + let mut written = self.written.lock().unwrap_or_else(PoisonError::into_inner); + written.retain(|segment| segment.entry != *entry); + } + fn remembered(&self, entry: &SegmentRef) -> Option { let written = self.written.lock().unwrap_or_else(PoisonError::into_inner); written diff --git a/crates/core/src/store/page_store/save.rs b/crates/core/src/store/page_store/save.rs index 3cd36f6b..c4093c46 100644 --- a/crates/core/src/store/page_store/save.rs +++ b/crates/core/src/store/page_store/save.rs @@ -65,8 +65,15 @@ impl PageStore { }), }; written?; - self.check_assets(dir, req.page)?; - let (page, bytes) = prepared?; + // A page that can't be written, or an asset that is missing, fails the same way on every retry: the + // segment just written goes, or every retry would leave one more orphan in `ink/`. + let (page, bytes) = match self.check_assets(dir, req.page).and(prepared) { + Ok(prepared) => prepared, + Err(err) => { + self.discard_planned(dir, &ink); + return Err(err); + } + }; before_commit(page.revision.id, req.through_seq)?; fail_point!("save.save_begin.flushed"); self.check_disk(dir, req.page, req.base_stamp)?; diff --git a/crates/core/src/store/page_store/save/ink.rs b/crates/core/src/store/page_store/save/ink.rs index 5f4377d2..a19d18c2 100644 --- a/crates/core/src/store/page_store/save/ink.rs +++ b/crates/core/src/store/page_store/save/ink.rs @@ -82,6 +82,16 @@ impl PageStore { Ok(()) } + /// Removes the segment a save wrote before it failed, so the retries of a save that can't succeed don't + /// leave an orphan file behind each time. `page.json` never named the segment, so nothing refers to it. + pub(super) fn discard_planned(&self, dir: &Path, plan: &InkPlan) { + if let Some(file) = &plan.file { + let path = NotebookLayout::segment_path(dir, file.entry.id); + let _ = self.config.fs.remove_file(&path); + self.forget(&file.entry); + } + } + /// Every listed segment, decoded without damage, or `None` when any can't be read. /// /// The later segments are decoded first. The base then skips the strokes they don't touch, which stay in it diff --git a/crates/core/src/store/page_store/tests.rs b/crates/core/src/store/page_store/tests.rs index d66b6d90..adcdde47 100644 --- a/crates/core/src/store/page_store/tests.rs +++ b/crates/core/src/store/page_store/tests.rs @@ -8,10 +8,12 @@ use super::*; use crate::format::ReadableState; -use crate::id::{Id, PageId, StrokeId}; -use crate::model::{Access, Asset, InkRecord, ReadOnlyReason}; +use crate::id::{BlockId, Id, PageId, StrokeId}; +use crate::model::{Access, Asset, Block, BlockData, InkRecord, ReadOnlyReason}; use crate::store::compact::plan_compaction; -use crate::testing::sample::{sample_asset_id, sample_device, sample_page, sample_stroke, test_clock}; +use crate::testing::sample::{ + sample_asset_id, sample_device, sample_ink_block, sample_page, sample_stroke, test_clock, +}; use crate::testing::{MemFs, NoLinks}; use crate::time::TestClock; @@ -281,6 +283,15 @@ fn a_page_that_reads_back_differently_is_not_saved() { "{err:?}" ); assert_eq!(h.store.fingerprint(&h.dir).unwrap(), None); + // The segment written for the pending stroke went with the failed save: a retry writes its own, and a save + // that fails every time (beta 4's T2-1 did, every 30 seconds) must not fill `ink/` with orphans. + let orphans: Vec<_> = + h.fs.files() + .into_iter() + .filter(|p| p.extension().is_some_and(|e| e == "onk")) + .collect(); + assert!(orphans.is_empty(), "{orphans:?}"); + assert!(h.store.written.lock().unwrap().is_empty()); } #[test] @@ -383,3 +394,41 @@ fn unconfirmed_durability_is_reported() { Durability::Unconfirmed ); } + +#[test] +fn ink_anchored_at_a_fractional_offset_saves() { + use crate::model::{AnchorQuote, InkAnchor, InkBlockData, InkRole, JsonMap, Named}; + let h = Harness::new(); + let mut page = sample_page(); + h.put_assets(&page); + let text = page + .blocks + .iter() + .find(|b| matches!(b.data, BlockData::Text(_))) + .unwrap() + .id; + let mut anchored = Block::clone(page.blocks.get(sample_ink_block()).unwrap()); + anchored.id = BlockId(Id::parse("01m3sa1242ayy4avvsz3yx5gxk").unwrap()); + anchored.data = BlockData::Ink(InkBlockData { + role: Named::Known(InkRole::Anchored), + stroke_count: 0, + anchor: Some(InkAnchor { + block: text, + para: None, + at: Some(0), + quote: Some(AnchorQuote { + prefix: String::new(), + exact: String::new(), + suffix: String::new(), + extra: JsonMap::new(), + }), + dx: 12.345_678, + dy: -0.001, + extra: JsonMap::new(), + }), + ..InkBlockData::default() + }); + page.blocks.insert(Arc::new(anchored)).unwrap(); + let (saved, _) = h.save(&page, None, CompactionPlan::None); + assert_eq!(h.store.load(&h.dir).unwrap().page, saved); +} diff --git a/crates/core/tests/session_anchored.rs b/crates/core/tests/session_anchored.rs index 9d03a09b..f21cdd66 100644 --- a/crates/core/tests/session_anchored.rs +++ b/crates/core/tests/session_anchored.rs @@ -29,6 +29,60 @@ fn new_block(id: BlockId, type_name: &str, data: serde_json::Value) -> Edit { } } +/// The interface measures where ink sits from its words on the screen, so the offset has as many decimals as +/// a pixel at the zoom has. The file holds geometry to 0.01, and the save's read-back check compares the two: +/// beta 4 then failed every save of such a page (T2-1). +#[test] +fn ink_anchored_at_a_measured_offset_saves_with_the_offset_as_the_file_holds_it() { + let mut real = Real::new(); + let handle = real.open(); + let text = BlockId(Id::from_parts(1_800_000_000_000, 7)); + let ink = BlockId(Id::from_parts(1_800_000_000_000, 8)); + let anchor = json!({"block": text.to_string(), "at": 0, "quote": {"exact": ""}, "dx": 12.345_678, "dy": -0.001}); + let inked = NewBlock { + id: ink, + type_name: "ink".into(), + frame: Some(Frame { + x: Some(0.004), + y: Some(10.0), + ..Frame::default() + }), + data: json!({"role": "anchored", "anchor": anchor}) + .as_object() + .cloned() + .unwrap(), + fallback: None, + }; + let setup = vec![ + new_block(text, "text", json!({"markdown": ""})), + Edit::InsertBlock { + block: inked, + after: None, + before: None, + }, + ]; + handle.apply(real.request(setup)).unwrap(); + handle.save_now().expect("the page saves"); + let page = page_json(&handle); + let block = page["blocks"] + .as_array() + .unwrap() + .iter() + .find(|b| b["id"] == ink.to_string()) + .unwrap(); + assert_eq!(block["data"]["anchor"]["dx"], 12.35); + assert_eq!(block["data"]["anchor"].get("dy"), None, "a zero offset is left out"); + assert_eq!(block["frame"]["x"], 0.0); + // The saved page reads back as the session holds it, so the next save has nothing to differ on. + handle + .apply(real.request(vec![Edit::SetText { + block: text, + markdown: "Now".into(), + }])) + .unwrap(); + handle.save_now().expect("the second save"); +} + #[test] fn typing_that_moves_anchored_ink_is_one_undo_step() { let mut real = Real::new();