From f516f5767cae6e5ee770ab991f0d00eef9270d96 Mon Sep 17 00:00:00 2001 From: andrussal Date: Fri, 17 Jul 2026 07:26:11 +0200 Subject: [PATCH] refactor: address snapshot store review comments --- testing-framework/core/src/scenario/mod.rs | 2 +- .../core/src/scenario/snapshot/mod.rs | 138 +++++++++--------- .../core/src/scenario/snapshot/store.rs | 115 +++++++++++---- 3 files changed, 158 insertions(+), 97 deletions(-) diff --git a/testing-framework/core/src/scenario/mod.rs b/testing-framework/core/src/scenario/mod.rs index 848bbe7..5190dcc 100644 --- a/testing-framework/core/src/scenario/mod.rs +++ b/testing-framework/core/src/scenario/mod.rs @@ -50,7 +50,7 @@ pub use runtime::{ wait_http_readiness, wait_until_stable, }; pub use snapshot::{ - NodeStateSource, SnapshotArtifact, SnapshotContext, SnapshotExtension, SnapshotFactory, + NodeStateSource, SnapshotArtifact, SnapshotArtifactProvider, SnapshotContext, SnapshotFactory, SnapshotHandle, SnapshotManifest, SnapshotNodeStateAdapter, SnapshotSpec, SnapshotStore, }; pub use sources::{ diff --git a/testing-framework/core/src/scenario/snapshot/mod.rs b/testing-framework/core/src/scenario/snapshot/mod.rs index 9882bbd..476b6a2 100644 --- a/testing-framework/core/src/scenario/snapshot/mod.rs +++ b/testing-framework/core/src/scenario/snapshot/mod.rs @@ -19,8 +19,8 @@ pub use store::SnapshotStore; /// Logical name of a snapshot inside the configured snapshot store. pub type SnapshotName = String; -/// Stable identifier for an application-specific snapshot extension artifact. -pub type SnapshotExtensionId = String; +/// Stable identifier for an application-specific snapshot artifact provider. +pub type SnapshotArtifactProviderId = String; /// Stable node identifier used as the key for per-node state in a snapshot. pub type SnapshotNodeName = String; @@ -33,7 +33,7 @@ pub type SnapshotNodeName = String; #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] pub struct SnapshotSpec { include_node_state: bool, - extensions: BTreeSet, + providers: BTreeSet, } impl SnapshotSpec { @@ -42,24 +42,24 @@ impl SnapshotSpec { pub fn node_state() -> Self { Self { include_node_state: true, - extensions: BTreeSet::new(), + providers: BTreeSet::new(), } } - /// Include only one extension artifact. + /// Include only artifacts from one provider. #[must_use] - pub fn extension(extension: impl Into) -> Self { - Self::extensions([extension]) + pub fn provider(provider: impl Into) -> Self { + Self::providers([provider]) } - /// Include only the given extension artifacts. + /// Include only artifacts from the given providers. #[must_use] - pub fn extensions( - extensions: impl IntoIterator>, + pub fn providers( + providers: impl IntoIterator>, ) -> Self { Self { include_node_state: false, - extensions: extensions.into_iter().map(Into::into).collect(), + providers: providers.into_iter().map(Into::into).collect(), } } @@ -77,21 +77,21 @@ impl SnapshotSpec { self } - /// Return a copy of this spec with one additional extension artifact. + /// Return a copy of this spec with one additional artifact provider. #[must_use] - pub fn with_extension(mut self, extension: impl Into) -> Self { - self.extensions.insert(extension.into()); + pub fn with_provider(mut self, provider: impl Into) -> Self { + self.providers.insert(provider.into()); self } - /// Return a copy of this spec with the given extension artifacts. + /// Return a copy of this spec with the given artifact providers. #[must_use] - pub fn with_extensions( + pub fn with_providers( mut self, - extensions: impl IntoIterator>, + providers: impl IntoIterator>, ) -> Self { - for extension in extensions { - self = self.with_extension(extension); + for provider in providers { + self = self.with_provider(provider); } self } @@ -102,10 +102,10 @@ impl SnapshotSpec { self.include_node_state } - /// Whether this spec includes an extension artifact with the given id. + /// Whether this spec includes artifacts from the given provider id. #[must_use] - pub fn includes_extension(&self, id: &str) -> bool { - self.extensions.contains(id) + pub fn includes_provider(&self, id: &str) -> bool { + self.providers.contains(id) } } @@ -118,8 +118,8 @@ impl Default for SnapshotSpec { /// Source for one node's local state when preparing or restoring a runtime dir. /// /// This type is deliberately narrower than a whole snapshot reference. -/// Extension artifacts and full snapshot loads are addressed by snapshot name -/// through [`SnapshotHandle::load`]. Node startup/restore needs a concrete +/// Provider-owned artifacts and full snapshot loads are addressed by snapshot +/// name through [`SnapshotHandle::load`]. Node startup/restore needs a concrete /// directory, so a snapshot source must name both the snapshot and the source /// node. #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] @@ -184,8 +184,8 @@ impl SnapshotArtifact { /// Manifest file describing the contents of one snapshot. /// -/// Node state entries are keyed by node name. Extension entries are keyed by -/// extension id. The actual node files live beside the manifest in +/// Node state entries are keyed by node name. Provider-owned artifacts are +/// keyed by provider id. The actual node files live beside the manifest in /// deployer/application-specific directories. #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] pub struct SnapshotManifest { @@ -197,8 +197,8 @@ pub struct SnapshotManifest { pub created_unix_millis: u128, /// Per-node state artifacts saved by the deployer/node-state adapter. pub node_state: BTreeMap, - /// Application-specific extension artifacts. - pub extensions: BTreeMap, + /// Application-specific artifacts keyed by provider id. + pub providers: BTreeMap, } impl SnapshotManifest { @@ -212,12 +212,12 @@ impl SnapshotManifest { name: name.into(), created_unix_millis: unix_millis_now(), node_state: BTreeMap::new(), - extensions: BTreeMap::new(), + providers: BTreeMap::new(), } } } -/// Runtime context passed to snapshot extensions. +/// Runtime context passed to snapshot artifact providers. pub struct SnapshotContext { deployment: E::Deployment, node_clients: NodeClients, @@ -233,7 +233,7 @@ impl Clone for SnapshotContext { } impl SnapshotContext { - /// Create snapshot extension context from the active deployment and + /// Create snapshot artifact provider context from the active deployment and /// clients. #[must_use] pub fn new(deployment: E::Deployment, node_clients: NodeClients) -> Self { @@ -256,26 +256,26 @@ impl SnapshotContext { } } -/// Application-specific snapshot extension. +/// Application-specific snapshot artifact provider. /// -/// Extensions save and load artifacts that are not owned by the deployer/node +/// Providers save and load artifacts that are not owned by the deployer/node /// runtime itself, such as test harness wallet state. Implementations should be /// deterministic and treat `spec` as a filter for optional sub-parts they own. #[async_trait] -pub trait SnapshotExtension: Send + Sync { - /// Stable id used as the key in [`SnapshotManifest::extensions`]. +pub trait SnapshotArtifactProvider: Send + Sync { + /// Stable id used as the key in [`SnapshotManifest::providers`]. fn id(&self) -> &'static str; - /// Save this extension's artifact for the active snapshot operation. + /// Save this provider's artifact for the active snapshot operation. /// - /// Returning `Ok(None)` means this extension has no state to write. + /// Returning `Ok(None)` means this provider has no state to write. async fn save( &self, context: &SnapshotContext, spec: &SnapshotSpec, ) -> Result, DynError>; - /// Load this extension's artifact from the active snapshot operation. + /// Load this provider's artifact from the active snapshot operation. async fn load( &self, context: &SnapshotContext, @@ -327,44 +327,45 @@ pub trait SnapshotNodeStateAdapter: Send + Sync { async fn read_manifest(&self, snapshot: &str) -> Result; } -/// Prepared snapshot runtime extension for an active scenario. +/// Prepared snapshot handle for an active scenario. pub struct SnapshotHandle { node_state: Arc>, context: SnapshotContext, - extensions: Vec>>, + providers: Vec>>, _phantom: PhantomData, } /// Factory that installs snapshot support into a scenario runtime. pub struct SnapshotFactory { node_state: Arc>, - extensions: Vec>>, + providers: Vec>>, } impl SnapshotFactory { - /// Create a snapshot factory with a node-state adapter and no extensions. + /// Create a snapshot factory with a node-state adapter and no artifact + /// providers. #[must_use] pub fn new(node_state: Arc>) -> Self { Self { node_state, - extensions: Vec::new(), + providers: Vec::new(), } } - /// Register one application-specific snapshot extension. + /// Register one application-specific snapshot artifact provider. #[must_use] - pub fn with_extension(mut self, extension: Arc>) -> Self { - self.extensions.push(extension); + pub fn with_provider(mut self, provider: Arc>) -> Self { + self.providers.push(provider); self } - /// Register multiple application-specific snapshot extensions. + /// Register multiple application-specific snapshot artifact providers. #[must_use] - pub fn with_extensions( + pub fn with_providers( mut self, - extensions: impl IntoIterator>>, + providers: impl IntoIterator>>, ) -> Self { - self.extensions.extend(extensions); + self.providers.extend(providers); self } } @@ -379,7 +380,7 @@ impl RuntimeExtensionFactory for SnapshotFactory { Ok(PreparedRuntimeExtension::new(SnapshotHandle::new( Arc::clone(&self.node_state), SnapshotContext::new(deployment.clone(), node_clients), - self.extensions.clone(), + self.providers.clone(), ))) } } @@ -389,7 +390,7 @@ impl Clone for SnapshotHandle { Self { node_state: Arc::clone(&self.node_state), context: self.context.clone(), - extensions: self.extensions.clone(), + providers: self.providers.clone(), _phantom: PhantomData, } } @@ -401,12 +402,12 @@ impl SnapshotHandle { pub fn new( node_state: Arc>, context: SnapshotContext, - extensions: Vec>>, + providers: Vec>>, ) -> Self { Self { node_state, context, - extensions, + providers, _phantom: PhantomData, } } @@ -414,8 +415,9 @@ impl SnapshotHandle { /// Save a named snapshot according to `spec`. /// /// This writes a new manifest containing the requested node-state and - /// extension artifacts. Lifecycle concerns such as stopping nodes before - /// saving are intentionally owned by the caller/deployer, not this method. + /// provider-owned artifacts. Lifecycle concerns such as stopping nodes + /// before saving are intentionally owned by the caller/deployer, not + /// this method. pub async fn save( &self, name: impl Into, @@ -428,15 +430,15 @@ impl SnapshotHandle { manifest.node_state = self.node_state.save_node_state(&snapshot, &spec).await?; } - for extension in &self.extensions { - if !spec.includes_extension(extension.id()) { + for provider in &self.providers { + if !spec.includes_provider(provider.id()) { continue; } - if let Some(artifact) = extension.save(&self.context, &spec).await? { + if let Some(artifact) = provider.save(&self.context, &spec).await? { manifest - .extensions - .insert(extension.id().to_owned(), artifact); + .providers + .insert(provider.id().to_owned(), artifact); } } @@ -464,20 +466,20 @@ impl SnapshotHandle { .await?; } - for extension in &self.extensions { - if !spec.includes_extension(extension.id()) { + for provider in &self.providers { + if !spec.includes_provider(provider.id()) { continue; } - let Some(artifact) = manifest.extensions.get(extension.id()) else { + let Some(artifact) = manifest.providers.get(provider.id()) else { return Err(format!( - "snapshot '{snapshot}' does not contain extension '{}'", - extension.id() + "snapshot '{snapshot}' does not contain provider '{}'", + provider.id() ) .into()); }; - extension.load(&self.context, artifact, &spec).await?; + provider.load(&self.context, artifact, &spec).await?; } Ok(()) @@ -486,7 +488,7 @@ impl SnapshotHandle { /// Resolve one node's saved state to a local startup directory. /// /// This is used when a node process should be started from existing local - /// state. It does not load extension artifacts. + /// state. It does not load provider-owned artifacts. pub async fn prepare_node_state_source_dir( &self, source: NodeStateSource, diff --git a/testing-framework/core/src/scenario/snapshot/store.rs b/testing-framework/core/src/scenario/snapshot/store.rs index 9bba5ec..d258cf2 100644 --- a/testing-framework/core/src/scenario/snapshot/store.rs +++ b/testing-framework/core/src/scenario/snapshot/store.rs @@ -121,62 +121,72 @@ impl SnapshotStore { Ok(()) } - /// Save or replace one extension artifact in a named snapshot manifest. + /// Save or replace one provider artifact in a named snapshot manifest. /// /// If the snapshot manifest does not exist it is created. Invalid existing /// manifests fail the save instead of being overwritten. - pub fn save_extension_artifact( + pub fn save_provider_artifact( &self, snapshot_name: &str, - extension_id: &str, + provider_id: &str, artifact: SnapshotArtifact, ) -> Result { validate_path_component(snapshot_name, "Snapshot name")?; - validate_path_component(extension_id, "Snapshot extension id")?; + validate_path_component(provider_id, "Snapshot provider id")?; let mut manifest = self.read_manifest_or_new(snapshot_name)?; - manifest - .extensions - .insert(extension_id.to_owned(), artifact); + manifest.providers.insert(provider_id.to_owned(), artifact); self.write_manifest(&manifest)?; Ok(snapshot_name.to_owned()) } - /// Load one extension artifact from a named snapshot manifest. - pub fn load_extension_artifact( + /// Load one provider artifact from a named snapshot manifest. + pub fn load_provider_artifact( &self, snapshot_name: &str, - extension_id: &str, + provider_id: &str, ) -> Result { validate_path_component(snapshot_name, "Snapshot name")?; - validate_path_component(extension_id, "Snapshot extension id")?; + validate_path_component(provider_id, "Snapshot provider id")?; let manifest = self.read_manifest(snapshot_name)?; - manifest - .extensions - .get(extension_id) - .cloned() - .ok_or_else(|| { - format!("snapshot '{snapshot_name}' does not contain extension '{extension_id}'") - .into() - }) + manifest.providers.get(provider_id).cloned().ok_or_else(|| { + format!("snapshot '{snapshot_name}' does not contain provider '{provider_id}'").into() + }) } /// Read a snapshot manifest by snapshot name. pub fn read_manifest(&self, snapshot: &str) -> Result { - let bytes = fs::read(self.manifest_path(snapshot))?; - Ok(serde_json::from_slice(&bytes)?) + let path = self.manifest_path(snapshot); + let bytes = fs::read(&path).map_err(|error| { + io::Error::new( + error.kind(), + format!( + "failed to read snapshot manifest at '{}': {error}", + path.display() + ), + ) + })?; + parse_manifest(&path, &bytes) } fn read_manifest_or_new(&self, snapshot: &str) -> Result { - match fs::read(self.manifest_path(snapshot)) { - Ok(bytes) => Ok(serde_json::from_slice(&bytes)?), + let path = self.manifest_path(snapshot); + match fs::read(&path) { + Ok(bytes) => parse_manifest(&path, &bytes), Err(error) if error.kind() == ErrorKind::NotFound => { Ok(SnapshotManifest::new(snapshot)) } - Err(error) => Err(error.into()), + Err(error) => Err(io::Error::new( + error.kind(), + format!( + "failed to read snapshot manifest at '{}': {error}", + path.display() + ), + ) + .into()), } } @@ -236,6 +246,16 @@ fn ensure_subdirs(path: &Path, required_subdirs: &[&str]) -> Result<(), DynError Ok(()) } +fn parse_manifest(path: &Path, bytes: &[u8]) -> Result { + serde_json::from_slice(bytes).map_err(|error| { + format!( + "failed to parse snapshot manifest at '{}': {error}", + path.display() + ) + .into() + }) +} + fn clear_dir(path: &Path) -> io::Result<()> { if path.exists() { if !path.is_dir() { @@ -292,7 +312,7 @@ mod tests { } #[test] - fn node_state_and_extension_artifacts_share_manifest() { + fn node_state_and_provider_artifacts_share_manifest() { let root = TempDir::new().expect("snapshot root should be created"); let runtime = TempDir::new().expect("runtime dir should be created"); let store = SnapshotStore::new(root.path()); @@ -300,7 +320,7 @@ mod tests { write_file(&runtime.path().join("db").join("state.txt"), "db"); store - .save_extension_artifact( + .save_provider_artifact( "snapshot", "wallet", SnapshotArtifact::new( @@ -309,7 +329,7 @@ mod tests { serde_json::json!({ "wallets": ["WALLET_1A"] }), ), ) - .expect("extension artifact should save"); + .expect("provider artifact should save"); store .save_node_dirs("snapshot", "NODE_1", runtime.path(), &["db"]) @@ -322,7 +342,7 @@ mod tests { assert!(manifest.node_state.contains_key("NODE_1")); assert_eq!( manifest - .extensions + .providers .get("wallet") .expect("wallet artifact should remain in manifest") .metadata, @@ -337,6 +357,40 @@ mod tests { ); } + #[test] + fn external_directory_node_state_source_resolves_existing_state() { + let root = TempDir::new().expect("snapshot root should be created"); + let external = TempDir::new().expect("external dir should be created"); + let store = SnapshotStore::new(root.path()); + + fs::create_dir_all(external.path().join("db")).expect("db dir should be created"); + fs::create_dir_all(external.path().join("recovery")) + .expect("recovery dir should be created"); + + let source = NodeStateSource::external_directory(external.path()); + let resolved = store + .prepare_node_dir(&source, &["db", "recovery"]) + .expect("external source should resolve"); + + assert_eq!(resolved, external.path()); + } + + #[test] + fn external_directory_node_state_source_requires_state_subdirs() { + let root = TempDir::new().expect("snapshot root should be created"); + let external = TempDir::new().expect("external dir should be created"); + let store = SnapshotStore::new(root.path()); + + fs::create_dir_all(external.path().join("db")).expect("db dir should be created"); + + let source = NodeStateSource::external_directory(external.path()); + let error = store + .prepare_node_dir(&source, &["db", "recovery"]) + .expect_err("missing required state dir should fail"); + + assert!(error.to_string().contains("recovery")); + } + #[test] fn saving_node_state_does_not_overwrite_invalid_manifest() { let root = TempDir::new().expect("snapshot root should be created"); @@ -350,6 +404,11 @@ mod tests { .save_node_dirs("snapshot", "NODE_1", runtime.path(), &["db"]) .expect_err("invalid manifest should fail the save"); + assert!( + error + .to_string() + .contains("failed to parse snapshot manifest") + ); assert!(error.to_string().contains("expected ident")); assert_eq!( fs::read_to_string(store.manifest_path("snapshot"))