From fa9e6ddcd54cc72ee7b16ac120f715b710beea5d Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Fri, 7 May 2021 22:35:42 +0000 Subject: [PATCH 1/5] Don't unwrap in production code --- cli/src/invocation/cmd/sync.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/src/invocation/cmd/sync.rs b/cli/src/invocation/cmd/sync.rs index 9a1bb18d2..2e9400642 100644 --- a/cli/src/invocation/cmd/sync.rs +++ b/cli/src/invocation/cmd/sync.rs @@ -6,7 +6,7 @@ pub(crate) fn execute( replica: &mut Replica, server: &mut Box, ) -> anyhow::Result<()> { - replica.sync(server).unwrap(); + replica.sync(server)?; writeln!(w, "sync complete.")?; Ok(()) } From 3a2450cb231a0bf078b44c81f63e74268405fde6 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Fri, 7 May 2021 22:36:10 +0000 Subject: [PATCH 2/5] provide context for errors to help debugging --- taskchampion/src/replica.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/taskchampion/src/replica.rs b/taskchampion/src/replica.rs index ac1baea88..1e0a47382 100644 --- a/taskchampion/src/replica.rs +++ b/taskchampion/src/replica.rs @@ -4,6 +4,7 @@ use crate::storage::{Operation, Storage, TaskMap}; use crate::task::{Status, Task}; use crate::taskdb::TaskDb; use crate::workingset::WorkingSet; +use anyhow::Context; use chrono::Utc; use log::trace; use std::collections::HashMap; @@ -123,8 +124,10 @@ impl Replica { /// this occurs, but without renumbering, so any newly-pending tasks should appear in /// the working set. pub fn sync(&mut self, server: &mut Box) -> anyhow::Result<()> { - self.taskdb.sync(server)?; + self.taskdb.sync(server).context("Failed to synchronize")?; self.rebuild_working_set(false) + .context("Failed to rebuild working set after sync")?; + Ok(()) } /// Rebuild this replica's working set, based on whether tasks are pending or not. If From fa7623ebe741f177956ead740d8491769c5cac1b Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Fri, 7 May 2021 22:36:23 +0000 Subject: [PATCH 3/5] Handle setting a working set item to None twice Without this, setting an item to None that did not already exist failed, because the kv delete operation did not find the referenced key. This also checks that the index is not 0, which is not allowed as the working set is 1-indexed. --- taskchampion/src/storage/kv.rs | 84 +++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 2 deletions(-) diff --git a/taskchampion/src/storage/kv.rs b/taskchampion/src/storage/kv.rs index e65e31dfa..505e3f095 100644 --- a/taskchampion/src/storage/kv.rs +++ b/taskchampion/src/storage/kv.rs @@ -308,7 +308,7 @@ impl<'t> StorageTxn for Txn<'t> { Err(e) => return Err(e.into()), }; - if index >= next_index { + if index < 1 || index >= next_index { anyhow::bail!("Index {} is not in the working set", index); } @@ -319,7 +319,11 @@ impl<'t> StorageTxn for Txn<'t> { Msgpack::to_value_buf(uuid)?, )?; } else { - kvtxn.del(working_set_bucket, index.into())?; + match kvtxn.del(working_set_bucket, index.into()) { + Ok(_) => {} + Err(Error::NotFound) => {} + Err(e) => return Err(e.into()), + }; } Ok(()) @@ -650,6 +654,82 @@ mod test { Ok(()) } + #[test] + fn set_working_set_item() -> anyhow::Result<()> { + let tmp_dir = TempDir::new()?; + let mut storage = KvStorage::new(&tmp_dir.path())?; + let uuid1 = Uuid::new_v4(); + let uuid2 = Uuid::new_v4(); + + { + let mut txn = storage.txn()?; + txn.add_to_working_set(uuid1)?; + txn.add_to_working_set(uuid2)?; + txn.commit()?; + } + + { + let mut txn = storage.txn()?; + txn.set_working_set_item(1, Some(uuid2))?; + txn.set_working_set_item(2, None)?; + txn.commit()?; + } + + { + let mut txn = storage.txn()?; + let ws = txn.get_working_set()?; + assert_eq!(ws, vec![None, Some(uuid2), None]); + } + + Ok(()) + } + + #[test] + fn set_working_set_item_nonexistent() -> anyhow::Result<()> { + let tmp_dir = TempDir::new()?; + let mut storage = KvStorage::new(&tmp_dir.path())?; + let uuid1 = Uuid::new_v4(); + + { + let mut txn = storage.txn()?; + txn.add_to_working_set(uuid1)?; + txn.commit()?; + } + + { + let mut txn = storage.txn()?; + txn.set_working_set_item(1, None)?; + txn.commit()?; + } + + { + let mut txn = storage.txn()?; + // set it to None again, to check idempotency + txn.set_working_set_item(1, None)?; + txn.commit()?; + } + + { + let mut txn = storage.txn()?; + let ws = txn.get_working_set()?; + assert_eq!(ws, vec![None, None]); + } + + Ok(()) + } + + #[test] + fn set_working_set_item_zero() -> anyhow::Result<()> { + let tmp_dir = TempDir::new()?; + let mut storage = KvStorage::new(&tmp_dir.path())?; + let uuid1 = Uuid::new_v4(); + + let mut txn = storage.txn()?; + assert!(txn.set_working_set_item(0, Some(uuid1)).is_err()); + + Ok(()) + } + #[test] fn clear_working_set() -> anyhow::Result<()> { let tmp_dir = TempDir::new()?; From 5f6918fbc7097e5b891164ca2db2ab3cde752f05 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Fri, 7 May 2021 22:37:40 +0000 Subject: [PATCH 4/5] Skip element 0 when rebuilding the working set The existing code was correct, assuming that element 0 is always None, but this is clearer. --- taskchampion/src/taskdb.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/taskchampion/src/taskdb.rs b/taskchampion/src/taskdb.rs index 06c1ff6d9..ef5fb6c8d 100644 --- a/taskchampion/src/taskdb.rs +++ b/taskchampion/src/taskdb.rs @@ -117,14 +117,14 @@ impl TaskDb { { let mut txn = self.storage.txn()?; - let mut new_ws = vec![]; + let mut new_ws = vec![None]; // index 0 is always None let mut seen = HashSet::new(); // The goal here is for existing working-set items to be "compressed' down to index 1, so // we begin by scanning the current working set and inserting any tasks that should still // be in the set into new_ws, implicitly dropping any tasks that are no longer in the // working set. - for elt in txn.get_working_set()? { + for elt in txn.get_working_set()?.drain(1..) { if let Some(uuid) = elt { if let Some(task) = txn.get_task(uuid)? { if in_working_set(&task) { @@ -144,12 +144,12 @@ impl TaskDb { // if renumbering, clear the working set and re-add if renumber { txn.clear_working_set()?; - for uuid in new_ws.drain(0..new_ws.len()).flatten() { - txn.add_to_working_set(uuid)?; + for elt in new_ws.drain(1..new_ws.len()).flatten() { + txn.add_to_working_set(elt)?; } } else { // ..otherwise, just clear the None items determined above from the working set - for (i, elt) in new_ws.iter().enumerate() { + for (i, elt) in new_ws.iter().enumerate().skip(1) { if elt.is_none() { txn.set_working_set_item(i, None)?; } From 73b6648d06a181b52b597e6968217b5011462197 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Fri, 7 May 2021 22:40:48 +0000 Subject: [PATCH 5/5] assert that working-set element 0 is None --- taskchampion/src/workingset.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/taskchampion/src/workingset.rs b/taskchampion/src/workingset.rs index 586a16cf4..04aa4dcc5 100644 --- a/taskchampion/src/workingset.rs +++ b/taskchampion/src/workingset.rs @@ -21,6 +21,10 @@ impl WorkingSet { /// Create a new WorkingSet. Typically this is acquired via `replica.working_set()` pub(crate) fn new(by_index: Vec>) -> Self { let mut by_uuid = HashMap::new(); + + // working sets are 1-indexed, so element 0 should always be None + assert!(by_index.is_empty() || by_index[0].is_none()); + for (index, uuid) in by_index.iter().enumerate() { if let Some(uuid) = uuid { by_uuid.insert(*uuid, index);