diff --git a/crates/notebook/src/session.rs b/crates/notebook/src/session.rs index 3c328ab8be4e27e443b65ab3f5e7d566bffd587c..579de4c8ba6956a8fa98fb6a7c8645b516ffbb69 100644 --- a/crates/notebook/src/session.rs +++ b/crates/notebook/src/session.rs @@ -114,6 +114,31 @@ pub trait Storage: Send + Sync { fn supersede(&self, path: &str, base: &Stamp, with: &str) -> Result<()>; } +struct Publication { + path: String, + transaction: Transaction, +} + +impl Publication { + async fn commit(self, storage: &dyn Storage) -> Result<()> { + match storage.commit(&self.path, &self.transaction) { + Err(Error::Remote(error)) + if error.state == onestore::CommitState::Unknown + && error.error.kind() == io::ErrorKind::WouldBlock => + { + let expected = self.transaction.stamp(); + fs::durable().await.map_err(|error| CommitError { + state: onestore::CommitState::Unknown, + error, + })?; + storage.confirm(&self.path, &expected)?; + Ok(()) + } + result => result, + } + } +} + /// A mounted notebook directory. struct Directory(PathBuf); @@ -687,6 +712,17 @@ impl Notebook { /// Commits `edits` to the TOC at `toc`. A name it still lists for a file gone from its /// folder passes to the file an edit gives that name; the stale entry goes. fn commit_toc(&self, toc: &str, edits: &[onestore::TocEdit]) -> Result<()> { + match self.toc_publication(toc, edits)? { + Some(commit) => self.storage.commit(&commit.path, &commit.transaction), + None => Ok(()), + } + } + + fn toc_publication( + &self, + toc: &str, + edits: &[onestore::TocEdit], + ) -> Result> { let unresolved = self .folder(split(toc).0) .ok() @@ -714,10 +750,14 @@ impl Notebook { superseded.push(edit.clone()); } let source = self.storage.read(toc)?; - match onestore::edit_table_of_contents(&source, &superseded)? { - Some(transaction) => self.storage.commit(toc, &transaction), - None => Ok(()), - } + Ok( + onestore::edit_table_of_contents(&source, &superseded)?.map(|transaction| { + Publication { + path: toc.into(), + transaction, + } + }), + ) } /// Creates `name.one` in `folder` holding the page `page` creates, in the next of @@ -1016,6 +1056,14 @@ impl Notebook { /// The recycle bin's TOC path and file identity, creating the bin as OneNote does: a /// section group the root TOC lists. A bin the root TOC misses is listed again. fn bin(&self) -> Result<(String, [u8; 16])> { + let (toc, identity, publication) = self.prepare_bin()?; + if let Some(commit) = publication { + self.storage.commit(&commit.path, &commit.transaction)?; + } + Ok((toc, identity)) + } + + fn prepare_bin(&self) -> Result<(String, [u8; 16], Option)> { if let Some(entry) = self .catalog .unavailable @@ -1046,18 +1094,20 @@ impl Notebook { .file_id; let (root, root_identity) = self.toc("")?; // OneNote takes a TOC placed under another parent for a new one. - if !lists(&self.storage.read(&root)?, identity)? { + let publication = if !lists(&self.storage.read(&root)?, identity)? { self.storage.place(&bin_toc, root_identity, RECYCLE_BIN)?; - self.commit_toc( + self.toc_publication( &root, &[onestore::TocEdit::Add { filename: RECYCLE_BIN.into(), identity, group: true, }], - )?; - } - Ok((bin_toc, identity)) + )? + } else { + None + }; + Ok((bin_toc, identity, publication)) } /// Moves the section file at `path`, its folder's `entry`, into the recycle bin, under a @@ -1093,8 +1143,18 @@ impl Notebook { /// level with its identity, title, date and creation time. The caller then deletes the /// pages from their section. pub fn recycle_pages(&mut self, pages: &[Page], author: &str) -> Result<()> { + crate::task::blocking(self.recycle_pages_async(pages, author))? + } + + /// Recycles pages after the browser host acknowledges each destination append. + pub async fn recycle_pages_async(&mut self, pages: &[Page], author: &str) -> Result<()> { const DELETED: &str = "OneNote_DeletedPages.one"; - let (bin_toc, bin_identity) = self.bin()?; + // A rejected host receipt leaves its append queued ahead of this retry. + fs::durable().await?; + let (bin_toc, bin_identity, publication) = self.prepare_bin()?; + if let Some(commit) = publication { + commit.commit(self.storage.as_ref()).await?; + } let path = catalog_path(RECYCLE_BIN, DELETED); if !self.storage.exists(&path) { // OneNote's "Deleted Pages" section is grey. @@ -1106,32 +1166,51 @@ impl Notebook { let identity = onestore::Store::parse(&bytes)?.header.file_id; if !lists(&self.storage.read(&bin_toc)?, identity)? { self.storage.place(&path, bin_identity, DELETED)?; - self.commit_toc( + if let Some(commit) = self.toc_publication( &bin_toc, &[onestore::TocEdit::Add { filename: DELETED.into(), identity, group: false, }], - )?; + )? { + commit.commit(self.storage.as_ref()).await?; + } bytes = self.storage.read(&path)?; } - let arena = onestore::Arena::default(); - let mut section = onestore::Section::open(&arena, bytes)?; - let mut ops = Vec::new(); - for page in pages { - ops.push(moved(page, author)?); - } - section.apply( - author, - &Edit { - at: crate::now(), - ops, - }, - )?; - if let Some(transaction) = section.seal()? { - self.storage.commit(&path, &transaction)?; + let transaction = { + let arena = onestore::Arena::default(); + let mut section = onestore::Section::open(&arena, bytes)?; + let mut replaced = Vec::new(); + for (space, ..) in section.pages()? { + if section.page(space)?.identity.is_some_and(|identity| { + pages.iter().any(|page| page.identity == Some(identity)) + }) { + replaced.push(space); + } + } + let mut ops = Vec::new(); + if !replaced.is_empty() { + ops.push(Op::Section(SectionOp::Delete(replaced))); + } + for page in pages { + ops.push(moved(page, author)?); + } + section.apply( + author, + &Edit { + at: crate::now(), + ops, + }, + )?; + section.seal()? + }; + if let Some(transaction) = transaction { + Publication { path, transaction } + .commit(self.storage.as_ref()) + .await?; } + fs::durable().await?; self.refresh().map(drop) } @@ -1139,6 +1218,12 @@ impl Notebook { /// revision, as OneNote 2010 does when Undo brings deleted pages back. Pages not there /// are passed over. pub fn unrecycle_pages(&mut self, identities: &[[u8; 16]]) -> Result<()> { + crate::task::blocking(self.unrecycle_pages_async(identities))? + } + + /// Removes recycled pages after the browser host acknowledges their deletion append. + pub async fn unrecycle_pages_async(&mut self, identities: &[[u8; 16]]) -> Result<()> { + fs::durable().await?; let path = catalog_path(RECYCLE_BIN, "OneNote_DeletedPages.one"); if !self.storage.exists(&path) { return Ok(()); @@ -1157,18 +1242,24 @@ impl Notebook { if binned.is_empty() { return Ok(()); } - let arena = onestore::Arena::default(); - let mut section = onestore::Section::open(&arena, bytes)?; - section.apply( - "", - &Edit { - at: crate::now(), - ops: vec![Op::Section(SectionOp::Delete(binned))], - }, - )?; - if let Some(transaction) = section.seal()? { - self.storage.commit(&path, &transaction)?; + let transaction = { + let arena = onestore::Arena::default(); + let mut section = onestore::Section::open(&arena, bytes)?; + section.apply( + "", + &Edit { + at: crate::now(), + ops: vec![Op::Section(SectionOp::Delete(binned))], + }, + )?; + section.seal()? + }; + if let Some(transaction) = transaction { + Publication { path, transaction } + .commit(self.storage.as_ref()) + .await?; } + fs::durable().await?; self.refresh().map(drop) } diff --git a/crates/notebook/src/session/tests.rs b/crates/notebook/src/session/tests.rs index e7b57b11c19e252624f3c95d9dcad4dd70e0a33a..b2bddeb3bc994821899eedcd8be449563352c5c5 100644 --- a/crates/notebook/src/session/tests.rs +++ b/crates/notebook/src/session/tests.rs @@ -73,14 +73,20 @@ fn renaming_an_unlisted_section_creates_its_toc_entry_and_preserves_its_neighbor } } -/// A mounted notebook where another writer commits `revision` to the section file just -/// before Snowbound's rewritten file takes its place. -struct Racing { +struct FaultStorage { inner: Directory, revision: Mutex>, + receipt: Option>>, } -impl Racing { +#[derive(Default)] +struct Receipt { + reject: Option, + reject_confirmation: Option, + confirmed: Vec, +} + +impl FaultStorage { fn race(&self, path: &str) { let revision = self.revision.lock().unwrap().take(); if let Some((section, transaction)) = revision { @@ -90,7 +96,7 @@ impl Racing { } } -impl Storage for Racing { +impl Storage for FaultStorage { fn discover( &self, cache: &mut discover::Cache, @@ -141,9 +147,29 @@ impl Storage for Racing { self.inner.place(path, ancestor, name) } fn commit(&self, path: &str, transaction: &Transaction) -> Result<()> { + if let Some(receipt) = &self.receipt { + if receipt.lock().unwrap().reject.as_deref() != Some(path) { + self.inner.commit(path, transaction)?; + } + return Err(CommitError { + state: onestore::CommitState::Unknown, + error: io::ErrorKind::WouldBlock.into(), + } + .into()); + } self.inner.commit(path, transaction) } fn confirm(&self, path: &str, base: &Stamp) -> std::result::Result<(), CommitError> { + if let Some(receipt) = &self.receipt { + let mut receipt = receipt.lock().unwrap(); + receipt.confirmed.push(path.into()); + if receipt.reject_confirmation.as_deref() == Some(path) { + return Err(CommitError { + state: onestore::CommitState::Unknown, + error: io::ErrorKind::PermissionDenied.into(), + }); + } + } self.inner.confirm(path, base) } fn supersede(&self, path: &str, base: &Stamp, with: &str) -> Result<()> { @@ -183,9 +209,10 @@ fn a_password_never_overwrites_a_concurrent_revision() { let revision = section.seal().unwrap().unwrap(); let root = folder.canonicalize().unwrap(); - let storage = Racing { + let storage = FaultStorage { inner: Directory(root.clone()), revision: Mutex::new(Some((path.clone(), revision))), + receipt: None, }; let mut notebook = Notebook::with(Box::new(storage), Some(root), &cache).unwrap(); let set = notebook.set_password(&path, None, Some("secret")); @@ -206,3 +233,152 @@ fn a_password_never_overwrites_a_concurrent_revision() { Err(_) => assert_eq!(titles(&stored), ["Mine", "Theirs"]), } } + +#[test] +fn recycling_confirms_uncertain_toc_and_page_appends_and_undo() { + let temporary = tempfile::tempdir().unwrap(); + let folder = temporary.path().join("notebook"); + let cache = temporary.path().join("cache"); + let creation = PageCreation::new(None, Some("Keep this page"), "Fixture").unwrap(); + let notebook = Notebook::create(&folder, &cache, Notebook::NEW_COLOR, &creation).unwrap(); + let path = notebook.catalog().sections[0].path.clone(); + let source = notebook.read_section(&path).unwrap(); + let page = stored_pages(&source).unwrap().remove(0).page; + drop(notebook); + let root = folder.canonicalize().unwrap(); + let receipt = Arc::new(Mutex::new(Receipt::default())); + let storage = FaultStorage { + inner: Directory(root.clone()), + revision: Mutex::new(None), + receipt: Some(receipt.clone()), + }; + let mut notebook = Notebook::with(Box::new(storage), Some(root), &cache).unwrap(); + notebook + .recycle_pages(std::slice::from_ref(&page), "Fixture") + .unwrap(); + let deleted = catalog_path(RECYCLE_BIN, "OneNote_DeletedPages.one"); + assert_eq!( + receipt.lock().unwrap().confirmed, + [ + TOC.to_string(), + catalog_path(RECYCLE_BIN, TOC), + deleted.clone(), + ] + ); + let binned = fs::read(folder.join(&deleted)).unwrap(); + let pages = stored_pages(&binned).unwrap(); + assert_eq!(pages.len(), 1); + assert_eq!(pages[0].page.title, page.title); + assert_eq!(pages[0].page.identity, page.identity); + assert_eq!(pages[0].page.created, page.created); + assert_eq!(pages[0].page.date_text(), page.date_text()); + let cold = Notebook::open(&folder, temporary.path().join("cold-cache")).unwrap(); + assert_eq!(cold.catalog().groups.len(), 1); + assert_eq!(cold.folder(RECYCLE_BIN).unwrap().sections.len(), 1); + assert_eq!( + stored_pages(&cold.read_section(&deleted).unwrap()) + .unwrap() + .len(), + 1 + ); + drop(cold); + notebook.unrecycle_pages(&[page.identity.unwrap()]).unwrap(); + let restored = fs::read(folder.join(&deleted)).unwrap(); + assert!( + restored.len() > binned.len(), + "Undo must append its deletion revision" + ); + assert!(stored_pages(&restored).unwrap().is_empty()); + assert_eq!(receipt.lock().unwrap().confirmed.last(), Some(&deleted)); + assert_eq!(receipt.lock().unwrap().confirmed.len(), 4); + assert_eq!(fs::read(folder.join(&path)).unwrap(), source); +} + +#[test] +fn recycling_rejects_unconfirmed_appends_and_can_retry_each_stage() { + let deleted = catalog_path(RECYCLE_BIN, "OneNote_DeletedPages.one"); + for (rejected, retained) in [ + (TOC.to_string(), false), + (catalog_path(RECYCLE_BIN, TOC), false), + (deleted.clone(), false), + (deleted.clone(), true), + ] { + let temporary = tempfile::tempdir().unwrap(); + let folder = temporary.path().join("notebook"); + let cache = temporary.path().join("cache"); + let creation = PageCreation::new(None, Some("Keep this page"), "Fixture").unwrap(); + let notebook = Notebook::create(&folder, &cache, Notebook::NEW_COLOR, &creation).unwrap(); + let path = notebook.catalog().sections[0].path.clone(); + let mut source = notebook.read_section(&path).unwrap(); + let mut page = stored_pages(&source).unwrap().remove(0).page; + drop(notebook); + let root = folder.canonicalize().unwrap(); + let receipt = Arc::new(Mutex::new(Receipt { + reject: (!retained).then(|| rejected.clone()), + reject_confirmation: retained.then(|| rejected.clone()), + confirmed: Vec::new(), + })); + let storage = FaultStorage { + inner: Directory(root.clone()), + revision: Mutex::new(None), + receipt: Some(receipt.clone()), + }; + let mut notebook = Notebook::with(Box::new(storage), Some(root), &cache).unwrap(); + assert!( + matches!( + notebook.recycle_pages(std::slice::from_ref(&page), "Fixture"), + Err(Error::Remote(_)) + ), + "{rejected}" + ); + assert_eq!(receipt.lock().unwrap().confirmed.last(), Some(&rejected)); + assert_eq!(fs::read(folder.join(&path)).unwrap(), source); + if folder.join(&deleted).exists() { + assert_eq!( + stored_pages(&fs::read(folder.join(&deleted)).unwrap()) + .unwrap() + .len(), + usize::from(retained) + ); + } + if retained { + let arena = onestore::Arena::default(); + let mut section = onestore::Section::open(&arena, source).unwrap(); + let space = section.pages().unwrap()[0].0; + section + .apply( + "Fixture", + &Edit { + at: crate::now(), + ops: vec![Op::Page { + space, + op: onestore::op::PageOp::Color(Some(0xabcdef)), + }], + }, + ) + .unwrap(); + section + .seal() + .unwrap() + .unwrap() + .commit_file(folder.join(&path)) + .unwrap(); + source = fs::read(folder.join(&path)).unwrap(); + page = stored_pages(&source).unwrap().remove(0).page; + } + { + let mut receipt = receipt.lock().unwrap(); + receipt.reject = None; + receipt.reject_confirmation = None; + } + notebook + .recycle_pages(std::slice::from_ref(&page), "Fixture") + .unwrap(); + let binned = stored_pages(&fs::read(folder.join(&deleted)).unwrap()).unwrap(); + assert_eq!(binned.len(), 1, "{rejected}"); + assert_eq!(binned[0].page.title, page.title); + assert_eq!(binned[0].page.identity, page.identity); + assert_eq!(binned[0].page.color, page.color); + assert_eq!(fs::read(folder.join(&path)).unwrap(), source); + } +} diff --git a/crates/onestore/src/commit.rs b/crates/onestore/src/commit.rs index 003324d821df2ab3894c062d1068361868a2c167..5f74df8f09a5264948eaf666b01272e8b0f29554 100644 --- a/crates/onestore/src/commit.rs +++ b/crates/onestore/src/commit.rs @@ -529,6 +529,14 @@ impl Transaction { &self.base } + /// The committed image's header and length, for confirmation after an uncertain answer. + pub fn stamp(&self) -> Stamp { + Stamp { + header: self.header, + length: self.base.length + self.append.len() as u64, + } + } + /// The transaction as bytes `from_bytes` reads back, to carry it to another machine to /// commit: the base header and length, the new header, the appended bytes' length and /// the bytes, then each patch's offset, length and bytes. Integers are little-endian. @@ -715,6 +723,7 @@ mod tests { let mut together = original; combined.apply(&mut together).unwrap(); assert_eq!(together, separate); + assert_eq!(combined.stamp(), Stamp::of(&together).unwrap()); } #[test] diff --git a/crates/snowbound/src/undo.rs b/crates/snowbound/src/undo.rs index a89422a38ae288a4e7507835c52bc4de3c91fb8f..7760f835127ba86ff52304ce99191c68015c7cfa 100644 --- a/crates/snowbound/src/undo.rs +++ b/crates/snowbound/src/undo.rs @@ -454,7 +454,10 @@ impl Site { let (show, added) = self.after(&listed, &pages, show, &mut ops); if !created && binned { let pages: Vec = kept.iter().map(|kept| kept.page.clone()).collect(); - self.library.reopen()?.recycle_pages(&pages, &self.author)?; + self.library + .reopen()? + .recycle_pages_async(&pages, &self.author) + .await?; self.replica.written_async().await?; } self.apply(ops).await?; @@ -505,7 +508,10 @@ impl Site { if !created && binned { let identities: Vec<[u8; 16]> = back.iter().filter_map(|kept| kept.page.identity).collect(); - self.library.reopen()?.unrecycle_pages(&identities)?; + self.library + .reopen()? + .unrecycle_pages_async(&identities) + .await?; } Applied { undo: Change::Delete {