From c062528557737298b32d47db84bb169b6544177e Mon Sep 17 00:00:00 2001 From: Emmanuel Leblond Date: Fri, 13 Sep 2024 15:50:43 +0200 Subject: [PATCH] Make `WorkspaceStore::retrieve_path_from_id` returns the entry manifest to improve concurrency hardening --- .../crates/client/src/workspace/store/mod.rs | 4 +- .../src/workspace/store/resolve_path.rs | 20 +++-- .../src/workspace/transactions/read_folder.rs | 46 +++------- .../src/workspace/transactions/stat_entry.rs | 89 ++++++++++--------- .../unit/workspace/retrieve_path_from_id.rs | 29 ++++-- 5 files changed, 89 insertions(+), 99 deletions(-) diff --git a/libparsec/crates/client/src/workspace/store/mod.rs b/libparsec/crates/client/src/workspace/store/mod.rs index 35097324d91..53164ae31e1 100644 --- a/libparsec/crates/client/src/workspace/store/mod.rs +++ b/libparsec/crates/client/src/workspace/store/mod.rs @@ -211,10 +211,10 @@ impl WorkspaceStore { resolve_path::resolve_path(self, path).await } - pub(crate) async fn retrieve_path_from_id( + pub async fn retrieve_path_from_id( &self, entry_id: VlobID, - ) -> Result<(FsPath, PathConfinementPoint), ResolvePathError> { + ) -> Result<(ArcLocalChildManifest, FsPath, PathConfinementPoint), ResolvePathError> { resolve_path::retrieve_path_from_id(self, entry_id).await } diff --git a/libparsec/crates/client/src/workspace/store/resolve_path.rs b/libparsec/crates/client/src/workspace/store/resolve_path.rs index e7c2f40774e..8274c3ca94d 100644 --- a/libparsec/crates/client/src/workspace/store/resolve_path.rs +++ b/libparsec/crates/client/src/workspace/store/resolve_path.rs @@ -660,7 +660,7 @@ pub(crate) async fn resolve_path_for_reparenting( } enum CacheOnlyRetrievalOutcome { - Done(FsPath, PathConfinementPoint), + Done((ArcLocalChildManifest, FsPath, PathConfinementPoint)), EntryNotFound, NeedPopulateCache(VlobID), } @@ -676,12 +676,14 @@ fn cache_only_retrieve_path_from_id( // Get the root ID let root_entry_id = cache.manifests.root_manifest().base.id; + let entry_manifest = match cache.manifests.get(&entry_id) { + Some(manifest) => manifest.clone(), + None => return CacheOnlyRetrievalOutcome::NeedPopulateCache(entry_id), + }; + // Initialize the loop state let mut current_entry_id = entry_id; - let mut current_parent_id = match cache.manifests.get(¤t_entry_id) { - Some(manifest) => manifest.parent(), - None => return CacheOnlyRetrievalOutcome::NeedPopulateCache(current_entry_id), - }; + let mut current_parent_id = entry_manifest.parent(); let mut seen: HashSet = HashSet::from_iter([current_entry_id]); // Loop until we reach the root @@ -731,14 +733,14 @@ fn cache_only_retrieve_path_from_id( // Reverse the parts to get the path parts.reverse(); - CacheOnlyRetrievalOutcome::Done(FsPath::from_parts(parts), confinement) + CacheOnlyRetrievalOutcome::Done((entry_manifest, FsPath::from_parts(parts), confinement)) } /// Retrieve the path and the confinement point of a given entry ID. pub(crate) async fn retrieve_path_from_id( store: &super::WorkspaceStore, entry_id: VlobID, -) -> Result<(FsPath, PathConfinementPoint), ResolvePathError> { +) -> Result<(ArcLocalChildManifest, FsPath, PathConfinementPoint), ResolvePathError> { loop { // Most of the time we should have each entry in the path already in the cache, // so we want to lock the cache once and only release it in the unlikely case @@ -748,8 +750,8 @@ pub(crate) async fn retrieve_path_from_id( cache_only_retrieve_path_from_id(&mut cache, entry_id) }; match cache_only_outcome { - CacheOnlyRetrievalOutcome::Done(path, confinement_point) => { - return Ok((path, confinement_point)) + CacheOnlyRetrievalOutcome::Done((entry_manifest, path, confinement_point)) => { + return Ok((entry_manifest, path, confinement_point)) } CacheOnlyRetrievalOutcome::EntryNotFound => { return Err(ResolvePathError::EntryNotFound) diff --git a/libparsec/crates/client/src/workspace/transactions/read_folder.rs b/libparsec/crates/client/src/workspace/transactions/read_folder.rs index 8dfe6d101bb..736929e9e46 100644 --- a/libparsec/crates/client/src/workspace/transactions/read_folder.rs +++ b/libparsec/crates/client/src/workspace/transactions/read_folder.rs @@ -8,10 +8,7 @@ use libparsec_types::prelude::*; use crate::{ certif::{InvalidCertificateError, InvalidKeysBundleError, InvalidManifestError}, workspace::{ - store::{ - EnsureManifestExistsWithParentError, GetManifestError, PathConfinementPoint, - ResolvePathError, - }, + store::{EnsureManifestExistsWithParentError, PathConfinementPoint, ResolvePathError}, EntryStat, WorkspaceOps, WorkspaceStatEntryError, }, }; @@ -229,48 +226,27 @@ pub async fn open_folder_reader_by_id( ops: &WorkspaceOps, entry_id: VlobID, ) -> Result { - let manifest = ops + let (manifest, _, confinement_point) = ops .store - .get_manifest(entry_id) + .retrieve_path_from_id(entry_id) .await .map_err(|err| match err { - GetManifestError::Offline => WorkspaceOpenFolderReaderError::Offline, - GetManifestError::Stopped => WorkspaceOpenFolderReaderError::Stopped, - GetManifestError::EntryNotFound => WorkspaceOpenFolderReaderError::EntryNotFound, - GetManifestError::NoRealmAccess => WorkspaceOpenFolderReaderError::NoRealmAccess, - GetManifestError::InvalidKeysBundle(err) => { + ResolvePathError::Offline => WorkspaceOpenFolderReaderError::Offline, + ResolvePathError::Stopped => WorkspaceOpenFolderReaderError::Stopped, + ResolvePathError::EntryNotFound => WorkspaceOpenFolderReaderError::EntryNotFound, + ResolvePathError::NoRealmAccess => WorkspaceOpenFolderReaderError::NoRealmAccess, + ResolvePathError::InvalidKeysBundle(err) => { WorkspaceOpenFolderReaderError::InvalidKeysBundle(err) } - GetManifestError::InvalidCertificate(err) => { + ResolvePathError::InvalidCertificate(err) => { WorkspaceOpenFolderReaderError::InvalidCertificate(err) } - GetManifestError::InvalidManifest(err) => { + ResolvePathError::InvalidManifest(err) => { WorkspaceOpenFolderReaderError::InvalidManifest(err) } - GetManifestError::Internal(err) => err.context("cannot get manifest").into(), + ResolvePathError::Internal(err) => err.context("cannot retrieve path").into(), })?; - let (_, confinement_point) = - ops.store - .retrieve_path_from_id(entry_id) - .await - .map_err(|err| match err { - ResolvePathError::Offline => WorkspaceOpenFolderReaderError::Offline, - ResolvePathError::Stopped => WorkspaceOpenFolderReaderError::Stopped, - ResolvePathError::EntryNotFound => WorkspaceOpenFolderReaderError::EntryNotFound, - ResolvePathError::NoRealmAccess => WorkspaceOpenFolderReaderError::NoRealmAccess, - ResolvePathError::InvalidKeysBundle(err) => { - WorkspaceOpenFolderReaderError::InvalidKeysBundle(err) - } - ResolvePathError::InvalidCertificate(err) => { - WorkspaceOpenFolderReaderError::InvalidCertificate(err) - } - ResolvePathError::InvalidManifest(err) => { - WorkspaceOpenFolderReaderError::InvalidManifest(err) - } - ResolvePathError::Internal(err) => err.context("cannot retrieve path").into(), - })?; - match manifest { ArcLocalChildManifest::Folder(manifest) => Ok(FolderReader { manifest, diff --git a/libparsec/crates/client/src/workspace/transactions/stat_entry.rs b/libparsec/crates/client/src/workspace/transactions/stat_entry.rs index a6409c44ff5..9724399745a 100644 --- a/libparsec/crates/client/src/workspace/transactions/stat_entry.rs +++ b/libparsec/crates/client/src/workspace/transactions/stat_entry.rs @@ -86,51 +86,52 @@ pub(crate) async fn stat_entry_by_id( entry_id: VlobID, maybe_known_confinement_point: Option, ) -> Result { - let manifest = ops - .store - .get_manifest(entry_id) - .await - .map_err(|err| match err { - GetManifestError::Offline => WorkspaceStatEntryError::Offline, - GetManifestError::Stopped => WorkspaceStatEntryError::Stopped, - GetManifestError::EntryNotFound => WorkspaceStatEntryError::EntryNotFound, - GetManifestError::NoRealmAccess => WorkspaceStatEntryError::NoRealmAccess, - GetManifestError::InvalidKeysBundle(err) => { - WorkspaceStatEntryError::InvalidKeysBundle(err) - } - GetManifestError::InvalidCertificate(err) => { - WorkspaceStatEntryError::InvalidCertificate(err) - } - GetManifestError::InvalidManifest(err) => WorkspaceStatEntryError::InvalidManifest(err), - GetManifestError::Internal(err) => err.context("cannot resolve path").into(), - })?; - - let confinement_point = match maybe_known_confinement_point { - Some(known_confinement_point) => known_confinement_point, + let (manifest, confinement_point) = match maybe_known_confinement_point { + Some(known_confinement_point) => { + let manifest = ops + .store + .get_manifest(entry_id) + .await + .map_err(|err| match err { + GetManifestError::Offline => WorkspaceStatEntryError::Offline, + GetManifestError::Stopped => WorkspaceStatEntryError::Stopped, + GetManifestError::EntryNotFound => WorkspaceStatEntryError::EntryNotFound, + GetManifestError::NoRealmAccess => WorkspaceStatEntryError::NoRealmAccess, + GetManifestError::InvalidKeysBundle(err) => { + WorkspaceStatEntryError::InvalidKeysBundle(err) + } + GetManifestError::InvalidCertificate(err) => { + WorkspaceStatEntryError::InvalidCertificate(err) + } + GetManifestError::InvalidManifest(err) => { + WorkspaceStatEntryError::InvalidManifest(err) + } + GetManifestError::Internal(err) => err.context("cannot resolve path").into(), + })?; + (manifest, known_confinement_point) + } None => { - let (_, confinement_point) = - ops.store - .retrieve_path_from_id(entry_id) - .await - .map_err(|err| match err { - ResolvePathError::Offline => WorkspaceStatEntryError::Offline, - ResolvePathError::Stopped => WorkspaceStatEntryError::Stopped, - ResolvePathError::EntryNotFound => WorkspaceStatEntryError::EntryNotFound, - ResolvePathError::NoRealmAccess => WorkspaceStatEntryError::NoRealmAccess, - ResolvePathError::InvalidKeysBundle(err) => { - WorkspaceStatEntryError::InvalidKeysBundle(err) - } - ResolvePathError::InvalidCertificate(err) => { - WorkspaceStatEntryError::InvalidCertificate(err) - } - ResolvePathError::InvalidManifest(err) => { - WorkspaceStatEntryError::InvalidManifest(err) - } - ResolvePathError::Internal(err) => { - err.context("cannot retrieve path").into() - } - })?; - confinement_point + let (manifest, _, confinement_point) = ops + .store + .retrieve_path_from_id(entry_id) + .await + .map_err(|err| match err { + ResolvePathError::Offline => WorkspaceStatEntryError::Offline, + ResolvePathError::Stopped => WorkspaceStatEntryError::Stopped, + ResolvePathError::EntryNotFound => WorkspaceStatEntryError::EntryNotFound, + ResolvePathError::NoRealmAccess => WorkspaceStatEntryError::NoRealmAccess, + ResolvePathError::InvalidKeysBundle(err) => { + WorkspaceStatEntryError::InvalidKeysBundle(err) + } + ResolvePathError::InvalidCertificate(err) => { + WorkspaceStatEntryError::InvalidCertificate(err) + } + ResolvePathError::InvalidManifest(err) => { + WorkspaceStatEntryError::InvalidManifest(err) + } + ResolvePathError::Internal(err) => err.context("cannot retrieve path").into(), + })?; + (manifest, confinement_point) } }; diff --git a/libparsec/crates/client/tests/unit/workspace/retrieve_path_from_id.rs b/libparsec/crates/client/tests/unit/workspace/retrieve_path_from_id.rs index e519e9f0f01..c7c51e8e385 100644 --- a/libparsec/crates/client/tests/unit/workspace/retrieve_path_from_id.rs +++ b/libparsec/crates/client/tests/unit/workspace/retrieve_path_from_id.rs @@ -21,23 +21,27 @@ async fn ok(env: &TestbedEnv) { let ops = workspace_ops_factory(&env.discriminant_dir, &alice, wksp1_id.to_owned()).await; // Resolve root - let (path, confinement) = ops.store.retrieve_path_from_id(wksp1_id).await.unwrap(); + let (manifest, path, confinement) = ops.store.retrieve_path_from_id(wksp1_id).await.unwrap(); p_assert_eq!(confinement, PathConfinementPoint::NotConfined); p_assert_eq!(path, "/".parse().unwrap()); + p_assert_matches!(manifest, ArcLocalChildManifest::Folder(manifest) if manifest.base.id == wksp1_id); // Resolve child folder - let (path, confinement) = ops.store.retrieve_path_from_id(wksp1_foo_id).await.unwrap(); + let (manifest, path, confinement) = + ops.store.retrieve_path_from_id(wksp1_foo_id).await.unwrap(); p_assert_eq!(confinement, PathConfinementPoint::NotConfined); p_assert_eq!(path, "/foo".parse().unwrap()); + p_assert_matches!(manifest, ArcLocalChildManifest::Folder(manifest) if manifest.base.id == wksp1_foo_id); // Resolve child file - let (path, confinement) = ops + let (manifest, path, confinement) = ops .store .retrieve_path_from_id(wksp1_foo_egg_txt_id) .await .unwrap(); p_assert_eq!(confinement, PathConfinementPoint::NotConfined); p_assert_eq!(path, "/foo/egg.txt".parse().unwrap()); + p_assert_matches!(manifest, ArcLocalChildManifest::File(manifest) if manifest.base.id == wksp1_foo_egg_txt_id); } #[parsec_test(testbed = "minimal_client_ready")] @@ -70,32 +74,37 @@ async fn ok_with_confinement(env: &TestbedEnv) { .unwrap(); // Resolve root - let (path, confinement) = ops.store.retrieve_path_from_id(wksp1_id).await.unwrap(); + let (manifest, path, confinement) = ops.store.retrieve_path_from_id(wksp1_id).await.unwrap(); p_assert_eq!(confinement, PathConfinementPoint::NotConfined); p_assert_eq!(path, "/".parse().unwrap()); + p_assert_matches!(manifest, ArcLocalChildManifest::Folder(manifest) if manifest.base.id == wksp1_id); // Resolve foo folder - let (path, confinement) = ops.store.retrieve_path_from_id(wksp1_foo_id).await.unwrap(); + let (manifest, path, confinement) = + ops.store.retrieve_path_from_id(wksp1_foo_id).await.unwrap(); p_assert_eq!(confinement, PathConfinementPoint::Confined(wksp1_id)); p_assert_eq!(path, "/foo.tmp".parse().unwrap()); + p_assert_matches!(manifest, ArcLocalChildManifest::Folder(manifest) if manifest.base.id == wksp1_foo_id); // Resolve spam folder - let (path, confinement) = ops + let (manifest, path, confinement) = ops .store .retrieve_path_from_id(wksp1_foo_spam_id) .await .unwrap(); p_assert_eq!(confinement, PathConfinementPoint::Confined(wksp1_id)); p_assert_eq!(path, "/foo.tmp/spam.tmp".parse().unwrap()); + p_assert_matches!(manifest, ArcLocalChildManifest::Folder(manifest) if manifest.base.id == wksp1_foo_spam_id); // Resolve egg file - let (path, confinement) = ops + let (manifest, path, confinement) = ops .store .retrieve_path_from_id(wksp1_foo_spam_egg_id) .await .unwrap(); p_assert_eq!(confinement, PathConfinementPoint::Confined(wksp1_id)); p_assert_eq!(path, "/foo.tmp/spam.tmp/egg.txt".parse().unwrap()); + p_assert_matches!(manifest, ArcLocalChildManifest::File(manifest) if manifest.base.id == wksp1_foo_spam_egg_id); } #[parsec_test(testbed = "minimal_client_ready", with_server)] @@ -155,13 +164,14 @@ async fn base_path_mismatch_is_ok(env: &TestbedEnv) { let alice = env.local_device("alice@dev1"); let ops = workspace_ops_factory(&env.discriminant_dir, &alice, wksp1_id.to_owned()).await; - let (path, confinement) = ops + let (manifest, path, confinement) = ops .store .retrieve_path_from_id(wksp1_bar_txt_id) .await .unwrap(); p_assert_eq!(confinement, PathConfinementPoint::NotConfined); p_assert_eq!(path, "/bar.txt".parse().unwrap()); + p_assert_matches!(manifest, ArcLocalChildManifest::File(manifest) if manifest.base.id == wksp1_bar_txt_id); } #[parsec_test(testbed = "minimal_client_ready", with_server)] @@ -256,13 +266,14 @@ async fn inconsistent_path_recursive_by_children( let alice = env.local_device("alice@dev1"); let ops = workspace_ops_factory(&env.discriminant_dir, &alice, wksp1_id.to_owned()).await; - let (path, confinement) = ops + let (manifest, path, confinement) = ops .store .retrieve_path_from_id(recursive_target_id) .await .unwrap(); p_assert_eq!(confinement, PathConfinementPoint::NotConfined); p_assert_eq!(path, expected_path); + p_assert_matches!(manifest, ArcLocalChildManifest::Folder(manifest) if manifest.base.id == recursive_target_id); } #[parsec_test(testbed = "minimal_client_ready")]