refactor: address snapshot store review comments

This commit is contained in:
andrussal 2026-07-17 07:26:11 +02:00
parent cbd367a546
commit f516f5767c
3 changed files with 158 additions and 97 deletions

View File

@ -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::{

View File

@ -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<SnapshotExtensionId>,
providers: BTreeSet<SnapshotArtifactProviderId>,
}
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<SnapshotExtensionId>) -> Self {
Self::extensions([extension])
pub fn provider(provider: impl Into<SnapshotArtifactProviderId>) -> 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<Item = impl Into<SnapshotExtensionId>>,
pub fn providers(
providers: impl IntoIterator<Item = impl Into<SnapshotArtifactProviderId>>,
) -> 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<SnapshotExtensionId>) -> Self {
self.extensions.insert(extension.into());
pub fn with_provider(mut self, provider: impl Into<SnapshotArtifactProviderId>) -> 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<Item = impl Into<SnapshotExtensionId>>,
providers: impl IntoIterator<Item = impl Into<SnapshotArtifactProviderId>>,
) -> 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<SnapshotNodeName, SnapshotArtifact>,
/// Application-specific extension artifacts.
pub extensions: BTreeMap<SnapshotExtensionId, SnapshotArtifact>,
/// Application-specific artifacts keyed by provider id.
pub providers: BTreeMap<SnapshotArtifactProviderId, SnapshotArtifact>,
}
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<E: Application> {
deployment: E::Deployment,
node_clients: NodeClients<E>,
@ -233,7 +233,7 @@ impl<E: Application> Clone for SnapshotContext<E> {
}
impl<E: Application> SnapshotContext<E> {
/// 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<E>) -> Self {
@ -256,26 +256,26 @@ impl<E: Application> SnapshotContext<E> {
}
}
/// 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<E: Application>: Send + Sync {
/// Stable id used as the key in [`SnapshotManifest::extensions`].
pub trait SnapshotArtifactProvider<E: Application>: 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<E>,
spec: &SnapshotSpec,
) -> Result<Option<SnapshotArtifact>, 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<E>,
@ -327,44 +327,45 @@ pub trait SnapshotNodeStateAdapter<E: Application>: Send + Sync {
async fn read_manifest(&self, snapshot: &str) -> Result<SnapshotManifest, DynError>;
}
/// Prepared snapshot runtime extension for an active scenario.
/// Prepared snapshot handle for an active scenario.
pub struct SnapshotHandle<E: Application> {
node_state: Arc<dyn SnapshotNodeStateAdapter<E>>,
context: SnapshotContext<E>,
extensions: Vec<Arc<dyn SnapshotExtension<E>>>,
providers: Vec<Arc<dyn SnapshotArtifactProvider<E>>>,
_phantom: PhantomData<E>,
}
/// Factory that installs snapshot support into a scenario runtime.
pub struct SnapshotFactory<E: Application> {
node_state: Arc<dyn SnapshotNodeStateAdapter<E>>,
extensions: Vec<Arc<dyn SnapshotExtension<E>>>,
providers: Vec<Arc<dyn SnapshotArtifactProvider<E>>>,
}
impl<E: Application> SnapshotFactory<E> {
/// 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<dyn SnapshotNodeStateAdapter<E>>) -> 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<dyn SnapshotExtension<E>>) -> Self {
self.extensions.push(extension);
pub fn with_provider(mut self, provider: Arc<dyn SnapshotArtifactProvider<E>>) -> 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<Item = Arc<dyn SnapshotExtension<E>>>,
providers: impl IntoIterator<Item = Arc<dyn SnapshotArtifactProvider<E>>>,
) -> Self {
self.extensions.extend(extensions);
self.providers.extend(providers);
self
}
}
@ -379,7 +380,7 @@ impl<E: Application> RuntimeExtensionFactory<E> for SnapshotFactory<E> {
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<E: Application> Clone for SnapshotHandle<E> {
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<E: Application> SnapshotHandle<E> {
pub fn new(
node_state: Arc<dyn SnapshotNodeStateAdapter<E>>,
context: SnapshotContext<E>,
extensions: Vec<Arc<dyn SnapshotExtension<E>>>,
providers: Vec<Arc<dyn SnapshotArtifactProvider<E>>>,
) -> Self {
Self {
node_state,
context,
extensions,
providers,
_phantom: PhantomData,
}
}
@ -414,8 +415,9 @@ impl<E: Application> SnapshotHandle<E> {
/// 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<SnapshotName>,
@ -428,15 +430,15 @@ impl<E: Application> SnapshotHandle<E> {
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<E: Application> SnapshotHandle<E> {
.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<E: Application> SnapshotHandle<E> {
/// 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,

View File

@ -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<String, DynError> {
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<SnapshotArtifact, DynError> {
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<SnapshotManifest, DynError> {
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<SnapshotManifest, DynError> {
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<SnapshotManifest, DynError> {
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"))