From 826955116a36f3b9592fd6dc5e075790ff969d7a Mon Sep 17 00:00:00 2001 From: Eric Dean Date: Wed, 7 Oct 2026 20:04:17 -0700 Subject: [PATCH 1/5] Read geometry as the file writes it, so ink on text saves A page's geometry is written rounded to 0.01 (frames, an anchored ink block's dx/dy, the view, paper and background, column widths), but the readers kept the value an edit gave them. The interface measures where ink sits on its words in screen pixels, so its offset has more decimals than the file keeps, and every save's read-back check (spec 17.7, S5) then failed with "blocks differs after reading back": the page never reached disk, a retry ran every 30 s, each one left another orphan .onk segment in ink/, and the title bar said Saved (beta 4, T2-1). Every reader of a geometry value now rounds it the way the writer does (file reads, edits, and resolved frames alike), so the page in memory is the page its bytes read back as. A save that fails after its ink segment was written removes the segment again. The title bar follows the core's save events: "Couldn't save" while a page's last save failed, "Saved" only once the core saves it. The components vitest project takes its port from OPENNOTE_VITEST_PORT, so parallel runs on one machine don't collide. Co-Authored-By: Claude Opus 5.5 --- app/src/features/tree/SaveStatus.test.tsx | 27 ++++++++ app/src/features/tree/SaveStatus.tsx | 5 +- app/src/services/pages/saveHealth.test.ts | 61 +++++++++++++++++++ app/src/services/pages/saveHealth.ts | 60 ++++++++++++++++++ app/src/services/pages/tauri.ts | 3 + app/vitest.config.ts | 3 + crates/core/src/format/json.rs | 2 +- crates/core/src/format/json/fields.rs | 10 +++ crates/core/src/format/json/write.rs | 17 ++++++ crates/core/src/format/page_json/blocks.rs | 10 +-- crates/core/src/format/page_json/data.rs | 16 +++-- crates/core/src/format/page_json/tests.rs | 39 ++++++++++++ crates/core/src/format/page_json/view.rs | 10 +-- crates/core/src/ops/merge_patch/view/data.rs | 16 ++--- .../core/src/ops/merge_patch/view/fields.rs | 5 ++ crates/core/src/ops/resolve/blocks.rs | 8 ++- crates/core/src/ops/resolve/view.rs | 14 +++-- crates/core/src/store/page_store.rs | 6 ++ crates/core/src/store/page_store/save.rs | 11 +++- crates/core/src/store/page_store/save/ink.rs | 10 +++ crates/core/src/store/page_store/tests.rs | 51 +++++++++++++++- crates/core/tests/session_anchored.rs | 46 ++++++++++++++ 22 files changed, 394 insertions(+), 36 deletions(-) create mode 100644 app/src/features/tree/SaveStatus.test.tsx create mode 100644 app/src/services/pages/saveHealth.test.ts create mode 100644 app/src/services/pages/saveHealth.ts diff --git a/app/src/features/tree/SaveStatus.test.tsx b/app/src/features/tree/SaveStatus.test.tsx new file mode 100644 index 00000000..90977ea6 --- /dev/null +++ b/app/src/features/tree/SaveStatus.test.tsx @@ -0,0 +1,27 @@ +// 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 } 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(); + }); +}); diff --git a/app/src/features/tree/SaveStatus.tsx b/app/src/features/tree/SaveStatus.tsx index dbe31fef..1d38cf7c 100644 --- a/app/src/features/tree/SaveStatus.tsx +++ b/app/src/features/tree/SaveStatus.tsx @@ -9,6 +9,7 @@ import { CloudWarningIcon } from '@phosphor-icons/react/dist/csr/CloudWarning'; import { useEffect } from 'react'; import type { ComponentType } from 'react'; import type { SaveStatus as Status } from '../../services/notes'; +import { anySaveFailing, combinedStatus, saveHealthStore } from '../../services/pages/saveHealth'; import { useStore } from '../../state/store'; import { t } from '../../strings/t'; import { announce } from '../../ui'; @@ -24,7 +25,9 @@ const ICONS: Record> = { }; 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 status = combinedStatus(notes, pageFailing); useEffect(() => { if (status === 'error') announce(t('tree.save.errorAnnounce'), 'assertive'); }, [status]); diff --git a/app/src/services/pages/saveHealth.test.ts b/app/src/services/pages/saveHealth.test.ts new file mode 100644 index 00000000..c9b47e68 --- /dev/null +++ b/app/src/services/pages/saveHealth.test.ts @@ -0,0 +1,61 @@ +// 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, + pageSaveFailed, + pageSaved, + saveHealthStore, + 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('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']); + 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'); + }); +}); diff --git a/app/src/services/pages/saveHealth.ts b/app/src/services/pages/saveHealth.ts new file mode 100644 index 00000000..725bd530 --- /dev/null +++ b/app/src/services/pages/saveHealth.ts @@ -0,0 +1,60 @@ +// Whether the core could save each page. 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). 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. + +import { createStore } from '../../state/store'; +import type { SaveStatus } from '../notes'; + +export interface SaveHealth { + /** The pages whose last save failed, by ID. */ + readonly failing: ReadonlySet; +} + +export const saveHealthStore = createStore({ failing: new Set() }, 'saveHealth'); + +/** Whether any page's last save failed. */ +export const anySaveFailing = (health: SaveHealth): boolean => health.failing.size > 0; + +/** The status to show: the notes bridge's, unless a page's last save failed while nothing is on its way. */ +export function combinedStatus(notes: SaveStatus, pageFailing: boolean): SaveStatus { + return notes === 'saved' && pageFailing ? 'error' : notes; +} + +export function pageSaveFailed(page: string): void { + saveHealthStore.set((state) => { + if (state.failing.has(page)) return state; + return { 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 { failing }; + }); +} + +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); + }), + ]; + return () => stops.forEach((stop) => stop()); +} diff --git a/app/src/services/pages/tauri.ts b/app/src/services/pages/tauri.ts index fe741724..be2195a7 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 { watchSaveHealth } from './saveHealth'; import type { ImagesClient, IpcError } from '../../platform/types'; import { inkListeners } from './ink'; import { PageServiceError } from './types'; @@ -245,6 +246,8 @@ function events(session: Session, frames: ReturnType { const name = `main-${++clients}`; 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..7654a3e3 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,8 @@ 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 +301,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..37c6583b 100644 --- a/crates/core/src/format/page_json/tests.rs +++ b/crates/core/src/format/page_json/tests.rs @@ -211,6 +211,45 @@ 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/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..ca7ceb8e 100644 --- a/crates/core/src/ops/resolve/blocks.rs +++ b/crates/core/src/ops/resolve/blocks.rs @@ -117,8 +117,12 @@ 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/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..bf8e738e 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,16 @@ 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 +395,36 @@ 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..9a3b55ec 100644 --- a/crates/core/tests/session_anchored.rs +++ b/crates/core/tests/session_anchored.rs @@ -29,6 +29,52 @@ 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(); From e914741f1fb13e27c7d4e6a2bfc492d317cc2400 Mon Sep 17 00:00:00 2001 From: Eric Dean Date: Thu, 8 Oct 2026 05:12:14 -0700 Subject: [PATCH 2/5] Read notebook styles back as rounded as they are written Setting the same style twice is a repeat, not a change, now that notebook.json's styles are read with the writer's rounding (T2-1 follow-up). Co-Authored-By: Claude Opus 5.5 --- crates/core/src/format/tree_json/styles.rs | 14 +++++++++----- crates/core/src/format/tree_json/tests.rs | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/crates/core/src/format/tree_json/styles.rs b/crates/core/src/format/tree_json/styles.rs index d3697fa0..79164f2d 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), so a style in memory is 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..fc0905f6 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, so a style in memory is 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); +} From 1f20e8853cf9d27ef79da7f59dc201bc61a0a599 Mon Sep 17 00:00:00 2001 From: Eric Dean Date: Thu, 8 Oct 2026 05:13:18 -0700 Subject: [PATCH 3/5] Keep the app answering when a command holds the core Beta 4's T2-3: once one command never returned, every later core command parked a runtime worker and an IPC request behind it; the webview carries only a few IPC requests at once, so soon nothing reached Rust at all, Close and the log included, and the status said "Saving" for the rest of the session. - Every core command runs on a blocking thread, and waits at most 15 s for the core before failing as `coreBusy`, naming the holder, so the IPC channel stays open. - Journal opens wait at most 20 s for the journal thread; a page whose journal doesn't answer runs degraded instead of holding the core. - The exit polls the core lock up to the flush time and goes on without the final save when the core is held. - A watchdog names the command that has held the core for 10 s, logs it, and sends core:stalled / core:responsive; the title bar shows "Not responding" (announced) instead of "Saving" forever, also on a `coreBusy` error. Co-Authored-By: Claude Opus 5.5 --- app/src-tauri/src/audio.rs | 35 +- app/src-tauri/src/audio_more/mod.rs | 41 +- app/src-tauri/src/core_bridge.rs | 414 ++++++++++++++---- app/src-tauri/src/core_bridge/search.rs | 4 +- app/src-tauri/src/core_bridge_tests.rs | 266 ++++++++++- app/src-tauri/src/ink_bridge.rs | 19 +- app/src-tauri/src/interop/commands.rs | 48 +- app/src-tauri/src/notes/mod.rs | 64 ++- app/src-tauri/src/shellqol/mod.rs | 38 +- app/src-tauri/src/shellqol/notes_ops.rs | 4 +- app/src/features/tree/SaveStatus.test.tsx | 12 +- app/src/features/tree/SaveStatus.tsx | 14 +- app/src/services/notes/types.ts | 2 +- app/src/services/pages/saveHealth.test.ts | 35 +- app/src/services/pages/saveHealth.ts | 50 ++- app/src/services/pages/tauri.ts | 4 +- app/src/services/pages/types.ts | 4 +- app/src/strings/en/tree.ts | 3 + crates/core/src/session/journal_thread.rs | 16 +- .../core/src/session/journal_thread/tests.rs | 70 +++ .../core/src/session/journal_thread/worker.rs | 23 +- 21 files changed, 985 insertions(+), 181 deletions(-) diff --git a/app/src-tauri/src/audio.rs b/app/src-tauri/src/audio.rs index 851317af..1fa0583c 100644 --- a/app/src-tauri/src/audio.rs +++ b/app/src-tauri/src/audio.rs @@ -197,7 +197,7 @@ 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 +217,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 +235,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 +298,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 +315,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 +392,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 +415,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 +441,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..76919fce 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, + sync::{mpsc, Arc, Mutex, OnceLock, PoisonError, TryLockError}, + time::{Duration, Instant}, }; use opennote_core::{ @@ -47,6 +49,46 @@ pub mod search; /// 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); +/// 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. +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. +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. +type Held = Arc)>>>; + +/// Clears the holder when the command returns, or unwinds. +struct Holding(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. +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()) +} + /// The longest page ID the commands take. Page IDs are 26 characters. const MAX_PAGE_ID: usize = 128; @@ -109,7 +151,9 @@ 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, @@ -117,6 +161,15 @@ pub struct CoreBridge { state: Arc>>, 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. @@ -270,9 +323,67 @@ impl CoreBridge { state: Arc::new(Mutex::new(None)), 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 + } + + /// Takes the core, waiting at most `timeout` for the command that holds it. `None` when it is still held + /// after that. + fn take_core_within(&self, timeout: Duration) -> Option>> { + let deadline = Instant::now().checked_add(timeout); + loop { + match self.state.try_lock() { + Ok(state) => return Some(state), + Err(TryLockError::Poisoned(poisoned)) => return Some(poisoned.into_inner()), + Err(TryLockError::WouldBlock) if deadline.is_some_and(|d| Instant::now() < d) => { + std::thread::sleep(Duration::from_millis(10)); + } + Err(TryLockError::WouldBlock) => return None, + } } } + /// 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; waiting on + /// one of the async runtime's few worker threads meant that one command that never returned took every + /// worker in turn, and then nothing the interface asked for came back, not even what has nothing to do + /// with the core (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()))? + } + + /// What holds the core now, for the log. + 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()) + }) + } + /// 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 +403,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.into())); + let _holding = Holding(self.held.clone()); if state.is_none() { match Bridge::start(&self.root, self.relay.clone(), &self.search) { Ok(bridge) => { @@ -315,8 +447,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(); @@ -327,6 +470,54 @@ impl CoreBridge { }) } + /// 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. + 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}"); + } + } + /// Starts the worker that turns tree changes the core makes on its own into notes events. fn watch_trees(&self) { let (sender, receiver) = mpsc::channel::(); @@ -365,8 +556,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 +597,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 +649,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 +671,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 +771,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 +792,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 +814,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 +836,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/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..baec791b 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,249 @@ 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 included (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 d7f307d8..677dc06c 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,7 @@ 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 +243,7 @@ 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 +253,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 +329,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/tree/SaveStatus.test.tsx b/app/src/features/tree/SaveStatus.test.tsx index 90977ea6..964efea1 100644 --- a/app/src/features/tree/SaveStatus.test.tsx +++ b/app/src/features/tree/SaveStatus.test.tsx @@ -3,7 +3,7 @@ import { act, cleanup, screen } from '@testing-library/react'; import { afterEach, describe, expect, it } from 'vitest'; -import { pageSaveFailed, pageSaved } from '../../services/pages/saveHealth'; +import { pageSaveFailed, pageSaved, setCoreStalled } from '../../services/pages/saveHealth'; import { announcements, renderUi } from '../../test'; import { SaveStatus } from './SaveStatus'; import { treeStore } from './store'; @@ -24,4 +24,14 @@ describe('the save status', () => { 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 1d38cf7c..e778b828 100644 --- a/app/src/features/tree/SaveStatus.tsx +++ b/app/src/features/tree/SaveStatus.tsx @@ -1,15 +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, saveHealthStore } from '../../services/pages/saveHealth'; +import { anySaveFailing, combinedStatus, coreStalled, saveHealthStore } from '../../services/pages/saveHealth'; import { useStore } from '../../state/store'; import { t } from '../../strings/t'; import { announce } from '../../ui'; @@ -22,14 +23,17 @@ const ICONS: Record> = { saving: CloudArrowUpIcon, offline: CloudSlashIcon, error: CloudWarningIcon, + stalled: CloudXIcon, }; export function SaveStatus({ presentation }: { presentation: 'full' | 'icon' | 'menuItem' }) { const notes = useStore(treeStore, (state) => state.saveStatus); const pageFailing = useStore(saveHealthStore, anySaveFailing); - const status = combinedStatus(notes, pageFailing); + 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 index c9b47e68..67c43cc8 100644 --- a/app/src/services/pages/saveHealth.test.ts +++ b/app/src/services/pages/saveHealth.test.ts @@ -4,9 +4,13 @@ import { resetStores } from '../../state/store'; import { anySaveFailing, combinedStatus, + coreBusy, + coreStalled, + pageClosed, pageSaveFailed, pageSaved, saveHealthStore, + setCoreStalled, watchSaveHealth, } from './saveHealth'; @@ -37,10 +41,37 @@ describe('save health', () => { 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']); + 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); @@ -57,5 +88,7 @@ describe('save health', () => { 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 index 725bd530..fd772257 100644 --- a/app/src/services/pages/saveHealth.ts +++ b/app/src/services/pages/saveHealth.ts @@ -1,7 +1,10 @@ -// Whether the core could save each page. 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). 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. +// 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'; @@ -9,22 +12,43 @@ 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() }, 'saveHealth'); +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; -/** The status to show: the notes bridge's, unless a page's last save failed while nothing is on its way. */ -export function combinedStatus(notes: SaveStatus, pageFailing: boolean): SaveStatus { +/** 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, whether or not its + * watchdog's event has arrived yet. `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 { failing: new Set([...state.failing, page]) }; + return { ...state, failing: new Set([...state.failing, page]) }; }); } @@ -33,10 +57,16 @@ export function pageSaved(page: string): void { if (!state.failing.has(page)) return state; const failing = new Set(state.failing); failing.delete(page); - return { failing }; + 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; @@ -55,6 +85,8 @@ export function watchSaveHealth(client: { 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.ts b/app/src/services/pages/tauri.ts index be2195a7..202866fb 100644 --- a/app/src/services/pages/tauri.ts +++ b/app/src/services/pages/tauri.ts @@ -2,7 +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 { watchSaveHealth } from './saveHealth'; +import { coreBusy, pageClosed, watchSaveHealth } from './saveHealth'; import type { ImagesClient, IpcError } from '../../platform/types'; import { inkListeners } from './ink'; import { PageServiceError } from './types'; @@ -25,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)); } @@ -317,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/crates/core/src/session/journal_thread.rs b/crates/core/src/session/journal_thread.rs index 98f674b6..d14ba8cc 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,11 @@ 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. +const OPEN_TIMEOUT: Duration = Duration::from_secs(20); + impl JournalThread { /// Starts the thread. pub fn start(config: JournalConfig) -> Result { @@ -212,6 +219,7 @@ impl JournalThread { commands, codec, thread: Some(thread), + open_wait: OPEN_TIMEOUT, }) } @@ -233,7 +241,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 +262,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) { From b16d747e3b4dbe00874bbd2cece3c2a0f07cf3c9 Mon Sep 17 00:00:00 2001 From: Eric Dean Date: Thu, 8 Oct 2026 06:12:01 -0700 Subject: [PATCH 4/5] Queue commands at a door instead of polling for the core A command that waits for the core slept 10 ms between try_lock polls, so a contended command paid up to 10 ms extra and a waiter could be passed by newer arrivals for as long as it waited. The commands now queue at a door (a flag and a condition variable): a waiter sleeps until the holder leaves and starts the moment it does, and still gives up as coreBusy after COMMAND_WAIT. The journal open timeout drops from 20 s to 10 s, below COMMAND_WAIT (15 s), so a stuck journal open gives up before the commands queued behind it do, and the two constants say how they relate. The interface's coreBusy path has tests: a busy page_apply keeps the text and its riders unsent, shows the "not kept" toast and sends them with the next flush (textSync.test), and marks the core stalled before the watchdog's event (tauri.test). One useless Cow conversion clippy flagged is gone. Co-Authored-By: Claude Opus 5.5 --- app/src-tauri/src/core_bridge.rs | 88 +++++++++++++++++---- app/src/features/page/sync/textSync.test.ts | 28 +++++++ app/src/services/pages/tauri.test.ts | 13 +++ crates/core/src/session/journal_thread.rs | 6 +- 4 files changed, 117 insertions(+), 18 deletions(-) diff --git a/app/src-tauri/src/core_bridge.rs b/app/src-tauri/src/core_bridge.rs index 76919fce..7b0a1324 100644 --- a/app/src-tauri/src/core_bridge.rs +++ b/app/src-tauri/src/core_bridge.rs @@ -15,7 +15,7 @@ use std::{ fs, panic::Location, path::{Path, PathBuf}, - sync::{mpsc, Arc, Mutex, OnceLock, PoisonError, TryLockError}, + sync::{mpsc, Arc, Condvar, Mutex, MutexGuard, OnceLock, PoisonError}, time::{Duration, Instant}, }; @@ -59,7 +59,13 @@ const HELD_WARNING: Duration = Duration::from_secs(10); /// 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. +/// 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. 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. @@ -73,6 +79,41 @@ pub const RESPONSIVE_EVENT: &str = "core:responsive"; /// What holds the core now: since when, and which command. 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. +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). +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. struct Holding(Held); @@ -159,6 +200,8 @@ pub struct CoreBridge { 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. @@ -321,6 +364,7 @@ impl CoreBridge { CoreBridge { root, state: Arc::new(Mutex::new(None)), + door: Arc::default(), relay: Arc::default(), search: Arc::default(), held: Arc::default(), @@ -345,19 +389,29 @@ impl CoreBridge { } /// Takes the core, waiting at most `timeout` for the command that holds it. `None` when it is still held - /// after that. - fn take_core_within(&self, timeout: Duration) -> Option>> { + /// 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. + fn take_core_within(&self, timeout: Duration) -> Option> { let deadline = Instant::now().checked_add(timeout); - loop { - match self.state.try_lock() { - Ok(state) => return Some(state), - Err(TryLockError::Poisoned(poisoned)) => return Some(poisoned.into_inner()), - Err(TryLockError::WouldBlock) if deadline.is_some_and(|d| Instant::now() < d) => { - std::thread::sleep(Duration::from_millis(10)); - } - Err(TryLockError::WouldBlock) => return None, + 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), + }) } /// Runs `work` on a blocking thread and answers when it is done. Every command that touches the core goes @@ -379,9 +433,11 @@ impl CoreBridge { /// What holds the core now, for the log. 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()) - }) + holder + .as_ref() + .map_or("nothing holds it now".to_owned(), |(since, what)| { + format!("{what} has held it for {} s", since.elapsed().as_secs()) + }) } /// Sends the core's events through `emit`. The first call wins. @@ -425,7 +481,7 @@ impl CoreBridge { 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.into())); + *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) { 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/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/crates/core/src/session/journal_thread.rs b/crates/core/src/session/journal_thread.rs index d14ba8cc..bc6fd926 100644 --- a/crates/core/src/session/journal_thread.rs +++ b/crates/core/src/session/journal_thread.rs @@ -201,8 +201,10 @@ 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. -const OPEN_TIMEOUT: Duration = Duration::from_secs(20); +/// 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. From e7a44f152e165534720f9ffca8b68a0c911253ef Mon Sep 17 00:00:00 2001 From: Eric Dean Date: Thu, 8 Oct 2026 06:40:10 -0700 Subject: [PATCH 5/5] Split the held-core code out of the bridge and satisfy the checks CI's checks on the pull request raised what the lane's own files owed: the bridge had grown past the 800-line limit, rustfmt wanted eight of the lane's files reflowed, and seven comments were too long or missed a serial comma. The constants, the door, the guard, the holder and the watchdog now live in core_bridge/held.rs, which reads as one thing: how a command takes the core, how long it waits, and who says when one holds it too long. The bridge keeps the commands. The lane's files are formatted as CI's rustfmt wants them, and the comments say the same in shorter sentences. Co-Authored-By: Claude Opus 5.5 --- app/src-tauri/src/audio.rs | 4 +- app/src-tauri/src/core_bridge.rs | 181 ++------------------- app/src-tauri/src/core_bridge/held.rs | 179 ++++++++++++++++++++ app/src-tauri/src/core_bridge_tests.rs | 42 +++-- app/src-tauri/src/interop/commands.rs | 8 +- app/src/services/pages/saveHealth.ts | 4 +- app/src/services/pages/tauri.ts | 2 +- crates/core/src/format/page_json/data.rs | 6 +- crates/core/src/format/page_json/tests.rs | 12 +- crates/core/src/format/tree_json/styles.rs | 6 +- crates/core/src/format/tree_json/tests.rs | 4 +- crates/core/src/ops/resolve/blocks.rs | 8 +- crates/core/src/store/page_store/tests.rs | 18 +- crates/core/tests/session_anchored.rs | 12 +- 14 files changed, 279 insertions(+), 207 deletions(-) create mode 100644 app/src-tauri/src/core_bridge/held.rs diff --git a/app/src-tauri/src/audio.rs b/app/src-tauri/src/audio.rs index 1fa0583c..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.run(move |bridge| bridge.with(|bridge| assets_dir(bridge, &page))).await?; + let dir = bridge + .run(move |bridge| bridge.with(|bridge| assets_dir(bridge, &page))) + .await?; Ok(dir.to_string_lossy().into_owned()) } diff --git a/app/src-tauri/src/core_bridge.rs b/app/src-tauri/src/core_bridge.rs index 7b0a1324..c0239304 100644 --- a/app/src-tauri/src/core_bridge.rs +++ b/app/src-tauri/src/core_bridge.rs @@ -15,7 +15,7 @@ use std::{ fs, panic::Location, path::{Path, PathBuf}, - sync::{mpsc, Arc, Condvar, Mutex, MutexGuard, OnceLock, PoisonError}, + sync::{mpsc, Arc, Mutex, OnceLock, PoisonError}, time::{Duration, Instant}, }; @@ -44,92 +44,15 @@ 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); -/// 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. -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. -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. -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. -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). -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. -struct Holding(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. -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()) -} - /// The longest page ID the commands take. Page IDs are 26 characters. const MAX_PAGE_ID: usize = 128; @@ -388,38 +311,12 @@ impl CoreBridge { self } - /// 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. - 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), - }) - } - /// 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; waiting on - /// one of the async runtime's few worker threads meant that one command that never returned took every - /// worker in turn, and then nothing the interface asked for came back, not even what has nothing to do - /// with the core (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. + /// 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, @@ -430,16 +327,6 @@ impl CoreBridge { .map_err(|error| IpcError::new(codes::INTERNAL, error.to_string()))? } - /// What holds the core now, for the log. - 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()) - }) - } - /// Sends the core's events through `emit`. The first call wins. pub fn listen(&self, emit: Emit) { let _ = self.relay.emit.set(emit); @@ -526,54 +413,6 @@ impl CoreBridge { }) } - /// 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. - 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}"); - } - } - /// Starts the worker that turns tree changes the core makes on its own into notes events. fn watch_trees(&self) { let (sender, receiver) = mpsc::channel::(); 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_tests.rs b/app/src-tauri/src/core_bridge_tests.rs index baec791b..144ab832 100644 --- a/app/src-tauri/src/core_bridge_tests.rs +++ b/app/src-tauri/src/core_bridge_tests.rs @@ -110,9 +110,15 @@ fn a_command_that_holds_the_core_too_long_is_reported_and_the_core_answers_again 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())) - }); + 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; } @@ -137,12 +143,18 @@ fn a_command_that_holds_the_core_too_long_is_reported_and_the_core_answers_again 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()); + 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 + .with_named("a command after it", |_| Ok(())) + .expect("the core is free"); bridge.shutdown(); } @@ -154,9 +166,7 @@ fn the_watchdog_knows_a_command_by_where_it_was_called_from() { 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"); + let holder = bridge.notes(None, |_| Ok(bridge.holder())).expect("runs"); assert!(holder.starts_with("the command at core_bridge_tests.rs:"), "{holder}"); bridge.shutdown(); } @@ -271,8 +281,9 @@ fn concurrent_edits_saves_renames_and_closes_never_wait_for_each_other_forever() } /// 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 included (beta 4's T2-3). The core serves the next command once the holder returns. +/// 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"); @@ -297,7 +308,11 @@ fn a_command_gives_up_on_a_held_core_and_names_the_holder() { .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); + assert!( + error.message.contains("a stuck command"), + "the message names the holder: {}", + error.message + ); release.send(()).expect("the holder is released"); }); bridge @@ -326,7 +341,10 @@ fn the_exit_gives_up_on_a_command_that_holds_the_core_too_long() { bridge.shutdown_within(Duration::from_millis(150)); at.elapsed() }); - assert!(took < Duration::from_millis(800), "the exit waited {took:?} for the command"); + 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" diff --git a/app/src-tauri/src/interop/commands.rs b/app/src-tauri/src/interop/commands.rs index 713dd9e5..163f334e 100644 --- a/app/src-tauri/src/interop/commands.rs +++ b/app/src-tauri/src/interop/commands.rs @@ -184,7 +184,9 @@ pub async fn interop_preview( path: String, choices: Option, ) -> IpcResult> { - let device = bridge.run(|bridge| bridge.with(|bridge| Ok(bridge.core.device()))).await?; + 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.run(|bridge| bridge.with(|bridge| Ok(bridge.core.device()))).await?; + 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(); diff --git a/app/src/services/pages/saveHealth.ts b/app/src/services/pages/saveHealth.ts index fd772257..d21ced92 100644 --- a/app/src/services/pages/saveHealth.ts +++ b/app/src/services/pages/saveHealth.ts @@ -38,8 +38,8 @@ export function setCoreStalled(stalled: boolean): void { } /** - * A command gave up waiting for the core (the `coreBusy` error): the core is stalled, whether or not its - * watchdog's event has arrived yet. `core:responsive` clears it once the holder returns. + * 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); diff --git a/app/src/services/pages/tauri.ts b/app/src/services/pages/tauri.ts index 202866fb..748ce14f 100644 --- a/app/src/services/pages/tauri.ts +++ b/app/src/services/pages/tauri.ts @@ -247,7 +247,7 @@ function events(session: Session, frames: ReturnType { diff --git a/crates/core/src/format/page_json/data.rs b/crates/core/src/format/page_json/data.rs index 7654a3e3..4a3cbb71 100644 --- a/crates/core/src/format/page_json/data.rs +++ b/crates/core/src/format/page_json/data.rs @@ -259,7 +259,11 @@ fn read_image(map: &JsonMap) -> Result { fn read_crop(map: &JsonMap) -> Result { let mut view = View::new(map); // 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")); + 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")?, diff --git a/crates/core/src/format/page_json/tests.rs b/crates/core/src/format/page_json/tests.rs index 37c6583b..54d32dc4 100644 --- a/crates/core/src/format/page_json/tests.rs +++ b/crates/core/src/format/page_json/tests.rs @@ -233,10 +233,18 @@ fn geometry_is_read_as_the_file_writes_it_so_a_page_reads_back_as_itself() { } ]); let page = read(&value).unwrap().page; - let text = page.blocks.iter().find(|b| matches!(b.data, BlockData::Text(_))).unwrap(); + 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 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)); diff --git a/crates/core/src/format/tree_json/styles.rs b/crates/core/src/format/tree_json/styles.rs index 79164f2d..09f67785 100644 --- a/crates/core/src/format/tree_json/styles.rs +++ b/crates/core/src/format/tree_json/styles.rs @@ -18,9 +18,9 @@ pub(super) fn read_styles(value: Value) -> Result { } /// Reads one style. Each number is rounded as [`write_style`] writes it (sizes and spacing to 0.01, the line -/// height to two decimals), so a style in memory is 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. +/// 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 { diff --git a/crates/core/src/format/tree_json/tests.rs b/crates/core/src/format/tree_json/tests.rs index fc0905f6..38e18b19 100644 --- a/crates/core/src/format/tree_json/tests.rs +++ b/crates/core/src/format/tree_json/tests.rs @@ -120,8 +120,8 @@ fn merged_copies_take_the_styles_of_the_copy_changed_later() { 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, so a style in memory is the style its bytes read back as: setting the same style twice is a +/// 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() { diff --git a/crates/core/src/ops/resolve/blocks.rs b/crates/core/src/ops/resolve/blocks.rs index ca7ceb8e..b3dae1cd 100644 --- a/crates/core/src/ops/resolve/blocks.rs +++ b/crates/core/src/ops/resolve/blocks.rs @@ -120,7 +120,13 @@ fn check_element_ids(page: &Page, block: &Block) -> Result<(), EditError> { /// 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] { + 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/store/page_store/tests.rs b/crates/core/src/store/page_store/tests.rs index bf8e738e..adcdde47 100644 --- a/crates/core/src/store/page_store/tests.rs +++ b/crates/core/src/store/page_store/tests.rs @@ -285,12 +285,11 @@ fn a_page_that_reads_back_differently_is_not_saved() { 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(); + 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()); } @@ -402,7 +401,12 @@ fn ink_anchored_at_a_fractional_offset_saves() { 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 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 { diff --git a/crates/core/tests/session_anchored.rs b/crates/core/tests/session_anchored.rs index 9a3b55ec..f21cdd66 100644 --- a/crates/core/tests/session_anchored.rs +++ b/crates/core/tests/session_anchored.rs @@ -47,7 +47,10 @@ fn ink_anchored_at_a_measured_offset_saves_with_the_offset_as_the_file_holds_it( y: Some(10.0), ..Frame::default() }), - data: json!({"role": "anchored", "anchor": anchor}).as_object().cloned().unwrap(), + data: json!({"role": "anchored", "anchor": anchor}) + .as_object() + .cloned() + .unwrap(), fallback: None, }; let setup = vec![ @@ -71,7 +74,12 @@ fn ink_anchored_at_a_measured_offset_saves_with_the_offset_as_the_file_holds_it( 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 + .apply(real.request(vec![Edit::SetText { + block: text, + markdown: "Now".into(), + }])) + .unwrap(); handle.save_now().expect("the second save"); }