Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Image-capacity handling and mixed-mount deletion have unresolved correctness issues, and unsupported OCI mounts perform unnecessary conversion.
3 open issues
What changed in this PR
Adds image filesystem mounts to sandbox starts, reusing volume infrastructure for lifecycle management, snapshots, and forks.
Changes:
- Adds
imageMountsAPI fields and read-only/writable CLI flags. - Introduces sandbox-owned volumes with restore and cleanup handling.
- Fixes cold-start read-only volume mounting and adds tests and documentation.
| File | Description |
|---|---|
| tests/integration/orchestrator.rs | Updates request fixtures. |
| src/volume/mod.rs | Tracks sandbox-owned volumes and inheritance. |
| src/volume/catalog.rs | Updates volume fixtures. |
| src/types/mod.rs | Exports image-mount metadata. |
| src/types/image_mount.rs | Defines image-mount metadata. |
| src/snapshot/types/snapshot.rs | Persists image-mount markers. |
| src/sandbox/firecracker/sandbox.rs | Mounts read-only volumes after boot. |
| src/orchestrator/types.rs | Extends creation requests. |
| src/orchestrator/tests.rs | Tests scoped-volume deletion. |
| src/orchestrator/store/metadata.rs | Tracks and classifies mounts. |
| src/orchestrator/service.rs | Adds scoped-volume cleanup. |
| src/image/resolver.rs | Exposes published image lookup. |
| src/api/openapi.yml | Defines image-mount API fields. |
| src/api/impls/volumes.rs | Supports read-only attachment and filtered listings. |
| src/api/impls/sandbox.rs | Integrates creation, restoration, and replacement checks. |
| src/api/impls/mod.rs | Registers image-mount handling. |
| src/api/impls/image_mounts.rs | Resolves images and creates scoped volumes. |
| src/api/impls/image_build/worker.rs | Adapts worker requests. |
| src/api/impls/image_build/tests.rs | Updates worker fixtures. |
| src/api/generated/src/models.rs | Regenerates API models. |
| docs/src/getting-started/aenv-cli/sandboxes.md | Documents CLI flags. |
| docs/src/concepts/sandboxes/starting.md | Explains mount behavior and limitations. |
| crates/aenv/src/commands/start.rs | Parses and validates mount flags. |
| crates/aenv/src/client/sandboxes.rs | Serializes image-mount requests. |
Review effort: Balanced
| .create_from_snapshot( | ||
| format!("image-mount-{}", Uuid::now_v7().simple()), | ||
| VolumeMode::Exclusive, | ||
| mount.size_mb.unwrap_or(DEFAULT_VOLUME_SIZE_MB), |
| { | ||
| return Ok(published.layers); | ||
| } | ||
| let resolved = resolver.resolve(image).await.map_err(resolve_error)?; |
| // Image mount volumes are deleted with the sandbox, so they are not captured. | ||
| let (volume_ids, scoped_volume_ids) = metadata | ||
| .map(|metadata| metadata.mounted_volume_ids()) |
|
I think we should set the default as writable rather than read-only. |
|
🔍 OpenCodeReview found 6 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| fn normalize_mount_path(value: &str) -> Result<String> { | ||
| if !value.starts_with('/') || value == "/" { | ||
| anyhow::bail!("volume mount path must be an absolute guest path other than /: {value}"); |
There was a problem hiding this comment.
This diagnostic is emitted for image mounts too, so it incorrectly calls the offending path a "volume mount path". That makes CLI errors misleading when users pass --image-mount or --image-mount-rw; use a generic mount-path message (or pass the mount kind into the validator).
| created.insert( | ||
| path, | ||
| volume.id, | ||
| ImageMount { | ||
| image: image.to_owned(), | ||
| read_only: mount.read_only.unwrap_or(true), | ||
| }, | ||
| ); |
There was a problem hiding this comment.
The requested size_mb is used to create the volume but is not retained in ImageMount. Consequently, image_mounts_model cannot report it and any API/lifecycle path that reconstructs an image mount from this metadata loses the requested writable-view size (and defaults it on recreation). Preserve the size in the internal mount metadata, or derive and serialize it from the associated volume consistently.
| async fn delete_volumes(manager: &VolumeManager, volume_ids: &[String]) { | ||
| for volume_id in volume_ids { | ||
| let _ = manager.delete(volume_id).await; | ||
| } | ||
| } |
There was a problem hiding this comment.
This rollback path discards deletion errors. If catalog deletion fails (for example due to a transient repository/storage error), the newly created sandbox-scoped volume remains in the catalog and its backing data is orphaned, while the request still returns the original failure. At minimum, surface/log the cleanup failure with the volume ID, and provide a retry/GC path for volumes that could not be rolled back.
| mounts | ||
| .iter() | ||
| .map(|(path, mount)| models::ImageMount { | ||
| image: mount.image.clone(), | ||
| mount_path: path.clone(), | ||
| read_only: Some(mount.read_only), | ||
| size_mb: None, | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
HashMap iteration makes the imageMounts array order nondeterministic across otherwise identical responses. Sort the paths (as the volume resolver does) before collecting so clients, snapshot tests, and serialized output remain stable.
| let mut all_mounts = mounts.cloned().unwrap_or_default(); | ||
| for (path, volume_id) in &image_mounts.volumes { | ||
| if all_mounts.insert(path.clone(), volume_id.clone()).is_some() { | ||
| return Err(ApiImpl::error( | ||
| 400, | ||
| format!("{path} is mounted as both a volume and an image"), | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
The duplicate check here compares the image mount's already-normalized key with the explicit volume mount keys as supplied by the client. Explicit keys are only normalized later in resolve_volume_mounts, where the normalized map can overwrite an earlier entry (and both drives have already been prepared). Thus equivalent paths such as /mnt/models/ and /mnt/models, or overlapping paths such as /mnt and /mnt/models, can bypass this check and leave two attached drives while metadata contains only one path. Normalize all mount paths and reject collisions/overlaps before combining the maps.
Suggestion:
| let mut all_mounts = mounts.cloned().unwrap_or_default(); | |
| for (path, volume_id) in &image_mounts.volumes { | |
| if all_mounts.insert(path.clone(), volume_id.clone()).is_some() { | |
| return Err(ApiImpl::error( | |
| 400, | |
| format!("{path} is mounted as both a volume and an image"), | |
| )); | |
| } | |
| } | |
| let mut all_mounts = normalize_volume_mounts(mounts)?; | |
| for (path, volume_id) in &image_mounts.volumes { | |
| if all_mounts.keys().any(|existing| { | |
| Path::new(existing).starts_with(path) || Path::new(path).starts_with(existing) | |
| }) { | |
| return Err(ApiImpl::error( | |
| 400, | |
| format!("{path} overlaps another volume or image mount"), | |
| )); | |
| } | |
| all_mounts.insert(path.clone(), volume_id.clone()); | |
| } |
| Ok(page) => Ok( | ||
| VolumesGetResponse::Status200_SuccessfullyListedTeamVolumes { | ||
| body: page.records.into_iter().map(Into::into).collect(), | ||
| body: page | ||
| .records | ||
| .into_iter() | ||
| .filter(|record| !record.sandbox_scoped) | ||
| .map(Into::into) | ||
| .collect(), | ||
| x_next_token: page.next_token, | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Filtering sandbox_scoped records after list_page makes hidden records consume the requested page size and continuation cursor. If a page contains only image-mount volumes, this returns an empty page with a cursor already advanced past those records; if hidden records precede visible ones, clients can receive short pages and pagination can skip visible volumes. The catalog query needs to exclude scoped records before applying the limit/cursor, or the API must keep fetching pages until it has filled the requested visible page and return a cursor based on the last scanned record.
| body: page | ||
| .records | ||
| .into_iter() | ||
| .filter(|record| !record.sandbox_scoped) | ||
| .map(Into::into) | ||
| .collect(), |
There was a problem hiding this comment.
Filtering scoped records after list_page has already applied the limit and cursor can permanently omit visible volumes. For example, if a page contains only scoped records followed by persistent records, next_token is based on the end of that page and the persistent records are skipped; similarly, a full page of scoped records may return an empty result with no usable continuation. Apply the visibility filter while constructing the paginated catalog result (and derive the cursor from returned visible records), or fetch/advance pages until the requested visible page is filled.
| async fn delete_scoped_volumes(manager: &VolumeManager, volume_ids: &[String]) { | ||
| for volume_id in volume_ids { | ||
| match manager.delete(volume_id).await { | ||
| Ok(()) | Err(VolumeError::NotFound(_)) => {} | ||
| Err(error) => warn!(%volume_id, %error, "failed to delete image mount volume"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
A transient delete failure is only logged and then ignored, while delete_sandbox_impl proceeds to remove the sandbox metadata (and terminal cleanup has no later metadata-driven retry). This can permanently orphan a sandbox-scoped volume after a repository/filesystem failure. Propagate a non-NotFound error from this helper so sandbox deletion remains retryable, or persist a cleanup task/record that will retry the volume deletion.
| for volume_id in volume_ids { | ||
| match manager.delete(volume_id).await { | ||
| Ok(()) | Err(VolumeError::NotFound(_)) => {} | ||
| Err(error) => warn!(%volume_id, %error, "failed to delete image mount volume"), | ||
| } | ||
| } |
There was a problem hiding this comment.
This deletes based solely on the metadata path classification (image_mounts contains the mount path), without verifying that the volume record is actually sandbox_scoped. A stale/corrupt record, or a metadata update that pairs an image path with an existing persistent volume ID, can therefore permanently delete a user-managed volume during sandbox deletion. Fetch the record and only delete when sandbox_scoped is true (and otherwise report/retain the volume).
Suggestion:
| for volume_id in volume_ids { | |
| match manager.delete(volume_id).await { | |
| Ok(()) | Err(VolumeError::NotFound(_)) => {} | |
| Err(error) => warn!(%volume_id, %error, "failed to delete image mount volume"), | |
| } | |
| } | |
| for volume_id in volume_ids { | |
| match manager.get(volume_id).await { | |
| Ok(record) if record.sandbox_scoped => match manager.delete(volume_id).await { | |
| Ok(()) | Err(VolumeError::NotFound(_)) => {} | |
| Err(error) => warn!(%volume_id, %error, "failed to delete image mount volume"), | |
| }, | |
| Ok(_) => warn!(%volume_id, "refusing to delete non-scoped image mount volume"), | |
| Err(VolumeError::NotFound(_)) => {} | |
| Err(error) => warn!(%volume_id, %error, "failed to inspect image mount volume"), | |
| } | |
| } |
| let (scoped, persistent): (Vec<_>, Vec<_>) = self | ||
| .volume_mounts | ||
| .iter() | ||
| .partition(|(path, _)| self.image_mounts.contains_key(*path)); |
There was a problem hiding this comment.
This assumes path membership is sufficient to identify the lifecycle of a volume. If persisted metadata is incomplete (for example, an image_mounts entry has no corresponding volume_mounts entry), the referenced scoped volume is never returned and cannot be released/deleted; if the path and ID are mismatched, a persistent volume is classified as scoped. Cleanup should validate the volume record's sandbox_scoped flag and handle inconsistent metadata explicitly rather than relying only on the two maps.
573aa97 to
1187974
Compare
|
Updated in 1187974. Image mounts now default to writable in both the API and CLI. |
| template_id: &str, | ||
| timeout: Option<u32>, | ||
| volume_mounts: Option<HashMap<String, String>>, | ||
| image_mounts: Option<Vec<ImageMount>>, |
There was a problem hiding this comment.
This adds a required parameter to the public Client::create_sandbox/create_cold_sandbox methods, so any downstream users of the aenv client crate now fail to compile even when they do not use image mounts. Please preserve source compatibility (for example with a separate image-mount-aware method or an options/request type) or account for this as a breaking API change.
| let mount_path = normalize_mount_path(mount_path) | ||
| .with_context(|| format!("invalid image mount: {value}"))?; |
There was a problem hiding this comment.
For invalid image mount paths, normalize_mount_path still emits diagnostics such as volume mount path must be... and volume mount path conflicts.... Because this function is now used by the image-mount flags, these messages identify the wrong input type and make CLI errors misleading; pass a mount-kind-specific label into normalization or map the errors to image-mount wording here.
| for mount in requested { | ||
| let path = normalize_mount_path(PathBuf::from(mount.mount_path.trim())) | ||
| .map_err(|error| ApiImpl::error(400, error.to_string()))? |
There was a problem hiding this comment.
If this future is cancelled after one or more create_from_snapshot calls complete (for example, while resolving the next image), the Err cleanup below is never reached because cancellation drops the future. The created IDs are only held in this stack frame, so those sandbox-scoped volumes become orphaned. Use a drop/cancellation guard or an operation that owns and rolls back the partial set on cancellation; the same issue applies to snapshot restoration below.
| mounts | ||
| .iter() | ||
| .map(|(path, mount)| models::ImageMount { |
There was a problem hiding this comment.
Iterating this HashMap directly makes the order of imageMounts vary between responses for the same sandbox. The existing volume-mount response is also map-backed, but this new field has no ordering guarantee or test; sort by guest path before collecting if clients/tests rely on stable API output.
| mounts | ||
| .iter() | ||
| .map(|(path, mount)| models::ImageMount { | ||
| image: mount.image.clone(), | ||
| mount_path: path.clone(), | ||
| read_only: Some(mount.read_only), | ||
| size_mb: None, | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
The requested sizeMB is discarded when the sandbox metadata is built: ImageMount only retains image and read_only, and this response always emits size_mb: None. Consequently, a client that GETs a sandbox and reuses its imageMounts loses any custom mount size and silently recreates it at the default 64 GiB. Preserve the size in the internal metadata (and snapshot/fork metadata as appropriate) or populate this field from the mounted volume record.
Suggestion:
| mounts | |
| .iter() | |
| .map(|(path, mount)| models::ImageMount { | |
| image: mount.image.clone(), | |
| mount_path: path.clone(), | |
| read_only: Some(mount.read_only), | |
| size_mb: None, | |
| }) | |
| .collect() | |
| mounts | |
| .iter() | |
| .map(|(path, mount)| models::ImageMount { | |
| image: mount.image.clone(), | |
| mount_path: path.clone(), | |
| read_only: Some(mount.read_only), | |
| size_mb: Some(mount.size_mb), | |
| }) | |
| .collect() |
| mounts.insert(mount_path.to_string_lossy().into_owned(), child.id); | ||
| let mount_path = mount_path.to_string_lossy().into_owned(); | ||
| match &volume_snapshot.image_mount { | ||
| Some(mount) => image_mounts.insert(mount_path, child.id, mount.clone()), |
There was a problem hiding this comment.
This insertion silently overwrites an earlier normalized path from the snapshot. The earlier child volume has already been created but is no longer present in either returned map, so it is neither cleaned up on later failure nor represented in the sandbox. Reject duplicate/overlapping normalized snapshot paths before creating the child (or clean up the overwritten ID).
| body: page | ||
| .records | ||
| .into_iter() | ||
| .filter(|record| !record.sandbox_scoped) | ||
| .map(Into::into) | ||
| .collect(), |
There was a problem hiding this comment.
Filtering after fetching a backend page breaks the continuation contract: if this page contains only sandbox-scoped records, the response is empty while x_next_token advances past them. Clients that stop on an empty page (and clients expecting a full page) can miss persistent volumes later in the listing. Apply the scope filter before pagination, or fetch subsequent backend pages until the response is filled/exhausted and return the corresponding token.
| // Image mount volumes are deleted with the sandbox, so they are not captured. | ||
| let (volume_ids, scoped_volume_ids) = metadata |
There was a problem hiding this comment.
This excludes image-mount volumes from the orchestrator's freeze/snapshot/publish step even when an image mount is writable (read_only == false). The API later serializes their snapshot_volume_state, but without freezing/restacking/publishing the current upper layer, the committed snapshot can contain only the original image layers and lose writes made to the image mount on capture. Writable scoped mounts need to participate in capture (while still being deleted after capture), or must be explicitly rejected as non-snapshotable.
| async fn delete_scoped_volumes(manager: &VolumeManager, volume_ids: &[String]) { | ||
| for volume_id in volume_ids { | ||
| match manager.delete(volume_id).await { | ||
| Ok(()) | Err(VolumeError::NotFound(_)) => {} | ||
| Err(error) => warn!(%volume_id, %error, "failed to delete image mount volume"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Deletion errors are only logged here, after which delete_sandbox_impl proceeds to remove_deleted_sandbox. A transient catalog/storage failure therefore leaves the sandbox-scoped volume orphaned with no sandbox metadata left for the normal delete retry path to discover. Propagate a non-NotFound error (or persist a retryable cleanup record) so sandbox deletion remains incomplete and retryable until these volumes are removed.
Suggestion:
| async fn delete_scoped_volumes(manager: &VolumeManager, volume_ids: &[String]) { | |
| for volume_id in volume_ids { | |
| match manager.delete(volume_id).await { | |
| Ok(()) | Err(VolumeError::NotFound(_)) => {} | |
| Err(error) => warn!(%volume_id, %error, "failed to delete image mount volume"), | |
| } | |
| } | |
| } | |
| async fn delete_scoped_volumes(manager: &VolumeManager, volume_ids: &[String]) -> Result<()> { | |
| for volume_id in volume_ids { | |
| match manager.delete(volume_id).await { | |
| Ok(()) | Err(VolumeError::NotFound(_)) => {} | |
| Err(error) => return Err(error.into()), | |
| } | |
| } | |
| Ok(()) | |
| } |
| let (scoped, persistent): (Vec<_>, Vec<_>) = self | ||
| .volume_mounts | ||
| .iter() | ||
| .partition(|(path, _)| self.image_mounts.contains_key(*path)); |
There was a problem hiding this comment.
This classifies a volume as deletable solely from the mount path and never verifies that the referenced VolumeRecord is actually sandbox_scoped. If a persisted/legacy record has an image-mount entry pointing at a persistent volume (or the same volume ID is also mounted at a normal path), terminal cleanup will release it and delete_scoped_volumes will delete user data. The cleanup classification should be based on the volume's scoped ownership/invariant, and inconsistent path/ID mappings should be rejected rather than treated as safe to delete.
Suggestion:
| let (scoped, persistent): (Vec<_>, Vec<_>) = self | |
| .volume_mounts | |
| .iter() | |
| .partition(|(path, _)| self.image_mounts.contains_key(*path)); | |
| // Classify using the volume record's sandbox-scoped flag, after validating | |
| // that image mount paths and volume IDs form a one-to-one mapping. |
1187974 to
020a9d4
Compare
| #[serde(skip_serializing_if = "Option::is_none", rename = "volumeMounts")] | ||
| pub volume_mounts: Option<Vec<SandboxVolumeMount>>, | ||
| #[serde(skip_serializing_if = "Option::is_none", rename = "imageMounts")] | ||
| pub image_mounts: Option<Vec<ImageMount>>, |
There was a problem hiding this comment.
NewSandbox and NewColdSandbox are public structs with public fields, so adding a required field makes existing downstream struct literals fail to compile, independently of the Client method signature change. Consider adding the field with a builder/options API or otherwise preserving construction compatibility for this public request type.
| volume_mounts: Option<HashMap<String, String>>, | ||
| image_mounts: Option<Vec<ImageMount>>, | ||
| ) -> Result<Sandbox> { |
There was a problem hiding this comment.
This is a public client API, so adding a required positional parameter is a source-incompatible change for any downstream caller of Client::create_sandbox (and likewise for create_cold_sandbox). Consider preserving the existing methods and adding *_with_image_mounts/an options struct, or otherwise provide a compatibility path for clients that do not use image mounts.
Suggestion:
| volume_mounts: Option<HashMap<String, String>>, | |
| image_mounts: Option<Vec<ImageMount>>, | |
| ) -> Result<Sandbox> { | |
| volume_mounts: Option<HashMap<String, String>>, | |
| ) -> Result<Sandbox> { |
| let mount_path = normalize_mount_path(mount_path) | ||
| .with_context(|| format!("invalid image mount: {value}"))?; |
There was a problem hiding this comment.
For invalid image-mount paths, normalize_mount_path emits diagnostics such as “volume mount path must...” and “volume mount path contains...”. This makes the new CLI flag report the wrong resource type; pass an image-specific path-validation error (or parameterize the helper) so users can distinguish image-mount failures from --volume failures.
Suggestion:
| let mount_path = normalize_mount_path(mount_path) | |
| .with_context(|| format!("invalid image mount: {value}"))?; | |
| let mount_path = normalize_mount_path(mount_path) | |
| .with_context(|| format!("invalid image mount path in: {value}"))?; |
| let result = create_image_mount_volumes_inner(api, requested, &mut created).await; | ||
| if result.is_err() { | ||
| delete_volumes(&api.volume_manager, &created.volume_ids()).await; | ||
| } |
There was a problem hiding this comment.
This cleanup runs only when the inner future returns Err. If the request is cancelled while a later image is being resolved/created, the future is dropped with already-created volume IDs still in created, so no deletion runs and sandbox-scoped volumes can be orphaned. Use a cancellation-safe cleanup guard (and apply the same pattern to restore_snapshot_volume_mounts, whose local volume_ids/image_mounts are also cleaned only on explicit error paths).
Suggestion:
| let result = create_image_mount_volumes_inner(api, requested, &mut created).await; | |
| if result.is_err() { | |
| delete_volumes(&api.volume_manager, &created.volume_ids()).await; | |
| } | |
| let result = create_image_mount_volumes_inner(api, requested, &mut created).await; | |
| if result.is_err() { | |
| delete_volumes(&api.volume_manager, &created.volume_ids()).await; | |
| } |
| let scoped_volume_ids = image_mounts.volume_ids(); | ||
| let result = prepare_volume_mounts_inner(api, mounts, image_mounts).await; | ||
| if result.is_err() { | ||
| cleanup_volume_ids(&api.volume_manager, &scoped_volume_ids).await; | ||
| } | ||
| result |
There was a problem hiding this comment.
The scoped IDs are cleaned only when prepare_volume_mounts_inner returns an error. If this future is cancelled while reserving/resolving drives, the already-created image-mount volumes are dropped with no cleanup guard and remain orphaned. This should use cancellation-safe ownership/cleanup, not only an is_err() branch.
Suggestion:
| let scoped_volume_ids = image_mounts.volume_ids(); | |
| let result = prepare_volume_mounts_inner(api, mounts, image_mounts).await; | |
| if result.is_err() { | |
| cleanup_volume_ids(&api.volume_manager, &scoped_volume_ids).await; | |
| } | |
| result | |
| let scoped_volume_ids = image_mounts.volume_ids(); | |
| let result = prepare_volume_mounts_inner(api, mounts, image_mounts).await; | |
| if result.is_err() { | |
| cleanup_volume_ids(&api.volume_manager, &scoped_volume_ids).await; | |
| } | |
| result |
| body: page | ||
| .records | ||
| .into_iter() | ||
| .filter(|record| !record.sandbox_scoped) | ||
| .map(Into::into) | ||
| .collect(), | ||
| x_next_token: page.next_token, |
There was a problem hiding this comment.
Filtering after list_page has already applied the limit makes pagination depend on hidden records. A page can contain only sandbox-scoped volumes and return an empty list with a non-empty cursor, and callers that treat an empty page as the end can stop before seeing later team volumes. Fetch/filter until the requested number of visible records is filled, or move the visibility predicate into the catalog query while computing the cursor from visible records.
Suggestion:
| body: page | |
| .records | |
| .into_iter() | |
| .filter(|record| !record.sandbox_scoped) | |
| .map(Into::into) | |
| .collect(), | |
| x_next_token: page.next_token, | |
| body: page | |
| .records | |
| .into_iter() | |
| .filter(|record| !record.sandbox_scoped) | |
| .map(Into::into) | |
| .collect(), | |
| x_next_token: page.next_token, |
| { | ||
| warn!(sandbox_id = %metadata.id, %error, "failed to release image mount volumes during terminal sandbox cleanup"); | ||
| } | ||
| delete_scoped_volumes(manager, &scoped_volume_ids).await; |
There was a problem hiding this comment.
This permanently deletes every ID classified as scoped solely from the mount-path metadata. The cleanup has already cleared ownership, so a stale/corrupt record (or an ID aliased by a regular mount) can cause a persistent volume to be deleted as an image-mount volume. Before deletion, load each VolumeRecord and require sandbox_scoped (and ensure it is not referenced by any persistent mount), or otherwise reject/repair inconsistent metadata rather than trusting the path classification.
Suggestion:
| delete_scoped_volumes(manager, &scoped_volume_ids).await; | |
| delete_scoped_volumes(manager, &owner, &volume_ids, &scoped_volume_ids).await; |
| async fn delete_scoped_volumes(manager: &VolumeManager, volume_ids: &[String]) { | ||
| for volume_id in volume_ids { | ||
| match manager.delete(volume_id).await { | ||
| Ok(()) | Err(VolumeError::NotFound(_)) => {} | ||
| Err(error) => warn!(%volume_id, %error, "failed to delete image mount volume"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Deletion errors are only logged and the caller still marks the sandbox Done/removes its metadata. A transient repository or filesystem failure therefore leaves the sandbox-scoped volume orphaned with no retry path, unlike the surrounding cleanup operations that return an error and retain deletion progress. Propagate a deletion failure (or persist a cleanup task) so a subsequent delete retries it before completing.
Suggestion:
| async fn delete_scoped_volumes(manager: &VolumeManager, volume_ids: &[String]) { | |
| for volume_id in volume_ids { | |
| match manager.delete(volume_id).await { | |
| Ok(()) | Err(VolumeError::NotFound(_)) => {} | |
| Err(error) => warn!(%volume_id, %error, "failed to delete image mount volume"), | |
| } | |
| } | |
| } | |
| async fn delete_scoped_volumes(manager: &VolumeManager, volume_ids: &[String]) -> anyhow::Result<()> { | |
| for volume_id in volume_ids { | |
| match manager.delete(volume_id).await { | |
| Ok(()) | Err(VolumeError::NotFound(_)) => {} | |
| Err(error) => return Err(anyhow::anyhow!("failed to delete image mount volume {volume_id}: {error}")), | |
| } | |
| } | |
| Ok(()) | |
| } |
| pub(crate) fn mounted_volume_ids(&self) -> (Vec<String>, Vec<String>) { | ||
| let (scoped, persistent): (Vec<_>, Vec<_>) = self | ||
| .volume_mounts | ||
| .iter() | ||
| .partition(|(path, _)| self.image_mounts.contains_key(*path)); |
There was a problem hiding this comment.
This split only iterates volume_mounts, so an image_mounts entry without a matching backing-volume entry is silently omitted from cleanup and from snapshot handling. The API normally creates both maps together, but metadata is persisted/deserialized and can be partially written or migrated; cleanup should validate the one-to-one invariant (or return/report an inconsistency) instead of treating it as a valid empty set, so orphaned scoped volumes are not hidden permanently.
Suggestion:
| pub(crate) fn mounted_volume_ids(&self) -> (Vec<String>, Vec<String>) { | |
| let (scoped, persistent): (Vec<_>, Vec<_>) = self | |
| .volume_mounts | |
| .iter() | |
| .partition(|(path, _)| self.image_mounts.contains_key(*path)); | |
| pub(crate) fn mounted_volume_ids(&self) -> Result<(Vec<String>, Vec<String>), MetadataError> { | |
| if self.image_mounts.keys().any(|path| !self.volume_mounts.contains_key(path)) { | |
| return Err(MetadataError::InconsistentMounts); | |
| } | |
| let (scoped, persistent): (Vec<_>, Vec<_>) = self | |
| .volume_mounts | |
| .iter() | |
| .partition(|(path, _)| self.image_mounts.contains_key(*path)); |
| .filter(|(_, drive)| !mounts_after_boot(drive)) | ||
| .map(|(i, drive)| { | ||
| let dev_letter = (b'c' + i as u8) as char; |
There was a problem hiding this comment.
The index is taken before filtering, so a post-boot read-only volume in the middle leaves a device-letter hole. For example, [writable, read-only volume, writable] emits vdc for the first and vde for the second, while the drive list/device attachment remains contiguous (vdc, vdd); init will therefore mount the wrong device or fail. Filter first and then enumerate the remaining drives, while retaining the original index only for the post-boot mount list.
Suggestion:
| .filter(|(_, drive)| !mounts_after_boot(drive)) | |
| .map(|(i, drive)| { | |
| let dev_letter = (b'c' + i as u8) as char; | |
| .iter() | |
| .filter(|drive| !mounts_after_boot(drive)) | |
| .enumerate() | |
| .map(|(i, drive)| { | |
| let dev_letter = (b'c' + i as u8) as char; |
020a9d4 to
0638c26
Compare
| image_mounts: Option<Vec<ImageMount>>, | ||
| ) -> Result<Sandbox> { |
There was a problem hiding this comment.
These public client methods now require a new positional argument, so any downstream caller (and any command/test path outside this repository) that uses the existing API will fail to compile. Prefer preserving the existing methods and adding image-mount-aware variants, or make the new parameter optional via a request/options type, while updating internal callers.
| let result = create_image_mount_volumes_inner(api, requested, &mut created).await; | ||
| if result.is_err() { | ||
| delete_volumes(&api.volume_manager, &created.volume_ids()).await; | ||
| } | ||
| result.map(|()| created) |
There was a problem hiding this comment.
This cleanup only runs when the future resolves to Err. If the request is cancelled while an image is being resolved or created (for example, after earlier mounts have already been created), the async function is dropped and created.volume_ids() is never deleted. The partially created sandbox-scoped volumes can therefore leak indefinitely because no sandbox owns them yet; use a cancellation-safe guard/cleanup mechanism around the in-progress creation.
| if created.mounts.contains_key(&path) { | ||
| return Err(ApiImpl::error( | ||
| 400, | ||
| format!("duplicate image mount path: {path}"), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Only exact duplicates are rejected here. Two requested image mounts such as /mnt/data and /mnt/data/sub both pass, and prepare_volume_mounts_inner only checks exact key collisions before delegating to resolve_volume_mounts; although that resolver rejects overlaps among the combined map, the API's image paths are normalized and it does catch there actually, but this validation is not performed until after all image volumes have been created. A malformed overlapping request can therefore create all sandbox-scoped volumes and only then fail, relying on cleanup; validate ancestor/descendant overlap while processing requests so the request is rejected before unnecessary volume creation.
| mounts | ||
| .iter() | ||
| .map(|(path, mount)| models::ImageMount { |
There was a problem hiding this comment.
Iterating a HashMap directly makes the order of imageMounts in list/detail responses nondeterministic. This produces unstable API payloads and can cause clients/tests that compare response arrays to observe spurious changes. Sort the paths before constructing the response vector (as mount resolution already does for volume mounts).
| let scoped_volume_ids = image_mounts.volume_ids(); | ||
| let result = prepare_volume_mounts_inner(api, mounts, image_mounts).await; | ||
| if result.is_err() { |
There was a problem hiding this comment.
This cleanup also only runs after prepare_volume_mounts_inner resolves. If the request is cancelled while reserving/materializing these mounts, or while the subsequent orchestrator call is in flight, the already-created sandbox-scoped volumes are dropped from the current stack without being deleted. The normal error branches do not cover cancellation, so a cancellation-safe ownership guard is needed for the whole launch operation.
| async fn delete_scoped_volumes(manager: &VolumeManager, volume_ids: &[String]) { | ||
| for volume_id in volume_ids { | ||
| match manager.delete(volume_id).await { | ||
| Ok(()) | Err(VolumeError::NotFound(_)) => {} | ||
| Err(error) => warn!(%volume_id, %error, "failed to delete image mount volume"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Deletion failures are only logged, while the sandbox metadata is subsequently removed. Since the cleanup retry in delete_sandbox_impl only retries errors returned by the cleanup future, a transient repository/filesystem failure here leaves the sandbox-scoped volume orphaned with no durable retry state. Propagate the first deletion error (or record it in persistent cleanup state) so the existing deletion retry can run.
| let ids = |mounts: Vec<(&String, &String)>| -> Vec<String> { | ||
| mounts.into_iter().map(|(_, id)| id.clone()).collect() | ||
| }; | ||
| (ids(persistent), ids(scoped)) |
There was a problem hiding this comment.
This split assumes a volume ID is associated with only one mount path. The API only rejects duplicate paths, not reusing the same volume ID at both an image-mount path and a normal volume path, so a scoped ID can appear in both vectors; terminal cleanup will then delete it even though the persistent mount still references it. Classify by IDs (excluding any scoped ID also present in persistent mounts) or reject duplicate volume IDs when preparing mounts.
| // and init mounts fresh drives except read-only volumes. Only the | ||
| // remaining drives need a guest-side mount. |
There was a problem hiding this comment.
initial_guest_drive_mounts_for_snapshot_launch currently returns every launch-time drive, including writable volumes. Once this list is non-empty, prepare_snapshot_backing_drives skips binding all launch drives before snapshot load, and start then asks envd to mount every listed drive. However init already mounts writable drives from agentenv_drives; this causes writable volumes to be mounted twice (and can fail with a busy mount or mask the init mount). Keep only mounts_after_boot drives in this list, while retaining the non-empty check so writable drives continue to be handled by init.
Suggestion:
| // and init mounts fresh drives except read-only volumes. Only the | |
| // remaining drives need a guest-side mount. | |
| // and init mounts fresh drives except read-only volumes. Only the | |
| // remaining drives need a guest-side mount. |
| .filter(|(_, drive)| !mounts_after_boot(drive)) | ||
| .map(|(i, drive)| { | ||
| let dev_letter = (b'c' + i as u8) as char; |
There was a problem hiding this comment.
The device letter intentionally uses the original extra_drives index, but the boot argument now omits read-only volume entries. This is only safe if init's parser treats the vd* names as explicit devices; please verify that contract (and add a test with an omitted middle drive). If the parser instead assigns entries sequentially, a later drive such as vde will be mounted as vdd, while the post-boot path uses the original slot and mounts the wrong device.
| match self | ||
| .call(request, self.inner.runtime_device_timeout) | ||
| .await? | ||
| { | ||
| DaemonResponse::OverlaybdVirtualSize { virtual_size } => Ok(virtual_size), |
There was a problem hiding this comment.
This new RPC is sent unconditionally, while the protocol has no version negotiation or compatibility fallback. During a rolling upgrade, a newer API process can send this variant to an older daemon, which will fail to deserialize/handle the request and make every image-mount creation fail. Add a daemon capability/version gate (or an explicitly supported fallback) before using this request.
0638c26 to
66b97e5
Compare
| let mut created = CreatedImageMounts::default(); | ||
| let result = create_image_mount_volumes_inner(api, requested, &mut created).await; | ||
| if result.is_err() { | ||
| delete_volumes(&api.volume_manager, &created.volume_ids()).await; | ||
| } | ||
| result.map(|()| created) |
There was a problem hiding this comment.
Cleanup only runs when the inner future returns Err; if the request is cancelled at any .await after one or more volumes have been created, this future is dropped and the created sandbox-scoped volumes are leaked. The same cancellation window exists after prepare_volume_mounts returns in the sandbox handlers, before the later success/error branches. Use an owned cleanup guard (disarmed only after ownership is transferred to the sandbox) or otherwise make cancellation clean up all created IDs.
| if created.mounts.contains_key(&path) { | ||
| return Err(ApiImpl::error( | ||
| 400, | ||
| format!("duplicate image mount path: {path}"), | ||
| )); | ||
| } |
There was a problem hiding this comment.
The API only rejects an exact duplicate here, while the CLI's parser rejects ancestor/descendant overlaps (for example /mnt and /mnt/models). Direct API callers can therefore create both mounts; later attachment treats them as separate drives, so one mount can shadow the other and expose unexpected filesystem contents. Validate mount_paths_overlap against all requested image paths and the requested volume mounts at the API boundary, rather than relying on CLI-only validation.
| if created.mounts.contains_key(&path) { | ||
| return Err(ApiImpl::error( | ||
| 400, | ||
| format!("duplicate image mount path: {path}"), | ||
| )); | ||
| } |
There was a problem hiding this comment.
This validation only detects exact duplicates among image mounts. The later merge with ordinary volume_mounts rejects only an exact key collision as well, so a request such as an image at /mnt/models plus a volume at /mnt (or the reverse) is accepted even though these paths overlap and one mount can shadow the other. Validate image paths against all requested volume paths (including ancestor/descendant relationships) before creating these volumes, or apply the shared overlap validator.
| let volume = api | ||
| .volume_manager | ||
| .create_from_snapshot( | ||
| format!("image-mount-{}", Uuid::now_v7().simple()), | ||
| VolumeMode::Exclusive, | ||
| size_mb, | ||
| layers, | ||
| true, | ||
| ) | ||
| .await | ||
| .map_err(|error| volume_error_response(error).1)?; | ||
| created.insert( |
There was a problem hiding this comment.
If this future is cancelled after create_from_snapshot has persisted the volume but before it returns, created.insert is never reached and the request-level rollback cannot discover the new ID. This can leave an unreserved sandbox-scoped volume orphaned permanently. Please make volume creation/registration cancellation-safe (for example, have the creation helper own a drop/cancellation guard or provide an atomic create-and-register/rollback mechanism).
| mounts | ||
| .iter() | ||
| .map(|(path, mount)| models::ImageMount { | ||
| image: mount.image.clone(), | ||
| mount_path: path.clone(), | ||
| read_only: Some(mount.read_only), | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
This serializes a HashMap directly, so the order of imageMounts in sandbox responses varies between processes. Other mount APIs sort paths before returning them; sorting these entries by mount_path would make responses deterministic for clients and snapshot comparisons.
Suggestion:
| mounts | |
| .iter() | |
| .map(|(path, mount)| models::ImageMount { | |
| image: mount.image.clone(), | |
| mount_path: path.clone(), | |
| read_only: Some(mount.read_only), | |
| }) | |
| .collect() | |
| let mut mounts: Vec<_> = mounts | |
| .iter() | |
| .map(|(path, mount)| models::ImageMount { | |
| image: mount.image.clone(), | |
| mount_path: path.clone(), | |
| read_only: Some(mount.read_only), | |
| }) | |
| .collect(); | |
| mounts.sort_unstable_by(|left, right| left.mount_path.cmp(&right.mount_path)); | |
| mounts |
| // Only writable drives produce new layers to keep in the volume backing. | ||
| let snapshot_output_dir = (!read_only).then(|| manager.data_dir(&volume.id)); |
There was a problem hiding this comment.
This changes the previous VolumeMode::Exclusive condition to all writable volumes, including VolumeMode::Shared. Shared volumes are intentionally writable but do not have exclusive snapshot semantics; assigning them a snapshot output directory makes capture/fork logic persist their writes as backing layers and changes the existing shared-volume behavior. Please retain the mode check (e.g. volume.mode == VolumeMode::Exclusive && !read_only) unless shared-volume snapshotting is explicitly intended and handled throughout the orchestrator.
| body: page | ||
| .records | ||
| .into_iter() | ||
| .filter(|record| !record.sandbox_scoped) | ||
| .map(Into::into) | ||
| .collect(), | ||
| x_next_token: page.next_token, |
There was a problem hiding this comment.
Filtering after list_page paginates the complete volume set makes the cursor describe the last unfiltered record, while the response contains only visible records. If a page consists entirely of sandbox-scoped volumes, this returns an empty page with a cursor; more generally, visible records can be skipped or clients can see short/unstable pages as hidden records are traversed. Apply the predicate before pagination (or make the repository query exclude scoped records) so the cursor and page size are based on the user-visible collection.
| async fn delete_scoped_volumes(manager: &VolumeManager, volume_ids: &[String]) { | ||
| for volume_id in volume_ids { | ||
| match manager.delete(volume_id).await { | ||
| Ok(()) | Err(VolumeError::NotFound(_)) => {} | ||
| Err(error) => warn!(%volume_id, %error, "failed to delete image mount volume"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Deletion errors are only logged after the sandbox metadata is removed, so a transient repository/storage failure leaves the scoped volume record and backing behind with no sandbox metadata to trigger another cleanup attempt. This can orphan storage indefinitely. Please propagate the failure into the delete progress (or persist a cleanup job/retry marker) so cleanup is retried rather than treating every error as successful deletion.
| let (scoped, persistent): (Vec<_>, Vec<_>) = self | ||
| .volume_mounts | ||
| .iter() | ||
| .partition(|(path, _)| self.image_mounts.contains_key(*path)); | ||
| let ids = |mounts: Vec<(&String, &String)>| -> Vec<String> { | ||
| mounts.into_iter().map(|(_, id)| id.clone()).collect() | ||
| }; |
There was a problem hiding this comment.
This classification does not deduplicate IDs or protect against the same volume being mounted at both an image path and a normal volume path. In that case the ID is returned in both lists; terminal cleanup first releases it as a persistent volume and then deletes it as scoped, even though the normal mount still references it. Please reject duplicate volume IDs when constructing/validating mount metadata, or classify/destroy only volumes whose ownership and sandbox_scoped record are verified.
| let (scoped, persistent): (Vec<_>, Vec<_>) = self | ||
| .volume_mounts | ||
| .iter() | ||
| .partition(|(path, _)| self.image_mounts.contains_key(*path)); | ||
| let ids = |mounts: Vec<(&String, &String)>| -> Vec<String> { | ||
| mounts.into_iter().map(|(_, id)| id.clone()).collect() | ||
| }; | ||
| (ids(persistent), ids(scoped)) |
There was a problem hiding this comment.
This relies on an unvalidated invariant that every image_mounts path has exactly one matching volume_mounts entry. An inconsistent persisted record (or a legacy/partial record) causes an image mount with no matching volume to be omitted from cleanup, while a volume at a path missing from image_mounts is treated as persistent and retained. Validate/normalize this invariant when persisting/restoring metadata, or make cleanup use the volume record's sandbox_scoped flag and handle orphaned image-mount entries explicitly.
yingdi-shan
left a comment
yingdi-shan
left a comment
There was a problem hiding this comment.
Worth fixing
-
Deleting a sandbox also freezes its writable image mounts (Copilot r4321427516). If the sandbox also has a persistent volume, delete calls
freeze_and_snapshot_volumes(). That function freezes and seals every writable drive with a snapshot output directory, and writable image mounts have one. So the delete path freezes and seals drives it is about to throw away. If that fails, the error is treated as recoverable and the sandbox goes back toRunning. In other words, a disposable mount can block the delete. -
Deleting a sandbox ignores failures to delete its image-mount volumes (raised 5 times, e.g. r4321531847, r4344073791).
delete_scoped_volumesonly logs the error, and then the sandbox metadata is removed, so nothing ever retries. This is worse than it looks: these volumes are hidden fromGET /volumes, so users can't see or remove the leftovers either. Two related comments end the same way: the request-rollback path that ignores deletes withlet _(r4321531810), and trying to delete afterreplace_owner_forfails, where delete rejects a reserved volume (r4332554193). -
A cancelled request leaves volumes behind (raised 7 times, e.g. r4344073731, r4344073747). Cleanup only runs when the code returns
Err. If the client disconnects and the request is dropped, volumes already created are never deleted. The existing snapshot-restore code has the same pattern, but again hidden volumes can't be cleaned up by users. A single fix covers both: a background sweep that deletes unreserved volumes markedsandbox_scoped. That's simpler than adding cancellation guards everywhere. -
Standard OCI images get converted before being rejected (Copilot r4321427485). When
convert_standard_ociis on,resolve()downloads and converts the whole image (resolver.rs:607). Only afterwards does the!layer.file.is_empty()check return 400. The user gets the right error, but only after a lot of wasted conversion work. -
Check that a volume is
sandbox_scopedbefore deleting it (r4321531851, r4323459648, r4344073795 and others). The code decides what to delete only from the mount path. I don't see a way to trigger this today, because the server always writes both maps together. But the delete can't be undone and the check is a few lines, so it's cheap protection against ever deleting a user's volume.
Minor
imageMountsorder in responses changes between calls, because they come straight out of aHashMap(raised 4 times). Easy to sort. The existingvolume_mounts_modelhas the same problem.- Volume listing can return short or empty pages that still carry a next-page token (raised 6 times). The comments claiming volumes get skipped are wrong: the cursor moves past hidden records, so nothing is lost. The in-repo CLI keeps paging until there's no token, so it's fine. Only clients that stop at an empty page would miss volumes.
- Overlapping mount paths are rejected only after the image volumes are created (r4332554156). The result is correct because of the rollback; it's just wasted work.
- The CLI says "volume mount path …" for image-mount errors (raised 3 times). It's already prefixed with
invalid image mount: …, so this is cosmetic.
Allow template, snapshot, and cold starts to mount image filesystems using sandbox-scoped volumes over committed image layers. Reuse volume capture, fork, and restore, and delete scoped volumes with their sandbox. Image mounts are writable by default through imageMounts and aenv start --image-mount. Use readOnly: true or --image-mount-ro for read-only access. Fix read-only volume mounting on cold start and cover the mount lifecycle with tests and documentation. Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
e811711 to
146a6de
Compare
|
Thanks for the review! Addressed:
Minor: mount lists are sorted by path, and mount paths/counts are validated before any volume is created. I left pagination and the CLI wording as is. |
| #[serde(skip_serializing_if = "Option::is_none", rename = "imageMounts")] | ||
| pub image_mounts: Option<Vec<ImageMount>>, | ||
| } | ||
|
|
||
| #[derive(Deserialize)] |
There was a problem hiding this comment.
Please add a serialization test for NewColdSandbox with one image mount (including mountPath and readOnly). The current cold test only sets image_mounts: None, so a serde rename or omission in the --cold request can regress without being detected.
Suggestion:
| #[serde(skip_serializing_if = "Option::is_none", rename = "imageMounts")] | |
| pub image_mounts: Option<Vec<ImageMount>>, | |
| } | |
| #[derive(Deserialize)] | |
| #[serde(skip_serializing_if = "Option::is_none", rename = "imageMounts")] | |
| pub image_mounts: Option<Vec<ImageMount>>, | |
| } | |
| #[derive(Deserialize)] |
| crate::snapshot::store::store_volume_layers( | ||
| api.snapshot_manager.repository().as_ref(), | ||
| &resolved.overlaybd_config_path, | ||
| ) | ||
| .await | ||
| .map_err(|error| { |
There was a problem hiding this comment.
store_volume_layers uploads local layers/blobs before the volume record exists, but this error path only maps the error and does not remove those newly stored blobs. If image_virtual_size_mb or create_from_snapshot fails afterward, the layer references are no longer reachable from any volume and repeated failed image-mount requests can leak repository storage. Please add rollback/ownership for the acquired layers (or defer/transactionally couple this operation to volume creation).
| .filter(|record| !record.sandbox_scoped) | ||
| .map(Into::into) | ||
| .collect(), | ||
| x_next_token: page.next_token, |
There was a problem hiding this comment.
Filtering after fetching a fixed-size page while returning that page's cursor can produce short or empty visible pages and skip user-visible volumes after the cursor. For example, a page containing only hidden scoped volumes advances x_next_token past them, and clients that stop on an empty page will never see later regular volumes. The handler should filter at the repository/query level or keep fetching until the requested visible page is filled and construct the cursor from the last examined record.
| let (seconds, nanos) = uuid.get_timestamp()?.to_unix(); | ||
| Some(UNIX_EPOCH + Duration::new(seconds, nanos)) |
There was a problem hiding this comment.
Timestamp::to_unix() returns a signed seconds value (i64 in the uuid 1.x API), while Duration::new requires u64. This does not compile; convert/check the seconds value before constructing the duration (and handle pre-epoch timestamps explicitly).
| pub(crate) async fn sweep_orphaned_scoped_volumes( | ||
| &self, | ||
| now: SystemTime, | ||
| grace: Duration, | ||
| ) -> Result<usize, VolumeError> { |
There was a problem hiding this comment.
This cleanup method is only defined here and has no caller in the changed codebase. Consequently, failed/cancelled image-mount creates leave their sandbox-scoped volumes indefinitely, so the intended one-hour orphan policy is never applied. Schedule it from startup or a periodic maintenance task on nodes sharing the catalog.
| if let Some(owner) = pending_owner { | ||
| self.replace_owner_for(owner, None, std::slice::from_ref(&record.id)) | ||
| .await?; | ||
| } | ||
| self.delete(&record.id).await |
There was a problem hiding this comment.
This release-then-delete sequence is racy with finish_volume_reservation: if the sweep clears pending_owner first, the concurrent finish sees no matching owner and cannot bind the volume to the sandbox; the subsequent delete then succeeds and removes a volume that the launch is about to use. Conversely, if finish wins, delete is protected by the sandbox reservation. Make orphan deletion conditional on the pending owner in one repository transaction (or otherwise serialize the owner transition with deletion) so a pending reservation cannot be cleared and deleted while it is being committed.
Suggestion:
| if let Some(owner) = pending_owner { | |
| self.replace_owner_for(owner, None, std::slice::from_ref(&record.id)) | |
| .await?; | |
| } | |
| self.delete(&record.id).await | |
| // Release and delete only if the pending owner is still present, atomically. | |
| self.delete_if_owned_by(&record.id, pending_owner).await |
What
Add
imageMountstoPOST /v2/sandboxesandPOST /sandboxes-cold, with CLI flags--image-mount PATH=IMAGEand--image-mount-ro PATH=IMAGE. CLI warm starts use v2. Mounts are writable by default and keep the source image's virtual size.Why
Mount models, datasets, or code without managing persistent volumes or building a template for each combination.
Related issue
Requested in a maintainer discussion. Builds on merged #405.
Scope and non-goals
Published images and OverlayBD-native registry images. Ordinary OCI images requiring conversion return a publication hint.
Design and behavior changes
Each image mount uses a sandbox-scoped exclusive volume over existing layers. It counts toward volume limits, stays hidden from
GET /volumes, and is deleted with its sandbox. Failed requests roll back newly allocated scoped volumes.Snapshots capture mount state; forks and snapshot launches receive independent copies. Pause/resume preserves writes and capacity. Following #405, snapshots with volume or image mounts reject explicit
volumeMountsorimageMounts, including empty arrays, before allocation. Omit the fields to restore captured mounts.Warm starts use shared internal creation logic. Cold starts attach read-only mounts through envd with
ro,noload.Compatibility and operations
imageMountson v2 and cold creation;readOnlydefaults to false. Images exceedingvolume.max_size_mbreturn 400.imageMountsnapshot markers andsandboxScopedvolume markers. Older servers treat these mounts as ordinary volumes; upgrade server and CLI together.Validation
Validated on Ubuntu 24.04 as
ubuntu, with isolated state and dependencies.make agentenv-serverwith OpenAPI Generator 7.22.0make fmtmake clippymake test-unit— 1220 passed, plus shell checks; CLI tests serializedRuntime E2E scenarios from earlier revisions were not rerun for this update.
Risks and reviewer notes
Review allocation/rollback in
src/api/impls/image_mounts.rs, shared creation and snapshot validation insrc/api/impls/sandbox.rs, and scoped-volume cleanup insrc/orchestrator/service.rs. Crashes during allocation or deletion can leave orphaned scoped volumes.Checklist
main.