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,