diff --git a/architecture-baseline.toml b/architecture-baseline.toml index 92803fffa6..93e4cabcb3 100644 --- a/architecture-baseline.toml +++ b/architecture-baseline.toml @@ -84,7 +84,7 @@ "crates/libs/sbx/mgr/server/src/service/create.rs" = 1195 "crates/libs/sbx/mgr/server/src/service/grpc_core.rs" = 1087 "crates/libs/sbx/mgr/server/src/service/grpc_lifecycle/suspend_snapshot.rs" = 1016 -"crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots.rs" = 1620 +"crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots.rs" = 1506 "crates/libs/sbx/mgr/server/src/service/grpc_state_snapshot.rs" = 1831 "crates/libs/sbx/mgr/server/src/service/mod.rs" = 1757 "crates/libs/sbx/mgr/server/src/service/network_runtime.rs" = 1112 diff --git a/crates/libs/common/core/src/host/linux.rs b/crates/libs/common/core/src/host/linux.rs index 93815c6e6b..48f0d8bda6 100644 --- a/crates/libs/common/core/src/host/linux.rs +++ b/crates/libs/common/core/src/host/linux.rs @@ -339,7 +339,18 @@ pub mod paths { /// Returns the directory path for one sandbox snapshot. pub fn sandbox_snapshot_dir(sandbox_id: &str, snapshot_name: &str) -> String { - format!("{}/{snapshot_name}", sandbox_snapshots_dir(sandbox_id)) + sandbox_snapshot_dir_in(SANDBOX_CACHE_DIR, sandbox_id, snapshot_name) + } + + /// Returns the directory path for one sandbox snapshot under an injectable + /// cache root. + pub fn sandbox_snapshot_dir_in( + cache_root: &str, + sandbox_id: &str, + snapshot_name: &str, + ) -> String { + let id = canonical_sandbox_id(sandbox_id); + format!("{cache_root}/{id}/snapshots/{snapshot_name}") } /// Returns the metadata path for one sandbox snapshot. diff --git a/crates/libs/common/core/src/runtime_contract_tests.rs b/crates/libs/common/core/src/runtime_contract_tests.rs index 173886e71a..cdd53ed6b6 100644 --- a/crates/libs/common/core/src/runtime_contract_tests.rs +++ b/crates/libs/common/core/src/runtime_contract_tests.rs @@ -68,6 +68,10 @@ fn host_linux_paths_canonicalize_sandbox_uuids() { host::linux::paths::sandbox_snapshot_dir(compact, "sn"), format!("{expected_cache}/snapshots/sn"), ); + assert_eq!( + host::linux::paths::sandbox_snapshot_dir_in("/root", compact, "sn"), + format!("/root/{hyphenated}/snapshots/sn"), + ); assert_eq!( host::linux::paths::spaced_host_socket(compact), format!("{expected_run}/spaced-host.sock"), diff --git a/crates/libs/node/mgr/server/src/service/snapshots.rs b/crates/libs/node/mgr/server/src/service/snapshots.rs index c20603602e..490566f9c0 100644 --- a/crates/libs/node/mgr/server/src/service/snapshots.rs +++ b/crates/libs/node/mgr/server/src/service/snapshots.rs @@ -1323,7 +1323,10 @@ fn validate_cache_artifact_path(local_path: &str) -> Result<(), Status> { { uuid::Uuid::parse_str(artifact).is_ok() } else { + // GC staging sits beside the artifact it held: `.gc-pending-`, or + // `snapshot.gc-pending-` from an earlier sandbox-manager. artifact == "snapshot" + || artifact.starts_with(".gc-pending-") || artifact.starts_with("snapshot.gc-pending-") || (matches!(artifact, "snapshots" | "template-snapshot-staging") && parts.next().is_some()) diff --git a/crates/libs/node/mgr/server/src/service/snapshots/tests.rs b/crates/libs/node/mgr/server/src/service/snapshots/tests.rs index 51adfb0714..923ca44c61 100644 --- a/crates/libs/node/mgr/server/src/service/snapshots/tests.rs +++ b/crates/libs/node/mgr/server/src/service/snapshots/tests.rs @@ -224,6 +224,10 @@ fn accepts_paths_strictly_inside_the_cache_root() { let id = uuid::Uuid::now_v7(); for suffix in [ format!("{id}/snapshot"), + // GC staging, in the fixed-length form and the earlier long form. + format!("{id}/.gc-pending-{id}"), + format!("{id}/snapshots/.gc-pending-{id}"), + format!("{id}/snapshot.gc-pending-{id}"), format!("{id}/snapshots/name.gc-pending-x"), // Sandbox-manager retires superseded snapshots here for the sweep. format!("{id}/snapshots/.suspend-gc-pending-{id}"), @@ -233,6 +237,8 @@ fn accepts_paths_strictly_inside_the_cache_root() { } for path in [ format!("{root}/{id}"), + format!("{root}/{id}/instance"), + format!("{root}/{id}/gc-pending-{id}"), format!("{root}/.fork-copies"), format!("{}/firecracker/templates/name/hash", cache_root()), ] { diff --git a/crates/libs/sbx/mgr/server/src/local_artifacts.rs b/crates/libs/sbx/mgr/server/src/local_artifacts.rs index 732e0aaf2b..20abf0dd24 100644 --- a/crates/libs/sbx/mgr/server/src/local_artifacts.rs +++ b/crates/libs/sbx/mgr/server/src/local_artifacts.rs @@ -136,20 +136,36 @@ impl CheckoutError { pub(crate) enum NamedSnapshotDelete { /// `local_path` is gone. Its row, if one was published, is deleted, or /// stays `deleting` for startup recovery when that commit failed. - Deleted, + /// `staging` is where a catalogued snapshot's bytes now wait, once nothing + /// owns them: the caller deletes it with `snapshot_gc::delete_gc_staging` + /// after releasing its locks, and the reaper's orphan sweep deletes what + /// that leaves. It is `None` when nothing was at `local_path`, when the + /// snapshot had no row, and when a failed row delete left the row owning + /// the staging for startup recovery. + Deleted { staging: Option }, /// The row refused deletion, so the bytes were left alone. Refused(CheckoutError), /// The row's `local_path` is not the path the caller deletes, so the /// bytes were left alone and the row is `ready` again. PathDiffers { local_path: String }, - /// The byte delete failed and `local_path` still exists, or could not be - /// read. The row, if any, is `ready` again. A partly deleted tree still - /// exists, so it is not detected and comes back `ready`. A delete that - /// panics returns no outcome: its row stays `deleting` for startup - /// recovery. + /// The bytes are still at `local_path`: the rename to GC staging failed + /// and the row is `ready` again, or the byte delete of a snapshot without + /// a row failed. BytesNotDeleted(tonic::Status), } +/// Why [`DeleteLease::commit_through_gc_staging`] did not finish. +#[derive(Debug, thiserror::Error)] +pub(crate) enum GcStagingError { + /// The bytes are still at `local_path`, and the row is `ready` again. + #[error("rename to GC staging: {0}")] + Rename(std::io::Error), + /// The bytes left `local_path`, but the row delete failed, so the row + /// stays `deleting` for startup recovery. + #[error("delete row after GC staging: {0}")] + Commit(crate::db::Error), +} + #[derive(Clone, Copy)] pub(crate) struct GcEligibility { pub now_secs: u64, @@ -634,31 +650,38 @@ impl LocalArtifacts { self.check_out_for_delete(id) } - /// Delete a named snapshot's bytes at `delete_path` with `delete_bytes` - /// under a delete lease on its row: checkout, then the bytes, then the row. - /// A row that refuses checkout, or whose `local_path` is not `delete_path`, - /// keeps its bytes. A byte delete that fails is judged from the disk: a - /// missing `local_path` commits the row delete, and a present or - /// unreadable one rolls the row back to `ready` so the delete can be - /// retried; `NotFound` is judged the same way. A snapshot without a row - /// (older snapshots predate the catalog) has only its bytes deleted, and - /// for it `NotFound` counts as deleted. + /// Delete a named snapshot at `delete_path` the way the snapshot reaper + /// deletes an artifact (`snapshot_gc::execute_one`): check its row out, + /// rename `local_path` to its GC staging sibling and delete the row + /// ([`DeleteLease::commit_through_gc_staging`]), and return the staging + /// for the caller to delete once its locks are released. Node-manager + /// deletes some trees entry by entry, so a failed delete can leave part of + /// one behind; the rename keeps such a tree away from `local_path`, and + /// the reaper's orphan sweep finishes it. + /// + /// A row that refuses checkout, or whose `local_path` is not + /// `delete_path`, keeps its bytes. A failed rename keeps them too and + /// returns the row to `ready`; a rename that finds nothing at + /// `local_path` deletes the row. A snapshot without a row (older + /// snapshots predate the catalog) has no id to stage under, so + /// `delete_in_place` deletes it, under the caller's locks as before, and + /// for it `NotFound` counts as deleted. `delete_in_place` runs only then. /// /// Callers run this to completion. DeleteSnapshots is a state-writer RPC, /// and restart admission runs its handler on a tracked task that a client /// deadline or disconnect does not cancel (`service::restart_admission`); /// a retention tick is not dropped mid-delete. Only a panic or a runtime - /// shutdown mid-delete drops the lease after the byte delete began, and + /// shutdown drops the lease between the rename and the row delete, and /// then the row stays `deleting` for startup recovery. pub(crate) async fn delete_named_snapshot( &self, sandbox_id: &str, snapshot_name: &str, delete_path: &str, - delete_bytes: impl Future>, + delete_in_place: impl Future>, ) -> NamedSnapshotDelete { // A name that does not parse can have no row. - let mut lease = match SnapshotName::parse(snapshot_name.to_string()) { + let lease = match SnapshotName::parse(snapshot_name.to_string()) { Ok(name) => match self.check_out_named_for_delete(sandbox_id, &name) { Ok(lease) if lease.local_path() != delete_path => { let local_path = lease.local_path().to_owned(); @@ -674,42 +697,43 @@ impl LocalArtifacts { }, Err(_) => None, }; - if let Some(lease) = lease.as_mut() { - lease.mark_path_vacated(); - } - let result = delete_bytes.await; let Some(lease) = lease else { - return match result { - Ok(()) => NamedSnapshotDelete::Deleted, + return match delete_in_place.await { + Ok(()) => NamedSnapshotDelete::Deleted { staging: None }, Err(status) if status.code() == tonic::Code::NotFound => { - NamedSnapshotDelete::Deleted + NamedSnapshotDelete::Deleted { staging: None } } Err(status) => NamedSnapshotDelete::BytesNotDeleted(status), }; }; - // Node-manager can answer NotFound when it fails to stat a path that - // still exists, so with a row NotFound is judged from the disk too. - if let Err(error) = result { - match tokio::fs::try_exists(lease.local_path()).await { - Ok(false) => {} - present => { - if let Err(probe) = present { - tracing::warn!(sandbox_id, snapshot_name, error = %probe, - "snapshot byte delete failed and its path is unreadable; returning local artifact row to 'ready'"); - } - if let Err(rollback) = lease.roll_back() { - tracing::warn!(sandbox_id, snapshot_name, error = %rollback, - "snapshot bytes were not deleted; failed to roll back local artifact row"); - } - return NamedSnapshotDelete::BytesNotDeleted(error); + // The reaper runs the rename and the row delete on the blocking pool + // too. A task that never ran dropped its lease before the rename, which + // returned the row to `ready`. + let staged = + match tokio::task::spawn_blocking(move || lease.commit_through_gc_staging()).await { + Ok(staged) => staged, + Err(error) if error.is_panic() => std::panic::resume_unwind(error.into_panic()), + Err(error) => { + return NamedSnapshotDelete::BytesNotDeleted(tonic::Status::unavailable( + format!("snapshot delete did not run: {error}"), + )); } + }; + match staged { + Ok(staging) => NamedSnapshotDelete::Deleted { staging }, + Err(error @ GcStagingError::Rename(_)) => { + tracing::warn!(sandbox_id, snapshot_name, %error, + "snapshot bytes were not moved aside; local artifact row returned to 'ready'"); + NamedSnapshotDelete::BytesNotDeleted(tonic::Status::internal(error.to_string())) + } + // The row still owns the staging, so it waits for startup recovery + // to delete the row and the sweep to delete the staging. + Err(error @ GcStagingError::Commit(_)) => { + tracing::warn!(sandbox_id, snapshot_name, %error, + "moved snapshot bytes aside but failed to remove local artifact row; startup recovery finishes it"); + NamedSnapshotDelete::Deleted { staging: None } } } - if let Err(error) = lease.commit() { - tracing::warn!(sandbox_id, snapshot_name, %error, - "deleted snapshot but failed to remove local artifact row; startup recovery finishes it"); - } - NamedSnapshotDelete::Deleted } /// Crash recovery for rows stranded in a non-`Ready` state. @@ -725,8 +749,9 @@ impl LocalArtifacts { /// artifact's bytes; whatever the crash interrupted, the disk /// contents are intact. /// - `deleting` → decide by disk state. The delete sequence is - /// checkout → `rename(local_path, .gc-pending-)` - /// → row DELETE, so: + /// checkout → `rename(local_path, /.gc-pending-)` + /// → row DELETE, so (an earlier version staged at + /// `.gc-pending-`, which counts as staging too): /// - original path present and no staging dir → crash was /// before the rename; the bytes are intact → reset to /// `ready`. @@ -754,13 +779,15 @@ impl LocalArtifacts { let mut deleting_deferred = 0usize; for (id, local_path) in stuck_deleting { let staging = gc_pending_staging_path(&local_path, &id); + let legacy_staging = legacy_gc_pending_staging_path(&local_path, &id); let (original_present, staging_present) = match ( std::path::Path::new(&local_path).try_exists(), std::path::Path::new(&staging).try_exists(), + legacy_staging_exists(&legacy_staging), ) { - (Ok(original), Ok(staging)) => (original, staging), - (original, staging) => { - tracing::warn!(artifact_id = %id, ?original, ?staging, + (Ok(original), Ok(staging), Ok(legacy)) => (original, staging || legacy), + (original, staging, legacy) => { + tracing::warn!(artifact_id = %id, ?original, ?staging, ?legacy, "retaining deleting artifact with unreadable recovery paths"); deleting_deferred += 1; continue; @@ -960,10 +987,60 @@ impl Drop for RestoreLease { } } -/// Where a deleted artifact's bytes wait for removal. Startup recovery and the -/// reaper's orphan sweep recognize exactly this shape. +/// Where a deleted artifact's bytes wait for removal: `.gc-pending-` beside +/// `local_path`. The name has a fixed length, so a rename can stage a +/// `local_path` whose own name is as long as the filesystem allows. Startup +/// recovery and the reaper's orphan sweep recognize this shape and the +/// [`legacy_gc_pending_staging_path`] one; a name of either shape is internal, +/// never a snapshot ([`gc_staging_id`]). pub(crate) fn gc_pending_staging_path(local_path: &str, id: impl std::fmt::Display) -> String { - format!("{local_path}.gc-pending-{id}") + match local_path.trim_end_matches('/').rsplit_once('/') { + Some((parent, _)) => format!("{parent}/{GC_PENDING_MARKER}{id}"), + None => format!("{GC_PENDING_MARKER}{id}"), + } +} + +/// What every GC staging name contains, in either shape, just before the +/// artifact id. +pub(crate) const GC_PENDING_MARKER: &str = ".gc-pending-"; + +/// The artifact id a GC staging name was staged under, in either shape: the +/// whole name is `.gc-pending-`, or it ends with `.gc-pending-`. The +/// orphan sweep reclaims exactly the names this parses, so a name such as +/// `run.gc-pending-1` stays an ordinary snapshot. +pub(crate) fn gc_staging_id(name: &str) -> Option { + let (_, id) = name.rsplit_once(GC_PENDING_MARKER)?; + id.parse().ok() +} + +/// Whether a directory name is GC staging, in either shape. Staging is +/// logically deleted bytes that only its deleter or the orphan sweep may +/// touch, so callers that list a sandbox's snapshots treat such a name as +/// internal. +pub(crate) fn is_gc_staging_name(name: &str) -> bool { + gc_staging_id(name).is_some() +} + +/// Whether `.gc-pending-` exists. That shape is 48 bytes longer +/// than `local_path`'s own name, so for a name of 208 bytes or more the path is +/// too long to exist, and the probe's ENAMETOOLONG means absent; treating it as +/// unreadable would defer the row on every restart. Every other error stays an +/// error. +fn legacy_staging_exists(legacy_staging: &str) -> std::io::Result { + match std::path::Path::new(legacy_staging).try_exists() { + Err(error) if error.kind() == std::io::ErrorKind::InvalidFilename => Ok(false), + result => result, + } +} + +/// The staging shape before [`gc_pending_staging_path`] dropped the original +/// name, `.gc-pending-`. An earlier version may have left it +/// behind, so startup recovery still looks for it. +pub(crate) fn legacy_gc_pending_staging_path( + local_path: &str, + id: impl std::fmt::Display, +) -> String { + format!("{local_path}{GC_PENDING_MARKER}{id}") } /// Active checkout of an artifact in `Deleting` state. The caller is @@ -1023,10 +1100,31 @@ impl DeleteLease { Ok(present.then_some(staging)) } - /// Return the row to `ready` because `local_path` still exists, surfacing - /// a failure that `Drop` could only log; on failure `Drop` retries the - /// rollback. Existence is all the caller checked: a partly deleted tree is - /// not detected and comes back `ready`. + /// GC's delete after checkout: rename the bytes to their staging sibling + /// ([`Self::vacate_to_gc_staging`]), then delete the row. Returns the + /// staging path for the caller to delete through node-manager, or `None` + /// when nothing was at `local_path`. On a rename error the bytes stay and + /// the row returns to `ready`; on a row-delete error the row stays + /// `deleting` and owns the staging until startup recovery. + pub(crate) fn commit_through_gc_staging(mut self) -> Result, GcStagingError> { + let staging = match self.vacate_to_gc_staging() { + Ok(staging) => staging, + Err(error) => { + let id = self.id; + if let Err(rollback) = self.roll_back() { + tracing::warn!(local_artifact_id = %id, error = %rollback, + "rename to GC staging failed; retrying the local artifact row rollback on drop"); + } + return Err(GcStagingError::Rename(error)); + } + }; + self.commit().map_err(GcStagingError::Commit)?; + Ok(staging) + } + + /// Return the row to `ready` because the bytes are still at `local_path`, + /// surfacing a failure that `Drop` could only log; on failure `Drop` + /// retries the rollback. pub fn roll_back(mut self) -> crate::db::Result<()> { self.on_drop = DeleteDrop::RollBack; let conn = lock_db(&self.db)?; diff --git a/crates/libs/sbx/mgr/server/src/local_artifacts/tests.rs b/crates/libs/sbx/mgr/server/src/local_artifacts/tests.rs index f9710e5169..6a06b5de59 100644 --- a/crates/libs/sbx/mgr/server/src/local_artifacts/tests.rs +++ b/crates/libs/sbx/mgr/server/src/local_artifacts/tests.rs @@ -1094,14 +1094,17 @@ fn list_pending_backups_fails_rather_than_dropping_an_undecodable_row() { ); } -/// `delete_named_snapshot` orders checkout, byte delete and row delete, and -/// judges a failed byte delete from the disk: only a missing path commits. The -/// snapshot's bytes are a real tempdir, so whether they survive is observed -/// rather than inferred from which RPC ran. +/// `delete_named_snapshot` follows the snapshot reaper's protocol: checkout, +/// the path check, the rename to `.gc-pending-` beside `local_path`, and +/// the row delete; it then hands the staging to the caller, which deletes it +/// after releasing its locks. The bytes are a real tempdir, so where they end +/// up is observed rather than inferred. The staging path is spelled out here +/// instead of taken from the code under test: startup recovery and the orphan +/// sweep recognize only that shape. #[tokio::test] -async fn named_delete_keeps_the_row_and_the_bytes_together() { +async fn named_delete_moves_the_bytes_aside_before_deleting_the_row() { use crate::db::OptionalExtension as _; - use std::sync::atomic::{AtomicBool, Ordering}; + use tonic::Code; #[derive(Clone, Copy, Debug, PartialEq)] enum Setup { @@ -1113,188 +1116,165 @@ async fn named_delete_keeps_the_row_and_the_bytes_together() { SharedPath, UnreadableRow, PathDiffers, - RowCommitFails, - FailsAfterRemoving, - FailsLeavingBytes, + /// Nothing is at `local_path` to rename. + AlreadyGone, + /// A file holds the staging name, so the rename fails with ENOTDIR, + /// for root too. + RenameFails, + /// As `RenameFails`, and the first rollback fails; dropping the lease + /// retries it. RollBackFailsOnce, - FailsOnUnreadablePath, - NotFoundAfterRemoving, - NotFoundLeavingBytes, + RowCommitFails, UncataloguedNotFound, + UncataloguedDeleteFails, } #[derive(Clone, Copy, Debug, PartialEq)] enum Outcome { + /// Deleted, and the staging handed back is the expected path. + Staged, + /// Deleted, with no staging handed back. Deleted, InUse, Unreadable, PathDiffers, BytesNotDeleted, } - use tonic::Code; - // (setup, the byte delete removes the bytes, its result, whether it runs, - // the outcome, whether the bytes remain, and the row state afterwards) + /// Where the snapshot's bytes are afterwards. + #[derive(Clone, Copy, Debug, PartialEq)] + enum Bytes { + AtLocalPath, + InStaging, + Gone, + } + // (setup, the in-place delete's result, whether it runs, the outcome, + // where the bytes are, and the row state afterwards). The in-place delete + // removes `local_path` unless it fails with `Unavailable`. let cases = [ ( Setup::Deletable, - true, Code::Ok, - true, - Outcome::Deleted, false, + Outcome::Staged, + Bytes::InStaging, None, ), ( Setup::Uncatalogued, - true, Code::Ok, true, Outcome::Deleted, - false, + Bytes::Gone, None, ), ( Setup::Pinned, - true, Code::Ok, false, Outcome::InUse, - true, + Bytes::AtLocalPath, Some("ready"), ), ( Setup::Restoring, - true, Code::Ok, false, Outcome::InUse, - true, + Bytes::AtLocalPath, Some("restoring"), ), ( Setup::AlreadyDeleting, - true, Code::Ok, false, Outcome::InUse, - true, + Bytes::AtLocalPath, Some("deleting"), ), ( Setup::SharedPath, - true, Code::Ok, false, Outcome::InUse, - true, + Bytes::AtLocalPath, Some("ready"), ), ( Setup::UnreadableRow, - true, Code::Ok, false, Outcome::Unreadable, - true, + Bytes::AtLocalPath, Some("ready"), ), // The row names a path other than the one the caller deletes. ( Setup::PathDiffers, - true, Code::Ok, false, Outcome::PathDiffers, - true, + Bytes::AtLocalPath, Some("ready"), ), ( - Setup::RowCommitFails, - true, + Setup::AlreadyGone, Code::Ok, - true, - Outcome::Deleted, false, - Some("deleting"), - ), - ( - Setup::FailsAfterRemoving, - true, - Code::Unavailable, - true, Outcome::Deleted, - false, + Bytes::Gone, None, ), ( - Setup::FailsLeavingBytes, + Setup::RenameFails, + Code::Ok, false, - Code::Unavailable, - true, Outcome::BytesNotDeleted, - true, + Bytes::AtLocalPath, Some("ready"), ), - // The first rollback fails, and dropping the lease retries it. ( Setup::RollBackFailsOnce, + Code::Ok, false, - Code::Unavailable, - true, Outcome::BytesNotDeleted, - true, + Bytes::AtLocalPath, Some("ready"), ), - // An unreadable path rolls back too, so the delete stays retryable. + // The row still owns the staging, for startup recovery to finish, so + // none is handed back. ( - Setup::FailsOnUnreadablePath, - false, - Code::Unavailable, - true, - Outcome::BytesNotDeleted, + Setup::RowCommitFails, + Code::Ok, false, - Some("ready"), + Outcome::Deleted, + Bytes::InStaging, + Some("deleting"), ), ( - Setup::NotFoundAfterRemoving, - true, + Setup::UncataloguedNotFound, Code::NotFound, true, Outcome::Deleted, - false, + Bytes::Gone, None, ), - // Node-manager can answer NotFound for a path it failed to stat. ( - Setup::NotFoundLeavingBytes, - false, - Code::NotFound, + Setup::UncataloguedDeleteFails, + Code::Unavailable, true, Outcome::BytesNotDeleted, - true, - Some("ready"), - ), - ( - Setup::UncataloguedNotFound, - true, - Code::NotFound, - true, - Outcome::Deleted, - false, + Bytes::AtLocalPath, None, ), ]; const NAME: &str = "d-named-delete"; - for (setup, removes, code, runs, outcome, kept, state) in cases { + const CONTENT: &[u8] = b"snapshot bytes"; + for (setup, code, runs, outcome, bytes, state) in cases { let tmp = tempfile::TempDir::new().unwrap(); - let mut local_path = tmp.path().join(NAME); - if setup == Setup::FailsOnUnreadablePath { - // A path under a regular file cannot be probed (ENOTDIR). - std::fs::write(tmp.path().join("file"), b"x").unwrap(); - local_path = tmp.path().join("file").join(NAME); - } else { + let local_path = tmp.path().join(NAME).to_str().unwrap().to_owned(); + if setup != Setup::AlreadyGone { std::fs::create_dir(&local_path).unwrap(); + std::fs::write(Path::new(&local_path).join("memory"), CONTENT).unwrap(); } - let local_path = local_path.to_str().unwrap().to_owned(); let sandbox = SandboxId::new().to_string(); let conn = open_test_db(); crate::db::fixture::insert_test_sandbox( @@ -1303,9 +1283,13 @@ async fn named_delete_keeps_the_row_and_the_bytes_together() { crate::state::SandboxState::Stopped, ); let id = LocalArtifactId::new(); + let staging = format!("{}/.gc-pending-{id}", tmp.path().display()); let catalogued = !matches!( setup, - Setup::Uncatalogued | Setup::UncataloguedNotFound | Setup::UnreadableRow + Setup::Uncatalogued + | Setup::UncataloguedNotFound + | Setup::UncataloguedDeleteFails + | Setup::UnreadableRow ); let db = crate::db::SharedConnection::new(conn); let store = LocalArtifacts::new(db.clone()); @@ -1320,6 +1304,9 @@ async fn named_delete_keeps_the_row_and_the_bytes_together() { ) .unwrap(); } + if matches!(setup, Setup::RenameFails | Setup::RollBackFailsOnce) { + std::fs::write(&staging, b"occupied").unwrap(); + } let conn = db.lock().unwrap(); match setup { Setup::Pinned => { @@ -1371,28 +1358,24 @@ async fn named_delete_keeps_the_row_and_the_bytes_together() { Setup::Deletable | Setup::Uncatalogued | Setup::Restoring - | Setup::FailsAfterRemoving | Setup::PathDiffers - | Setup::FailsLeavingBytes - | Setup::FailsOnUnreadablePath - | Setup::NotFoundAfterRemoving - | Setup::NotFoundLeavingBytes - | Setup::UncataloguedNotFound => {} + | Setup::AlreadyGone + | Setup::RenameFails + | Setup::UncataloguedNotFound + | Setup::UncataloguedDeleteFails => {} } drop(conn); let reader = (setup == Setup::Restoring).then(|| store.check_out_for_restore(id).unwrap()); - let ran = Arc::new(AtomicBool::new(false)); - let delete_bytes = { - let (ran, local_path) = (Arc::clone(&ran), local_path.clone()); - async move { - ran.store(true, Ordering::SeqCst); - if removes { - std::fs::remove_dir(&local_path).unwrap(); - } - match code { - Code::Ok => Ok(()), - code => Err(tonic::Status::new(code, "injected byte delete result")), - } + let ran = std::sync::atomic::AtomicBool::new(false); + let delete_in_place = async { + ran.store(true, std::sync::atomic::Ordering::SeqCst); + assert!(Path::new(&local_path).starts_with(tmp.path())); + if code != Code::Unavailable { + std::fs::remove_dir_all(&local_path).unwrap(); + } + match code { + Code::Ok => Ok(()), + code => Err(tonic::Status::new(code, "injected byte delete result")), } }; let delete_path = if setup == Setup::PathDiffers { @@ -1402,19 +1385,38 @@ async fn named_delete_keeps_the_row_and_the_bytes_together() { }; let result = store - .delete_named_snapshot(&sandbox, NAME, &delete_path, delete_bytes) + .delete_named_snapshot(&sandbox, NAME, &delete_path, delete_in_place) .await; let observed = match result { - NamedSnapshotDelete::Deleted => Outcome::Deleted, + NamedSnapshotDelete::Deleted { + staging: Some(handed), + } => { + assert_eq!(handed, staging, "{setup:?}"); + Outcome::Staged + } + NamedSnapshotDelete::Deleted { staging: None } => Outcome::Deleted, NamedSnapshotDelete::Refused(error) if error.is_in_use() => Outcome::InUse, NamedSnapshotDelete::Refused(_) => Outcome::Unreadable, NamedSnapshotDelete::PathDiffers { .. } => Outcome::PathDiffers, NamedSnapshotDelete::BytesNotDeleted(_) => Outcome::BytesNotDeleted, }; assert_eq!(observed, outcome, "{setup:?}"); - assert_eq!(ran.load(Ordering::SeqCst), runs, "{setup:?}"); - assert_eq!(Path::new(&local_path).exists(), kept, "{setup:?}"); + assert_eq!( + ran.load(std::sync::atomic::Ordering::SeqCst), + runs, + "{setup:?}" + ); + let content = |dir: &str| std::fs::read(Path::new(dir).join("memory")).ok(); + let at = match (content(&local_path), content(&staging)) { + (Some(at_local), None) if at_local == CONTENT => Bytes::AtLocalPath, + (None, Some(staged)) if staged == CONTENT => Bytes::InStaging, + (None, None) if !Path::new(&local_path).exists() && !Path::new(&staging).is_dir() => { + Bytes::Gone + } + (local, staged) => panic!("{setup:?}: local_path {local:?}, staging {staged:?}"), + }; + assert_eq!(at, bytes, "{setup:?}"); let row: Option = db .lock() .unwrap() @@ -1429,3 +1431,196 @@ async fn named_delete_keeps_the_row_and_the_bytes_together() { drop(reader); } } + +/// The staging name has a fixed length, so a snapshot whose own name is as long +/// as a filename can be (255 bytes) still moves aside in one rename. Appending +/// `.gc-pending-` (48 bytes) to the name, as staging once did, fails from +/// 208 bytes with ENAMETOOLONG and leaves the snapshot undeletable. +#[tokio::test] +async fn named_delete_stages_names_of_every_length() { + for length in [207, 208, 255] { + let tmp = tempfile::TempDir::new().unwrap(); + let name = format!("d-{}", "x".repeat(length - 2)); + let local_path = tmp.path().join(&name).to_str().unwrap().to_owned(); + std::fs::create_dir(&local_path).unwrap(); + std::fs::write(Path::new(&local_path).join("memory"), b"bytes").unwrap(); + let sandbox = SandboxId::new().to_string(); + let conn = open_test_db(); + crate::db::fixture::insert_test_sandbox( + &conn, + &sandbox, + crate::state::SandboxState::Stopped, + ); + let store = LocalArtifacts::new(crate::db::SharedConnection::new(conn)); + let id = LocalArtifactId::new(); + store + .publish_named_snapshot( + &sandbox, + id, + ArtifactKind::Disk, + &SnapshotName::parse(name.clone()).unwrap(), + &local_path, + ) + .unwrap(); + + let result = store + .delete_named_snapshot(&sandbox, &name, &local_path, async { + // Had it run, the outcome would be BytesNotDeleted. + Err(tonic::Status::internal( + "a catalogued snapshot is never deleted in place", + )) + }) + .await; + + let staging = format!("{}/.gc-pending-{id}", tmp.path().display()); + assert!( + matches!(&result, NamedSnapshotDelete::Deleted { staging: Some(s) } if *s == staging), + "{length} bytes: {result:?}" + ); + assert!(!Path::new(&local_path).exists(), "{length} bytes"); + assert_eq!( + std::fs::read(Path::new(&staging).join("memory")).unwrap(), + b"bytes", + "{length} bytes" + ); + assert!(store.get(id).unwrap().is_none(), "{length} bytes"); + } +} + +/// Startup recovery treats a `deleting` row as staged when staging exists in +/// either shape: the fixed-length `.gc-pending-` beside `local_path`, or +/// `.gc-pending-`, which an earlier version left. With the +/// original path present too, a row that saw no staging would come back +/// `ready` over bytes it no longer owns. +#[test] +fn recover_recognizes_staging_in_either_shape() { + for legacy in [false, true] { + for pinned in [false, true] { + let case = format!("legacy={legacy}, pinned={pinned}"); + let conn = open_test_db(); + let tmp = tempfile::TempDir::new().unwrap(); + let local_path = tmp.path().join("artifact").to_str().unwrap().to_string(); + let id = "01939e3e-8886-7000-8000-000000000001"; + std::fs::create_dir_all(&local_path).unwrap(); + let staging = if legacy { + format!("{local_path}.gc-pending-{id}") + } else { + format!("{}/.gc-pending-{id}", tmp.path().display()) + }; + std::fs::create_dir_all(&staging).unwrap(); + insert_row(&conn, id, "sbx-r6", &local_path); + if pinned { + conn.execute( + "INSERT INTO sandboxes ( + id, uuid8, state, template, user, runtime, created_at, updated_at, + process_env_json, auto_lifecycle_json, auto_lifecycle_state_json, + current_local_artifact_id, current_local_artifact_kind, + current_local_artifact_origin + ) VALUES ('sbx-r6', 'uuid8', 'suspended', 'seed', 'u', 'fc', '0', '0', + '{}', '{}', '{}', ?1, 'full', 'suspend')", + params![id], + ) + .unwrap(); + } + let db = crate::db::SharedConnection::new(conn); + let store = LocalArtifacts::new(db.clone()); + force_state(&db, id, "deleting"); + + let report = store.recover_stuck_states().unwrap(); + + assert_eq!(report.deleting_rolled_back, 0, "{case}"); + assert_eq!(report.deleting_finished, usize::from(!pinned), "{case}"); + assert_eq!(report.deleting_deferred, usize::from(pinned), "{case}"); + assert_eq!( + store.get(artifact_id(id)).unwrap().is_some(), + pinned, + "{case}" + ); + } + } +} + +/// Startup recovery resolves `deleting` rows whose names are as long as a +/// filename can be. The earlier `.gc-pending-` shape adds 48 +/// bytes, so from 208 bytes its probe fails with ENAMETOOLONG; such a path +/// cannot exist, so it reads as absent rather than unreadable, which would +/// defer the row on every restart. Before the rename the row comes back +/// `ready` over its bytes. After a rename whose row commit failed, the row is +/// deleted and its staging is left unowned for the orphan sweep. +#[tokio::test] +async fn recover_resolves_deleting_rows_with_long_names() { + for length in [207, 208, 255] { + for renamed in [false, true] { + let case = format!("{length} bytes, renamed={renamed}"); + let tmp = tempfile::TempDir::new().unwrap(); + let name = format!("d-{}", "x".repeat(length - 2)); + let local_path = tmp.path().join(&name).to_str().unwrap().to_owned(); + std::fs::create_dir(&local_path).unwrap(); + std::fs::write(Path::new(&local_path).join("memory"), b"bytes").unwrap(); + let sandbox = SandboxId::new().to_string(); + let conn = open_test_db(); + crate::db::fixture::insert_test_sandbox( + &conn, + &sandbox, + crate::state::SandboxState::Stopped, + ); + let db = crate::db::SharedConnection::new(conn); + let store = LocalArtifacts::new(db.clone()); + let id = LocalArtifactId::new(); + store + .publish_named_snapshot( + &sandbox, + id, + ArtifactKind::Disk, + &SnapshotName::parse(name.clone()).unwrap(), + &local_path, + ) + .unwrap(); + let staging = format!("{}/.gc-pending-{id}", tmp.path().display()); + if renamed { + // A named delete whose row commit fails after the rename. + db.lock() + .unwrap() + .execute_batch( + "CREATE TEMP TRIGGER refuse_artifact_delete BEFORE DELETE ON local_artifacts + BEGIN SELECT RAISE(ABORT, 'injected row commit failure'); END;", + ) + .unwrap(); + let result = store + .delete_named_snapshot(&sandbox, &name, &local_path, async { + Err(tonic::Status::internal("never deleted in place")) + }) + .await; + assert!( + matches!(result, NamedSnapshotDelete::Deleted { staging: None }), + "{case}: {result:?}" + ); + db.lock() + .unwrap() + .execute_batch("DROP TRIGGER refuse_artifact_delete") + .unwrap(); + assert!(Path::new(&staging).join("memory").exists(), "{case}"); + assert!(!store.gc_staging_is_unowned(id).unwrap(), "{case}"); + } else { + force_state(&db, &id.to_string(), "deleting"); + } + + let report = store.recover_stuck_states().unwrap(); + + assert_eq!(report.deleting_deferred, 0, "{case}"); + assert_eq!(report.deleting_rolled_back, usize::from(!renamed), "{case}"); + assert_eq!(report.deleting_finished, usize::from(renamed), "{case}"); + let row = store.get(id).unwrap(); + if renamed { + assert!(row.is_none(), "{case}"); + assert!(store.gc_staging_is_unowned(id).unwrap(), "{case}"); + assert!(Path::new(&staging).join("memory").exists(), "{case}"); + } else { + assert!(row.is_some(), "{case}"); + // `ready` again: a delete can check it out. + store.check_out_for_delete(id).unwrap(); + assert!(Path::new(&local_path).join("memory").exists(), "{case}"); + } + } + } +} diff --git a/crates/libs/sbx/mgr/server/src/service/grpc_lifecycle/backup_policy_tests.rs b/crates/libs/sbx/mgr/server/src/service/grpc_lifecycle/backup_policy_tests.rs index 23ccd40c06..49c0d43a2f 100644 --- a/crates/libs/sbx/mgr/server/src/service/grpc_lifecycle/backup_policy_tests.rs +++ b/crates/libs/sbx/mgr/server/src/service/grpc_lifecycle/backup_policy_tests.rs @@ -1252,3 +1252,52 @@ async fn kill_refuses_signals_it_cannot_deliver_before_cleanup_admission() { cleanup_untouched(&service, &id, state, case); } } + +/// GC staging is logically deleted bytes. Backing one up by name would adopt +/// it as a new `ready` row that the orphan sweep then deletes from under it, so +/// both staging shapes are refused as internal before any row is adopted. +#[tokio::test] +async fn artifact_backup_refuses_gc_staging() { + let token_dir = tempfile::tempdir().unwrap(); + let token_path = token_dir.path().join("cluster-api.token"); + if !pplx_space_test_env::enter_test_process(&cluster_api_env( + LOOPBACK_ENDPOINT, + STATIC_TOKEN, + &token_path, + )) { + return; + } + let conn = crate::db::test_conn(); + let id = SandboxId::new().to_string(); + insert_test_sandbox(&conn, &id, SandboxState::Stopped); + let service = SandboxManagerService::for_test(conn); + let artifact = LocalArtifactId::new(); + for name in [ + format!(".gc-pending-{artifact}"), + format!("d-manual-20260101T000000Z.gc-pending-{artifact}"), + ] { + let refused = service + .enqueue_artifact_backup(artifact_backup_request( + &id, + crate::proto::ArtifactKind::Disk, + Some(Source::SnapshotName(name.clone())), + )) + .await + .unwrap_err(); + assert_eq!( + refused.code(), + tonic::Code::FailedPrecondition, + "{name}: {refused}" + ); + assert!(refused.message().contains("internal"), "{name}: {refused}"); + let name = SnapshotName::parse(name).unwrap(); + assert!( + service + .local_artifacts + .get_named(&id, &name) + .unwrap() + .is_none() + ); + } + assert_eq!(snapshot_upload_jobs(&service, &id), 0); +} diff --git a/crates/libs/sbx/mgr/server/src/service/grpc_snapshot_transfer.rs b/crates/libs/sbx/mgr/server/src/service/grpc_snapshot_transfer.rs index e9ca7718c3..2339d076e3 100644 --- a/crates/libs/sbx/mgr/server/src/service/grpc_snapshot_transfer.rs +++ b/crates/libs/sbx/mgr/server/src/service/grpc_snapshot_transfer.rs @@ -2,6 +2,30 @@ use super::cleanup_support::CleanupLockTryResult; use super::create_support::{validate_path_component, validate_snapshot_name}; use super::*; +/// The name an import publishes its snapshot under: the client's +/// `target_name`, which must be a valid name of the snapshot's type, or a +/// generated one. A name ending in `.gc-pending-` is GC staging's, +/// which no snapshot RPC treats as a snapshot, so it is refused rather than +/// imported and then hidden. +fn import_target_name(target_name: Option<&str>, prefix: &str) -> Result { + let Some(name) = target_name.filter(|name| !name.is_empty()) else { + return Ok(format!("{prefix}-imported-{}", uuid::Uuid::now_v7())); + }; + validate_snapshot_name(name)?; + if !name.starts_with(&format!("{prefix}-")) { + return Err(Status::invalid_argument( + "snapshot name does not match snapshot type", + )); + } + if crate::local_artifacts::is_gc_staging_name(name) { + return Err(Status::invalid_argument(format!( + "invalid snapshot_name: {name}: a name ending in `{}` is reserved", + crate::local_artifacts::GC_PENDING_MARKER + ))); + } + Ok(name.to_owned()) +} + #[cfg(test)] mod tests; @@ -75,6 +99,12 @@ impl SandboxManagerService { return Err(Status::invalid_argument("snapshot_name is required")); } validate_snapshot_name(&snapshot_name)?; + // GC staging is logically deleted bytes, never a snapshot. + if crate::local_artifacts::is_gc_staging_name(&snapshot_name) { + return Err(Status::failed_precondition( + "cannot export an internal snapshot artifact", + )); + } // Both d-* (disk) and f-* (full) snapshots are exportable. // d-* contains rootfs.ext4 only, f-* contains rootfs.ext4 + VM state files. @@ -297,22 +327,7 @@ impl SandboxManagerService { } else { "f" }; - let name = match header - .target_name - .as_deref() - .filter(|name| !name.is_empty()) - { - Some(name) => { - validate_snapshot_name(name)?; - if !name.starts_with(&format!("{prefix}-")) { - return Err(Status::invalid_argument( - "snapshot name does not match snapshot type", - )); - } - name.to_owned() - } - None => format!("{prefix}-imported-{}", uuid::Uuid::now_v7()), - }; + let name = import_target_name(header.target_name.as_deref(), prefix)?; let _wake = super::restart_admission::wait_for_activation_admission( &sandbox_id, self.auto_lifecycle.wake_lock(&sandbox_id).lock_owned(), diff --git a/crates/libs/sbx/mgr/server/src/service/grpc_snapshot_transfer/tests.rs b/crates/libs/sbx/mgr/server/src/service/grpc_snapshot_transfer/tests.rs index a04f1bc96e..1a3e94ecf0 100644 --- a/crates/libs/sbx/mgr/server/src/service/grpc_snapshot_transfer/tests.rs +++ b/crates/libs/sbx/mgr/server/src/service/grpc_snapshot_transfer/tests.rs @@ -728,3 +728,44 @@ fn snapshot_publication_refuses_an_unusable_destination_without_cataloging_it() fixture.assert_staging_retained(&case); } } + +/// An import may name its snapshot, but not with the `.gc-pending-` marker GC +/// staging reserves: such a snapshot would be imported and then hidden from, +/// and refused by, every snapshot RPC. +#[test] +fn import_refuses_the_reserved_staging_marker() { + for (prefix, name) in [ + ( + "d", + "d-user.gc-pending-019f9ce3-2370-7a33-8706-fda0d0496d33", + ), + ( + "f", + "f-user.gc-pending-019f9ce3-2370-7a33-8706-fda0d0496d33", + ), + ] { + let status = import_target_name(Some(name), prefix).unwrap_err(); + assert_eq!(status.code(), tonic::Code::InvalidArgument, "{name}"); + assert!(status.message().contains("reserved"), "{name}: {status}"); + } + // Only staging's own shape, the marker followed by an artifact id, is + // reserved: the orphan sweep could never reclaim any other name. + for name in [ + "d-user.gc-pending", + "d-user.gc-pending-note", + "d-run.gc-pending-1", + "d-user.gc-pending-019f9ce3-2370-7a33-8706-fda0d0496d33.tar", + ] { + assert_eq!(import_target_name(Some(name), "d").unwrap(), name); + } + assert_eq!(import_target_name(Some("d-user"), "d").unwrap(), "d-user"); + assert!( + import_target_name(None, "f") + .unwrap() + .starts_with("f-imported-") + ); + assert_eq!( + import_target_name(Some("f-user"), "d").unwrap_err().code(), + tonic::Code::InvalidArgument + ); +} diff --git a/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots.rs b/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots.rs index a3047453cf..32d18ddef9 100644 --- a/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots.rs +++ b/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots.rs @@ -13,7 +13,6 @@ use super::create_support::*; use super::spaced_bridge::*; use super::tail_support::*; use super::*; -use crate::local_artifacts::{CheckoutError, NamedSnapshotDelete}; use futures_util::StreamExt; use pplx_space_common_core::types::{GuestPort, SandboxId}; use pplx_space_events::{ @@ -25,13 +24,20 @@ use rollback_state::restage_rolled_back_full_snapshot; use std::str::FromStr; use tracing::Instrument; +/// This module's log target, kept by the handlers that moved into children. +const LOG_TARGET: &str = module_path!(); + const CURRENT_INSTANCE_DISK_ARTIFACT_PREFIX: &str = "d-stop-"; const OPERATION_DISK_ARTIFACT_PREFIX: &str = pplx_space_common_core::host::linux::paths::INTERNAL_OPERATION_SNAPSHOT_PREFIX; +/// A snapshot directory users can't list, delete, roll back to, export or +/// back up: sbx-mgr's own disk artifacts, and GC staging (logically deleted +/// bytes that only the orphan sweep removes). pub(super) fn is_internal_snapshot_name(snapshot_name: &str) -> bool { snapshot_name.starts_with(CURRENT_INSTANCE_DISK_ARTIFACT_PREFIX) || snapshot_name.starts_with(OPERATION_DISK_ARTIFACT_PREFIX) + || crate::local_artifacts::is_gc_staging_name(snapshot_name) } fn disk_snapshot_rollback_start_request( @@ -1025,6 +1031,7 @@ fn project_snapshot_inventory( .charge(name.len()) .map_err(crate::inventory_budget::exceeded)?; if !name.starts_with("f-") + || is_internal_snapshot_name(&name) || !entry .file_type() .map_err(|error| Status::internal(format!("read snapshot type: {error}")))? @@ -1175,32 +1182,6 @@ fn project_snapshot_inventory( Ok(snapshots.into_values().collect()) } -/// [`crate::local_artifacts::LocalArtifacts::delete_named_snapshot`] with -/// node-manager's DeleteCacheArtifact of `local_path` as the byte delete. -async fn delete_cached_snapshot( - service: &SandboxManagerService, - (sandbox_id, snapshot_name): (&str, &str), - local_path: String, - node_client: &mut pplx_space_node_mgr_client::NodeManagerGrpcClient, -) -> NamedSnapshotDelete { - let request = pplx_space_node_mgr_client::proto::DeleteCacheArtifactRequest { - local_path: local_path.clone(), - }; - let delete_bytes = async { node_client.delete_cache_artifact(request).await.map(drop) }; - service - .local_artifacts - .delete_named_snapshot(sandbox_id, snapshot_name, &local_path, delete_bytes) - .await -} - -fn snapshot_delete_refusal(error: CheckoutError) -> Status { - if error.is_in_use() { - Status::failed_precondition(format!("snapshot is in use: {error}")) - } else { - Status::internal(format!("inspect snapshot publication: {error}")) - } -} - impl SandboxManagerService { fn checkout_rollback_snapshot( &self, @@ -1253,125 +1234,6 @@ impl SandboxManagerService { super::runtime_upgrade::refuse_runtime_upgrade_fence_in(&db, sandbox_id) } - pub(super) async fn delete_snapshots( - &self, - request: Request, - ) -> Result, Status> { - self.authorize_peer(&request, "DeleteSnapshots", ALLOW_SANDBOX_OPERATOR)?; - let req = request.get_ref(); - let sandbox = self.lookup_sandbox(&req.sandbox_id)?; - let artifact_lock = self.auto_lifecycle.artifact_lock(&sandbox.id); - let _guard = artifact_lock.lock().await; - let sandbox = self.lookup_sandbox(&sandbox.id)?; - self.require_snapshot_mutation_ownership(&sandbox.id)?; - let protected_instance_path = sandbox.current_local_artifact_id.and_then(|artifact_id| { - self.local_artifacts - .get(artifact_id) - .ok() - .flatten() - .filter(|artifact| { - artifact.artifact_origin - != pplx_space_common_core::types::ArtifactOrigin::Snapshot - }) - .map(|artifact| artifact.local_path) - }); - - let mut node_client = connect_node(&self.node_manager) - .await - .map_err(|e| Status::internal(format!("cannot connect to node-manager: {e}")))?; - - if req.all { - let mut deleted = Vec::new(); - - // Delete btrfs snapshots (d-* subvolumes) via node-manager - let sandbox_id = sandbox - .id - .parse::() - .map_err(|error| Status::internal(format!("invalid stored sandbox id: {error}")))?; - let mut pages = pplx_space_node_mgr_client::SnapshotPages::new(sandbox_id, false); - while let Some(page) = pages.next(&mut node_client).await? { - for snap in page { - if is_internal_snapshot_name(&snap.name) { - continue; - } - let local_path = - pplx_space_common_core::host::linux::paths::sandbox_snapshot_dir( - &sandbox.id, - &snap.name, - ); - if protected_instance_path.as_deref() == Some(local_path.as_str()) { - continue; - } - if deleted.len() >= 4096 { - return Err(Status::resource_exhausted( - "deleted 4096 snapshots; retry delete-all to continue", - )); - } - let names = (sandbox.id.as_str(), snap.name.as_str()); - match delete_cached_snapshot(self, names, local_path, &mut node_client).await { - NamedSnapshotDelete::Deleted => deleted.push(snap.name), - NamedSnapshotDelete::BytesNotDeleted(status) => return Err(status), - // A refused row or a differing path: kept and left out of - // `deleted`, as delete-all did before rows were checked out. - kept => tracing::warn!( - sandbox_id = %sandbox.id, - snapshot_name = %snap.name, - outcome = ?kept, - "delete-all retains a snapshot it cannot delete", - ), - } - } - } - // Note: list_btrfs_snapshots returns both d-* and f-* entries, so the loop - // above already covers full snapshots. No separate f-* scan needed. - - Ok(Response::new(DeleteSnapshotsResponse { deleted })) - } else if let Some(snapshot_name) = req.snapshot_name.as_deref().filter(|s| !s.is_empty()) { - validate_snapshot_name(snapshot_name)?; - if is_internal_snapshot_name(snapshot_name) { - return Err(Status::failed_precondition( - "cannot delete an internal snapshot artifact", - )); - } - let local_path = pplx_space_common_core::host::linux::paths::sandbox_snapshot_dir( - &sandbox.id, - snapshot_name, - ); - if protected_instance_path.as_deref() == Some(local_path.as_str()) { - return Err(Status::failed_precondition( - "cannot delete the current instance artifact", - )); - } - - // Both disk and full snapshots may be btrfs subvolumes — - // delete via node-manager (root). DeleteCacheArtifact - // handles both the subvolume case (privileged - // `btrfs subvolume delete`) and the plain-directory case - // (`remove_dir_all`), and is idempotent on a missing path. - let names = (sandbox.id.as_str(), snapshot_name); - match delete_cached_snapshot(self, names, local_path, &mut node_client).await { - NamedSnapshotDelete::Deleted => {} - NamedSnapshotDelete::Refused(error) => return Err(snapshot_delete_refusal(error)), - NamedSnapshotDelete::PathDiffers { .. } => { - return Err(Status::failed_precondition( - "delete snapshot publication path differs", - )); - } - NamedSnapshotDelete::BytesNotDeleted(e) => { - return Err(Status::internal(format!("delete snapshot failed: {e}"))); - } - } - - Ok(Response::new(DeleteSnapshotsResponse { - deleted: vec![snapshot_name.to_string()], - })) - } else { - Err(Status::invalid_argument( - "provide snapshot_name or set all=true", - )) - } - } - pub(super) async fn rollback( &self, request: Request, @@ -1795,6 +1657,7 @@ fn start_process_options( }) } +mod named_delete; mod rollback_state; #[cfg(test)] diff --git a/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/internal_snapshot_name_tests.rs b/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/internal_snapshot_name_tests.rs index d623a0e9d8..e42affd067 100644 --- a/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/internal_snapshot_name_tests.rs +++ b/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/internal_snapshot_name_tests.rs @@ -8,6 +8,24 @@ fn operation_snapshots_are_internal() { "operation snapshots must not be exposed to list, delete, or rollback" ); assert!(!is_internal_snapshot_name("d-manual-20260804T180000Z")); + // GC staging in either shape: `.gc-pending-` as the whole name, or as + // the suffix of the snapshot's own name. + for staging in [ + ".gc-pending-019f9ce3-2370-7a33-8706-fda0d0496d33", + "d-manual-20260804T180000Z.gc-pending-019f9ce3-2370-7a33-8706-fda0d0496d33", + ] { + assert!(is_internal_snapshot_name(staging), "{staging}"); + } + // The marker without an artifact id after it is part of an ordinary name, + // which the orphan sweep would never reclaim. + for ordinary in [ + "run.gc-pending-1", + "d-x.gc-pending-note", + ".gc-pending-", + "d-x.gc-pending-019f9ce3-2370-7a33-8706-fda0d0496d33.old", + ] { + assert!(!is_internal_snapshot_name(ordinary), "{ordinary}"); + } } #[test] diff --git a/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/named_delete.rs b/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/named_delete.rs new file mode 100644 index 0000000000..c6f20a06c4 --- /dev/null +++ b/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/named_delete.rs @@ -0,0 +1,185 @@ +//! DeleteSnapshots: named snapshots by name or all at once, through +//! [`crate::local_artifacts::LocalArtifacts::delete_named_snapshot`]. + +use super::*; +use crate::local_artifacts::{CheckoutError, NamedSnapshotDelete}; +use crate::snapshot_gc::delete_gc_staging; +use pplx_space_common_core::host::linux::paths::sandbox_snapshot_dir_in; + +/// [`crate::local_artifacts::LocalArtifacts::delete_named_snapshot`] with +/// node-manager's DeleteCacheArtifact of `local_path` as the in-place delete of +/// a snapshot without a row. +async fn delete_cached_snapshot( + service: &SandboxManagerService, + (sandbox_id, snapshot_name): (&str, &str), + local_path: String, + node_client: &mut pplx_space_node_mgr_client::NodeManagerGrpcClient, +) -> NamedSnapshotDelete { + let request = pplx_space_node_mgr_client::proto::DeleteCacheArtifactRequest { + local_path: local_path.clone(), + }; + let delete_in_place = async { node_client.delete_cache_artifact(request).await.map(drop) }; + service + .local_artifacts + .delete_named_snapshot(sandbox_id, snapshot_name, &local_path, delete_in_place) + .await +} + +fn snapshot_delete_refusal(error: CheckoutError) -> Status { + if error.is_in_use() { + Status::failed_precondition(format!("snapshot is in use: {error}")) + } else { + Status::internal(format!("inspect snapshot publication: {error}")) + } +} + +impl SandboxManagerService { + pub(in crate::service) async fn delete_snapshots( + &self, + request: Request, + ) -> Result, Status> { + self.authorize_peer(&request, "DeleteSnapshots", ALLOW_SANDBOX_OPERATOR)?; + let mut node_client = None; + let mut staged = Vec::new(); + let result = self + .delete_snapshots_locked(request.get_ref(), &mut node_client, &mut staged) + .await; + // The deleted rows owned this staging, so nothing does now and its + // delete waits for no lock. Under the artifact lock it would hold the + // sandbox's publication, rollback, export and backup enqueue behind + // node-manager's deletion lane. A failure is the orphan sweep's. + if let Some(client) = node_client.as_mut() { + for (snapshot_name, staging) in &staged { + let _ = delete_gc_staging(client, staging, "DeleteSnapshots", snapshot_name).await; + } + } + result + } + + /// The deletes under the sandbox's artifact lock. Each catalogued + /// snapshot's staging, with its name, goes to `staged` for the caller to + /// delete once the lock is released, on every return path. + async fn delete_snapshots_locked( + &self, + req: &DeleteSnapshotsRequest, + node_client: &mut Option, + staged: &mut Vec<(String, String)>, + ) -> Result, Status> { + let sandbox = self.lookup_sandbox(&req.sandbox_id)?; + let artifact_lock = self.auto_lifecycle.artifact_lock(&sandbox.id); + let _guard = artifact_lock.lock().await; + let sandbox = self.lookup_sandbox(&sandbox.id)?; + self.require_snapshot_mutation_ownership(&sandbox.id)?; + let protected_instance_path = sandbox.current_local_artifact_id.and_then(|artifact_id| { + self.local_artifacts + .get(artifact_id) + .ok() + .flatten() + .filter(|artifact| { + artifact.artifact_origin + != pplx_space_common_core::types::ArtifactOrigin::Snapshot + }) + .map(|artifact| artifact.local_path) + }); + // A test's own root, or the production cache. + let cache_root = self.cache_root().to_string_lossy(); + + let node_client = node_client.insert( + connect_node(&self.node_manager) + .await + .map_err(|e| Status::internal(format!("cannot connect to node-manager: {e}")))?, + ); + + if req.all { + let mut deleted = Vec::new(); + + // Delete btrfs snapshots (d-* subvolumes) via node-manager + let sandbox_id = sandbox + .id + .parse::() + .map_err(|error| Status::internal(format!("invalid stored sandbox id: {error}")))?; + let mut pages = pplx_space_node_mgr_client::SnapshotPages::new(sandbox_id, false); + while let Some(page) = pages.next(node_client).await? { + for snap in page { + if is_internal_snapshot_name(&snap.name) { + continue; + } + let local_path = sandbox_snapshot_dir_in(&cache_root, &sandbox.id, &snap.name); + if protected_instance_path.as_deref() == Some(local_path.as_str()) { + continue; + } + if deleted.len() >= 4096 { + return Err(Status::resource_exhausted( + "deleted 4096 snapshots; retry delete-all to continue", + )); + } + let names = (sandbox.id.as_str(), snap.name.as_str()); + match delete_cached_snapshot(self, names, local_path, node_client).await { + NamedSnapshotDelete::Deleted { staging } => { + staged.extend(staging.map(|staging| (snap.name.clone(), staging))); + deleted.push(snap.name); + } + NamedSnapshotDelete::BytesNotDeleted(status) => return Err(status), + // A refused row or a differing path: kept and left out of + // `deleted`, as delete-all did before rows were checked out. + kept => tracing::warn!( + target: super::LOG_TARGET, + sandbox_id = %sandbox.id, + snapshot_name = %snap.name, + outcome = ?kept, + "delete-all retains a snapshot it cannot delete", + ), + } + } + } + // Note: list_btrfs_snapshots returns both d-* and f-* entries, so the loop + // above already covers full snapshots. No separate f-* scan needed. + + Ok(Response::new(DeleteSnapshotsResponse { deleted })) + } else if let Some(snapshot_name) = req.snapshot_name.as_deref().filter(|s| !s.is_empty()) { + validate_snapshot_name(snapshot_name)?; + if is_internal_snapshot_name(snapshot_name) { + return Err(Status::failed_precondition( + "cannot delete an internal snapshot artifact", + )); + } + let local_path = sandbox_snapshot_dir_in(&cache_root, &sandbox.id, snapshot_name); + if protected_instance_path.as_deref() == Some(local_path.as_str()) { + return Err(Status::failed_precondition( + "cannot delete the current instance artifact", + )); + } + + // Both disk and full snapshots may be btrfs subvolumes — + // delete via node-manager (root). DeleteCacheArtifact + // handles both the subvolume case (privileged + // `btrfs subvolume delete`) and the plain-directory case, + // and is idempotent on a missing path. A catalogued snapshot + // is renamed to its GC staging here instead; the caller + // deletes that staging once the lock is released. + let names = (sandbox.id.as_str(), snapshot_name); + match delete_cached_snapshot(self, names, local_path, node_client).await { + NamedSnapshotDelete::Deleted { staging } => { + staged.extend(staging.map(|staging| (snapshot_name.to_owned(), staging))); + } + NamedSnapshotDelete::Refused(error) => return Err(snapshot_delete_refusal(error)), + NamedSnapshotDelete::PathDiffers { .. } => { + return Err(Status::failed_precondition( + "delete snapshot publication path differs", + )); + } + NamedSnapshotDelete::BytesNotDeleted(e) => { + return Err(Status::internal(format!("delete snapshot failed: {e}"))); + } + } + + Ok(Response::new(DeleteSnapshotsResponse { + deleted: vec![snapshot_name.to_string()], + })) + } else { + Err(Status::invalid_argument( + "provide snapshot_name or set all=true", + )) + } + } +} diff --git a/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/tests.rs b/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/tests.rs index f68e4be289..05b227c2ac 100644 --- a/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/tests.rs +++ b/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/tests.rs @@ -469,8 +469,13 @@ fn full_rollback_restage_writes_only_over_a_stopped_row() { /// ordering. An in-use row, including one a failed row commit left `deleting`, /// refuses the name. An unreadable row answers `Internal`, and a row naming /// another path `FailedPrecondition`. Delete-all omits each of them from -/// `deleted` and goes on. None reaches node-manager. A failed byte delete -/// answers `Internal` by name and passes node-manager's status through for all. +/// `deleted` and goes on. None reaches node-manager. A catalogued snapshot is +/// renamed to GC staging, and node-manager deletes that path once the +/// artifact lock is released; once the rename and the row delete are done the +/// snapshot is deleted, even if node-manager then fails. A snapshot without a +/// row is deleted in place under the lock, as before, and a failed +/// byte delete answers `Internal` by name and passes node-manager's status +/// through for all. #[tokio::test] async fn delete_snapshots_answers_each_named_delete_outcome() { use pplx_space_common_core::types::{ArtifactKind, LocalArtifactId, SnapshotName}; @@ -478,45 +483,60 @@ async fn delete_snapshots_answers_each_named_delete_outcome() { BtrfsSnapshotInfo, DeleteCacheArtifactRequest, DeleteCacheArtifactResponse, ListBtrfsSnapshotsRequest, ListBtrfsSnapshotsResponse, }; + use std::path::Path; use tonic::Code; #[derive(Clone, Copy, Debug, PartialEq)] enum Setup { Deletable, + StagingDeleteFails, Pinned, AlreadyDeleting, UnreadableRow, PathDiffers, UncataloguedDeleteFails, } + /// The path node-manager is asked to delete. + #[derive(Clone, Copy, Debug, PartialEq)] + enum Deletes { + Nothing, + LocalPath, + Staging, + } const NAME: &str = "d-user-keep"; // (setup, byte delete requested, single-name answer, delete-all answer: // whether the snapshot is listed as deleted; an error is its code and the // start of its message) type Answer = Result; - let cases: [(Setup, bool, Answer<()>, Answer); 6] = [ - (Setup::Deletable, true, Ok(()), Ok(true)), + let cases: [(Setup, Deletes, Answer<()>, Answer); 7] = [ + (Setup::Deletable, Deletes::Staging, Ok(()), Ok(true)), + ( + Setup::StagingDeleteFails, + Deletes::Staging, + Ok(()), + Ok(true), + ), ( Setup::Pinned, - false, + Deletes::Nothing, Err((Code::FailedPrecondition, "snapshot is in use")), Ok(false), ), ( Setup::AlreadyDeleting, - false, + Deletes::Nothing, Err((Code::FailedPrecondition, "snapshot is in use")), Ok(false), ), ( Setup::UnreadableRow, - false, + Deletes::Nothing, Err((Code::Internal, "inspect snapshot publication")), Ok(false), ), ( Setup::PathDiffers, - false, + Deletes::Nothing, Err(( Code::FailedPrecondition, "delete snapshot publication path differs", @@ -525,7 +545,7 @@ async fn delete_snapshots_answers_each_named_delete_outcome() { ), ( Setup::UncataloguedDeleteFails, - true, + Deletes::LocalPath, Err((Code::Internal, "delete snapshot failed")), Err((Code::Unavailable, "injected byte delete failure")), ), @@ -533,7 +553,13 @@ async fn delete_snapshots_answers_each_named_delete_outcome() { for all in [false, true] { for (setup, byte_delete, single, delete_all) in cases { let case = format!("{setup:?}, all={all}"); + let sandbox = SandboxId::new().to_string(); + // Each requested path, with whether the sandbox's artifact lock + // was held while node-manager deleted it. let deletions = Arc::new(std::sync::Mutex::new(Vec::new())); + let coordinator = Arc::new(std::sync::OnceLock::< + crate::auto_lifecycle::AutoLifecycleCoordinator, + >::new()); let node_manager = crate::node_manager_transport::fake::FakeNodeManager::default() .unary("ListBtrfsSnapshots", |_: ListBtrfsSnapshotsRequest| { Ok(ListBtrfsSnapshotsResponse { @@ -545,10 +571,26 @@ async fn delete_snapshots_answers_each_named_delete_outcome() { }) }) .unary("DeleteCacheArtifact", { - let deletions = Arc::clone(&deletions); + let (deletions, coordinator, sandbox) = ( + Arc::clone(&deletions), + Arc::clone(&coordinator), + sandbox.clone(), + ); move |request: DeleteCacheArtifactRequest| { - deletions.lock().unwrap().push(request.local_path); - if setup == Setup::UncataloguedDeleteFails { + let lock_held = coordinator + .get() + .expect("the service is built before any delete") + .artifact_lock(&sandbox) + .try_lock_owned() + .is_err(); + deletions + .lock() + .unwrap() + .push((request.local_path, lock_held)); + if matches!( + setup, + Setup::StagingDeleteFails | Setup::UncataloguedDeleteFails + ) { return Err(Status::unavailable("injected byte delete failure")); } Ok(DeleteCacheArtifactResponse { was_present: true }) @@ -557,7 +599,6 @@ async fn delete_snapshots_answers_each_named_delete_outcome() { .serve(); let scratch = tempfile::tempdir().unwrap(); let conn = crate::db::test_conn(); - let sandbox = SandboxId::new().to_string(); crate::db::fixture::insert_test_sandbox(&conn, &sandbox, SandboxState::Stopped); let service = SandboxManagerService::new_with_options( conn, @@ -566,12 +607,27 @@ async fn delete_snapshots_answers_each_named_delete_outcome() { ..SandboxManagerServiceOptions::for_test_in(scratch.path()) }, ); - let local_path = - pplx_space_common_core::host::linux::paths::sandbox_snapshot_dir(&sandbox, NAME); + coordinator + .set(service.auto_lifecycle.clone()) + .unwrap_or_else(|_| unreachable!()); + // `for_test_in` roots the sandbox cache here. + let local_path = format!( + "{}/cache/{sandbox}/snapshots/{NAME}", + scratch.path().display() + ); + std::fs::create_dir_all(&local_path).unwrap(); let artifact = LocalArtifactId::new(); + let staging = format!( + "{}/cache/{sandbox}/snapshots/.gc-pending-{artifact}", + scratch.path().display() + ); if matches!( setup, - Setup::Deletable | Setup::Pinned | Setup::AlreadyDeleting | Setup::PathDiffers + Setup::Deletable + | Setup::StagingDeleteFails + | Setup::Pinned + | Setup::AlreadyDeleting + | Setup::PathDiffers ) { let published = if setup == Setup::PathDiffers { format!("{local_path}-elsewhere") @@ -614,7 +670,10 @@ async fn delete_snapshots_answers_each_named_delete_outcome() { ) .unwrap(); } - Setup::Deletable | Setup::PathDiffers | Setup::UncataloguedDeleteFails => {} + Setup::Deletable + | Setup::StagingDeleteFails + | Setup::PathDiffers + | Setup::UncataloguedDeleteFails => {} } drop(db); @@ -645,12 +704,225 @@ async fn delete_snapshots_answers_each_named_delete_outcome() { } (result, expected) => panic!("{case}: {result:?}, expected {expected:?}"), } - let requested = if byte_delete { - vec![local_path] - } else { - Vec::new() + // A snapshot without a row is deleted in place under the lock, as + // before; staging is deleted once the lock is released. + let requested = match byte_delete { + Deletes::Nothing => Vec::new(), + Deletes::LocalPath => vec![(local_path.clone(), true)], + Deletes::Staging => vec![(staging.clone(), false)], }; assert_eq!(*deletions.lock().unwrap(), requested, "{case}"); + // The fake deletes nothing, so the bytes are wherever sbx-mgr left them. + let staged = byte_delete == Deletes::Staging; + assert_eq!(Path::new(&local_path).exists(), !staged, "{case}"); + assert_eq!(Path::new(&staging).exists(), staged, "{case}"); } } } + +/// GC staging, in the fixed-length `.gc-pending-` shape and in the earlier +/// `.gc-pending-` one that keeps the snapshot's own name. +fn gc_staging_names() -> [String; 2] { + let id = pplx_space_common_core::types::LocalArtifactId::new(); + [ + format!(".gc-pending-{id}"), + format!("d-user-keep.gc-pending-{id}"), + ] +} + +/// Names that contain the staging marker without an artifact id after it, so +/// they are ordinary snapshots. +const MARKER_IN_ORDINARY_NAMES: [&str; 2] = ["d-x.gc-pending-note", "run.gc-pending-1"]; + +/// Node-manager lists GC staging among a sandbox's snapshots, and ListSnapshots +/// leaves it out, from that listing and from its own scan for full snapshots. +/// A name that merely contains the marker is listed. +#[test] +fn snapshot_inventory_omits_gc_staging() { + let root = tempfile::tempdir().unwrap(); + let id = pplx_space_common_core::types::LocalArtifactId::new(); + // Only on disk: the scan for full snapshots finds it. + std::fs::create_dir( + root.path() + .join(format!("f-manual-20260101T000000Z.gc-pending-{id}")), + ) + .unwrap(); + let btrfs = std::iter::once("d-user-keep".to_string()) + .chain(MARKER_IN_ORDINARY_NAMES.map(str::to_owned)) + .chain(gc_staging_names()) + .map(|name| { + std::fs::create_dir(root.path().join(&name)).unwrap(); + pplx_space_node_mgr_client::proto::BtrfsSnapshotInfo { + name, + ..Default::default() + } + }) + .collect(); + let snapshots = + project_snapshot_inventory("sbx", "template", None, root.path(), btrfs, Vec::new()) + .unwrap(); + let listed: Vec<_> = snapshots + .iter() + .map(|snapshot| snapshot.id.as_str()) + .collect(); + let mut expected = ["d-user-keep"] + .into_iter() + .chain(MARKER_IN_ORDINARY_NAMES) + .collect::>(); + let mut listed = listed; + listed.sort_unstable(); + expected.sort_unstable(); + assert_eq!(listed, expected); +} + +/// GC staging is logically deleted bytes that only the orphan sweep removes, so +/// no snapshot RPC treats it as a snapshot. Deleting it by name, rolling back to +/// it and exporting it are refused, and delete-all passes over it. None reaches +/// node-manager's delete, and the staging stays for the sweep. A name that +/// merely contains the marker is an ordinary snapshot, deleted by name and by +/// delete-all. +#[tokio::test] +async fn snapshot_rpcs_refuse_gc_staging() { + use pplx_space_node_mgr_client::proto::{ + BtrfsSnapshotInfo, DeleteCacheArtifactRequest, DeleteCacheArtifactResponse, + ListBtrfsSnapshotsRequest, ListBtrfsSnapshotsResponse, + }; + + let staging = gc_staging_names(); + let deletions = Arc::new(std::sync::Mutex::new(Vec::new())); + let listed: Vec = staging + .iter() + .cloned() + .chain(MARKER_IN_ORDINARY_NAMES.map(str::to_owned)) + .collect(); + let node_manager = crate::node_manager_transport::fake::FakeNodeManager::default() + .unary("ListBtrfsSnapshots", move |_: ListBtrfsSnapshotsRequest| { + Ok(ListBtrfsSnapshotsResponse { + snapshots: listed + .iter() + .map(|name| BtrfsSnapshotInfo { + name: name.clone(), + ..Default::default() + }) + .collect(), + next_page_token: String::new(), + }) + }) + .unary("DeleteCacheArtifact", { + let deletions = Arc::clone(&deletions); + move |request: DeleteCacheArtifactRequest| { + deletions.lock().unwrap().push(request.local_path); + Ok(DeleteCacheArtifactResponse { was_present: true }) + } + }) + .serve(); + let scratch = tempfile::tempdir().unwrap(); + let conn = crate::db::test_conn(); + let sandbox = SandboxId::new().to_string(); + crate::db::fixture::insert_test_sandbox(&conn, &sandbox, SandboxState::Stopped); + let service = SandboxManagerService::new_with_options( + conn, + SandboxManagerServiceOptions { + node_manager: node_manager.connector(), + ..SandboxManagerServiceOptions::for_test_in(scratch.path()) + }, + ); + let refused = |result: Result<(), Status>, rpc: &str, name: &str, message: &str| { + let status = result.expect_err(rpc); + assert_eq!( + status.code(), + tonic::Code::FailedPrecondition, + "{rpc} {name}: {status}" + ); + assert_eq!(status.message(), message, "{rpc} {name}"); + }; + for name in &staging { + let deleted = service + .delete_snapshots(crate::service::activation_support::internal_request( + DeleteSnapshotsRequest { + sandbox_id: sandbox.clone(), + all: false, + snapshot_name: Some(name.clone()), + }, + )) + .await + .map(drop); + refused( + deleted, + "DeleteSnapshots", + name, + "cannot delete an internal snapshot artifact", + ); + let rolled_back = service + .rollback(crate::service::activation_support::internal_request( + RollbackRequest { + sandbox_id: sandbox.clone(), + snapshot_name: name.clone(), + ..Default::default() + }, + )) + .await + .map(drop); + refused( + rolled_back, + "Rollback", + name, + "cannot rollback to an internal snapshot artifact", + ); + let exported = service + .export_snapshot(crate::service::activation_support::internal_request( + crate::proto::ExportSnapshotRequest { + sandbox_id: sandbox.clone(), + snapshot_name: name.clone(), + ..Default::default() + }, + )) + .await + .map(drop); + refused( + exported, + "ExportSnapshot", + name, + "cannot export an internal snapshot artifact", + ); + } + assert!(deletions.lock().unwrap().is_empty()); + let ordinary_path = |name: &str| { + format!( + "{}/{sandbox}/snapshots/{name}", + service.cache_root().display() + ) + }; + for name in MARKER_IN_ORDINARY_NAMES { + let deleted = service + .delete_snapshots(crate::service::activation_support::internal_request( + DeleteSnapshotsRequest { + sandbox_id: sandbox.clone(), + all: false, + snapshot_name: Some(name.to_owned()), + }, + )) + .await + .unwrap() + .into_inner(); + assert_eq!(deleted.deleted, [name]); + } + let all = service + .delete_snapshots(crate::service::activation_support::internal_request( + DeleteSnapshotsRequest { + sandbox_id: sandbox.clone(), + all: true, + snapshot_name: None, + }, + )) + .await + .unwrap() + .into_inner(); + assert_eq!(all.deleted, MARKER_IN_ORDINARY_NAMES); + let by_name_then_all: Vec<_> = MARKER_IN_ORDINARY_NAMES + .iter() + .chain(&MARKER_IN_ORDINARY_NAMES) + .map(|name| ordinary_path(name)) + .collect(); + assert_eq!(*deletions.lock().unwrap(), by_name_then_all); +} diff --git a/crates/libs/sbx/mgr/server/src/snapshot_gc.rs b/crates/libs/sbx/mgr/server/src/snapshot_gc.rs index b3fed6845e..d2c5b64668 100644 --- a/crates/libs/sbx/mgr/server/src/snapshot_gc.rs +++ b/crates/libs/sbx/mgr/server/src/snapshot_gc.rs @@ -49,7 +49,7 @@ //! commit; a state-writer lease keeps this work owned through shutdown. //! //! The rmdir of the staged directory runs after commit and lock release. If it fails, the staging -//! directory persists under `.gc-pending-*` with no row +//! directory persists as `.gc-pending-` beside `local_path` with no row //! referencing it — invisible to the planner. The orphan sweep //! covers that gap: it runs once at startup (even when the reaper //! is disabled — the bytes are logically deleted either way) and @@ -58,7 +58,7 @@ use std::collections::HashSet; use std::os::unix::fs::MetadataExt; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::sync::Arc; use std::time::{Duration, Instant}; @@ -72,7 +72,9 @@ use crate::auto_lifecycle::now_secs; use crate::db::snapshot_gc::ArtifactScan; mod orphans; -use crate::local_artifacts::{CheckoutError, GcDeleteCheckout, GcEligibility, LocalArtifacts}; +use crate::local_artifacts::{ + CheckoutError, GcDeleteCheckout, GcEligibility, GcStagingError, LocalArtifacts, +}; pub(crate) struct GcOwnership { pub coordinator: crate::auto_lifecycle::AutoLifecycleCoordinator, @@ -268,12 +270,13 @@ pub(crate) fn dir_size_bytes(path: &Path) -> u64 { /// Ready → Deleting under the db lock. The check_out also /// validates backup, pin, idle/grace, and shared-path evidence. On /// success the row is reserved. -/// 2. atomic `rename(local_path → .gc-pending-)` outside the -/// lock. If it fails for a non-ENOENT reason the lease drops -/// and the row rolls back to Ready. -/// 3. `lease.commit()` — DELETEs the row under the lock again. If -/// it fails the row stays Deleting, and startup recovery -/// finishes the delete. +/// 2. atomic `rename(local_path → /.gc-pending-)` outside +/// the lock. If it fails for a non-ENOENT reason the row is rolled +/// back to Ready explicitly, and dropping the lease retries a +/// rollback that failed. +/// 3. the row DELETE under the lock again. If it fails the row stays +/// Deleting, and startup recovery finishes the delete. Steps 2 and 3 +/// are `DeleteLease::commit_through_gc_staging`. /// The on-disk cleanup of the staging dir runs **after** this /// function returns, via node-mgr's `DeleteCacheArtifact` RPC /// (which can `btrfs subvolume delete` a subvolume root that @@ -292,7 +295,7 @@ fn execute_one( let candidate_id: LocalArtifactId = candidate_id_str .parse() .map_err(|e| format!("invalid local_artifact_id: {e}"))?; - let mut lease = match artifacts.check_out_for_gc(candidate_id, sandbox_id, eligibility) { + let lease = match artifacts.check_out_for_gc(candidate_id, sandbox_id, eligibility) { Ok(GcDeleteCheckout::Leased(lease)) => lease, Ok(GcDeleteCheckout::Pinned) | Err(CheckoutError::Pinned(_)) => { return Ok((EvictionOutcome::SkippedPinnedCurrentAtExecute, None)); @@ -329,33 +332,31 @@ fn execute_one( } }; - // Atomic rename into a `.gc-pending-` sibling. Linux's - // open-file semantics keep any in-flight reader's fd valid; new - // opens of the original path fail with ENOENT. The lease holds - // the row in 'deleting' across the rename, and from here a failed - // commit leaves it 'deleting' for startup recovery rather than - // 'ready' advertising a missing path. - let staging_to_clean = match lease.vacate_to_gc_staging() { + // Atomic rename into a `.gc-pending-` sibling, then DELETE the row. + // Linux's open-file semantics keep any in-flight reader's fd valid; new + // opens of the original path fail with ENOENT. The lease holds the row in + // 'deleting' across the rename, and from there a failed commit leaves it + // 'deleting' for startup recovery rather than 'ready' advertising a + // missing path. The DELETE is gated by `state = 'deleting'` so a + // concurrent rollback (impossible while we hold the lease, but defensive) + // cannot turn this into a spurious deletion. Named-snapshot deletes share + // this protocol (`LocalArtifacts::delete_named_snapshot`). + let local_path = lease.local_path().to_owned(); + let staging_to_clean = match lease.commit_through_gc_staging() { Ok(staging) => staging, - Err(err) => { + Err(GcStagingError::Rename(err)) => { warn!( local_artifact_id = %candidate_id, - local_path = %lease.local_path(), + local_path = %local_path, error = %err, - "snapshot reaper: rename to staging path failed; lease will roll state back", + "snapshot reaper: rename to staging path failed; row rolled back to 'ready'", ); - // Drop the lease without commit → DeleteLease::drop - // rolls back to 'ready'. Next tick retries. + // Next tick retries. return Err(format!("rename failed: {err}")); } + Err(GcStagingError::Commit(e)) => return Err(format!("delete commit: {e}")), }; - // Finalize: DELETE the row. The DELETE is gated by `state = - // 'deleting'` so a concurrent rollback (impossible while we - // hold the lease, but defensive) cannot turn this into a - // spurious deletion. - lease.commit().map_err(|e| format!("delete commit: {e}"))?; - // The staging directory cleanup runs after this function // returns, via node-mgr's `DeleteCacheArtifact` RPC. Returning // the path lets the async caller invoke that without making @@ -524,9 +525,9 @@ async fn gc_tick( // named snapshots and d-stop-* stopped artifacts — // sbx-mgr's UID can't `rmdir` the subvol root) and // plain directories (suspend snapshots written file- - // by-file). On RPC failure the staging dir leaks - // until the next sbx-mgr startup sweep, which now - // also goes through this RPC. + // by-file). On RPC failure the staging dir waits + // for the orphan sweep, which also goes through + // this RPC. if let Some(staging) = staging_to_clean { delete_cache_artifact_via_node_mgr( &staging, @@ -598,8 +599,10 @@ async fn gc_tick( const CACHE_ROOT_FOR_SWEEP: &str = pplx_space_common_core::host::linux::paths::SANDBOX_CACHE_DIR; /// Maximum directory-walk depth below [`CACHE_ROOT_FOR_SWEEP`]. Staging -/// directories sit at `/snapshot.gc-pending-` and -/// `/{snapshots,template-snapshot-staging}/.gc-pending-`, +/// directories sit at `/.gc-pending-` and +/// `/{snapshots,template-snapshot-staging}/.gc-pending-`, or, +/// left by an earlier version, at `/snapshot.gc-pending-` and +/// `/{snapshots,template-snapshot-staging}/.gc-pending-`: /// the shapes node-mgr accepts. The walk reads no deeper, and inside a /// sandbox's directory it enters only those snapshot directories /// (`OrphanKind::descends_into`): the rest are artifact contents and the jail @@ -608,7 +611,7 @@ const SWEEP_MAX_DEPTH: usize = 3; const FORK_COPY_ORPHAN_MIN_AGE_SECS: u64 = 60 * 60; /// Issue a `DeleteCacheArtifact` RPC against node-mgr. Best-effort; -/// callers (per-candidate reap finalize, startup sweep) only log on +/// callers (per-candidate reap finalize, orphan sweep) only log on /// failure rather than abort, because the parent operation has /// already committed (the row is gone, or this was orphan cleanup /// to begin with). @@ -624,32 +627,53 @@ async fn delete_cache_artifact_via_node_mgr( candidate = %candidate_label, staging_path, error = %err, - "snapshot reaper: node-mgr connect for DeleteCacheArtifact failed; staging directory will be retried at next sbx-mgr start", + "snapshot reaper: node-mgr connect for DeleteCacheArtifact failed; the orphan sweep retries the staging directory", ); return; } }; + if delete_gc_staging( + &mut client, + staging_path, + "snapshot reaper", + candidate_label, + ) + .await + .is_ok() + { + info!( + candidate = %candidate_label, + staging_path, + "snapshot reaper: deleted cache artifact via node-mgr", + ); + } +} + +/// Delete GC staging through node-mgr's `DeleteCacheArtifact`, which removes a +/// btrfs subvolume root that sbx-mgr's UID cannot `rmdir` as well as a plain +/// directory. Its row is already gone, so a failure is only logged here: the +/// orphan sweep finds unowned staging on every pass and retries it. Named +/// snapshot deletes and retention call this too, after releasing their locks, +/// so the log names the deleter in `deleter`. +pub(crate) async fn delete_gc_staging( + client: &mut pplx_space_node_mgr_client::NodeManagerGrpcClient, + staging_path: &str, + deleter: &str, + candidate_label: &str, +) -> Result<(), tonic::Status> { let req = pplx_space_node_mgr_client::proto::DeleteCacheArtifactRequest { local_path: staging_path.to_string(), }; - match client.delete_cache_artifact(req).await { - Ok(_) => { - info!( - candidate = %candidate_label, - staging_path, - "snapshot reaper: deleted cache artifact via node-mgr", - ); - } - Err(status) => { - warn!( - candidate = %candidate_label, - staging_path, - code = %status.code(), - message = %status.message(), - "snapshot reaper: DeleteCacheArtifact RPC failed; staging directory will be retried at next sbx-mgr start", - ); - } - } + client.delete_cache_artifact(req).await.map(drop).inspect_err(|status| { + warn!( + deleter, + candidate = %candidate_label, + staging_path, + code = %status.code(), + message = %status.message(), + "GC staging: DeleteCacheArtifact RPC failed; the orphan sweep retries the staging directory", + ); + }) } /// Visit one bounded namespace page. Failed deletions stay on disk and are @@ -657,6 +681,7 @@ async fn delete_cache_artifact_via_node_mgr( async fn sweep_orphans( scan: &mut OrphanScan, kind: OrphanKind, + cache_root: &Path, artifacts: &LocalArtifacts, active_copy_operations: Arc, ownership: &GcOwnership, @@ -665,10 +690,8 @@ async fn sweep_orphans( return; } let root = match kind { - OrphanKind::GcPending => PathBuf::from(CACHE_ROOT_FOR_SWEEP), - OrphanKind::ForkCopy => crate::service::fork_artifact::copy_staging_root_under(Path::new( - pplx_space_common_core::host::linux::paths::SANDBOX_CACHE_DIR, - )), + OrphanKind::GcPending => cache_root.to_path_buf(), + OrphanKind::ForkCopy => crate::service::fork_artifact::copy_staging_root_under(cache_root), }; let mut next_scan = std::mem::take(scan); let discovery_active_operations = Arc::clone(&active_copy_operations); @@ -740,12 +763,12 @@ async fn sweep_orphans( } } -fn gc_pending_is_unowned(artifacts: &LocalArtifacts, path: &Path) -> bool { +/// Whether `path` is GC staging that no row owns, so its bytes may be deleted. +pub(crate) fn gc_pending_is_unowned(artifacts: &LocalArtifacts, path: &Path) -> bool { let Some(id) = path .file_name() .and_then(|name| name.to_str()) - .and_then(|name| name.rsplit_once(".gc-pending-")) - .and_then(|(_, id)| id.parse::().ok()) + .and_then(crate::local_artifacts::gc_staging_id) else { warn!(path = %path.display(), "retaining GC staging with invalid artifact identity"); return false; @@ -812,6 +835,7 @@ pub(crate) async fn run_loop( sweep_orphans( &mut gc_orphans, OrphanKind::GcPending, + Path::new(CACHE_ROOT_FOR_SWEEP), &artifacts, Arc::clone(&active_fork_copy_operations), &ownership, @@ -820,6 +844,7 @@ pub(crate) async fn run_loop( sweep_orphans( &mut fork_orphans, OrphanKind::ForkCopy, + Path::new(CACHE_ROOT_FOR_SWEEP), &artifacts, Arc::clone(&active_fork_copy_operations), &ownership, diff --git a/crates/libs/sbx/mgr/server/src/snapshot_gc/orphans.rs b/crates/libs/sbx/mgr/server/src/snapshot_gc/orphans.rs index d87c4f458a..e91d2feaf5 100644 --- a/crates/libs/sbx/mgr/server/src/snapshot_gc/orphans.rs +++ b/crates/libs/sbx/mgr/server/src/snapshot_gc/orphans.rs @@ -90,7 +90,7 @@ impl OrphanScan { OrphanKind::GcPending => entry .file_name() .to_str() - .is_some_and(|name| name.contains(".gc-pending-")), + .is_some_and(crate::local_artifacts::is_gc_staging_name), OrphanKind::ForkCopy => { if crate::service::fork_artifact::is_copy_operation_reserved(active, &path) { continue; diff --git a/crates/libs/sbx/mgr/server/src/snapshot_gc/tests.rs b/crates/libs/sbx/mgr/server/src/snapshot_gc/tests.rs index 14837f145b..5f68a7c48c 100644 --- a/crates/libs/sbx/mgr/server/src/snapshot_gc/tests.rs +++ b/crates/libs/sbx/mgr/server/src/snapshot_gc/tests.rs @@ -196,7 +196,7 @@ async fn within_node_manager_wait(future: F) -> F::Outpu } /// Backup-completed + idle + not pinned → reaped. The row is gone, and the -/// artifact directory moves to its `.gc-pending-` staging sibling, which +/// artifact directory moves to `.gc-pending-` beside it, which /// node-manager is asked to delete. #[tokio::test(start_paused = true)] async fn reaps_idle_uploaded_unpinned_artifact() { @@ -242,7 +242,7 @@ async fn reaps_idle_uploaded_unpinned_artifact() { assert_eq!(report.deleted, 1); assert!(report.bytes_freed >= 4096); assert!(!std::path::Path::new(&path).exists()); - let staging = format!("{path}.gc-pending-{TEST_ART_ID}"); + let staging = format!("{}/.gc-pending-{TEST_ART_ID}", tmp.path().display()); // The fake acknowledges without deleting, so the staging directory stays. assert!(std::path::Path::new(&staging).is_dir()); assert_eq!(*deletions.lock().unwrap(), vec![staging]); @@ -479,7 +479,9 @@ fn sweep_discovers_gc_pending_orphans_and_skips_unrelated() { let tmp = TempDir::new().unwrap(); let sandbox_dir = tmp.path().join("sbx-abc/snapshot"); std::fs::create_dir_all(&sandbox_dir).unwrap(); - let orphan = tmp.path().join("sbx-abc/snapshot.gc-pending-deadbeef"); + let orphan = tmp + .path() + .join(format!("sbx-abc/snapshot.gc-pending-{TEST_ART_ID}")); std::fs::create_dir_all(&orphan).unwrap(); std::fs::write(orphan.join("memory"), b"stale").unwrap(); let snapshots = tmp.path().join("sbx-abc/snapshots"); @@ -491,9 +493,12 @@ fn sweep_discovers_gc_pending_orphans_and_skips_unrelated() { std::fs::create_dir(unrelated.join("stray.gc-pending-held")).unwrap(); // A wide first directory spans pages without growing retained discovery. - for index in 0..270 { - std::fs::create_dir(snapshots.join(format!("item.gc-pending-{index}"))).unwrap(); + for _ in 0..270 { + std::fs::create_dir(snapshots.join(format!("item.gc-pending-{}", LocalArtifactId::new()))) + .unwrap(); } + // The marker without an artifact id is part of a snapshot's own name. + std::fs::create_dir(snapshots.join("run.gc-pending-1")).unwrap(); let outside = TempDir::new().unwrap(); std::fs::create_dir(outside.path().join("outside.gc-pending-held")).unwrap(); std::os::unix::fs::symlink(outside.path(), tmp.path().join("escape")).unwrap(); @@ -774,7 +779,7 @@ fn row_commit_failure_after_the_rename_is_finished_by_startup_recovery() { if present { make_artifact_dir(&tmp, "art1", 4096); } - let staging = format!("{path}.gc-pending-{TEST_ART_ID}"); + let staging = format!("{}/.gc-pending-{TEST_ART_ID}", tmp.path().display()); if staging_occupied { fs::create_dir_all(&staging).unwrap(); fs::write(Path::new(&staging).join("occupant"), b"x").unwrap(); @@ -823,3 +828,242 @@ fn row_commit_failure_after_the_rename_is_finished_by_startup_recovery() { assert_eq!(row.is_none(), finished == 1, "{case}"); } } + +/// Node-manager deletes some snapshot trees entry by entry, so a delete can +/// fail partway. A named delete stages the bytes as the reaper does and hands +/// the staging back, so such a failure in its caller's staging delete (this +/// module's `delete_gc_staging`) leaves part of the tree in +/// `.gc-pending-`, never at `local_path`; with its row gone, this module's +/// orphan sweep reclaims it. +#[tokio::test] +async fn named_delete_failing_partway_leaves_staging_for_the_orphan_sweep() { + use crate::local_artifacts::NamedSnapshotDelete; + use pplx_space_common_core::types::{ArtifactKind, SandboxId, SnapshotName}; + use pplx_space_node_mgr_client::proto::{ + DeleteCacheArtifactRequest, DeleteCacheArtifactResponse, + }; + + const NAME: &str = "d-user-partial"; + let root = TempDir::new().unwrap(); + let cache_root = root.path().to_str().unwrap().to_owned(); + let sandbox = SandboxId::new().to_string(); + let local_path = pplx_space_common_core::host::linux::paths::sandbox_snapshot_dir_in( + &cache_root, + &sandbox, + NAME, + ); + fs::create_dir_all(Path::new(&local_path).join("nested")).unwrap(); + for file in ["memory", "vmstate", "nested/rootfs"] { + fs::write(Path::new(&local_path).join(file), file).unwrap(); + } + let conn = open_test_db(); + crate::db::fixture::insert_test_sandbox(&conn, &sandbox, crate::state::SandboxState::Stopped); + let db = crate::db::SharedConnection::new(conn); + let artifacts = LocalArtifacts::new(db.clone()); + let id = LocalArtifactId::new(); + artifacts + .publish_named_snapshot( + &sandbox, + id, + ArtifactKind::Disk, + &SnapshotName::parse(NAME).unwrap(), + &local_path, + ) + .unwrap(); + let staging = format!("{cache_root}/{sandbox}/snapshots/.gc-pending-{id}"); + // The first delete removes one file of the tree and one of its directory, + // then fails; later ones delete the whole path. + let deletions = Arc::new(Mutex::new(Vec::new())); + let node_manager = crate::node_manager_transport::fake::FakeNodeManager::default() + .unary("DeleteCacheArtifact", { + let (deletions, root) = (Arc::clone(&deletions), root.path().to_owned()); + move |request: DeleteCacheArtifactRequest| { + let path = Path::new(&request.local_path); + assert!(path.starts_with(&root)); + let mut deletions = deletions.lock().unwrap(); + deletions.push(request.local_path.clone()); + if deletions.len() == 1 { + fs::remove_file(path.join("memory")).unwrap(); + fs::remove_file(path.join("nested/rootfs")).unwrap(); + return Err(tonic::Status::deadline_exceeded("injected partial delete")); + } + fs::remove_dir_all(path).unwrap(); + Ok(DeleteCacheArtifactResponse { was_present: true }) + } + }) + .serve(); + + let result = artifacts + .delete_named_snapshot(&sandbox, NAME, &local_path, async { + Err(tonic::Status::internal("never deleted in place")) + }) + .await; + let NamedSnapshotDelete::Deleted { + staging: Some(handed), + } = result + else { + panic!("expected the staging back, got {result:?}"); + }; + assert_eq!(handed, staging); + let mut client = node_manager.connector().lazy(); + delete_gc_staging(&mut client, &handed, "test", NAME) + .await + .unwrap_err(); + + assert!(artifacts.get(id).unwrap().is_none()); + assert!(!Path::new(&local_path).exists()); + assert_eq!( + fs::read(Path::new(&staging).join("vmstate")).unwrap(), + b"vmstate" + ); + assert!(!Path::new(&staging).join("memory").exists()); + + let ownership = GcOwnership { + node_manager: node_manager.connector(), + ..test_ownership() + }; + sweep_orphans( + &mut OrphanScan::default(), + OrphanKind::GcPending, + root.path(), + &artifacts, + Arc::new(crate::service::fork_artifact::CopyOperationReservations::default()), + &ownership, + ) + .await; + + assert_eq!( + *deletions.lock().unwrap(), + vec![staging.clone(), staging.clone()] + ); + assert!(!Path::new(&staging).exists()); +} + +/// The sweep reclaims unowned staging in both shapes: the fixed-length +/// `.gc-pending-` and `.gc-pending-`, which an earlier version +/// left, beside a suspend artifact and inside `snapshots/`. Staging whose row +/// still exists is kept, and so is a snapshot whose name contains the marker +/// with no artifact id after it. +#[tokio::test] +async fn orphan_sweep_reclaims_staging_in_either_shape() { + use pplx_space_node_mgr_client::proto::{ + DeleteCacheArtifactRequest, DeleteCacheArtifactResponse, + }; + + let root = TempDir::new().unwrap(); + let sandbox = root.path().join("01939e3e-0000-7000-8000-0000000000aa"); + fs::create_dir_all(sandbox.join("snapshots")).unwrap(); + let ids: Vec = (0..5).map(|_| LocalArtifactId::new()).collect(); + let unowned = [ + sandbox.join(format!(".gc-pending-{}", ids[0])), + sandbox.join(format!("snapshot.gc-pending-{}", ids[1])), + sandbox.join(format!("snapshots/.gc-pending-{}", ids[2])), + sandbox.join(format!( + "snapshots/d-1m-20260101T000000Z.gc-pending-{}", + ids[3] + )), + ]; + let owned = sandbox.join(format!("snapshots/.gc-pending-{}", ids[4])); + let ordinary = sandbox.join("snapshots/run.gc-pending-1"); + for dir in unowned.iter().chain([&owned, &ordinary]) { + fs::create_dir(dir).unwrap(); + fs::write(dir.join("memory"), b"staged").unwrap(); + } + let conn = open_test_db(); + insert_artifact_row( + &conn, + &ids[4].to_string(), + "sbx-owned", + "/missing", + 0, + 0, + None, + ); + let artifacts = LocalArtifacts::new(crate::db::SharedConnection::new(conn)); + let deletions = Arc::new(Mutex::new(Vec::new())); + let node_manager = crate::node_manager_transport::fake::FakeNodeManager::default() + .unary("DeleteCacheArtifact", { + let (deletions, root) = (Arc::clone(&deletions), root.path().to_owned()); + move |request: DeleteCacheArtifactRequest| { + assert!(Path::new(&request.local_path).starts_with(&root)); + fs::remove_dir_all(&request.local_path).unwrap(); + deletions.lock().unwrap().push(request.local_path); + Ok(DeleteCacheArtifactResponse { was_present: true }) + } + }) + .serve(); + let ownership = GcOwnership { + node_manager: node_manager.connector(), + ..test_ownership() + }; + + sweep_orphans( + &mut OrphanScan::default(), + OrphanKind::GcPending, + root.path(), + &artifacts, + Arc::new(crate::service::fork_artifact::CopyOperationReservations::default()), + &ownership, + ) + .await; + + let mut deleted = deletions.lock().unwrap().clone(); + deleted.sort(); + let mut expected: Vec = unowned + .iter() + .map(|dir| dir.to_string_lossy().into_owned()) + .collect(); + expected.sort(); + assert_eq!(deleted, expected); + assert!(unowned.iter().all(|dir| !dir.exists())); + assert!(owned.join("memory").exists()); + assert!(ordinary.join("memory").exists()); +} + +/// The reaper evicts artifacts whose names are as long as a filename can be: +/// staging has a fixed-length name, where `.gc-pending-` would fail +/// the rename with ENAMETOOLONG from 208 bytes and leave the artifact +/// unevictable. +#[test] +fn reaper_evicts_artifacts_whose_names_are_long() { + for length in [207, 208, 255] { + let conn = open_test_db(); + let tmp = TempDir::new().unwrap(); + let name = format!("d-{}", "x".repeat(length - 2)); + let path = make_artifact_dir(&tmp, &name, 4096); + insert_artifact_row(&conn, TEST_ART_ID, "sbx1", &path, 0, 0, Some(0)); + insert_sandbox_row(&conn, "sbx1", None); + let artifacts = LocalArtifacts::new(crate::db::SharedConnection::new(conn)); + + let (outcome, staging) = execute_one( + &artifacts, + TEST_ART_ID, + "sbx1", + GcEligibility { + now_secs: now_secs(), + idle_secs: 0, + grace_secs: 0, + }, + ) + .unwrap_or_else(|error| panic!("{length} bytes: {error}")); + + let expected = format!("{}/.gc-pending-{TEST_ART_ID}", tmp.path().display()); + assert_eq!(outcome, EvictionOutcome::Deleted, "{length} bytes"); + assert_eq!( + staging.as_deref(), + Some(expected.as_str()), + "{length} bytes" + ); + assert!(!Path::new(&path).exists(), "{length} bytes"); + assert!( + Path::new(&expected).join("memory").exists(), + "{length} bytes" + ); + assert!( + artifacts + .get(TEST_ART_ID.parse().unwrap()) + .unwrap() + .is_none() + ); + } +} diff --git a/crates/libs/sbx/mgr/server/src/snapshot_scheduler.rs b/crates/libs/sbx/mgr/server/src/snapshot_scheduler.rs index c912f10dea..b38f1deafd 100644 --- a/crates/libs/sbx/mgr/server/src/snapshot_scheduler.rs +++ b/crates/libs/sbx/mgr/server/src/snapshot_scheduler.rs @@ -498,64 +498,71 @@ async fn process_tick( } // Disk snapshots publish under the artifact lock so backup selection never - // races their creation or their retention deletion. - { - let (_wake_guard, _artifact_guard) = ctx - .auto_lifecycle - .artifact_publication_locks(sandbox_id, ArtifactKind::Disk) - .await; - // Prune once before creation so a failed byte delete blocks further - // growth. Retention keeps a pinned, leased or unreadable snapshot and - // capture goes on, so such snapshots can hold a tier above its `KEEP_*` - // count. This tick may also add one snapshot per due tier above its - // retention count; the next successful prune removes that surplus - // before creating more. Deletions from completed pages remain useful - // progress on a retry. - prune_scheduled_snapshots(&mut node_client, ctx, sandbox_id).await?; - // Always create a 1m disk snapshot. - let disk_1m_name = format!("d-1m-{ts}"); - create_disk_snapshot( - &mut node_client, - sandbox_id, - &disk_1m_name, - "1m", - execution_fence.clone(), - ) - .await?; - if let Err(error) = - publish_named_artifact(ctx, sandbox_id, &disk_1m_name, ArtifactKind::Disk) - { - tracing::warn!( - sandbox_id, - snapshot_name = %disk_1m_name, - error = %error, - "created scheduled disk snapshot but failed to publish local artifact registry entry" - ); - } - - // Every 5th tick: d-5m (filesystem state ~5 min ago). - if tick.is_multiple_of(5) { - let disk_5m_name = format!("d-5m-{ts}"); + // races their creation or their retention deletion; `prune_and_capture` + // takes it for each, and deletes the pruned snapshots' staging between + // them with it released. + // + // Retention keeps a pinned, leased or unreadable snapshot and capture goes + // on, so such snapshots can hold a tier above its `KEEP_*` count. This tick + // may also add one snapshot per due tier above its retention count; the + // next successful prune removes that surplus before creating more. A + // failed prune, or GC staging it cannot delete, fails the tick before any + // capture, the full checkpoint included, so a failing node-manager delete + // stops growth instead of adding a snapshot and its staging every tick. + // Deletions from completed pages remain useful progress on a retry. + prune_and_capture( + &mut node_client, + ctx, + sandbox_id, + CACHE_DIR, + async |node_client| { + // Always create a 1m disk snapshot. + let disk_1m_name = format!("d-1m-{ts}"); create_disk_snapshot( - &mut node_client, + node_client, sandbox_id, - &disk_5m_name, - "5m", + &disk_1m_name, + "1m", execution_fence.clone(), ) .await?; if let Err(error) = - publish_named_artifact(ctx, sandbox_id, &disk_5m_name, ArtifactKind::Disk) + publish_named_artifact(ctx, sandbox_id, &disk_1m_name, ArtifactKind::Disk) { tracing::warn!( sandbox_id, - snapshot_name = %disk_5m_name, + snapshot_name = %disk_1m_name, error = %error, "created scheduled disk snapshot but failed to publish local artifact registry entry" ); } - } - } + + // Every 5th tick: d-5m (filesystem state ~5 min ago). + if tick.is_multiple_of(5) { + let disk_5m_name = format!("d-5m-{ts}"); + create_disk_snapshot( + node_client, + sandbox_id, + &disk_5m_name, + "5m", + execution_fence.clone(), + ) + .await?; + if let Err(error) = + publish_named_artifact(ctx, sandbox_id, &disk_5m_name, ArtifactKind::Disk) + { + tracing::warn!( + sandbox_id, + snapshot_name = %disk_5m_name, + error = %error, + "created scheduled disk snapshot but failed to publish local artifact registry entry" + ); + } + } + Ok(()) + }, + ) + .await?; if let Some(tier) = checkpoint_tier { let tier = tier.as_str(); @@ -1070,7 +1077,7 @@ pub(crate) fn snapshot_timestamp(now: jiff::Timestamp) -> String { } mod retention; -use retention::prune_scheduled_snapshots; +use retention::prune_and_capture; #[cfg(test)] mod tests; diff --git a/crates/libs/sbx/mgr/server/src/snapshot_scheduler/retention.rs b/crates/libs/sbx/mgr/server/src/snapshot_scheduler/retention.rs index f88673adee..0a10ee89a6 100644 --- a/crates/libs/sbx/mgr/server/src/snapshot_scheduler/retention.rs +++ b/crates/libs/sbx/mgr/server/src/snapshot_scheduler/retention.rs @@ -1,9 +1,14 @@ //! Scheduled-snapshot retention: which snapshots each tier keeps, and the //! prune that deletes the rest. +use std::path::Path; + use pplx_space_common_core::types::SandboxId; use super::CheckpointContext; +use pplx_space_common_core::types::ArtifactKind; +use pplx_space_node_mgr_client::NodeManagerGrpcClient; + use crate::local_artifacts::{CheckoutError, NamedSnapshotDelete}; /// Snapshot retention per tier. Each tier's count covers the gap to the next tier. @@ -24,6 +29,11 @@ pub(super) fn retention_candidate( retained: &mut [std::collections::BTreeSet<(String, String)>; 5], name: String, ) -> Option { + // GC staging keeps the tier prefix of the snapshot it once was. It is no + // snapshot: it must neither take a retained slot nor be pruned by name. + if crate::local_artifacts::is_gc_staging_name(&name) { + return None; + } let (index, keep, timestamp) = [ ("d-1m-", 0, KEEP_D_1M), ("1m-", 0, KEEP_D_1M), @@ -46,10 +56,72 @@ pub(super) fn retention_candidate( .map(|(_, name)| name) } -pub(super) async fn prune_scheduled_snapshots( - node_client: &mut pplx_space_node_mgr_client::NodeManagerGrpcClient, +/// Prune `sandbox_id`'s scheduled snapshots under `cache_root` (a test's own +/// directory, or the sandbox cache), delete the staging the prune leaves, and +/// then run `capture`. The prune and the capture each run under the sandbox's +/// disk artifact publication locks, so backup selection never races a +/// snapshot's creation or its retention deletion. The capture depends on +/// nothing the prune read, so the locks are released between them. +/// +/// The staging is deleted with the locks released: the pruned rows are gone, +/// so nothing owns it, and under the locks a node-manager deletion backlog +/// would stall the sandbox's publication, rollback, export and backup +/// enqueue. Besides this prune's own staging, the prune hands back every +/// unowned staging it lists, such as one an earlier delete failed to remove. +/// +/// `capture` runs only after a successful prune whose staging is all deleted, +/// so a failing byte delete blocks further growth. While DeleteCacheArtifact +/// keeps failing, each tick retries the staging and captures nothing: the +/// staging holds at most what one prune moved aside, and no tier gains a +/// snapshot. +pub(super) async fn prune_and_capture( + node_client: &mut NodeManagerGrpcClient, ctx: &CheckpointContext, sandbox_id: &str, + cache_root: &str, + capture: impl AsyncFnOnce(&mut NodeManagerGrpcClient) -> Result<(), String>, +) -> Result<(), String> { + let mut staged = Vec::new(); + let pruned = { + let (_wake_guard, _artifact_guard) = ctx + .auto_lifecycle + .artifact_publication_locks(sandbox_id, ArtifactKind::Disk) + .await; + prune_scheduled_snapshots(node_client, ctx, sandbox_id, cache_root, &mut staged).await + }; + let mut not_deleted = 0usize; + for (snapshot_name, staging) in &staged { + if crate::snapshot_gc::delete_gc_staging(node_client, staging, "retention", snapshot_name) + .await + .is_err() + { + not_deleted += 1; + } + } + pruned?; + if not_deleted > 0 { + return Err(format!( + "{not_deleted} of {} GC staging directories not deleted; \ + capture skipped until a later tick deletes them", + staged.len() + )); + } + let (_wake_guard, _artifact_guard) = ctx + .auto_lifecycle + .artifact_publication_locks(sandbox_id, ArtifactKind::Disk) + .await; + capture(node_client).await +} + +/// Delete the scheduled snapshots outside each tier's retention. Each +/// catalogued snapshot's GC staging, with its name, goes to `staged`, and so +/// does each unowned GC staging the listing shows. +async fn prune_scheduled_snapshots( + node_client: &mut NodeManagerGrpcClient, + ctx: &CheckpointContext, + sandbox_id: &str, + cache_root: &str, + staged: &mut Vec<(String, String)>, ) -> Result<(), String> { let sandbox = sandbox_id .parse::() @@ -63,22 +135,40 @@ pub(super) async fn prune_scheduled_snapshots( .map_err(|e| format!("list snapshots: {e}"))? { for snapshot in snapshots { + if crate::local_artifacts::is_gc_staging_name(&snapshot.name) { + let staging = pplx_space_common_core::host::linux::paths::sandbox_snapshot_dir_in( + cache_root, + sandbox_id, + &snapshot.name, + ); + if crate::snapshot_gc::gc_pending_is_unowned(&artifacts, Path::new(&staging)) { + staged.push((snapshot.name, staging)); + } + continue; + } let Some(name) = retention_candidate(&mut retained, snapshot.name) else { continue; }; + // A snapshot without a row is deleted by name: node-manager's + // DeleteBtrfsSnapshot deletes this path. A catalogued one is + // renamed to its GC staging sibling, which the caller deletes + // with DeleteCacheArtifact once it releases its locks. + let delete_path = pplx_space_common_core::host::linux::paths::sandbox_snapshot_dir_in( + cache_root, sandbox_id, &name, + ); let request = pplx_space_node_mgr_client::proto::DeleteBtrfsSnapshotRequest { sandbox_id: sandbox_id.to_string(), snapshot_name: name.clone(), }; - let delete_bytes = async { node_client.delete_btrfs_snapshot(request).await.map(drop) }; - // Node-manager deletes this sandbox's snapshot of that name here. - let delete_path = - pplx_space_common_core::host::linux::paths::sandbox_snapshot_dir(sandbox_id, &name); + let delete_in_place = + async { node_client.delete_btrfs_snapshot(request).await.map(drop) }; match artifacts - .delete_named_snapshot(sandbox_id, &name, &delete_path, delete_bytes) + .delete_named_snapshot(sandbox_id, &name, &delete_path, delete_in_place) .await { - NamedSnapshotDelete::Deleted => {} + NamedSnapshotDelete::Deleted { staging } => { + staged.extend(staging.map(|staging| (name.clone(), staging))); + } // The prune runs before every scheduled capture, so a snapshot // it keeps must not stop the rest of retention or capture, and // one a later prune can retry (pinned, leased or still being diff --git a/crates/libs/sbx/mgr/server/src/snapshot_scheduler/tests.rs b/crates/libs/sbx/mgr/server/src/snapshot_scheduler/tests.rs index 60ce7058dc..53d4d72888 100644 --- a/crates/libs/sbx/mgr/server/src/snapshot_scheduler/tests.rs +++ b/crates/libs/sbx/mgr/server/src/snapshot_scheduler/tests.rs @@ -515,15 +515,23 @@ fn checkpoint_sidecar_execution_metadata_records_the_capturing_host_cpu() { /// that helper's tests own the row and byte ordering. A pinned or leased /// snapshot is kept, and so is one whose row cannot be read or names another /// path, without stopping the rest of the prune: it runs before every -/// scheduled capture. A failed -/// byte delete stops the prune at that snapshot. +/// scheduled capture. A catalogued snapshot is renamed to its GC staging +/// sibling, which DeleteCacheArtifact deletes once the artifact lock is +/// released, and the capture then retakes the lock. One whose staging delete +/// fails partway is still pruned, because its row and `local_path` are gone, +/// but the tick captures nothing, so a failing delete cannot add a snapshot +/// and its staging every tick. A failed byte delete of a snapshot without a +/// row stops the prune at that snapshot, and the capture with it. #[tokio::test] async fn retention_prune_skips_pinned_and_leased_snapshot_artifacts() { + use crate::db::OptionalExtension as _; use crate::local_artifacts::LocalArtifacts; use pplx_space_node_mgr_client::proto::{ BtrfsSnapshotInfo, DeleteBtrfsSnapshotRequest, DeleteBtrfsSnapshotResponse, - ListBtrfsSnapshotsRequest, ListBtrfsSnapshotsResponse, + DeleteCacheArtifactRequest, DeleteCacheArtifactResponse, ListBtrfsSnapshotsRequest, + ListBtrfsSnapshotsResponse, }; + use std::path::Path; #[derive(Clone, Copy, Debug, PartialEq)] enum Setup { @@ -534,6 +542,11 @@ async fn retention_prune_skips_pinned_and_leased_snapshot_artifacts() { Uncatalogued, Deletable, } + #[derive(Clone, Copy, Debug, PartialEq)] + enum StagingDelete { + Removes, + FailsPartway, + } // The six oldest of eleven 1m snapshots fall outside retention and are // considered oldest first. The snapshot without a row precedes the // deletable one, so a failing delete stops before any row is checked out. @@ -548,19 +561,52 @@ async fn retention_prune_skips_pinned_and_leased_snapshot_artifacts() { let names: Vec = (0..11) .map(|minute| format!("d-1m-20260101T00{minute:02}00Z")) .collect(); - // (byte deletes fail, prune fails, snapshots whose deletion is requested) - for (delete_fails, fails, requested) in [ - (false, false, vec![names[4].clone(), names[5].clone()]), - (true, true, vec![names[4].clone()]), + // (the uncatalogued byte delete fails, how a staging delete ends, the + // tick fails before capturing, snapshots deleted by name, whether the + // deletable one is staged) + for (uncatalogued_fails, staging_delete, fails, by_name, staged) in [ + ( + false, + StagingDelete::Removes, + false, + vec![names[4].clone()], + true, + ), + ( + false, + StagingDelete::FailsPartway, + true, + vec![names[4].clone()], + true, + ), + ( + true, + StagingDelete::Removes, + true, + vec![names[4].clone()], + false, + ), ] { + let case = format!("uncatalogued_fails={uncatalogued_fails}, {staging_delete:?}"); + let root = tempfile::TempDir::new().unwrap(); + let cache_root = root.path().to_str().unwrap().to_owned(); let (ctx, sandbox_id) = checkpoint_context(SandboxState::Started); let artifacts = LocalArtifacts::new(ctx.db.clone()); let mut ids = Vec::new(); + let mut paths = Vec::new(); for (name, setup) in names.iter().zip(candidates) { let id = LocalArtifactId::new(); ids.push(id); - let local_path = - pplx_space_common_core::host::linux::paths::sandbox_snapshot_dir(&sandbox_id, name); + let local_path = pplx_space_common_core::host::linux::paths::sandbox_snapshot_dir_in( + &cache_root, + &sandbox_id, + name, + ); + std::fs::create_dir_all(&local_path).unwrap(); + for file in ["memory", "vmstate"] { + std::fs::write(Path::new(&local_path).join(file), file).unwrap(); + } + paths.push(local_path.clone()); if matches!( setup, Setup::Pinned | Setup::Restoring | Setup::PathDiffers | Setup::Deletable @@ -604,7 +650,17 @@ async fn retention_prune_skips_pinned_and_leased_snapshot_artifacts() { } } let reader = artifacts.check_out_for_restore(ids[1]).unwrap(); - let deletions = Arc::new(std::sync::Mutex::new(Vec::new())); + let deletable = (paths[5].clone(), ids[5]); + let staging = format!( + "{cache_root}/{sandbox_id}/snapshots/.gc-pending-{}", + deletable.1 + ); + // Whether the sandbox's artifact lock was free while each staging + // delete ran, and whether it was held while the capture ran. + let lock_free_at_staging_delete = Arc::new(std::sync::Mutex::new(Vec::new())); + let coordinator = ctx.auto_lifecycle.clone(); + let deleted_by_name = Arc::new(std::sync::Mutex::new(Vec::new())); + let deleted_by_path = Arc::new(std::sync::Mutex::new(Vec::new())); let listed = names.clone(); let node_manager = crate::node_manager_transport::fake::FakeNodeManager::default() .unary("ListBtrfsSnapshots", move |_: ListBtrfsSnapshotsRequest| { @@ -620,27 +676,282 @@ async fn retention_prune_skips_pinned_and_leased_snapshot_artifacts() { }) }) .unary("DeleteBtrfsSnapshot", { - let deletions = Arc::clone(&deletions); + let deleted_by_name = Arc::clone(&deleted_by_name); move |request: DeleteBtrfsSnapshotRequest| { - deletions.lock().unwrap().push(request.snapshot_name); - if delete_fails { + deleted_by_name.lock().unwrap().push(request.snapshot_name); + if uncatalogued_fails { return Err(tonic::Status::unavailable("injected byte delete failure")); } Ok(DeleteBtrfsSnapshotResponse {}) } }) + .unary("DeleteCacheArtifact", { + let (deleted_by_path, root) = + (Arc::clone(&deleted_by_path), root.path().to_owned()); + let (lock_free, coordinator, sandbox_id) = ( + Arc::clone(&lock_free_at_staging_delete), + coordinator.clone(), + sandbox_id.clone(), + ); + move |request: DeleteCacheArtifactRequest| { + lock_free.lock().unwrap().push( + coordinator + .artifact_lock(&sandbox_id) + .try_lock_owned() + .is_ok(), + ); + let path = Path::new(&request.local_path); + assert!(path.starts_with(&root)); + let result = match staging_delete { + StagingDelete::Removes => { + std::fs::remove_dir_all(path).unwrap(); + Ok(DeleteCacheArtifactResponse { was_present: true }) + } + StagingDelete::FailsPartway => { + std::fs::remove_file(path.join("memory")).unwrap(); + Err(tonic::Status::internal("injected partial delete")) + } + }; + deleted_by_path.lock().unwrap().push(request.local_path); + result + } + }) .serve(); - let result = - prune_scheduled_snapshots(&mut node_manager.connector().lazy(), &ctx, &sandbox_id) - .await; + let captured_under_lock = std::sync::Mutex::new(None); + let result = super::retention::prune_and_capture( + &mut node_manager.connector().lazy(), + &ctx, + &sandbox_id, + &cache_root, + async |_| { + let held = coordinator + .artifact_lock(&sandbox_id) + .try_lock_owned() + .is_err(); + *captured_under_lock.lock().unwrap() = Some(held); + Ok(()) + }, + ) + .await; - assert_eq!(result.is_err(), fails, "delete_fails={delete_fails}"); + assert_eq!(result.is_err(), fails, "{case}"); + if staging_delete == StagingDelete::FailsPartway { + assert!(result.unwrap_err().contains("capture skipped"), "{case}"); + } + // A failed prune or staging delete skips the capture; otherwise the + // capture retakes the lock. The staging is deleted with it released. assert_eq!( - *deletions.lock().unwrap(), - requested, - "delete_fails={delete_fails}" + *captured_under_lock.lock().unwrap(), + (!fails).then_some(true), + "{case}" ); + assert_eq!( + *lock_free_at_staging_delete.lock().unwrap(), + vec![true; usize::from(staged)], + "{case}" + ); + assert_eq!(*deleted_by_name.lock().unwrap(), by_name, "{case}"); + let by_path = if staged { + vec![staging.clone()] + } else { + Vec::new() + }; + assert_eq!(*deleted_by_path.lock().unwrap(), by_path, "{case}"); + // The kept snapshots were not renamed or touched. + for path in &paths[..4] { + assert_eq!( + std::fs::read(Path::new(path).join("memory")).unwrap(), + b"memory", + "{case}: {path}" + ); + } + let row: Option = ctx + .db + .lock() + .unwrap() + .query_row( + "SELECT state FROM local_artifacts WHERE local_artifact_id = ?1", + [deletable.1.to_string()], + |row| row.get(0), + ) + .optional() + .unwrap(); + if staged { + assert_eq!(row, None, "{case}"); + assert!(!Path::new(&deletable.0).exists(), "{case}"); + let staged_bytes = std::fs::read(Path::new(&staging).join("vmstate")).ok(); + let expected = match staging_delete { + StagingDelete::Removes => None, + StagingDelete::FailsPartway => Some(b"vmstate".to_vec()), + }; + assert_eq!(staged_bytes, expected, "{case}"); + assert!(!Path::new(&staging).join("memory").exists(), "{case}"); + } else { + assert_eq!(row.as_deref(), Some("ready"), "{case}"); + assert!(Path::new(&deletable.0).join("memory").exists(), "{case}"); + assert!(!Path::new(&staging).exists(), "{case}"); + } drop(reader); } } + +/// Staging that an earlier delete could not remove has no row, so no prune +/// stages it again. Retention lists it, retries its delete with the artifact +/// lock released, and captures nothing until it is gone: a failing +/// node-manager delete holds the sandbox at what it already staged, tick after +/// tick. Staging a row still owns is left to its owner and does not hold up +/// the capture. +#[tokio::test] +async fn retention_captures_only_once_leftover_staging_is_deleted() { + use crate::local_artifacts::LocalArtifacts; + use pplx_space_node_mgr_client::proto::{ + BtrfsSnapshotInfo, DeleteCacheArtifactRequest, DeleteCacheArtifactResponse, + ListBtrfsSnapshotsRequest, ListBtrfsSnapshotsResponse, + }; + use std::path::Path; + use std::sync::atomic::{AtomicBool, Ordering}; + + let root = tempfile::TempDir::new().unwrap(); + let cache_root = root.path().to_str().unwrap().to_owned(); + let (ctx, sandbox_id) = checkpoint_context(SandboxState::Started); + let snapshots = format!("{cache_root}/{sandbox_id}/snapshots"); + let (unowned, owned) = (LocalArtifactId::new(), LocalArtifactId::new()); + LocalArtifacts::new(ctx.db.clone()) + .publish_named_snapshot( + &sandbox_id, + owned, + ArtifactKind::Disk, + &SnapshotName::parse("d-1m-20260101T000000Z".to_owned()).unwrap(), + &format!("{snapshots}/d-1m-20260101T000000Z"), + ) + .unwrap(); + let unowned_staging = format!("{snapshots}/.gc-pending-{unowned}"); + let owned_staging = format!("{snapshots}/.gc-pending-{owned}"); + for staging in [&unowned_staging, &owned_staging] { + std::fs::create_dir_all(staging).unwrap(); + std::fs::write(Path::new(staging).join("memory"), "memory").unwrap(); + } + + let fails = Arc::new(AtomicBool::new(true)); + let deletions = Arc::new(std::sync::Mutex::new(Vec::new())); + let coordinator = ctx.auto_lifecycle.clone(); + let node_manager = crate::node_manager_transport::fake::FakeNodeManager::default() + .unary("ListBtrfsSnapshots", { + let snapshots = snapshots.clone(); + move |_: ListBtrfsSnapshotsRequest| { + let mut names: Vec<_> = std::fs::read_dir(&snapshots) + .unwrap() + .map(|entry| entry.unwrap().file_name().into_string().unwrap()) + .collect(); + names.sort(); + Ok(ListBtrfsSnapshotsResponse { + snapshots: names + .into_iter() + .map(|name| BtrfsSnapshotInfo { + name, + ..Default::default() + }) + .collect(), + next_page_token: String::new(), + }) + } + }) + .unary("DeleteCacheArtifact", { + let (fails, deletions, root) = ( + Arc::clone(&fails), + Arc::clone(&deletions), + root.path().to_owned(), + ); + let (coordinator, sandbox_id) = (coordinator.clone(), sandbox_id.clone()); + move |request: DeleteCacheArtifactRequest| { + let path = Path::new(&request.local_path); + assert!(path.starts_with(&root)); + let lock_free = coordinator + .artifact_lock(&sandbox_id) + .try_lock_owned() + .is_ok(); + deletions + .lock() + .unwrap() + .push((request.local_path.clone(), lock_free)); + if fails.load(Ordering::SeqCst) { + return Err(tonic::Status::internal("injected staging delete failure")); + } + std::fs::remove_dir_all(path).unwrap(); + Ok(DeleteCacheArtifactResponse { was_present: true }) + } + }) + .serve(); + + // Two failing ticks, then one whose delete succeeds. + for (tick, delete_fails) in [(1, true), (2, true), (3, false)] { + fails.store(delete_fails, Ordering::SeqCst); + deletions.lock().unwrap().clear(); + let captured_under_lock = std::sync::Mutex::new(None); + let result = super::retention::prune_and_capture( + &mut node_manager.connector().lazy(), + &ctx, + &sandbox_id, + &cache_root, + async |_| { + let held = coordinator + .artifact_lock(&sandbox_id) + .try_lock_owned() + .is_err(); + *captured_under_lock.lock().unwrap() = Some(held); + Ok(()) + }, + ) + .await; + + assert_eq!( + *deletions.lock().unwrap(), + [(unowned_staging.clone(), true)], + "tick {tick}" + ); + if delete_fails { + let error = result.unwrap_err(); + assert!(error.contains("capture skipped"), "tick {tick}: {error}"); + assert_eq!(*captured_under_lock.lock().unwrap(), None, "tick {tick}"); + assert!(Path::new(&unowned_staging).exists(), "tick {tick}"); + } else { + result.unwrap(); + assert_eq!(*captured_under_lock.lock().unwrap(), Some(true)); + assert!(!Path::new(&unowned_staging).exists()); + } + assert!( + Path::new(&owned_staging).join("memory").exists(), + "tick {tick}" + ); + } +} + +/// GC staging keeps the tier prefix of the snapshot it once was, in the earlier +/// `.gc-pending-` shape. It takes no retained slot, so it cannot +/// push a real snapshot out early, and is never a candidate itself. +#[test] +fn retention_ignores_gc_staging() { + let mut retained = std::array::from_fn(|_| std::collections::BTreeSet::new()); + let id = "01939e3e-0000-7000-8000-000000000001"; + let names = (1..=KEEP_D_1M) + .map(|minute| format!("d-1m-20260101T00{minute:02}00Z")) + .chain([ + format!("d-1m-20260101T005900Z.gc-pending-{id}"), + format!(".gc-pending-{id}"), + ]); + for name in names { + assert_eq!( + retention_candidate(&mut retained, name.clone()), + None, + "{name}" + ); + } + // A name with the marker but no artifact id is an ordinary snapshot: the + // oldest 1m one here, so it is pruned. + let ordinary = "d-1m-20260101T000000Z.gc-pending-note".to_owned(); + assert_eq!( + retention_candidate(&mut retained, ordinary.clone()), + Some(ordinary) + ); +} diff --git a/proto/sandbox/manager/v1/manager.proto b/proto/sandbox/manager/v1/manager.proto index 2a8cdb82db..a988009367 100644 --- a/proto/sandbox/manager/v1/manager.proto +++ b/proto/sandbox/manager/v1/manager.proto @@ -132,11 +132,15 @@ service SandboxManager { // By name, a snapshot whose local artifact is pinned, checked out, still // being deleted, shares its path with another artifact, or records another // path answers FAILED_PRECONDITION and keeps its bytes; one whose artifact - // record cannot be read answers INTERNAL. A failed byte delete answers - // INTERNAL "delete snapshot failed", whatever node-manager's status. With - // `all`, each such snapshot is kept, left out of `deleted`, and the rest are - // still deleted; a failed byte delete fails the request with node-manager's - // status. + // record cannot be read answers INTERNAL. A snapshot with an artifact record + // is first renamed aside, and once it is renamed it counts as deleted, even if + // deleting its record then fails (startup recovery finishes it) or + // node-manager fails to remove the renamed bytes (sandbox-manager's orphan + // sweep finishes them). A failed rename, or a failed byte delete of a + // snapshot without a record, answers INTERNAL + // "delete snapshot failed", whatever node-manager's status. With `all`, each + // such kept snapshot is left out of `deleted` and the rest are still + // deleted; a failed rename or byte delete fails the request. rpc DeleteSnapshots(DeleteSnapshotsRequest) returns (DeleteSnapshotsResponse); // Roll back a sandbox's filesystem to a previous snapshot. diff --git a/shim/proto/sandbox/manager/v1/manager_grpc.pb.go b/shim/proto/sandbox/manager/v1/manager_grpc.pb.go index 76fbdd5253..12d21b78c8 100644 --- a/shim/proto/sandbox/manager/v1/manager_grpc.pb.go +++ b/shim/proto/sandbox/manager/v1/manager_grpc.pb.go @@ -201,11 +201,15 @@ type SandboxManagerClient interface { // By name, a snapshot whose local artifact is pinned, checked out, still // being deleted, shares its path with another artifact, or records another // path answers FAILED_PRECONDITION and keeps its bytes; one whose artifact - // record cannot be read answers INTERNAL. A failed byte delete answers - // INTERNAL "delete snapshot failed", whatever node-manager's status. With - // `all`, each such snapshot is kept, left out of `deleted`, and the rest are - // still deleted; a failed byte delete fails the request with node-manager's - // status. + // record cannot be read answers INTERNAL. A snapshot with an artifact record + // is first renamed aside, and once it is renamed it counts as deleted, even if + // deleting its record then fails (startup recovery finishes it) or + // node-manager fails to remove the renamed bytes (sandbox-manager's orphan + // sweep finishes them). A failed rename, or a failed byte delete of a + // snapshot without a record, answers INTERNAL + // "delete snapshot failed", whatever node-manager's status. With `all`, each + // such kept snapshot is left out of `deleted` and the rest are still + // deleted; a failed rename or byte delete fails the request. DeleteSnapshots(ctx context.Context, in *DeleteSnapshotsRequest, opts ...grpc.CallOption) (*DeleteSnapshotsResponse, error) // Roll back a sandbox's filesystem to a previous snapshot. Rollback(ctx context.Context, in *RollbackRequest, opts ...grpc.CallOption) (*RollbackResponse, error) @@ -1296,11 +1300,15 @@ type SandboxManagerServer interface { // By name, a snapshot whose local artifact is pinned, checked out, still // being deleted, shares its path with another artifact, or records another // path answers FAILED_PRECONDITION and keeps its bytes; one whose artifact - // record cannot be read answers INTERNAL. A failed byte delete answers - // INTERNAL "delete snapshot failed", whatever node-manager's status. With - // `all`, each such snapshot is kept, left out of `deleted`, and the rest are - // still deleted; a failed byte delete fails the request with node-manager's - // status. + // record cannot be read answers INTERNAL. A snapshot with an artifact record + // is first renamed aside, and once it is renamed it counts as deleted, even if + // deleting its record then fails (startup recovery finishes it) or + // node-manager fails to remove the renamed bytes (sandbox-manager's orphan + // sweep finishes them). A failed rename, or a failed byte delete of a + // snapshot without a record, answers INTERNAL + // "delete snapshot failed", whatever node-manager's status. With `all`, each + // such kept snapshot is left out of `deleted` and the rest are still + // deleted; a failed rename or byte delete fails the request. DeleteSnapshots(context.Context, *DeleteSnapshotsRequest) (*DeleteSnapshotsResponse, error) // Roll back a sandbox's filesystem to a previous snapshot. Rollback(context.Context, *RollbackRequest) (*RollbackResponse, error) diff --git a/space_smoke/adapters.py b/space_smoke/adapters.py index 9a2d5331c1..26af53118d 100644 --- a/space_smoke/adapters.py +++ b/space_smoke/adapters.py @@ -2516,9 +2516,10 @@ def sandbox_snapshot_dirs(self, node_name: str, sandbox_id: str) -> tuple[str, . # A snapshots dir that does not exist is a legitimate "none". Any # other `ls` failure (it exits 2 for unreadable paths too) or an exec # failure must propagate rather than read as "reaped". + # `-A`: GC staging, `.gc-pending-`, is a dot entry. result = self._node_shell_exec( node_name, - f"if [ ! -e {path} ]; then exit 0; fi; ls -1 {path}", + f"if [ ! -e {path} ]; then exit 0; fi; ls -1A {path}", timeout=30, ) if result.returncode != 0: diff --git a/space_smoke/fixtures.py b/space_smoke/fixtures.py index 964dec4f45..351bd6cf1c 100644 --- a/space_smoke/fixtures.py +++ b/space_smoke/fixtures.py @@ -8,7 +8,7 @@ import shlex import time from datetime import UTC, datetime, timedelta -from collections.abc import Callable, Iterator, Mapping, Sequence +from collections.abc import Callable, Iterable, Iterator, Mapping, Sequence from contextlib import AbstractContextManager, contextmanager from dataclasses import asdict, dataclass from enum import StrEnum @@ -141,6 +141,23 @@ LOCAL_CLEANUP_QUIESCENCE_MARGIN_SECONDS = 2 +def snapshot_reap_residue(before: Iterable[str], after: Iterable[str]) -> list[str]: + """Entries of a reaped sandbox's snapshots directory that show the reap unfinished. + + An entry named after one of `before` is still there, or GC staging is: the + reaper stages a directory as `.gc-pending-` beside it (or, before + staging names had a fixed length, `.gc-pending-`) and only + then has node-mgr delete it. + """ + + names = tuple(before) + return sorted( + entry + for entry in after + if ".gc-pending-" in entry or any(entry.startswith(name) for name in names) + ) + + def retry_sandbox_delete(delete: Callable[[], None]) -> int: """Run one sandbox delete under the shared cleanup retry policy. @@ -4848,10 +4865,11 @@ def wait_until_reaped( ) -> None: """Block until no trace of the fork's snapshot entries remains on disk. - The reaper renames the directory to `.gc-pending-` - before deleting the row, then asks node-mgr to remove it; a failed - RPC leaves the sibling behind for the orphan sweep. Waiting on the - original name alone would pass over exactly that residue. + The reaper renames the directory to `.gc-pending-` beside + it (`.gc-pending-` before staging names had a fixed + length) before deleting the row, then asks node-mgr to remove it; a + failed RPC leaves the sibling behind for the orphan sweep. Waiting on + the original name alone would pass over exactly that residue. """ config = self._config @@ -4867,9 +4885,7 @@ def wait_until_reaped( deadline = time.monotonic() + timeout while True: after = self._control.sandbox_snapshot_dirs(node_name, self._fixture.sandbox.sandbox_id) - residue = sorted( - entry for entry in after if any(entry.startswith(name) for name in before) - ) + residue = snapshot_reap_residue(before, after) if not residue: self._fixture.record("snapshots_after_reap", list(after)) return diff --git a/space_smoke/tests/test_snapshot_reaper_scenario.py b/space_smoke/tests/test_snapshot_reaper_scenario.py index e4765d4c5e..08aa4c1a88 100644 --- a/space_smoke/tests/test_snapshot_reaper_scenario.py +++ b/space_smoke/tests/test_snapshot_reaper_scenario.py @@ -24,7 +24,11 @@ _SdkSandbox, ) from space_smoke.capabilities import EcwSandboxBusy, EcwSandboxDeleteRetryable -from space_smoke.fixtures import SANDBOX_CLEANUP_ATTEMPTS, retry_sandbox_delete +from space_smoke.fixtures import ( + SANDBOX_CLEANUP_ATTEMPTS, + retry_sandbox_delete, + snapshot_reap_residue, +) from space_smoke.runtime import CommandResult @@ -156,5 +160,28 @@ def test_non_retryable_failures_propagate_at_once(self) -> None: sleep.assert_not_called() +class SnapshotReapResidue(unittest.TestCase): + def test_original_names_and_staging_in_either_shape_are_residue(self) -> None: + artifact = "01a0b0c9-b8a3-7473-9c12-3821a701bc28" + before = ("d-operation-x-artifact",) + self.assertEqual(snapshot_reap_residue(before, ("d-1m-kept",)), []) + self.assertEqual( + snapshot_reap_residue( + before, + ( + "d-1m-kept", + "d-operation-x-artifact", + f".gc-pending-{artifact}", + f"d-operation-x-artifact.gc-pending-{artifact}", + ), + ), + [ + f".gc-pending-{artifact}", + "d-operation-x-artifact", + f"d-operation-x-artifact.gc-pending-{artifact}", + ], + ) + + if __name__ == "__main__": unittest.main()