refactor(hm-vm): Magic "ephemeral" sentinel string passed through snapshot() label couples vm.rs and docker.rs (#121)

Author: markovejnovic

crates/hm-vm/src/backend.rs

@@ -6,7 +6,7 @@ use std::path::Path;
 use anyhow::Result;
 use async_trait::async_trait;
 
-use crate::types::{OutputSink, SnapshotId, VmConfig};
+use crate::types::{OutputSink, SnapshotId, SnapshotLabel, VmConfig};
 
 /// Factory that creates and manages virtual machines.
 #[async_trait]
@@ -40,7 +40,7 @@ pub trait Vm: Send {
     ) -> Result<i32>;
 
     /// Capture the current VM state as a named snapshot.
-    async fn snapshot(&mut self, label: &str) -> Result<SnapshotId>;
+    async fn snapshot(&mut self, label: &SnapshotLabel) -> Result<SnapshotId>;
 
     /// Tear down the VM and release all resources.
     async fn destroy(&mut self) -> Result<()>;

crates/hm-vm/src/docker.rs

@@ -22,7 +22,7 @@ use futures::StreamExt;
 use tracing::instrument;
 
 use crate::backend::{Vm, VmBackend};
-use crate::types::{OutputSink, SnapshotId, VmConfig};
+use crate::types::{OutputSink, SnapshotId, SnapshotLabel, VmConfig};
 
 /// Docker-based VM backend.
 ///
@@ -308,20 +308,22 @@ impl Vm for DockerVm {
     }
 
     #[instrument(skip(self))]
-    async fn snapshot(&mut self, label: &str) -> Result<SnapshotId> {
+    async fn snapshot(&mut self, label: &SnapshotLabel) -> Result<SnapshotId> {
         let cid = self
             .container_id
             .as_deref()
             .context("container already destroyed")?;
-        let parts: Vec<&str> = label.splitn(2, ':').collect();
-        // A bare label (no explicit `repo:tag`) is an ephemeral, uncached
-        // snapshot. Tag it with the unique container id rather than a shared
-        // `:latest`: concurrent sibling leaf steps off the same parent all
-        // commit ephemeral snapshots, and racing to write the same
-        // `ephemeral:latest` image fails the loser of the race in dockerd.
-        let (repo, tag) = match parts.as_slice() {
-            [r, v] => (*r, *v),
-            _ => (label, cid),
+        // An ephemeral, uncached snapshot is committed under a unique tag (the
+        // container id) rather than a shared `:latest`: concurrent sibling leaf
+        // steps off the same parent all commit ephemeral snapshots, and racing
+        // to write the same `ephemeral:latest` image fails the loser of the
+        // race in dockerd. A cached snapshot parses its cache key as `repo:tag`.
+        let (repo, tag) = match label {
+            SnapshotLabel::Ephemeral => ("ephemeral", cid),
+            SnapshotLabel::Cached(key) => match key.split_once(':') {
+                Some((r, v)) => (r, v),
+                None => (key.as_str(), cid),
+            },
         };
         let opts = CommitContainerOptions {
             container: cid,

crates/hm-vm/src/lib.rs

@@ -12,6 +12,7 @@ pub mod docker;
 pub use backend::{Vm, VmBackend};
 pub use registry::ImageRegistry;
 pub use types::{
-    Action, CachingPolicy, ExecutionResult, ImageSource, NullSink, OutputSink, SnapshotId, VmConfig,
+    Action, CachingPolicy, ExecutionResult, ImageSource, NullSink, OutputSink, SnapshotId,
+    SnapshotLabel, VmConfig,
 };
 pub use vm::HmVm;

crates/hm-vm/src/types.rs

@@ -32,6 +32,23 @@ pub enum CachingPolicy {
     Cache { key: String },
 }
 
+/// Typed instruction for `Vm::snapshot`, describing how the committed
+/// snapshot should be tagged.
+///
+/// This replaces a previously stringly-encoded convention where a bare label
+/// meant "ephemeral" and a `repo:tag` label meant "cached", a contract that
+/// the producer (`vm.rs`) and consumer (the backend) had to agree on
+/// out-of-band. Encoding the distinction as an enum makes it compiler-checked.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum SnapshotLabel {
+    /// Uncached snapshot. The backend chooses a unique tag (e.g. the
+    /// container id) so concurrent sibling steps do not race to write the
+    /// same image reference.
+    Ephemeral,
+    /// Cached snapshot tagged from this cache key (parsed as `repo:tag`).
+    Cached(String),
+}
+
 /// Opaque snapshot handle. Backend-specific contents.
 #[derive(Clone, Debug, Hash, PartialEq, Eq, derive_more::Display)]
 #[display("{_0}")]

crates/hm-vm/src/vm.rs

@@ -7,7 +7,9 @@ use tracing::{instrument, warn};
 
 use crate::backend::VmBackend;
 use crate::registry::ImageRegistry;
-use crate::types::{Action, CachingPolicy, ExecutionResult, ImageSource, OutputSink, VmConfig};
+use crate::types::{
+    Action, CachingPolicy, ExecutionResult, ImageSource, OutputSink, SnapshotLabel, VmConfig,
+};
 
 /// High-level orchestrator that drives the VM lifecycle.
 ///
@@ -107,10 +109,10 @@ impl HmVm {
         // 5. Snapshot and cache on success
         let snapshot = if exit_code == 0 {
             let label = match policy {
-                CachingPolicy::Cache { key } => key.as_str(),
-                CachingPolicy::None => "ephemeral",
+                CachingPolicy::Cache { key } => SnapshotLabel::Cached(key.clone()),
+                CachingPolicy::None => SnapshotLabel::Ephemeral,
             };
-            let snap = vm.snapshot(label).await?;
+            let snap = vm.snapshot(&label).await?;
 
             if let CachingPolicy::Cache { key } = policy {
                 let evicted = self.registry.put(key, &snap);
@@ -236,7 +238,11 @@ mod tests {
             Ok(self.exit_code)
         }
 
-        async fn snapshot(&mut self, label: &str) -> Result<SnapshotId> {
+        async fn snapshot(&mut self, label: &SnapshotLabel) -> Result<SnapshotId> {
+            let label = match label {
+                SnapshotLabel::Ephemeral => "ephemeral".to_string(),
+                SnapshotLabel::Cached(key) => key.clone(),
+            };
             self.calls
                 .lock()
                 .map_or_else(|_| {}, |mut c| c.push(format!("snapshot:{label}")));