From c830e2971fbbcfeb96cfb5286d43d88e0edff1d5 Mon Sep 17 00:00:00 2001 From: clover caruso Date: Thu, 15 Oct 2026 05:55:27 -0700 Subject: [PATCH] fix: a move that only copies a page says so and keeps the history whole A move reads the page together with its conflict pages and versions, which no copy carries, and retires it only while all three are as read, so a conflict page kept or a version made during the move is no longer deleted with the page. The API is named for moves: Replica::moving reads the page, and Replica::retire deletes it, refusing with "The page changed while it moved." otherwise. Once the copy has landed, any failure to retire the original, whether a change or a cache that cannot write, leaves the page in both sections. The desktop then shows "Page kept in both sections" with the reason and records no step that cannot be taken back, and an Undo of a move that ends this way shows the copy it brought back. iOS reports the same through the copied field of sb_library_move_page and its own alert. A desktop move no longer fails after its copy when the target section still has edits waiting for the cache: the section is kept open, retrying them, rather than closed. Refs #138 Assisted-by: claude-opus-5.5 --- Cargo.lock | 1 + apps/ios/Snowbound/Arranging.swift | 7 +- crates/mobile/src/library.rs | 26 +-- crates/mobile/src/tests.rs | 61 ++++++- crates/notebook/README.md | 9 +- crates/notebook/src/lib.rs | 52 +++--- crates/notebook/src/session.rs | 3 +- crates/notebook/src/working.rs | 37 +++- crates/notebook/tests/support/vfs.rs | 2 + crates/notebook/tests/sync_pages.rs | 66 ++++++- crates/snowbound/Cargo.toml | 2 + crates/snowbound/src/main.rs | 4 + crates/snowbound/src/undo.rs | 251 +++++++++++++++++++-------- 13 files changed, 395 insertions(+), 126 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c0c4f969714e94d5675e4288d48beae02fe55b6c..0ea4fed9a1bf38eb49aa8251b25d4e5e49007302 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3649,6 +3649,7 @@ dependencies = [ "png", "pollster", "ring", + "rusqlite", "rustc-demangle", "rustls-native-certs", "sctk-adwaita", diff --git a/apps/ios/Snowbound/Arranging.swift b/apps/ios/Snowbound/Arranging.swift index 1f2014b2773c8d45fa73d4c742d5fc4a0ada5f84..6903ba3f35f8c7dd7453e5dc6fdcb41250ed4270 100644 --- a/apps/ios/Snowbound/Arranging.swift +++ b/apps/ios/Snowbound/Arranging.swift @@ -12,6 +12,8 @@ private struct MovedPage: Decodable { let added: String? let before: String? let level: Int + /// Why the page also stayed where it was: the move only copied it. + let copied: String? } /// The catalog path of the folder holding the section or group at `path`; "" for the @@ -155,6 +157,7 @@ enum Arranging { section.reloadRows() } if moved == nil { failed("Can’t Move Page") } + if let copied = moved?.copied { failed("Page Kept in Both Sections", copied) } } } @@ -244,7 +247,9 @@ enum Arranging { release() source.reloadRows() target.reloadRows() - if !succeeded { + if let copied = reported.lazy.compactMap(\.copied).first { + failed("Page Kept in Both Sections", copied) + } else if !succeeded { failed("Can’t Move All Pages", reported.isEmpty ? "Check both sections, then try moving the remaining pages again." : "Check both sections. Use \(undoing ? "Redo" : "Undo") to reverse completed moves, or try moving the remaining pages again.") diff --git a/crates/mobile/src/library.rs b/crates/mobile/src/library.rs index 87f71f2bd4a3bcb4addae182105e91c051f91fac..f316334c0269445e0bd7ab4ec965df3630052bc0 100644 --- a/crates/mobile/src/library.rs +++ b/crates/mobile/src/library.rs @@ -1387,9 +1387,9 @@ impl Library { let at = (listed.iter().position(|(listed, ..)| *listed == space)) .ok_or("The page is not listed")?; let next = listed.get(at + 1).map(|(next, ..)| *next); - Ok((section.page(space)?, (next, listed[at].2))) + Ok((section.replica().moving(space)?, (next, listed[at].2))) })?; - let import = session::moved(&page, author)?; + let import = session::moved(page.page(), author)?; let Op::Section(SectionOp::Import { creation, .. }) = &import else { unreachable!("notebook::session::moved imports"); }; @@ -1415,7 +1415,7 @@ impl Library { }; Ok(section.replica().apply(author, edit)?) })?; - let added = self.with_section(from, |section| { + let retired = self.with_section(from, |section| { let mut ops = vec![Op::Section(SectionOp::Delete(vec![space]))]; let added = match section.pages()?.as_slice() { [(only, ..)] if *only == space => { @@ -1430,17 +1430,20 @@ impl Library { at: filetime(), ops, }; - section - .replica() - .apply_unchanged(author, edit, space, page)?; + section.replica().retire(author, edit, page)?; Ok(added) - })?; + }); let id = |space: Option| space.map(|space| space.to_string()); + let (added, copied) = match retired { + Ok(added) => (id(added), None), + Err(error) => (None, Some(error.to_string())), + }; Ok(MovedPage { page: arrived.to_string(), - added: id(added), + added, before: id(was.0), level: was.1, + copied, }) } @@ -1481,6 +1484,8 @@ pub(crate) struct MovedPage { pub(crate) added: Option, pub(crate) before: Option, pub(crate) level: u32, + /// Why the page also stayed where it was: the move only copied it. + pub(crate) copied: Option, } #[derive(serde::Serialize)] @@ -1937,8 +1942,9 @@ pub unsafe extern "C" fn sb_library_new_section( /// before page `before` at `level`, or last with `before` null; `discard`, unless null, is /// the page `to` gained when this page left it. Returns JSON with `page`, its id there, /// `added`, the page titled `date` and `time` that `from` gained, or null, and `before` and -/// `level`, where it was in `from`; null when it cannot move. Writes to both sections, so -/// call it off the main thread. +/// `level`, where it was in `from`, and `copied`, why it also stayed in `from` where the move +/// only copied it, or null; null when it cannot move. Writes to both sections, so call it +/// off the main thread. /// /// # Safety /// Every string is NUL-terminated UTF-8; `before` and `discard` may be null. diff --git a/crates/mobile/src/tests.rs b/crates/mobile/src/tests.rs index 4d588e0206e2d299259613fdf120c1c1402c5b35..8f72cb28cc207356a327e0c8f97dff199de5adf2 100644 --- a/crates/mobile/src/tests.rs +++ b/crates/mobile/src/tests.rs @@ -1813,7 +1813,7 @@ fn a_page_edited_while_it_moves_stays_with_its_edit() { "Clover Test", date, ) - .map(|_| ()) + .map(|moved| moved.copied) .map_err(|error| error.to_string()) }); holding.recv_timeout(Duration::from_secs(10)).unwrap(); @@ -1830,8 +1830,8 @@ fn a_page_edited_while_it_moves_stays_with_its_edit() { } features.shared.section.written().unwrap(); drop(release); - let moved = move_page.join().unwrap(); - assert_eq!(moved.is_ok(), !edited, "{moved:?}"); + let copied = move_page.join().unwrap().unwrap(); + assert_eq!(copied.is_some(), edited, "{copied:?}"); drop(features); library.close(Duration::from_secs(20)).unwrap(); let arena = onestore::Arena::default(); @@ -1853,6 +1853,58 @@ fn a_page_edited_while_it_moves_stays_with_its_edit() { } } +/// A page whose section cannot write as it leaves stays there beside the copy that arrived, +/// and the move says so; both files hold it once the notebook closes. +#[test] +fn a_page_its_section_cannot_retire_stays_beside_its_copy() { + vfs::install(); + let _exclusive = vfs::EXCLUSIVE + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let (directory, root, library, empty) = notebook_open("Empty.one"); + let features = open(&library, "Features.one"); + let space = space(&features, "Task controls"); + drop(features); + let file = std::fs::read(root.join("Features.one")).unwrap(); + let identity = onestore::RevisionIndex::parse(&onestore::Store::parse(&file).unwrap()) + .unwrap() + .root + .guid; + let location = notebook::location::local(&root).unwrap(); + let replica = + notebook::location::folder(&directory.path().join("cache"), &location).join(format!( + "{}.sqlite", + (identity.iter()) + .map(|byte| format!("{byte:02x}")) + .collect::() + )); + *vfs::FAILING.lock().unwrap() = Some(replica.canonicalize().unwrap()); + let date = ["Thursday, October 1, 2026", "9:00 AM"]; + let moved = library.move_page( + "Features.one", + space, + "Empty.one", + None, + 1, + None, + "Clover Test", + date, + ); + *vfs::FAILING.lock().unwrap() = None; + assert!(moved.unwrap().copied.is_some()); + drop(empty); + library.close(Duration::from_secs(20)).unwrap(); + let titles = |name: &str| { + let arena = onestore::Arena::default(); + let stored = onestore::read_file(root.join(name)).unwrap(); + let mut section = onestore::Section::open(&arena, stored).unwrap(); + (section.pages().unwrap().into_iter()) + .filter(|(_, title, _)| title == "Task controls") + .count() + }; + assert_eq!((titles("Features.one"), titles("Empty.one")), (1, 1)); +} + /// A page the section it moves to refuses stays where it was, and the move fails. #[test] fn a_page_the_other_section_refuses_stays() { @@ -3386,6 +3438,9 @@ fn reader_dropped_and_synchronous_failed_apply_callbacks_never_release_the_reloa #[test] fn reader_deferred_reload_preserves_rich_composition_after_real_local_write_failure() { + let _exclusive = vfs::EXCLUSIVE + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); for select in [false, true] { let directory = tempfile::tempdir().unwrap(); let file = directory.path().join("Reader fault.one"); diff --git a/crates/notebook/README.md b/crates/notebook/README.md index e331f76c7bc6ac9a390946986965b88a2325261c..f352974701fc16d330cb0b752b81f18d8c178a6e 100644 --- a/crates/notebook/README.md +++ b/crates/notebook/README.md @@ -38,10 +38,11 @@ assert_eq!(reopened.pending()?.last().map(|edit| edit.id), Some(id)); # } ``` -`apply_unchanged(author, edit, space, page)` applies the edit only while page `space` -still holds `page`, checked on the section thread in order with every other edit, and -refuses it as `Rejected` otherwise: a move across sections copies a page there, then retires -it here this way, so an edit made since the copy keeps the page. +A move across sections reads the page with `moving(space)`, together with its conflict pages +and versions, which no copy carries; copies it there; then retires it here with +`retire(author, edit, moving)`, which applies the edit only while all of them are as read, +checked on the section thread in order with every other edit, and refuses it as `Rejected` +otherwise, so an edit made since the copy keeps the page. Page reads never wait for the sync thread: a rebase or reread of the queue runs on a new thread while the current one keeps answering `page` and `pages` from the section as diff --git a/crates/notebook/src/lib.rs b/crates/notebook/src/lib.rs index 693a16903a35610ddeaa711221f7ab0f9dfaec89..15f146b6120c043fb75514cbf26627b8c52ef548 100644 --- a/crates/notebook/src/lib.rs +++ b/crates/notebook/src/lib.rs @@ -88,6 +88,22 @@ type Result = std::result::Result; const APPLICATION_ID: u32 = 0x4f4e454f; +/// A page as a move copies it (`Replica::moving`), with its conflict pages and versions, which +/// stay behind, read together. +#[derive(Debug, PartialEq)] +pub struct Moving { + space: ExGuid, + page: Page, + conflicts: Vec, + versions: Vec, +} + +impl Moving { + pub fn page(&self) -> &Page { + &self.page + } +} + /// A locally durable edit; its ID remains stable across cache reopen. #[derive(Debug, Clone, PartialEq)] pub struct PendingEdit { @@ -308,39 +324,33 @@ impl Replica { edit, reply, waiting: None, - unchanged: None, + moving: None, }) }) .await } - /// `apply`, provided page `space` still holds `page`; refused as `Rejected` otherwise. - pub fn apply_unchanged( - &self, - author: &str, - edit: Edit, - space: ExGuid, - page: Page, - ) -> Result { - task::blocking(self.apply_unchanged_async(author, edit, space, page))? + /// Page `space` as a move copies it, read at once with what the copy leaves behind. + pub fn moving(&self, space: ExGuid) -> Result { + self.ask(|reply| working::Request::Moving { space, reply }) } - /// `apply_async`, provided page `space` still holds `page`, checked where edits apply in - /// order: a move retires the page it copied this way, keeping any edit made since. - pub async fn apply_unchanged_async( - &self, - author: &str, - edit: Edit, - space: ExGuid, - page: Page, - ) -> Result { + /// `retire_async`, blocking. + pub fn retire(&self, author: &str, edit: Edit, moving: Moving) -> Result { + task::blocking(self.retire_async(author, edit, moving))? + } + + /// Applies `edit`, which deletes the page `moving` read once its copy has landed elsewhere, + /// only while the page and what it leaves behind are as read: checked in order with every + /// other edit, so none made since is deleted with it. One changed is refused as `Rejected`. + pub async fn retire_async(&self, author: &str, edit: Edit, moving: Moving) -> Result { self.ask_async(|reply| { working::Request::Apply(working::Accepted { author: author.to_owned(), edit, reply, waiting: None, - unchanged: Some((space, page)), + moving: Some(Box::new(moving)), }) }) .await @@ -360,7 +370,7 @@ impl Replica { edit, reply, waiting: Some(waiting), - unchanged: None, + moving: None, })) } diff --git a/crates/notebook/src/session.rs b/crates/notebook/src/session.rs index b132e46cbf3c5ea8ac89a8b28510549daa0f3662..603d56e2e1695b8107a46c211b86b92d8568f668 100644 --- a/crates/notebook/src/session.rs +++ b/crates/notebook/src/session.rs @@ -2565,8 +2565,7 @@ impl Section { } /// Removes pages or conflict pages permanently, queued like the user's own edits (a - /// move across sections imports a page there, then retires it here with - /// `Replica::apply_unchanged`). + /// move across sections imports a page there, then retires it here with `Replica::retire`). pub fn delete_pages(&self, pages: &[ExGuid]) -> Result { crate::task::blocking(self.delete_pages_async(pages))? } diff --git a/crates/notebook/src/working.rs b/crates/notebook/src/working.rs index cf8ef019fab2910a41e8e13616798b403fb48300..63d4fa2283f9022000485207e2419724ac70ea63 100644 --- a/crates/notebook/src/working.rs +++ b/crates/notebook/src/working.rs @@ -4,7 +4,7 @@ //! and, when the remote changed, to rebase the queue. In the browser, which has one thread, //! the section is served as each request is sent, and a rebuild runs as it is asked for. -use crate::{Result, base, lock, merge, queue, worker::Signal}; +use crate::{Moving, Result, base, lock, merge, queue, worker::Signal}; use onestore::{ Arena, ExGuid, Section, Transaction, op::{Edit, OpError}, @@ -42,6 +42,10 @@ pub(crate) enum Request { version: ExGuid, reply: Reply, }, + Moving { + space: ExGuid, + reply: Reply, + }, /// Answers once the edits before it are written, optionally awaiting host durability. Flush { reply: Reply<()>, @@ -89,6 +93,7 @@ impl Request { Self::Conflicts { reply } => reply(Err(error())), Self::Versions { reply } => reply(Err(error())), Self::Version { reply, .. } => reply(Err(error())), + Self::Moving { reply, .. } => reply(Err(error())), Self::Flush { reply, .. } => reply(Err(error())), Self::Seal { reply } => reply(Err(error())), Self::Rebase { reply, .. } => reply(Err(error())), @@ -482,7 +487,7 @@ pub(crate) struct Accepted { /// Set where nobody blocks on the reply: a failed write keeps the edit and reports here. pub waiting: Option, /// A page as a move copied it, which the edit applies only while it still holds. - pub unchanged: Option<(ExGuid, Page)>, + pub moving: Option>, } struct Working<'a> { @@ -553,6 +558,7 @@ impl<'a> Working<'a> { version, reply, } => reply(self.section.version(space, version).map_err(Into::into)), + Request::Moving { space, reply } => reply(self.moving(space)), Request::Handover(to) => { let mut waiting = held.take().unwrap_or_default(); waiting.extend(burst.drain(..).chain(later.drain(..))); @@ -613,6 +619,10 @@ impl<'a> Working<'a> { reply(self.section.version(space, version).map_err(Into::into)); false } + Request::Moving { space, reply } => { + reply(self.moving(space)); + false + } Request::Flush { reply, durable } => { #[cfg(target_arch = "wasm32")] if durable { @@ -699,15 +709,28 @@ impl<'a> Working<'a> { .is_ok() } + /// Page `space` as a move copies it, with the conflict pages and versions it leaves. + fn moving(&mut self, space: ExGuid) -> Result { + fn of(space: ExGuid, listed: Vec<(ExGuid, Vec)>) -> Vec { + (listed.into_iter()) + .find_map(|(page, of)| (page == space).then_some(of)) + .unwrap_or_default() + } + Ok(Moving { + space, + page: self.section.page(space)?, + conflicts: of(space, self.section.conflicts()?), + versions: of(space, self.section.versions()?), + }) + } + /// Applies an edit, keeping it to be written with the burst. A refused edit is answered /// here; true when it failed part way and the section must be reread. fn accept(&mut self, accepted: Accepted) -> bool { - if let Some((space, page)) = &accepted.unchanged - && !self.section.page(*space).is_ok_and(|now| now == *page) + if let Some(moving) = &accepted.moving + && !self.moving(moving.space).is_ok_and(|now| now == **moving) { - let changed = OpError::StructureChanged( - "The page changed while it moved. Both sections now have a copy.", - ); + let changed = OpError::StructureChanged("The page changed while it moved."); (accepted.reply)(Err(changed.into())); return false; } diff --git a/crates/notebook/tests/support/vfs.rs b/crates/notebook/tests/support/vfs.rs index bbf98abe986aa686b2e6a4623e4b6654c72d3281..0af3c4f05363d11095277c9d3ac5c5f745eb0b5a 100644 --- a/crates/notebook/tests/support/vfs.rs +++ b/crates/notebook/tests/support/vfs.rs @@ -34,6 +34,8 @@ static REAL: OnceLock = OnceLock::new(); static WAL: OnceLock<(usize, usize, usize)> = OnceLock::new(); /// While set, every write to this database's WAL fails, for WALs opened while it was set. pub static FAILING: Mutex> = Mutex::new(None); +/// Held by a test for as long as it sets `FAILING`, which one test sets at a time. +pub static EXCLUSIVE: Mutex<()> = Mutex::new(()); fn fire(fault: u8) -> bool { let fired = ARMED.fetch_and(!fault, Ordering::SeqCst) & fault != 0; diff --git a/crates/notebook/tests/sync_pages.rs b/crates/notebook/tests/sync_pages.rs index 2a3cfe0c4f954fbedbc933a15264b5f3bc09455d..0d923f95e82d9c1d4afcfa9ffdf33dbf9afb283b 100644 --- a/crates/notebook/tests/sync_pages.rs +++ b/crates/notebook/tests/sync_pages.rs @@ -745,8 +745,9 @@ fn page_moves_and_a_removed_page_merge_as_onenote_merges_them() { } /// A page a move copied leaves only as it was copied: an edit made here since, published or -/// not, or another writer's that a rebase took, keeps it; edits to other pages, published or -/// rebased, do not. Either way the queue reopens as it was left and publishes it. +/// not, another writer's that a rebase took, or a conflict page kept under it keeps it; edits +/// to other pages, published or rebased, do not. Either way the queue reopens as it was left +/// and publishes it. #[test] fn a_moved_page_leaves_only_as_it_was_copied() { // The first two pages holding text. @@ -774,7 +775,7 @@ fn a_moved_page_leaves_only_as_it_was_copied() { server.durable.clone_from(&server.visible); } type Between = fn(&Replica, &mut Server, [(ExGuid, ExGuid); 2]); - let schedules: [(&str, bool, Between); 5] = [ + let schedules: [(&str, bool, Between); 6] = [ ("unrelated", true, |cache, server, [_, other]| { here(cache, other); cache.sync_once(server).unwrap(); @@ -801,13 +802,33 @@ fn a_moved_page_leaves_only_as_it_was_copied() { cache.sync_once(server).unwrap(); }, ), + ( + "kept as a conflict page", + false, + |cache, _, [(moving, _), _]| { + let page = cache.page(moving).unwrap(); + let none = std::collections::BTreeSet::new(); + let kept = + notebook::session::conflict(moving, &page, &none, "Keeper", true).unwrap(); + let at = 133_000_000_000_000_001; + cache + .apply( + "Keeper", + onestore::op::Edit { + at, + ops: vec![kept], + }, + ) + .unwrap(); + }, + ), ]; for (schedule, leaves, between) in schedules { let directory = tempfile::tempdir().unwrap(); let path = directory.path().join("moving.sqlite"); let cache = Replica::create(&path, SOURCE).unwrap(); let mut server = Server::new(SOURCE); - let copied = cache.page(moving).unwrap(); + let copied = cache.moving(moving).unwrap(); between( &cache, &mut server, @@ -818,7 +839,7 @@ fn a_moved_page_leaves_only_as_it_was_copied() { at: 133_000_000_000_000_002, ops: vec![Op::Section(SectionOp::Delete(vec![moving]))], }; - let retired = cache.apply_unchanged("Mover", delete, moving, copied); + let retired = cache.retire("Mover", delete, copied); assert_eq!(retired.is_ok(), leaves, "{schedule}: {retired:?}"); assert!( leaves || matches!(retired, Err(notebook::Error::Rejected(_))), @@ -837,3 +858,38 @@ fn a_moved_page_leaves_only_as_it_was_copied() { assert!(cache.pending().unwrap().is_empty(), "{schedule}"); } } + +/// A version of a page deleted while the page moves keeps the page, which no copy carries +/// the versions of. +#[test] +fn a_moved_page_leaves_only_with_the_versions_it_had() { + let source = + include_bytes!("../../../corpus/page-versions/native/step-02/notebook/History.one"); + let directory = tempfile::tempdir().unwrap(); + let cache = Replica::create(directory.path().join("history.sqlite"), source).unwrap(); + let (moving, versions) = cache.versions().unwrap().remove(0); + let copied = cache.moving(moving).unwrap(); + section_op( + &cache, + SectionOp::DeleteVersions { + page: moving, + versions: vec![versions[0].context], + }, + ); + let delete = onestore::op::Edit { + at: 133_000_000_000_000_002, + ops: vec![Op::Section(SectionOp::Delete(vec![moving]))], + }; + let retired = cache.retire("Mover", delete, copied); + assert!( + matches!(retired, Err(notebook::Error::Rejected(_))), + "{retired:?}" + ); + assert!( + cache + .pages() + .unwrap() + .iter() + .any(|(space, ..)| *space == moving) + ); +} diff --git a/crates/snowbound/Cargo.toml b/crates/snowbound/Cargo.toml index 4efb2bd455b51cc9456bebba44fb890d56828f5d..1989f28adf6464a4a6c0c55eb5611eb5b34b479f 100644 --- a/crates/snowbound/Cargo.toml +++ b/crates/snowbound/Cargo.toml @@ -126,3 +126,5 @@ x11rb = "0.13.2" [dev-dependencies] # Draws the pictures of the video recording the tests store. image = { version = "0.25", default-features = false } +# Fails a replica's writes, as a full disk does (`notebook/tests/support/vfs.rs`). +rusqlite = { version = "=0.40.2", features = ["bundled", "backup"] } diff --git a/crates/snowbound/src/main.rs b/crates/snowbound/src/main.rs index dde03e34d266e7ebeeb17e74ba5e24835ad50e5a..9201203ff704050c79faf5df24f8a8d4583cdd6b 100644 --- a/crates/snowbound/src/main.rs +++ b/crates/snowbound/src/main.rs @@ -94,6 +94,10 @@ mod undo; mod unpack; mod unread; mod update; +/// Fails a replica's writes on demand, for tests of what a full disk does. +#[cfg(test)] +#[path = "../../notebook/tests/support/vfs.rs"] +mod vfs; #[cfg_attr(target_arch = "wasm32", path = "watch_web.rs")] mod watch; diff --git a/crates/snowbound/src/undo.rs b/crates/snowbound/src/undo.rs index 3dea7aa09b7f082e6227ebdc4d9a473e869cad19..022941d2f5a0220d65f8568aed46343b1d23a896 100644 --- a/crates/snowbound/src/undo.rs +++ b/crates/snowbound/src/undo.rs @@ -351,7 +351,8 @@ struct Site { /// A change to pages done: the change taking it back, the page to show, and pages gone. struct Applied { - undo: Change, + /// What takes it back; where a move only copied its page, why the page also stayed. + undo: Result, show: ExGuid, gone: Vec, } @@ -415,6 +416,19 @@ impl Site { entry_of(self.library.catalog()?, identity).map(|(_, path)| path) } + /// Closes `section`, at catalog `path`, once its edits are written; while some wait to be, + /// keeps it open, where they are tried again until they are. + async fn release(&self, path: &str, section: notebook::session::Section) { + match section.replica().written_async().await { + Ok(()) => { + if let Err(error) = section.close_async().await { + eprintln!("Synchronization stopped: {error}"); + } + } + Err(_) => self.library.keep(path, section), + } + } + /// Does `change` to the section's pages, `shown` the page open; `None` where what it /// changes is gone or holds what it did not make, which it leaves alone. async fn change( @@ -468,11 +482,11 @@ impl Site { } self.apply(ops).await?; Applied { - undo: Change::Restore { + undo: Ok(Change::Restore { pages: kept, added, created, - }, + }), show, gone: pages, } @@ -519,11 +533,11 @@ impl Site { self.library = Arc::new(self.library.with(notebook)); } Applied { - undo: Change::Delete { + undo: Ok(Change::Delete { pages: spaces.clone(), show: Some(shown), created, - }, + }), show: spaces[0], gone, } @@ -536,13 +550,13 @@ impl Site { self.apply(vec![Op::Section(SectionOp::Pages(edits))]) .await?; Applied { - undo: Change::Arrange { + undo: Ok(Change::Arrange { order: listed .iter() .map(|(space, _, level)| (*space, *level)) .collect(), moved, - }, + }), show, gone: Vec::new(), } @@ -554,11 +568,11 @@ impl Site { let op = crate::rename::retitled(&self.replica.page(page)?, page, to.clone())?; self.apply(vec![op]).await?; Applied { - undo: Change::Retitle { + undo: Ok(Change::Retitle { page, from: to, to: from, - }, + }), show: page, gone: Vec::new(), } @@ -567,8 +581,8 @@ impl Site { let (Some(from), Some(path)) = (at(page), self.section_path(target)) else { return Ok(None); }; - let sent = self.replica.page(page)?; - let import = notebook::session::moved(&sent, &self.author)?; + let sent = self.replica.moving(page)?; + let import = notebook::session::moved(sent.page(), &self.author)?; let arrived = imported(&import); let before = listed[from + 1..].first().map(|(space, ..)| *space); let level = listed[from].2; @@ -585,24 +599,29 @@ impl Site { }, ) .await?; - section.close_async().await?; + self.release(&path, section).await; let edit = Edit { at: crate::filetime(), ops, }; - self.replica - .apply_unchanged_async(&self.author, edit, page, sent) - .await?; - Applied { - undo: Change::Return { - page: arrived, - target, - before, - level, - added, + let retired = self.replica.retire_async(&self.author, edit, sent).await; + match retired { + Ok(_) => Applied { + undo: Ok(Change::Return { + page: arrived, + target, + before, + level, + added, + }), + show, + gone: vec![page], + }, + Err(error) => Applied { + undo: Err(crate::plain(&error, "page")), + show: page, + gone: Vec::new(), }, - show, - gone: vec![page], } } Change::Return { @@ -616,15 +635,12 @@ impl Site { return Ok(None); }; let section = self.library.open_async(&path, || {}).await?; - let sent = match section.pages()?.iter().any(|(listed, ..)| *listed == page) { - true => Some(section.page(page)?), - false => None, - }; - let Some(sent) = sent else { - section.close_async().await?; + if !(section.pages()?.iter()).any(|(listed, ..)| *listed == page) { + self.release(&path, section).await; return Ok(None); - }; - let import = notebook::session::moved(&sent, &self.author)?; + } + let sent = section.replica().moving(page)?; + let import = notebook::session::moved(sent.page(), &self.author)?; let back = imported(&import); let mut ops = vec![ import, @@ -640,13 +656,14 @@ impl Site { at: crate::filetime(), ops: vec![Op::Section(SectionOp::Delete(vec![page]))], }; - section + let retired = section .replica() - .apply_unchanged_async(&self.author, delete, page, sent) - .await?; - section.close_async().await?; + .retire_async(&self.author, delete, sent) + .await; + self.release(&path, section).await; Applied { - undo: Change::Send { page: back, target }, + undo: (retired.map(|_| Change::Send { page: back, target })) + .map_err(|error| crate::plain(&error, "page")), show: back, gone, } @@ -1152,12 +1169,17 @@ impl State { }; }; state.undo.forget(&applied.gone); - let undo = Action { - notebook, - section: Some(section), - change: applied.undo, - }; - state.undo.note(taken.map(|(redo, _)| redo), undo); + match applied.undo { + Ok(change) => { + let undo = Action { + notebook, + section: Some(section), + change, + }; + state.undo.note(taken.map(|(redo, _)| redo), undo); + } + Err(copied) => crate::platform::alert("Page kept in both sections", &copied), + } state.edited(Vec::new()); Ok(()) }))); @@ -1503,6 +1525,24 @@ mod tests { ) } + /// The titles of the section at `path`, opened again. + fn stored(&self, path: &str) -> Vec<(String, u32)> { + let section = self.library.open(path, || {}).unwrap(); + let titles = (section.pages().unwrap().into_iter()) + .map(|(_, title, level)| (title, level)) + .collect(); + section.close().unwrap(); + titles + } + + /// The replica of the section at `path`, which `vfs::FAILING` names. + fn replica(&self, path: &str) -> std::path::PathBuf { + let Ok(Some(notebook)) = &self.library.notebook else { + panic!("a notebook") + }; + notebook.replica_path(path).unwrap().canonicalize().unwrap() + } + /// Does `change`, as the app's thread does, returning what takes it back. fn change(&self, change: Change) -> Option { let shown = self.pages()[0].0; @@ -1554,7 +1594,7 @@ mod tests { assert_eq!(fixture.titles(), titled(&["First", "Third"])); assert_eq!(deleted.show, fixture.space("Third")); assert_eq!(fixture.binned(), ["Second"]); - let restored = pollster::block_on(site.change(deleted.undo, deleted.show)) + let restored = pollster::block_on(site.change(deleted.undo.unwrap(), deleted.show)) .unwrap() .unwrap(); assert!(!Arc::ptr_eq(&site.library, &deleted_library)); @@ -1562,19 +1602,19 @@ mod tests { assert_eq!(fixture.titles(), titled(&["First", "Second", "Third"])); assert_eq!(restored.show, fixture.space("Second")); assert!(fixture.binned().is_empty()); - let again = pollster::block_on(site.change(restored.undo, restored.show)) + let again = pollster::block_on(site.change(restored.undo.unwrap(), restored.show)) .unwrap() .unwrap(); assert_eq!(fixture.titles(), titled(&["First", "Third"])); assert_eq!(fixture.binned(), ["Second"]); // Put back elsewhere first, the page stays as that device left it. - let Change::Restore { pages, .. } = &again.undo else { + let Ok(Change::Restore { pages, .. }) = &again.undo else { panic!() }; let copy = notebook::session::moved(&pages[0].page, AUTHOR).unwrap(); fixture.edit(vec![copy]); - assert!(fixture.change(again.undo).is_none()); + assert!(fixture.change(again.undo.unwrap()).is_none()); assert_eq!(fixture.titles(), titled(&["First", "Third", "Second"])); } @@ -1598,13 +1638,13 @@ mod tests { assert_eq!(fixture.titles(), titled(&["First", "Second"])); assert_eq!(removed.show, first); assert!(fixture.binned().is_empty()); - let back = fixture.change(removed.undo).unwrap(); + let back = fixture.change(removed.undo.unwrap()).unwrap(); assert_eq!(fixture.titles(), titled(&["First", "Second", ""])); - let Change::Delete { pages, .. } = &back.undo else { + let Ok(Change::Delete { pages, .. }) = &back.undo else { panic!() }; fixture.retitle(pages[0], "Written elsewhere"); - assert!(fixture.change(back.undo).is_none()); + assert!(fixture.change(back.undo.unwrap()).is_none()); assert_eq!( fixture.titles(), titled(&["First", "Second", "Written elsewhere"]) @@ -1637,7 +1677,7 @@ mod tests { let arranged = fixture.change(arrange).unwrap(); assert_eq!(fixture.titles(), titled(&["A", "B", "C", "D"])); assert_eq!(arranged.show, a); - fixture.change(arranged.undo).unwrap(); + fixture.change(arranged.undo.unwrap()).unwrap(); fixture.edit(vec![Op::Section(SectionOp::Delete(vec![b]))]); fixture .change(Change::Arrange { @@ -1673,23 +1713,23 @@ mod tests { .unwrap(); assert_eq!(fixture.titles(), titled(&["First", "Third"])); assert_eq!(in_b(&fixture), ["", "Second"]); - let returned = fixture.change(sent.undo).unwrap(); + let returned = fixture.change(sent.undo.unwrap()).unwrap(); assert_eq!(fixture.titles(), titled(&["First", "Second", "Third"])); assert_eq!(in_b(&fixture), [""]); - let sent = fixture.change(returned.undo).unwrap(); - let Change::Return { page, .. } = &sent.undo else { + let sent = fixture.change(returned.undo.unwrap()).unwrap(); + let Ok(Change::Return { page, .. }) = &sent.undo else { panic!() }; let section = fixture.library.open("B.one", || {}).unwrap(); section.delete_pages(&[*page]).unwrap(); section.close().unwrap(); - assert!(fixture.change(sent.undo).is_none()); + assert!(fixture.change(sent.undo.unwrap()).is_none()); assert_eq!(fixture.titles(), titled(&["First", "Third"])); } /// A page edited while it is on its way to another section stays, with the edit, beside - /// the copy that arrived there; an edit to another page meanwhile lets it go. Both hold - /// once each section is opened again. + /// the copy that arrived there, with nothing for Undo to take back; an edit to another page + /// meanwhile lets it go. Both hold once each section is opened again. #[test] fn a_page_edited_while_it_is_sent_stays_with_its_edit() { struct Unreached; @@ -1731,41 +1771,106 @@ mod tests { let shown = fixture.pages()[0].0; let sending = std::thread::spawn(move || { pollster::block_on(site.change(Change::Send { page, target }, shown)) - .map(|applied| applied.is_some()) .map_err(|error| error.to_string()) }); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(20); while !(arrived.pages().unwrap().iter()).any(|(_, title, _)| title == "Second") { + assert!( + std::time::Instant::now() < deadline, + "the copy never arrived" + ); std::thread::sleep(std::time::Duration::from_millis(5)); } fixture.retitle(fixture.space(edited), &format!("{edited} edited")); drop(release); drop(arrived); - let sent = sending.join().unwrap(); + let sent = sending.join().unwrap().unwrap().unwrap(); let stayed = match edited { "Second" => { - assert!(sent.is_err(), "{sent:?}"); + assert!(sent.undo.is_err()); titled(&["First", "Second edited", "Third"]) } _ => { - assert_eq!(sent, Ok(true)); + assert!(sent.undo.is_ok()); titled(&["First", "Third edited"]) } }; fixture.section.take().unwrap().close().unwrap(); - let titles = |path: &str| { - let section = fixture.library.open(path, || {}).unwrap(); - let titles: Vec<(String, u32)> = (section.pages().unwrap().into_iter()) - .map(|(_, title, level)| (title, level)) - .collect(); - section.close().unwrap(); - titles - }; - assert_eq!(titles("New Section 1.one"), stayed); - assert_eq!(titles("B.one"), titled(&["", "Second"])); + assert_eq!(fixture.stored("New Section 1.one"), stayed); + assert_eq!(fixture.stored("B.one"), titled(&["", "Second"])); } } + use crate::vfs; + + /// A page Undo brings back stays in the section it was sent to as well where that one + /// cannot write, with nothing left for Redo; both hold once each is opened again. + #[test] + fn a_page_brought_back_where_the_cache_fails_stays_in_both_sections() { + vfs::install(); + let _exclusive = vfs::EXCLUSIVE + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let mut fixture = Fixture::new("return-fault", &["First", "Second", "Third"]); + let target = fixture.library.section_identity("B.one").unwrap(); + let sent = (fixture.change(Change::Send { + page: fixture.space("Second"), + target, + })) + .unwrap(); + *vfs::FAILING.lock().unwrap() = Some(fixture.replica("B.one")); + let back = fixture.change(sent.undo.unwrap()); + *vfs::FAILING.lock().unwrap() = None; + assert!(back.unwrap().undo.is_err()); + assert_eq!(fixture.titles(), titled(&["First", "Second", "Third"])); + fixture.section.take().unwrap().close().unwrap(); + let stayed = titled(&["First", "Second", "Third"]); + assert_eq!(fixture.stored("New Section 1.one"), stayed); + assert_eq!(fixture.stored("B.one"), titled(&["", "Second"])); + } + + /// A section a move leaves with edits waiting for the cache stays open, writing them once + /// it can, rather than closing without them. + #[test] + fn a_section_left_with_edits_waiting_keeps_them() { + vfs::install(); + let _exclusive = vfs::EXCLUSIVE + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let fixture = Fixture::new("release", &["First"]); + fixture + .library + .open("B.one", || {}) + .unwrap() + .close() + .unwrap(); + *vfs::FAILING.lock().unwrap() = Some(fixture.replica("B.one")); + let b = fixture.library.open("B.one", || {}).unwrap(); + let space = b.pages().unwrap()[0].0; + let op = crate::rename::retitled(&b.page(space).unwrap(), space, "Waiting".into()); + let edit = Edit { + at: crate::filetime(), + ops: vec![op.unwrap()], + }; + b.apply(AUTHOR, edit).unwrap(); + let replica = Arc::clone(b.replica()); + assert!(replica.written().is_err()); + pollster::block_on(fixture.site().release("B.one", b)); + *vfs::FAILING.lock().unwrap() = None; + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(20); + while replica.written().is_err() { + assert!( + std::time::Instant::now() < deadline, + "the edit was never written" + ); + std::thread::sleep(std::time::Duration::from_millis(50)); + } + drop(replica); + fixture.library.close_kept(); + assert_eq!(fixture.stored("B.one"), titled(&["Waiting"])); + } + /// A title given in the page list goes back while the page still has it. #[test] fn a_title_goes_back_while_the_page_has_it() { @@ -1780,7 +1885,7 @@ mod tests { .unwrap(); assert_eq!(fixture.titles(), titled(&["First", "Second"])); fixture.retitle(page, "Changed elsewhere"); - assert!(fixture.change(back.undo).is_none()); + assert!(fixture.change(back.undo.unwrap()).is_none()); } /// Carries out `change` as `manage::State::restructure` does. @@ -1886,7 +1991,7 @@ mod tests { let mut fixture = Fixture::new("export", &["First", "Second", "Third", "Fourth"]); let back = |fixture: &Fixture, change| { let done = fixture.change(change).unwrap(); - fixture.change(done.undo).unwrap() + fixture.change(done.undo.unwrap()).unwrap() }; back( &fixture, -- 2.54.0