From 154c4513e04dd03a9cf68c95de92b385c7b5abdc Mon Sep 17 00:00:00 2001 From: Abhinav Chordia Date: Tue, 27 Oct 2026 20:47:38 +0000 Subject: [PATCH 1/3] sbx-mgr: delete named snapshots and retention candidates through GC's staging rename Node-manager deletes non-subvolume snapshot trees, and trees with nested subvolumes, in place and entry by entry. A named delete (DeleteSnapshots by name or with all=true) or a retention prune whose node-manager delete failed partway left a partial tree at local_path, judged it present, and rolled the row back to ready, advertising missing bytes. These deletes now follow the snapshot reaper's protocol. After checkout and the path check, LocalArtifacts::delete_named_snapshot renames local_path to its GC staging sibling and deletes the row (DeleteLease::commit_through_gc_staging, which snapshot_gc::execute_one now uses too) and hands the staging back. DeleteSnapshots and retention delete it through node-manager's DeleteCacheArtifact (snapshot_gc::delete_gc_staging, factored out of the reaper's helper) only after releasing the sandbox's artifact locks. A node-manager failure there leaves only unowned staging, which the reaper's orphan sweep reclaims, so the snapshot reports deleted. A failed rename keeps the bytes and returns the row to ready. Refusals, path mismatches and snapshots without a row are unchanged, and the disk probe after a failed delete is gone. GC staging is now `.gc-pending-` beside local_path, a fixed-length name: `.gc-pending-` made names of 208 bytes or more fail the rename with ENAMETOOLONG, so neither GC nor a named delete could remove them. Startup recovery and the orphan sweep still recognize the earlier shape; for a long name the earlier shape's path cannot exist, so its ENAMETOOLONG probe now reads as absent instead of deferring the row on every restart. Node-manager accepts the new shape beside a suspend artifact. Every name containing `.gc-pending-` is internal: ListSnapshots, DeleteSnapshots, Rollback, ExportSnapshot, backup adoption and retention never treat staging as a snapshot, and ImportSnapshot refuses a target name containing the marker. DeleteSnapshots moves into its own module, and it and retention compute snapshot paths under an injectable cache root (sandbox_snapshot_dir_in) so their tests stay in tempdirs; the orphan sweep takes its root as a parameter for the same reason. --- architecture-baseline.toml | 2 +- crates/libs/common/core/src/host/linux.rs | 13 +- .../common/core/src/runtime_contract_tests.rs | 4 + .../node/mgr/server/src/service/snapshots.rs | 3 + .../mgr/server/src/service/snapshots/tests.rs | 6 + .../sbx/mgr/server/src/local_artifacts.rs | 202 ++++++--- .../mgr/server/src/local_artifacts/tests.rs | 409 +++++++++++++----- .../grpc_lifecycle/backup_policy_tests.rs | 49 +++ .../src/service/grpc_snapshot_transfer.rs | 47 +- .../service/grpc_snapshot_transfer/tests.rs | 30 ++ .../service/grpc_spaced_process_snapshots.rs | 155 +------ .../internal_snapshot_name_tests.rs | 8 + .../named_delete.rs | 184 ++++++++ .../grpc_spaced_process_snapshots/tests.rs | 269 +++++++++++- crates/libs/sbx/mgr/server/src/snapshot_gc.rs | 141 +++--- .../sbx/mgr/server/src/snapshot_gc/orphans.rs | 2 +- .../sbx/mgr/server/src/snapshot_gc/tests.rs | 242 ++++++++++- .../sbx/mgr/server/src/snapshot_scheduler.rs | 96 ++-- .../src/snapshot_scheduler/retention.rs | 73 +++- .../server/src/snapshot_scheduler/tests.rs | 208 ++++++++- proto/sandbox/manager/v1/manager.proto | 14 +- .../sandbox/manager/v1/manager_grpc.pb.go | 28 +- space_smoke/adapters.py | 3 +- space_smoke/fixtures.py | 32 +- .../tests/test_snapshot_reaper_scenario.py | 29 +- 25 files changed, 1739 insertions(+), 510 deletions(-) create mode 100644 crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/named_delete.rs 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..fe62aa819c 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,50 @@ 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; every name containing +/// `.gc-pending-` is internal, never a snapshot. 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-"; + +/// Whether a directory name is GC staging, in either shape. Staging is +/// logically deleted bytes that only 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 { + name.contains(GC_PENDING_MARKER) +} + +/// 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 +1090,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..508a0fcf80 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. `.gc-pending-` is reserved for GC staging, which no snapshot +/// RPC treats as a snapshot, so a name containing 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}: `{}` 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..9a8672c8b6 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,33 @@ 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 name in [ + "d-user.gc-pending-note", + "d-user.gc-pending-019f9ce3-2370-7a33-8706-fda0d0496d33", + ] { + let status = import_target_name(Some(name), "d").unwrap_err(); + assert_eq!(status.code(), tonic::Code::InvalidArgument, "{name}"); + assert!(status.message().contains("reserved"), "{name}: {status}"); + } + assert_eq!( + import_target_name(Some("d-user.gc-pending"), "d").unwrap(), + "d-user.gc-pending", + "only the marker with its trailing `-` is reserved" + ); + 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..795786ebda 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,14 @@ 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, the fixed-length one included: a leading + // `.gc-pending-` still contains the marker. + 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}"); + } } #[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..c00c28099c --- /dev/null +++ b/crates/libs/sbx/mgr/server/src/service/grpc_spaced_process_snapshots/named_delete.rs @@ -0,0 +1,184 @@ +//! 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 + // (of GC staging, for a catalogued snapshot, after the lock is + // released), and is idempotent on a missing path. + 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..1af3346f89 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,180 @@ 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}"), + ] +} + +/// 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. +#[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(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(); + assert_eq!(listed, ["d-user-keep"]); +} + +/// 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. +#[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 = staging.clone(); + 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", + ); + } + 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!(all.deleted.is_empty(), "{:?}", all.deleted); + assert!(deletions.lock().unwrap().is_empty()); +} diff --git a/crates/libs/sbx/mgr/server/src/snapshot_gc.rs b/crates/libs/sbx/mgr/server/src/snapshot_gc.rs index b3fed6845e..f5b3d6f81e 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); @@ -744,7 +767,7 @@ 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(|name| name.rsplit_once(crate::local_artifacts::GC_PENDING_MARKER)) .and_then(|(_, id)| id.parse::().ok()) else { warn!(path = %path.display(), "retaining GC staging with invalid artifact identity"); @@ -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..5d55be516a 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]); @@ -774,7 +774,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 +823,239 @@ 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. +#[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])); + for dir in unowned.iter().chain([&owned]) { + 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()); +} + +/// 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..b0780f6f91 100644 --- a/crates/libs/sbx/mgr/server/src/snapshot_scheduler.rs +++ b/crates/libs/sbx/mgr/server/src/snapshot_scheduler.rs @@ -498,64 +498,68 @@ 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` + // holds it across both. + // + // 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. + let cache_root = pplx_space_common_core::host::linux::paths::SANDBOX_CACHE_DIR; + prune_and_capture( + &mut node_client, + ctx, + sandbox_id, + cache_root, + 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 +1074,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..c8d19d2cbb 100644 --- a/crates/libs/sbx/mgr/server/src/snapshot_scheduler/retention.rs +++ b/crates/libs/sbx/mgr/server/src/snapshot_scheduler/retention.rs @@ -4,6 +4,9 @@ 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 +27,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 +54,52 @@ 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) and run `capture`, both under the +/// sandbox's disk artifact publication locks, so backup selection never races +/// a snapshot's creation or its retention deletion. `capture` runs only after +/// a successful prune, so a failed byte delete blocks further growth. +/// +/// The pruned snapshots' GC staging is deleted after the locks are released, +/// whatever the prune or capture returned: their 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. A failed +/// staging delete is the orphan sweep's. +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 result = { + let (_wake_guard, _artifact_guard) = ctx + .auto_lifecycle + .artifact_publication_locks(sandbox_id, ArtifactKind::Disk) + .await; + match prune_scheduled_snapshots(node_client, ctx, sandbox_id, cache_root, &mut staged).await + { + Ok(()) => capture(node_client).await, + Err(error) => Err(error), + } + }; + for (snapshot_name, staging) in &staged { + let _ = + crate::snapshot_gc::delete_gc_staging(node_client, staging, "retention", snapshot_name) + .await; + } + result +} + +/// Delete the scheduled snapshots outside each tier's retention. Each +/// catalogued snapshot's GC staging, with its name, goes to `staged`. +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::() @@ -66,19 +116,26 @@ pub(super) async fn prune_scheduled_snapshots( 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..c3557f3d62 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,22 @@ 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; one whose delete fails partway there is still pruned, because its +/// row and `local_path` are gone and the reaper's orphan sweep owns the +/// staging. 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 +541,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 +560,51 @@ 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, prune + // fails, 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, + false, + 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 +648,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 +674,141 @@ 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}"); + // A failed prune skips the capture; a successful one captures under + // the lock. The staging is deleted after the lock is released. + assert_eq!( + *captured_under_lock.lock().unwrap(), + (!fails).then_some(true), + "{case}" + ); assert_eq!( - *deletions.lock().unwrap(), - requested, - "delete_fails={delete_fails}" + *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); } } + +/// 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}" + ); + } +} 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() From 1fc6e39a755b4729d2369dec9849a042cc7e1af2 Mon Sep 17 00:00:00 2001 From: Abhinav Chordia Date: Mon, 2 Nov 2026 17:41:43 +0000 Subject: [PATCH 2/3] sbx-mgr: address review on staged snapshot deletes - Retention prunes under the publication locks, releases them, deletes the staging, and retakes them to capture only if every staging delete succeeded. The prune also hands back unowned staging it lists, so a staging delete that keeps failing is retried every tick and blocks capture instead of adding a snapshot and its staging each tick. - The DeleteSnapshots by-name comment says a catalogued snapshot is renamed there and its staging deleted by the caller. - Retention's call site uses the scheduler's CACHE_DIR. - Only `.gc-pending-`, as the whole name or its suffix, is GC staging: the same parse the orphan sweep reclaims by. A name such as `run.gc-pending-1` is an ordinary snapshot again: listed, deletable, importable and retained by tier. --- .../sbx/mgr/server/src/local_artifacts.rs | 20 ++- .../src/service/grpc_snapshot_transfer.rs | 6 +- .../service/grpc_snapshot_transfer/tests.rs | 27 ++- .../internal_snapshot_name_tests.rs | 14 +- .../named_delete.rs | 7 +- .../grpc_spaced_process_snapshots/tests.rs | 55 +++++- crates/libs/sbx/mgr/server/src/snapshot_gc.rs | 6 +- .../sbx/mgr/server/src/snapshot_gc/tests.rs | 18 +- .../sbx/mgr/server/src/snapshot_scheduler.rs | 11 +- .../src/snapshot_scheduler/retention.rs | 73 +++++--- .../server/src/snapshot_scheduler/tests.rs | 161 +++++++++++++++++- 11 files changed, 331 insertions(+), 67 deletions(-) diff --git a/crates/libs/sbx/mgr/server/src/local_artifacts.rs b/crates/libs/sbx/mgr/server/src/local_artifacts.rs index fe62aa819c..20abf0dd24 100644 --- a/crates/libs/sbx/mgr/server/src/local_artifacts.rs +++ b/crates/libs/sbx/mgr/server/src/local_artifacts.rs @@ -991,8 +991,8 @@ impl Drop for RestoreLease { /// `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; every name containing -/// `.gc-pending-` is internal, never a snapshot. +/// [`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 { match local_path.trim_end_matches('/').rsplit_once('/') { Some((parent, _)) => format!("{parent}/{GC_PENDING_MARKER}{id}"), @@ -1004,11 +1004,21 @@ pub(crate) fn gc_pending_staging_path(local_path: &str, id: impl std::fmt::Displ /// 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 the orphan sweep may touch, so callers -/// that list a sandbox's snapshots treat such a name as internal. +/// 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 { - name.contains(GC_PENDING_MARKER) + gc_staging_id(name).is_some() } /// Whether `.gc-pending-` exists. That shape is 48 bytes longer 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 508a0fcf80..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 @@ -4,8 +4,8 @@ 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. `.gc-pending-` is reserved for GC staging, which no snapshot -/// RPC treats as a snapshot, so a name containing it is refused rather than +/// 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 { @@ -19,7 +19,7 @@ fn import_target_name(target_name: Option<&str>, prefix: &str) -> Result` is reserved", crate::local_artifacts::GC_PENDING_MARKER ))); } 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 9a8672c8b6..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 @@ -734,19 +734,30 @@ fn snapshot_publication_refuses_an_unusable_destination_without_cataloging_it() /// and refused by, every snapshot RPC. #[test] fn import_refuses_the_reserved_staging_marker() { - for name in [ - "d-user.gc-pending-note", - "d-user.gc-pending-019f9ce3-2370-7a33-8706-fda0d0496d33", + 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), "d").unwrap_err(); + 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}"); } - assert_eq!( - import_target_name(Some("d-user.gc-pending"), "d").unwrap(), + // 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", - "only the marker with its trailing `-` is reserved" - ); + "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") 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 795786ebda..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,14 +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, the fixed-length one included: a leading - // `.gc-pending-` still contains the marker. + // 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 index c00c28099c..c6f20a06c4 100644 --- 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 @@ -153,9 +153,10 @@ impl SandboxManagerService { // 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 - // (of GC staging, for a catalogued snapshot, after the lock is - // released), and is idempotent on a missing path. + // `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 } => { 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 1af3346f89..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 @@ -730,8 +730,13 @@ fn gc_staging_names() -> [String; 2] { ] } +/// 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(); @@ -743,6 +748,7 @@ fn snapshot_inventory_omits_gc_staging() { ) .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(); @@ -759,13 +765,22 @@ fn snapshot_inventory_omits_gc_staging() { .iter() .map(|snapshot| snapshot.id.as_str()) .collect(); - assert_eq!(listed, ["d-user-keep"]); + 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. +/// 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::{ @@ -775,7 +790,11 @@ async fn snapshot_rpcs_refuse_gc_staging() { let staging = gc_staging_names(); let deletions = Arc::new(std::sync::Mutex::new(Vec::new())); - let listed = staging.clone(); + 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 { @@ -867,6 +886,27 @@ async fn snapshot_rpcs_refuse_gc_staging() { "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 { @@ -878,6 +918,11 @@ async fn snapshot_rpcs_refuse_gc_staging() { .await .unwrap() .into_inner(); - assert!(all.deleted.is_empty(), "{:?}", all.deleted); - assert!(deletions.lock().unwrap().is_empty()); + 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 f5b3d6f81e..d2c5b64668 100644 --- a/crates/libs/sbx/mgr/server/src/snapshot_gc.rs +++ b/crates/libs/sbx/mgr/server/src/snapshot_gc.rs @@ -763,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(crate::local_artifacts::GC_PENDING_MARKER)) - .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; 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 5d55be516a..5f68a7c48c 100644 --- a/crates/libs/sbx/mgr/server/src/snapshot_gc/tests.rs +++ b/crates/libs/sbx/mgr/server/src/snapshot_gc/tests.rs @@ -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(); @@ -937,7 +942,8 @@ async fn named_delete_failing_partway_leaves_staging_for_the_orphan_sweep() { /// 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. +/// 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::{ @@ -958,7 +964,8 @@ async fn orphan_sweep_reclaims_staging_in_either_shape() { )), ]; let owned = sandbox.join(format!("snapshots/.gc-pending-{}", ids[4])); - for dir in unowned.iter().chain([&owned]) { + 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(); } @@ -1010,6 +1017,7 @@ async fn orphan_sweep_reclaims_staging_in_either_shape() { 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: diff --git a/crates/libs/sbx/mgr/server/src/snapshot_scheduler.rs b/crates/libs/sbx/mgr/server/src/snapshot_scheduler.rs index b0780f6f91..b38f1deafd 100644 --- a/crates/libs/sbx/mgr/server/src/snapshot_scheduler.rs +++ b/crates/libs/sbx/mgr/server/src/snapshot_scheduler.rs @@ -499,19 +499,22 @@ async fn process_tick( // Disk snapshots publish under the artifact lock so backup selection never // races their creation or their retention deletion; `prune_and_capture` - // holds it across both. + // 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. + // 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. - let cache_root = pplx_space_common_core::host::linux::paths::SANDBOX_CACHE_DIR; prune_and_capture( &mut node_client, ctx, sandbox_id, - cache_root, + CACHE_DIR, async |node_client| { // Always create a 1m disk snapshot. let disk_1m_name = format!("d-1m-{ts}"); 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 c8d19d2cbb..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,6 +1,8 @@ //! 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; @@ -55,16 +57,23 @@ pub(super) fn retention_candidate( } /// Prune `sandbox_id`'s scheduled snapshots under `cache_root` (a test's own -/// directory, or the sandbox cache) and run `capture`, both under the -/// sandbox's disk artifact publication locks, so backup selection never races -/// a snapshot's creation or its retention deletion. `capture` runs only after -/// a successful prune, so a failed byte delete blocks further growth. +/// 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. /// -/// The pruned snapshots' GC staging is deleted after the locks are released, -/// whatever the prune or capture returned: their 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. A failed -/// staging delete is the orphan sweep's. +/// `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, @@ -73,27 +82,40 @@ pub(super) async fn prune_and_capture( capture: impl AsyncFnOnce(&mut NodeManagerGrpcClient) -> Result<(), String>, ) -> Result<(), String> { let mut staged = Vec::new(); - let result = { + let pruned = { let (_wake_guard, _artifact_guard) = ctx .auto_lifecycle .artifact_publication_locks(sandbox_id, ArtifactKind::Disk) .await; - match prune_scheduled_snapshots(node_client, ctx, sandbox_id, cache_root, &mut staged).await - { - Ok(()) => capture(node_client).await, - Err(error) => Err(error), - } + prune_scheduled_snapshots(node_client, ctx, sandbox_id, cache_root, &mut staged).await }; + let mut not_deleted = 0usize; for (snapshot_name, staging) in &staged { - let _ = - crate::snapshot_gc::delete_gc_staging(node_client, staging, "retention", snapshot_name) - .await; + 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() + )); } - result + 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`. +/// 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, @@ -113,6 +135,17 @@ 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; }; 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 c3557f3d62..53d4d72888 100644 --- a/crates/libs/sbx/mgr/server/src/snapshot_scheduler/tests.rs +++ b/crates/libs/sbx/mgr/server/src/snapshot_scheduler/tests.rs @@ -517,10 +517,11 @@ fn checkpoint_sidecar_execution_metadata_records_the_capturing_host_cpu() { /// path, without stopping the rest of the prune: it runs before every /// scheduled capture. A catalogued snapshot is renamed to its GC staging /// sibling, which DeleteCacheArtifact deletes once the artifact lock is -/// released; one whose delete fails partway there is still pruned, because its -/// row and `local_path` are gone and the reaper's orphan sweep owns the -/// staging. A failed byte delete of a snapshot without a row stops the prune -/// at that snapshot, and the capture with it. +/// 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 _; @@ -560,8 +561,9 @@ 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(); - // (the uncatalogued byte delete fails, how a staging delete ends, prune - // fails, snapshots deleted by name, whether the deletable one is staged) + // (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, @@ -573,7 +575,7 @@ async fn retention_prune_skips_pinned_and_leased_snapshot_artifacts() { ( false, StagingDelete::FailsPartway, - false, + true, vec![names[4].clone()], true, ), @@ -734,8 +736,11 @@ async fn retention_prune_skips_pinned_and_leased_snapshot_artifacts() { .await; assert_eq!(result.is_err(), fails, "{case}"); - // A failed prune skips the capture; a successful one captures under - // the lock. The staging is deleted after the lock is released. + 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!( *captured_under_lock.lock().unwrap(), (!fails).then_some(true), @@ -791,6 +796,137 @@ async fn retention_prune_skips_pinned_and_leased_snapshot_artifacts() { } } +/// 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. @@ -811,4 +947,11 @@ fn retention_ignores_gc_staging() { "{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) + ); } From 7dce928a200abf6414c9c02a9206ea9b9de274a0 Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Wed, 4 Nov 2026 17:24:21 -0500 Subject: [PATCH 3/3] [skip ci] cr-test: preview only, do not merge