From 0a835f4711e2472e0ac9e7e654ff07fc2f30ece8 Mon Sep 17 00:00:00 2001 From: Gigi Sayfan Date: Sun, 8 Nov 2026 18:36:45 -0800 Subject: [PATCH] [sbx-mgr] replace a backed-up stale suspended copy before a runtime upgrade stages its checkpoint --- crates/l20_domain/events/README.md | 2 +- .../sbx/mgr/store/src/auto_lifecycle.rs | 27 + crates/l30_storage/sbx/mgr/store/src/db.rs | 2 + .../sbx/mgr/store/src/db/eviction.rs | 249 ++- .../sbx/mgr/store/src/db/eviction/tests.rs | 194 +++ .../sbx/mgr/store/src/db/eviction_scan.rs | 12 + .../sbx/mgr/store/src/db/read/eviction.rs | 20 + .../sbx/mgr/store/src/db/read/sandbox_view.rs | 16 +- .../sbx/mgr/store/src/db/read/tests.rs | 2 +- .../sbx/mgr/store/src/db/runtime_upgrades.rs | 20 +- .../store/src/db/runtime_upgrades/tests.rs | 69 + .../sbx/mgr/store/src/db/state_write.rs | 2 + crates/l30_storage/snapshot/store/src/lib.rs | 57 +- .../l30_storage/snapshot/store/src/tests.rs | 73 +- crates/l40_api/internal/api/server/src/lib.rs | 31 +- .../l40_api/internal/api/server/src/tests.rs | 4 + .../src/tests/backup_generation_fence.rs | 165 ++ .../src/facets/sandbox_desired_state.rs | 4 +- .../sbx/mgr/server/src/capture_record.rs | 56 +- .../mgr/server/src/capture_record/tests.rs | 77 +- .../server/src/metrics/sandbox_eviction.rs | 21 +- .../l40_api/sbx/mgr/server/src/run/tests.rs | 8 +- .../sbx/mgr/server/src/sandbox_eviction.rs | 207 ++- .../mgr/server/src/sandbox_eviction/tests.rs | 26 + .../server/src/service/activation_support.rs | 55 +- .../sbx/mgr/server/src/service/eviction.rs | 369 ++++- .../mgr/server/src/service/grpc_lifecycle.rs | 7 + .../service/grpc_lifecycle/artifact_backup.rs | 49 + .../grpc_lifecycle/backup_policy_tests.rs | 113 ++ .../grpc_lifecycle/backup_submission.rs | 24 + .../grpc_lifecycle/retired_teardown_tests.rs | 3 +- .../cluster_api_stub_tests.rs | 40 +- .../registration_tests.rs | 205 ++- .../retired_teardown_tests/release_tests.rs | 4 +- .../retired_teardown_tests/verdict_tests.rs | 7 +- .../grpc_lifecycle/teardown_backup_tests.rs | 33 - .../grpc_lifecycle/teardown_suspend.rs | 9 + .../service/grpc_spaced_process_snapshots.rs | 69 +- .../l40_api/sbx/mgr/server/src/service/mod.rs | 2 +- .../sbx/mgr/server/src/service/recovery.rs | 8 +- .../mgr/server/src/service/runtime_upgrade.rs | 26 +- .../runtime_upgrade/node_effects/tests.rs | 12 +- .../server/src/service/runtime_upgrade/rpc.rs | 17 +- .../src/service/runtime_upgrade/rpc/tests.rs | 8 +- .../src/service/runtime_upgrade/stale_copy.rs | 306 ++++ .../runtime_upgrade/stale_copy/tests.rs | 1337 +++++++++++++++++ .../stale_copy/tests/unrecorded.rs | 474 ++++++ .../sbx/mgr/server/src/service/start.rs | 34 + .../service/suspend_snapshot_publication.rs | 43 +- .../suspend_snapshot_publication/tests.rs | 37 + .../sbx/mgr/server/src/service/tests.rs | 263 +++- .../sbx/mgr/server/src/snapshot_scheduler.rs | 66 +- .../server/src/snapshot_scheduler/tests.rs | 2 + .../tests/incomplete_checkpoint.rs | 136 ++ .../sbx/mgr/server/src/snapshot_upload.rs | 7 + .../mgr/server/src/snapshot_upload/tests.rs | 3 + .../mgr/server/src/snapshot_upload_jobs.rs | 36 +- .../server/src/snapshot_upload_jobs/tests.rs | 35 +- docs/execution-environments.md | 78 +- proto/node/api/v1/cluster.proto | 23 + 60 files changed, 5074 insertions(+), 210 deletions(-) create mode 100644 crates/l30_storage/sbx/mgr/store/src/db/eviction/tests.rs create mode 100644 crates/l30_storage/sbx/mgr/store/src/db/read/eviction.rs create mode 100644 crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/stale_copy.rs create mode 100644 crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/stale_copy/tests.rs create mode 100644 crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/stale_copy/tests/unrecorded.rs create mode 100644 crates/l40_api/sbx/mgr/server/src/snapshot_scheduler/tests/incomplete_checkpoint.rs diff --git a/crates/l20_domain/events/README.md b/crates/l20_domain/events/README.md index 5154dbbaff..d6166866af 100644 --- a/crates/l20_domain/events/README.md +++ b/crates/l20_domain/events/README.md @@ -69,7 +69,7 @@ context. Keep event names domain-level: prefer `pause_requested` or | `artifact_backup_queued`, `artifact_backup_completed`, `artifact_backup_failed` | Back up an immutable node-local artifact to durable storage. | Carries the exact `local_artifact_id` and `backup_attempt_id` that gate cross-node restore and node release. | | `local_snapshot_promotion_abandoned` | Node-drain gave up promoting a suspended sandbox's local snapshot (the on-node reupload was permanently refused because the sandbox was wedged in `restoring`) and adopted the current suspend cycle's ready remote snapshot so the node could finish draining. | Records the drain-progress decision: the sandbox stays `suspended` and recoverable from the adopted snapshot, abandoning the un-uploadable local state. Carries `source_node_id`. | | `sandbox_lost` | A reconciler determined no recoverable copy of a suspended sandbox's state exists anywhere — the local suspended artifact is gone (pruned, or the node's backing instance confirmed terminated) and no ready remote snapshot exists — and flipped the row to `failed`. | Explains a terminal `suspended → failed` flip on the sandbox timeline: the data was already lost before the flip, and the user's next wake/restore gets a clean lifecycle error instead of a snapshot-not-found. Carries `source_node_id`, from/to state, and a `reason` naming which give-up fired (`drain_sandbox_lost` on a draining node, `gone_node_unrecoverable` on a drained node whose EC2 instance is confirmed terminated). | -| `local_eviction_completed`, `local_eviction_failed` | The node removed (or failed to remove) a sandbox's entire local presence — cache directory including the rootfs instance, plus node-local DB rows. The cold-sandbox eviction loop does it for a suspended sandbox whose state is durably restorable from a ready remote snapshot; node-ctl's facet sync does it for a sandbox the cluster reports deleted; the teardown driver does it for a teardown suspend whose backup the cluster refused because it retired the execution. A sandbox the cluster still has stays restorable from its own snapshots. | Carries `reason` (`latest_backed_idle` — every eviction requires proof tied to the exact bytes: the pinned artifact's confirmed backup, or exact generation equality for legacy unpinned rows; `cluster_row_absent` — the cluster has no row for the sandbox; `execution_retired` — the cluster refused the capture's backup for a retired execution, carrying the discarded `local_artifact_id` and `execution_generation` instead of a remote snapshot or `local_path`), the remote `snapshot_id` relied on where there is one, and the freed `local_path` (logical size only; CoW sharing makes physical reclaim lower); failures carry the error, and one after the local row was claimed leaves it in `deleting` for cleanup retry. | +| `local_eviction_completed`, `local_eviction_failed` | The node removed (or failed to remove) a sandbox's entire local presence — cache directory including the rootfs instance, plus node-local DB rows. The cold-sandbox eviction loop does it for a suspended sandbox whose state is durably restorable from a ready remote snapshot; node-ctl's facet sync does it for a sandbox the cluster reports deleted; the teardown driver does it for a teardown suspend whose backup the cluster refused because it retired the execution; a runtime upgrade does it for a suspended copy of another checkpoint than the one it was decided from, before staging that checkpoint. A sandbox the cluster still has stays restorable from its own snapshots. | Carries `reason` (`latest_backed_idle` — every eviction requires proof tied to the exact bytes: the pinned artifact's confirmed backup, or exact generation equality for legacy unpinned rows; `cluster_row_absent` — the cluster has no row for the sandbox; `execution_retired` — the cluster refused the capture's backup for a retired execution, carrying the discarded `local_artifact_id` and `execution_generation` instead of a remote snapshot or `local_path`; `stale_upgrade_copy` — a runtime upgrade replaced a suspended copy whose pinned artifact was proven backed for its exact id, carrying that `local_artifact_id` instead of a remote snapshot or `local_path`), the remote `snapshot_id` relied on where there is one, and the freed `local_path` (logical size only; CoW sharing makes physical reclaim lower); failures carry the error, and one after the local row was claimed leaves it in `deleting` for cleanup retry. | | `rootfs_stream_reclaimed` | The local rootfs diff stream (`rootfs.btrfs-send`) of a suspended sandbox's current artifact was deleted after its backup reached the durable ready state. Same-node wake is preserved: the snapshot's `memory`/`vmstate` files and the `instance/` rootfs subvolume stay; only the upload-format stream, which no restore path reads locally, is removed. | Carries the exact `local_artifact_id`/`backup_attempt_id` the reclaim was proven against and the reclaimed `local_path`, so a later "where did the stream go" investigation lands on the durable backup that superseded it. | | `snapshot_pruned` | A retention sweep deleted an old durable snapshot of a still-live sandbox because it fell outside the newest-N `ready` full snapshots the cluster keeps per sandbox — its S3 prefix and DB rows were removed. The sandbox stays alive and restorable from a newer snapshot. | Carries the pruned `snapshot_id` and its `age_secs`; `source = cluster_controller`, `trigger = system`. Emitted by the `snapshot_retention_sweeper` reconciler. | | `scheduled_snapshots_blocked`, `scheduled_snapshots_unblocked` | A node's snapshot scheduler stopped, or resumed, capturing a sandbox's scheduled snapshots and full checkpoints because the unowned GC staging under its `snapshots/` reached, or fell back below, the scheduler's limit. | Explains a gap in a live sandbox's scheduled snapshots: carries `reason = gc_staging_backlog` and, on `blocked`, an `error` with the staging count and limit; `source = sbx_manager`, `trigger = system`. Emitted on the gate's edges, and again after a sandbox-manager restart or re-registration finds it still closed. The edge state is in memory, so a gate that reopens across a restart or re-registration has no `unblocked`. The `snapshot.scheduler.capture_skipped{reason:staging_backlog}` counter counts each blocked tick and the `snapshot.scheduler.unowned_staging` gauge sums the node's sandboxes; neither has a sandbox tag. | diff --git a/crates/l30_storage/sbx/mgr/store/src/auto_lifecycle.rs b/crates/l30_storage/sbx/mgr/store/src/auto_lifecycle.rs index 166ea4efec..6b6291600c 100644 --- a/crates/l30_storage/sbx/mgr/store/src/auto_lifecycle.rs +++ b/crates/l30_storage/sbx/mgr/store/src/auto_lifecycle.rs @@ -136,9 +136,36 @@ pub struct AutoLifecycleCoordinator { /// `suspended` row, so the test can move the row in between. #[cfg(any(test, feature = "test-support"))] pub suspended_stop_barrier: Arc>>>, + /// A test's two-phase rendezvous in backup admission, with the + /// sandbox's artifact lock held, just before the upload job is persisted. + #[cfg(any(test, feature = "test-support"))] + pub backup_admission_barrier: Arc>>>, + /// A test's two-phase rendezvous in a suspended row's eviction, with the + /// sandbox's artifact lock held, just before the claim. + #[cfg(any(test, feature = "test-support"))] + pub eviction_claim_barrier: Arc>>>, + /// Notified when a backup's admission is about to wait for the + /// sandbox's artifact lock, so a test knows the admission reached it. + #[cfg(any(test, feature = "test-support"))] + pub artifact_lock_wait_hook: Arc>>>, } impl AutoLifecycleCoordinator { + /// A test's two-phase rendezvous at one of the barriers above: when + /// `slot` holds a barrier, wait on it twice, once to tell the test the + /// caller arrived and once for the test to let it go on. + #[cfg(any(test, feature = "test-support"))] + pub async fn rendezvous(slot: &std::sync::Mutex>>) { + let barrier = slot + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .clone(); + if let Some(barrier) = barrier { + barrier.wait().await; + barrier.wait().await; + } + } + pub fn wake_lock(&self, sandbox_id: &str) -> Arc { lifecycle_lock(&self.wake_locks, sandbox_id) } diff --git a/crates/l30_storage/sbx/mgr/store/src/db.rs b/crates/l30_storage/sbx/mgr/store/src/db.rs index 4530fb5a2f..a6111ebc5d 100644 --- a/crates/l30_storage/sbx/mgr/store/src/db.rs +++ b/crates/l30_storage/sbx/mgr/store/src/db.rs @@ -131,6 +131,7 @@ pub use pplx_space_common_core::types::QuarantineRecoveryNoProgressReason; pub mod read { mod cleanup_job; mod counts; + mod eviction; mod local_artifact; mod runtime_upgrade; mod sandbox; @@ -138,6 +139,7 @@ pub mod read { mod state_page; pub use cleanup_job::*; pub use counts::*; + pub use eviction::*; pub use local_artifact::*; pub use runtime_upgrade::*; pub use sandbox::*; diff --git a/crates/l30_storage/sbx/mgr/store/src/db/eviction.rs b/crates/l30_storage/sbx/mgr/store/src/db/eviction.rs index c3c7690b68..1337397d3c 100644 --- a/crates/l30_storage/sbx/mgr/store/src/db/eviction.rs +++ b/crates/l30_storage/sbx/mgr/store/src/db/eviction.rs @@ -1,7 +1,11 @@ //! Eviction claims over settled sandboxes. +use super::runtime_upgrades::UNRETIRED_UPGRADE_PHASES_SQL; use super::*; +#[cfg(test)] +mod tests; + /// Atomically claim a `suspended` sandbox for cold-sandbox eviction by /// flipping it to `deleting`. The CAS guards two invariants at once: /// @@ -25,6 +29,22 @@ use super::*; /// state + pin contract for the disk-pressure loop, whose proof is /// the pin itself. /// +/// - no runtime upgrade still owns the row (it fences it, or still owes the +/// retirement of the checkpoint it superseded): the upgrade's preserved +/// disk and its recovery record live in the presence the eviction would +/// delete. The fence doesn't move `state`, the pin or `updated_at`, so +/// only this clause keeps an upgrade that began after the decision read +/// from losing them. A rolled-back upgrade's preserved disk lives under +/// the sandbox's cache directory, which the eviction's cleanup removes. +/// - no upload of the sandbox's artifacts is pending, running or still +/// cancelling: the eviction deletes every local artifact of the sandbox, +/// not only the proven pin, and an upload that hasn't read its source yet +/// would lose it. The `cancelling` probe is its own clause so the +/// pending-or-running one keeps the per-sandbox in-flight index, which +/// covers only those two. The disk-pressure loop's candidate scan skips such rows, and ones +/// an upgrade owns, before paying for a proof; this holds both conditions +/// through the claim, for every caller. +/// /// Returns `false` (and changes nothing) when any condition no longer /// holds. pub fn claim_suspended_sandbox_for_eviction( @@ -32,48 +52,187 @@ pub fn claim_suspended_sandbox_for_eviction( id: &str, expected_pin: Option<&str>, expected_updated_at: Option<&str>, +) -> Result { + claim_suspended(conn, id, expected_pin, expected_updated_at, true) +} + +/// [`claim_suspended_sandbox_for_eviction`] for a sandbox the cluster deleted, +/// whose other local artifacts don't hold the claim back: the cluster's +/// snapshot rows go with its sandbox row (`snapshots` references `sandboxes` +/// `ON DELETE CASCADE`), so a named snapshot no longer exists once its +/// sandbox doesn't. +pub fn claim_deleted_suspended_sandbox_for_eviction( + conn: &Connection, + id: &str, + expected_pin: Option<&str>, + expected_updated_at: Option<&str>, +) -> Result { + claim_suspended(conn, id, expected_pin, expected_updated_at, false) +} + +fn claim_suspended( + conn: &Connection, + id: &str, + expected_pin: Option<&str>, + expected_updated_at: Option<&str>, + others_backed: bool, ) -> Result { let tx = immediate_transaction(conn)?; - let changed = match (expected_pin, expected_updated_at) { - (Some(pin), Some(updated_at)) => tx.execute( - "UPDATE sandboxes - SET state = 'deleting', - state_changed_at = CASE WHEN state != 'deleting' THEN ?1 ELSE state_changed_at END, - updated_at = ?1 - WHERE id = ?2 AND state = 'suspended' - AND current_local_artifact_id = ?3 - AND updated_at = ?4", - params![now()?, id, pin, updated_at], - )?, - (Some(pin), None) => tx.execute( - "UPDATE sandboxes - SET state = 'deleting', - state_changed_at = CASE WHEN state != 'deleting' THEN ?1 ELSE state_changed_at END, - updated_at = ?1 - WHERE id = ?2 AND state = 'suspended' - AND current_local_artifact_id = ?3", - params![now()?, id, pin], - )?, - (None, Some(updated_at)) => tx.execute( + // `IS` matches an unpinned row for a `None` pin. + let changed = tx.execute( + &format!( "UPDATE sandboxes - SET state = 'deleting', - state_changed_at = CASE WHEN state != 'deleting' THEN ?1 ELSE state_changed_at END, - updated_at = ?1 - WHERE id = ?2 AND state = 'suspended' - AND current_local_artifact_id IS NULL - AND updated_at = ?3", - params![now()?, id, updated_at], - )?, - (None, None) => tx.execute( + SET state = 'deleting', + state_changed_at = CASE WHEN state != 'deleting' THEN ?1 ELSE state_changed_at END, + updated_at = ?1 + WHERE id = ?2 AND state = 'suspended' + AND current_local_artifact_id IS ?3 + AND (?4 IS NULL OR updated_at = ?4) + AND NOT EXISTS ( + SELECT 1 FROM sandbox_runtime_upgrades + WHERE sandbox_id = ?2 + AND phase IN {UNRETIRED_UPGRADE_PHASES_SQL} + ) + AND NOT EXISTS ( + SELECT 1 FROM jobs + WHERE kind = 'snapshot_upload' AND subject_id = ?2 + AND status IN ('pending', 'running') + ) + AND NOT EXISTS ( + SELECT 1 FROM jobs + WHERE kind = 'snapshot_upload' AND status = 'cancelling' + AND subject_id = ?2 + ) + AND (?5 = 0 OR NOT EXISTS ( + SELECT 1 FROM local_artifacts a + WHERE a.sandbox_id = ?2 + AND a.local_artifact_id IS NOT ?3 + AND a.backup_completed_at IS NULL + ))" + ), + params![now()?, id, expected_pin, expected_updated_at, others_backed], + )?; + if changed == 1 { + teardown_jobs::fail_active(&tx, id, TeardownSuspendFailure::Deleting)?; + } + tx.commit()?; + Ok(changed == 1) +} + +/// What a `sandboxes` row records of its activations' generations: the +/// current and the retained route and execution generations, which an +/// activation's generation fence reads. A claim re-asserts the whole witness: +/// `updated_at` is kept in whole seconds, so an activation and its +/// compensation within one second can move a generation and leave it +/// unchanged. +#[derive(Clone, Debug, PartialEq)] +pub struct ActivationGenerations([rusqlite::types::Value; 4]); + +/// `sandbox_id`'s [`ActivationGenerations`], `None` without a row. +pub(crate) fn activation_generations( + conn: &Connection, + sandbox_id: &str, +) -> Result> { + conn.query_row( + "SELECT space_api_route_generation, + space_api_route_execution_generation, + last_observed_route_generation, + last_observed_execution_generation + FROM sandboxes + WHERE id = ?1", + params![sandbox_id], + |row| { + Ok(ActivationGenerations([ + row.get(0)?, + row.get(1)?, + row.get(2)?, + row.get(3)?, + ])) + }, + ) + .optional() +} + +/// Whether an upload of `sandbox_id`'s artifacts is pending, running or still +/// cancelling, and so may still read its source. +pub(crate) fn snapshot_upload_in_flight(conn: &Connection, sandbox_id: &str) -> Result { + conn.query_row( + "SELECT EXISTS ( + SELECT 1 FROM jobs + WHERE kind = 'snapshot_upload' AND subject_id = ?1 + AND status IN ('pending', 'running') + ) OR EXISTS ( + SELECT 1 FROM jobs + WHERE kind = 'snapshot_upload' AND status = 'cancelling' + AND subject_id = ?1 + )", + params![sandbox_id], + |row| row.get(0), + ) +} + +/// Atomically claim a `suspended` copy a runtime upgrade replaces with the +/// checkpoint it was decided from, flipping it to `deleting`. The CAS holds +/// what [`claim_suspended_sandbox_for_eviction`] holds for a pinned row (the +/// state, the proven pin, no upgrade owning the row, no upload in flight), +/// and in place of a row version re-asserts `generations`: the upgrade +/// checked its activation against them, and removing the row removes the +/// fence they keep. +/// +/// Returns `false` (and changes nothing) when any condition no longer +/// holds. +pub fn claim_stale_upgrade_copy_for_eviction( + conn: &Connection, + id: &str, + pin: &str, + generations: &ActivationGenerations, +) -> Result { + let tx = immediate_transaction(conn)?; + let [route, execution, retained_route, retained_execution] = &generations.0; + let changed = tx.execute( + &format!( "UPDATE sandboxes - SET state = 'deleting', - state_changed_at = CASE WHEN state != 'deleting' THEN ?1 ELSE state_changed_at END, - updated_at = ?1 - WHERE id = ?2 AND state = 'suspended' - AND current_local_artifact_id IS NULL", - params![now()?, id], - )?, - }; + SET state = 'deleting', + state_changed_at = CASE WHEN state != 'deleting' THEN ?1 ELSE state_changed_at END, + updated_at = ?1 + WHERE id = ?2 AND state = 'suspended' + AND current_local_artifact_id = ?3 + AND space_api_route_generation IS ?4 + AND space_api_route_execution_generation IS ?5 + AND last_observed_route_generation IS ?6 + AND last_observed_execution_generation IS ?7 + AND NOT EXISTS ( + SELECT 1 FROM sandbox_runtime_upgrades + WHERE sandbox_id = ?2 + AND phase IN {UNRETIRED_UPGRADE_PHASES_SQL} + ) + AND NOT EXISTS ( + SELECT 1 FROM jobs + WHERE kind = 'snapshot_upload' AND subject_id = ?2 + AND status IN ('pending', 'running') + ) + AND NOT EXISTS ( + SELECT 1 FROM jobs + WHERE kind = 'snapshot_upload' AND status = 'cancelling' + AND subject_id = ?2 + ) + AND NOT EXISTS ( + SELECT 1 FROM local_artifacts a + WHERE a.sandbox_id = ?2 + AND a.local_artifact_id <> ?3 + AND a.backup_completed_at IS NULL + )" + ), + params![ + now()?, + id, + pin, + route, + execution, + retained_route, + retained_execution + ], + )?; if changed == 1 { teardown_jobs::fail_active(&tx, id, TeardownSuspendFailure::Deleting)?; } @@ -326,27 +485,27 @@ fn cancel_live_upload_job(conn: &Connection, artifact: LocalArtifactId) -> Resul } db_private_outside_tests! { - /// The local artifacts of the sandbox other than `staged_artifact` with no - /// `backup_completed_at` stamp: the rows that hold - /// [`claim_retired_teardown_execution_for_release`] back until each is - /// stamped. + /// The local artifacts of the sandbox other than `staged_artifact`, or + /// all of them for `None`, with no `backup_completed_at` stamp: the rows + /// that hold [`claim_retired_teardown_execution_for_release`] and the + /// suspended claims back until each is stamped. fn other_local_artifacts_awaiting_backup( conn: &Connection, id: &str, - staged_artifact: LocalArtifactId, + staged_artifact: Option, ) -> Result> { let mut statement = conn.prepare( "SELECT local_artifact_id, sandbox_id, artifact_kind, artifact_origin, snapshot_name, local_path FROM local_artifacts WHERE sandbox_id = ?1 - AND local_artifact_id <> ?2 + AND local_artifact_id IS NOT ?2 AND backup_completed_at IS NULL ORDER BY local_artifact_id", )?; Ok(statement .query_map( - params![id, staged_artifact.to_string()], + params![id, staged_artifact.map(|id| id.to_string())], row_to_local_artifact, )? .collect::>()?) diff --git a/crates/l30_storage/sbx/mgr/store/src/db/eviction/tests.rs b/crates/l30_storage/sbx/mgr/store/src/db/eviction/tests.rs new file mode 100644 index 0000000000..f5554e015d --- /dev/null +++ b/crates/l30_storage/sbx/mgr/store/src/db/eviction/tests.rs @@ -0,0 +1,194 @@ +use super::*; +use crate::db::fixture::insert_test_sandbox; + +/// A suspended sandbox pinning a checkpoint artifact; returns its id and pin. +fn pinned_suspended(conn: &Connection) -> (String, String) { + let id = SandboxId::new().to_string(); + insert_test_sandbox(conn, &id, SandboxState::Suspended); + let artifact = LocalArtifactId::new(); + insert_local_artifact( + conn, + &id, + artifact, + ArtifactKind::Full, + ArtifactOrigin::Suspend, + None, + "/space/cache/test/snapshot", + ) + .unwrap(); + conn.execute( + "UPDATE sandboxes + SET current_local_artifact_id = ?1, + current_local_artifact_kind = 'full', + current_local_artifact_origin = 'suspend' + WHERE id = ?2", + params![artifact.to_string(), id], + ) + .unwrap(); + (id, artifact.to_string()) +} + +/// The suspended eviction claim deletes every local artifact of the sandbox, +/// so it waits for an upload that is pending, running or cancelling, leaving +/// the row unchanged, and proceeds once the upload finished either way. The +/// in-flight read that names the hold agrees with it. +#[test] +fn a_suspended_eviction_claim_waits_for_a_pending_upload() { + for (status, claimable) in [ + ("pending", false), + ("running", false), + ("cancelling", false), + ("completed", true), + ("failed", true), + ] { + let conn = test_conn(); + let (id, pin) = pinned_suspended(&conn); + conn.execute( + "INSERT INTO jobs (job_id, kind, status, payload, subject_id, created_at) + VALUES (?1, 'snapshot_upload', ?2, '{}', ?3, '1')", + params![uuid::Uuid::now_v7().to_string(), status, id], + ) + .unwrap(); + let before = get_sandbox(&conn, &id).unwrap().unwrap(); + // The read the cluster-deleted eviction names the hold by agrees. + assert_eq!( + snapshot_upload_in_flight(&conn, &id).unwrap(), + !claimable, + "{status}" + ); + + let claimed = claim_suspended_sandbox_for_eviction(&conn, &id, Some(&pin), None).unwrap(); + + assert_eq!(claimed, claimable, "{status}"); + if !claimable { + let after = get_sandbox(&conn, &id).unwrap().unwrap(); + assert_eq!( + (after.state, &after.updated_at), + ( + SandboxStatus::Unowned(SandboxState::Suspended), + &before.updated_at + ), + "{status}" + ); + } + } +} + +/// The stale-copy claim re-asserts the generations its caller checked the +/// activation against, not a row version: a generation retained after the +/// read fails it, with the row unchanged, even within the same second, and +/// an unchanged witness claims the row. An in-flight upload fails it too. +#[test] +fn a_stale_upgrade_copy_claim_needs_the_generations_it_was_decided_against() { + // (the row's change after the witness was read, whether it still claims) + for (change, claimable) in [ + (None, true), + ( + Some("UPDATE sandboxes SET last_observed_execution_generation = 9 WHERE id = ?1"), + false, + ), + ( + Some("UPDATE sandboxes SET last_observed_route_generation = 9 WHERE id = ?1"), + false, + ), + ( + Some( + "INSERT INTO jobs (job_id, kind, status, payload, subject_id, created_at) + VALUES ('upload-in-flight', 'snapshot_upload', 'pending', '{}', ?1, '1')", + ), + false, + ), + ] { + let conn = test_conn(); + let (id, pin) = pinned_suspended(&conn); + let generations = activation_generations(&conn, &id).unwrap().unwrap(); + let before = get_sandbox(&conn, &id).unwrap().unwrap(); + if let Some(change) = change { + conn.execute(change, params![id]).unwrap(); + } + + let claimed = + claim_stale_upgrade_copy_for_eviction(&conn, &id, &pin, &generations).unwrap(); + + assert_eq!(claimed, claimable, "{change:?}"); + if !claimable { + let after = get_sandbox(&conn, &id).unwrap().unwrap(); + assert_eq!( + after.state, + SandboxStatus::Unowned(SandboxState::Suspended), + "{change:?}" + ); + assert_eq!(after.updated_at, before.updated_at, "{change:?}"); + } + } + let conn = test_conn(); + assert!(activation_generations(&conn, "absent").unwrap().is_none()); +} + +/// Every suspended eviction deletes all of the sandbox's local artifacts, so +/// the disk-pressure claim and the stale-copy claim refuse while another +/// artifact than the proven pin, a named snapshot say, has no backup stamp, +/// leaving the row unchanged, and claim it once that artifact is stamped. A +/// cluster-deleted sandbox's claim doesn't wait: the cluster's snapshots went +/// with its row. The pin itself needs no stamp; its proof is the caller's. +#[test] +fn a_suspended_eviction_claim_waits_for_the_sandboxes_other_artifacts() { + type Claim = fn(&Connection, &str, &str) -> bool; + let suspended: Claim = + |conn, id, pin| claim_suspended_sandbox_for_eviction(conn, id, Some(pin), None).unwrap(); + let stale_copy: Claim = |conn, id, pin| { + let generations = activation_generations(conn, id).unwrap().unwrap(); + claim_stale_upgrade_copy_for_eviction(conn, id, pin, &generations).unwrap() + }; + let deleted: Claim = |conn, id, pin| { + claim_deleted_suspended_sandbox_for_eviction(conn, id, Some(pin), None).unwrap() + }; + // (claim, whether it waits for an unbacked other artifact) + for (name, claim, waits) in [ + ("suspended", suspended, true), + ("stale copy", stale_copy, true), + ("cluster-deleted", deleted, false), + ] { + for stamped in [false, true] { + let conn = test_conn(); + let (id, pin) = pinned_suspended(&conn); + let named = LocalArtifactId::new(); + let snapshot_name = + pplx_space_common_core::types::SnapshotName::parse("d-named".to_string()).unwrap(); + insert_local_artifact( + &conn, + &id, + named, + ArtifactKind::Disk, + ArtifactOrigin::Snapshot, + Some(&snapshot_name), + "/space/cache/test/snapshots/d-named", + ) + .unwrap(); + if stamped { + conn.execute( + "UPDATE local_artifacts SET backup_completed_at = '1' + WHERE local_artifact_id = ?1", + params![named.to_string()], + ) + .unwrap(); + } + let before = get_sandbox(&conn, &id).unwrap().unwrap(); + + let claimed = claim(&conn, &id, &pin); + + assert_eq!(claimed, stamped || !waits, "{name}, stamped {stamped}"); + if !claimed { + let after = get_sandbox(&conn, &id).unwrap().unwrap(); + assert_eq!( + (after.state, &after.updated_at), + ( + SandboxStatus::Unowned(SandboxState::Suspended), + &before.updated_at + ), + "{name}" + ); + } + } + } +} diff --git a/crates/l30_storage/sbx/mgr/store/src/db/eviction_scan.rs b/crates/l30_storage/sbx/mgr/store/src/db/eviction_scan.rs index a2571d57d9..a1f70fc95f 100644 --- a/crates/l30_storage/sbx/mgr/store/src/db/eviction_scan.rs +++ b/crates/l30_storage/sbx/mgr/store/src/db/eviction_scan.rs @@ -6,6 +6,8 @@ use crate::db::{Connection, OptionalExtension, Result}; use pplx_space_sqlite::SqliteU64; use rusqlite::params; +use super::runtime_upgrades::UNRETIRED_UPGRADE_PHASES_SQL; + use crate::db::timestamps::EpochSeconds; const VALID_SUSPENDED_UPDATED_AT_SQL: &str = @@ -97,6 +99,16 @@ db_private_outside_tests! { AND j.subject_id = s.id AND j.status IN ('pending', 'running') ) + AND NOT EXISTS ( + SELECT 1 FROM jobs j + WHERE j.kind = 'snapshot_upload' AND j.status = 'cancelling' + AND j.subject_id = s.id + ) + AND NOT EXISTS ( + SELECT 1 FROM sandbox_runtime_upgrades u + WHERE u.sandbox_id = s.id + AND u.phase IN {UNRETIRED_UPGRADE_PHASES_SQL} + ) ORDER BY {IDLE_WITNESS_SQL} ASC, s.id ASC LIMIT ?4" ); diff --git a/crates/l30_storage/sbx/mgr/store/src/db/read/eviction.rs b/crates/l30_storage/sbx/mgr/store/src/db/read/eviction.rs new file mode 100644 index 0000000000..abbc2d4b0d --- /dev/null +++ b/crates/l30_storage/sbx/mgr/store/src/db/read/eviction.rs @@ -0,0 +1,20 @@ +//! What an eviction decides against: a row's activation generations, and +//! whether an upload still reads its artifacts. + +use crate::db::{ActivationGenerations, Connection, Result}; + +/// `sandbox_id`'s activation generations, as a witness +/// [`crate::db::claim_stale_upgrade_copy_for_eviction`] re-asserts; `None` +/// without a row. +pub fn activation_generations( + conn: &Connection, + sandbox_id: &str, +) -> Result> { + crate::db::activation_generations(conn, sandbox_id) +} + +/// Whether an upload of `sandbox_id`'s artifacts is pending, running or +/// still cancelling. +pub fn snapshot_upload_in_flight(conn: &Connection, sandbox_id: &str) -> Result { + crate::db::snapshot_upload_in_flight(conn, sandbox_id) +} diff --git a/crates/l30_storage/sbx/mgr/store/src/db/read/sandbox_view.rs b/crates/l30_storage/sbx/mgr/store/src/db/read/sandbox_view.rs index de9e744f96..555ccecace 100644 --- a/crates/l30_storage/sbx/mgr/store/src/db/read/sandbox_view.rs +++ b/crates/l30_storage/sbx/mgr/store/src/db/read/sandbox_view.rs @@ -475,7 +475,21 @@ impl ArtifactsFacet<'_> { self, staged_artifact: LocalArtifactId, ) -> Result> { - crate::db::other_local_artifacts_awaiting_backup(self.0.conn, self.0.id, staged_artifact) + crate::db::other_local_artifacts_awaiting_backup( + self.0.conn, + self.0.id, + Some(staged_artifact), + ) + } + + /// The artifacts other than `pin`, or all of them for `None`, without a + /// backup-completed stamp: what a suspended eviction, which deletes every + /// local artifact of the sandbox, must prove before it claims the row. + pub fn awaiting_backup_besides( + self, + pin: Option, + ) -> Result> { + crate::db::other_local_artifacts_awaiting_backup(self.0.conn, self.0.id, pin) } } diff --git a/crates/l30_storage/sbx/mgr/store/src/db/read/tests.rs b/crates/l30_storage/sbx/mgr/store/src/db/read/tests.rs index 6a7790bbd3..7ca9ec1173 100644 --- a/crates/l30_storage/sbx/mgr/store/src/db/read/tests.rs +++ b/crates/l30_storage/sbx/mgr/store/src/db/read/tests.rs @@ -922,7 +922,7 @@ fn cases() -> Vec { list(crate::db::other_local_artifacts_awaiting_backup( &s.conn, &s.suspended, - s.pin, + Some(s.pin), )) }, expected: [S, S, S, S], diff --git a/crates/l30_storage/sbx/mgr/store/src/db/runtime_upgrades.rs b/crates/l30_storage/sbx/mgr/store/src/db/runtime_upgrades.rs index e4d22c6503..7c61b1eb81 100644 --- a/crates/l30_storage/sbx/mgr/store/src/db/runtime_upgrades.rs +++ b/crates/l30_storage/sbx/mgr/store/src/db/runtime_upgrades.rs @@ -353,6 +353,12 @@ db_private_outside_tests! { } } +/// The phases of an upgrade that still owns its sandbox, as an SQL list: a +/// fencing one, or a verified one whose retirement is pending. A suspended +/// eviction and its candidate scan hold off a sandbox in these too. +pub(crate) const UNRETIRED_UPGRADE_PHASES_SQL: &str = + "('fenced', 'preserved', 'booting', 'verified')"; + db_private_outside_tests! { /// The upgrade of `sandbox_id` that hasn't retired the checkpoint it /// superseded yet, if any: a fencing one, or a verified one whose @@ -363,12 +369,14 @@ db_private_outside_tests! { ) -> Result> { let row: Option = conn .query_row( - "SELECT operation_id - FROM sandbox_runtime_upgrades - WHERE sandbox_id = ?1 - AND phase IN ('fenced', 'preserved', 'booting', 'verified') - ORDER BY created_at, operation_id - LIMIT 1", + &format!( + "SELECT operation_id + FROM sandbox_runtime_upgrades + WHERE sandbox_id = ?1 + AND phase IN {UNRETIRED_UPGRADE_PHASES_SQL} + ORDER BY created_at, operation_id + LIMIT 1" + ), params![sandbox_id], |row| row.get(0), ) diff --git a/crates/l30_storage/sbx/mgr/store/src/db/runtime_upgrades/tests.rs b/crates/l30_storage/sbx/mgr/store/src/db/runtime_upgrades/tests.rs index 374c6eeac3..aa17f53a55 100644 --- a/crates/l30_storage/sbx/mgr/store/src/db/runtime_upgrades/tests.rs +++ b/crates/l30_storage/sbx/mgr/store/src/db/runtime_upgrades/tests.rs @@ -1131,3 +1131,72 @@ fn an_activation_decided_on_an_execution_refuses_a_sandbox_moved_off_it() { SandboxStatus::from(SandboxState::Starting) ); } + +/// A suspended sandbox's eviction claim waits for every upgrade that still +/// owns its disk, fencing it or owing the old checkpoint's retirement, and +/// leaves the row unchanged while it does. A rolled-back or retired upgrade +/// owns nothing. The claim spells its phases out, so this also holds it to +/// `unretired_runtime_upgrade`'s. +#[test] +fn a_suspended_eviction_claim_waits_for_an_unretired_upgrade() { + use RuntimeUpgradePhase::*; + for (phase, claimable) in [ + (Fenced, false), + (Preserved, false), + (Booting, false), + (Verified, false), + (RolledBack, true), + (Retired, true), + ] { + let conn = test_conn(); + let envs = environments(); + let id = sandbox(&conn, SandboxState::Suspended, &envs.source); + let pin = pin_suspend_artifact(&conn, &id).to_string(); + fenced(begin(&conn, operation(), &id, envs.target_reference, true)); + conn.execute( + "UPDATE sandbox_runtime_upgrades SET phase = ?1", + params![phase.as_str()], + ) + .unwrap(); + let before = get_sandbox(&conn, &id).unwrap().unwrap(); + assert_eq!( + claimable, + unretired_runtime_upgrade(&conn, &id).unwrap().is_none(), + "{phase}: the claim and the unretired-upgrade reader disagree" + ); + + for expected_updated_at in [None, Some(before.updated_at.as_str())] { + if claimable { + break; + } + assert!( + !claim_suspended_sandbox_for_eviction(&conn, &id, Some(&pin), expected_updated_at) + .unwrap(), + "{phase}" + ); + } + if !claimable { + // So does the claim of a copy a runtime upgrade replaces. + let generations = activation_generations(&conn, &id).unwrap().unwrap(); + assert!( + !claim_stale_upgrade_copy_for_eviction(&conn, &id, &pin, &generations).unwrap(), + "{phase}" + ); + } + let after = get_sandbox(&conn, &id).unwrap().unwrap(); + assert_eq!( + (after.state, &after.updated_at), + ( + SandboxStatus::Unowned(SandboxState::Suspended), + &before.updated_at + ), + "{phase}" + ); + if claimable { + assert!( + claim_suspended_sandbox_for_eviction(&conn, &id, Some(&pin), None).unwrap(), + "{phase}" + ); + } + } +} diff --git a/crates/l30_storage/sbx/mgr/store/src/db/state_write.rs b/crates/l30_storage/sbx/mgr/store/src/db/state_write.rs index 60333b8f76..1865204871 100644 --- a/crates/l30_storage/sbx/mgr/store/src/db/state_write.rs +++ b/crates/l30_storage/sbx/mgr/store/src/db/state_write.rs @@ -508,6 +508,8 @@ edge_table! { DELETE_ADMISSION: Request, ALL.without(Deleting) => Deleting; /// The eviction claims: `claim_stopped_sandbox_for_eviction`, /// `claim_suspended_sandbox_for_eviction`, + /// `claim_deleted_suspended_sandbox_for_eviction`, + /// `claim_stale_upgrade_copy_for_eviction`, /// `claim_failed_restoring_sandbox_for_eviction`, /// `claim_teardown_suspend_failed_sandbox_for_eviction`, /// `claim_retired_teardown_execution_for_release` and diff --git a/crates/l30_storage/snapshot/store/src/lib.rs b/crates/l30_storage/snapshot/store/src/lib.rs index 5acb48b621..47a5caaefa 100644 --- a/crates/l30_storage/snapshot/store/src/lib.rs +++ b/crates/l30_storage/snapshot/store/src/lib.rs @@ -146,6 +146,7 @@ pub struct NewSnapshot { org_id: OrgId, artifact: Option, execution_metadata: Option, + generation: Option, storage_uri: ExactStorageUri, } @@ -173,6 +174,7 @@ impl NewSnapshot { org_id, artifact: None, execution_metadata: None, + generation: None, storage_uri, } } @@ -221,6 +223,13 @@ impl NewSnapshot { self.execution_metadata = Some(execution_metadata); self } + + /// Set the generation of the execution that captured the bytes, which + /// orders this capture against the sandbox's others. + pub fn with_generation(mut self, generation: Option) -> Self { + self.generation = generation; + self + } } /// The common read model for one durable `snapshots` row. @@ -788,6 +797,49 @@ pub async fn artifact_backup_ready( .await?) } +/// Whether `later`'s ready backup was captured by a later execution of +/// `sandbox_id` than `earlier`'s: the least capture generation `later`'s ready, +/// non-orphaned backups record is greater than the greatest `earlier`'s do. +/// Executions' generations only grow, so this orders the two captures +/// whatever the clocks of the nodes that took them, or the order their backups +/// registered in. `false` when either artifact has no such backup recording a +/// generation. +pub async fn artifact_captured_after( + executor: impl sqlx::PgExecutor<'_>, + sandbox_id: SandboxId, + later: (LocalArtifactId, ArtifactKind), + earlier: (LocalArtifactId, ArtifactKind), +) -> Result { + Ok(sqlx::query_scalar(sql!( + "SELECT COALESCE(( + SELECT MIN(s.generation) + FROM snapshots s + JOIN snapshot_storage_locations ssl ON ssl.snapshot_id = s.id + WHERE s.sandbox_id = $1 + AND s.local_artifact_id = $2 + AND s.artifact_kind = $3 + AND s.state = 'ready' + AND ssl.orphaned_at IS NULL + ) > ( + SELECT MAX(s.generation) + FROM snapshots s + JOIN snapshot_storage_locations ssl ON ssl.snapshot_id = s.id + WHERE s.sandbox_id = $1 + AND s.local_artifact_id = $4 + AND s.artifact_kind = $5 + AND s.state = 'ready' + AND ssl.orphaned_at IS NULL + ), FALSE)" + )) + .bind(sandbox_id) + .bind(later.0.as_uuid()) + .bind(later.1.as_str()) + .bind(earlier.0.as_uuid()) + .bind(earlier.1.as_str()) + .fetch_one(executor) + .await?) +} + #[derive(Debug, thiserror::Error)] pub enum CheckArtifactRegistrationError { #[error(transparent)] @@ -1104,9 +1156,9 @@ pub async fn insert( local_artifact_id, artifact_kind, artifact_origin, artifact_snapshot_name, runtime, runtime_version, guest_os, guest_arch, state_format_name, state_format_version, - execution_metadata) + execution_metadata, generation) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, - $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22) + $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, $23) ON CONFLICT DO NOTHING RETURNING id" )) @@ -1140,6 +1192,7 @@ pub async fn insert( .bind(state_format.map(|identity| identity.format.as_str())) .bind(state_format.map(|identity| identity.version.as_str())) .bind(snapshot.execution_metadata.as_ref().map(Json)) + .bind(snapshot.generation.map(ExecutionGeneration::get)) .fetch_optional(&mut **tx) .await?; diff --git a/crates/l30_storage/snapshot/store/src/tests.rs b/crates/l30_storage/snapshot/store/src/tests.rs index 45a1af1fc4..b42cb6966a 100644 --- a/crates/l30_storage/snapshot/store/src/tests.rs +++ b/crates/l30_storage/snapshot/store/src/tests.rs @@ -1,6 +1,6 @@ use super::{ GetSnapshotError, InsertOutcome, InsertSnapshotError, NewSnapshot, RecordArtifactDigestsError, - SnapshotArtifact, SnapshotArtifactDigest, artifact_digests, + SnapshotArtifact, SnapshotArtifactDigest, artifact_captured_after, artifact_digests, artifact_registered_by_another_attempt, delete_snapshot_catalog, get, insert, record_artifact_digests, }; @@ -196,12 +196,15 @@ async fn snapshot_catalog_store_contract() -> TestResult { Some(SnapshotContentHash::from(inserted_content_hash)), ) .with_execution_metadata(inserted_execution_metadata.clone()) + .with_generation(Some(ExecutionGeneration::new(7).expect("valid generation"))) }; let snapshot = make_snapshot(expected_uri.clone()); let mut insert_tx = db.pool.begin().await?; let (inserted, outcome) = insert(&mut insert_tx, &snapshot).await?; assert_eq!(outcome, InsertOutcome::Created); assert_eq!(inserted.id(), inserted_snapshot_id); + // The capture's generation is recorded with the attempt. + assert_eq!(inserted.generation(), Some(ExecutionGeneration::new(7)?)); assert_eq!( inserted .storage_location() @@ -518,6 +521,74 @@ async fn another_attempts_registration_of_an_artifact_is_found() -> TestResult { Ok(()) } +/// Captures are ordered by the generation of the execution that took them, +/// not by when their backups registered: an older capture backed up again +/// after a newer one still orders before it. A capture whose backups record +/// no generation, or whose only ready backup lost its location, orders against +/// nothing. +#[tokio::test(flavor = "current_thread")] +async fn captures_order_by_the_generation_that_took_them() -> TestResult { + let Some(db) = try_pg_test_db().await? else { + return Ok(()); + }; + let sandbox_id = SandboxId::new(); + sqlx::query(sql!( + "INSERT INTO sandboxes (id, user_id, org_id, network_policy_generation) + VALUES ($1, $2::UUID, $3::UUID, 1)" + )) + .bind(sandbox_id) + .bind(SEEDED_USER_ID) + .bind(SEEDED_ORG_ID) + .execute(&db.pool) + .await?; + let (older, newer, unrecorded, orphaned) = ( + LocalArtifactId::new(), + LocalArtifactId::new(), + LocalArtifactId::new(), + LocalArtifactId::new(), + ); + // (artifact, its capture generation, minutes since its backup registered, + // whether its location is orphaned) + for (artifact, generation, minutes_ago, orphan) in [ + (older, Some(4), 30, false), + (newer, Some(5), 20, false), + // The older capture, backed up again after the newer one. + (older, Some(4), 1, false), + (unrecorded, None, 10, false), + (orphaned, Some(9), 5, true), + ] { + let snapshot_id = insert_snapshot( + &db.pool, + sandbox_id, + "ready", + artifact, + None, + Some(orphan), + minutes_ago, + ) + .await?; + sqlx::query(sql!("UPDATE snapshots SET generation = $2 WHERE id = $1")) + .bind(snapshot_id.as_uuid()) + .bind(generation) + .execute(&db.pool) + .await?; + } + let full = |artifact| (artifact, ArtifactKind::Full); + let after = + |later, earlier| artifact_captured_after(&db.pool, sandbox_id, full(later), full(earlier)); + + assert!(after(newer, older).await?); + assert!(!after(older, newer).await?); + assert!(!after(older, older).await?, "a capture isn't after itself"); + assert!(!after(unrecorded, older).await?); + assert!(!after(newer, unrecorded).await?); + assert!( + !after(orphaned, newer).await?, + "an orphaned backup orders nothing" + ); + Ok(()) +} + /// A catalog snapshot is in the object store only when it is `ready` with a /// live location, and node-local only when it is the current local artifact /// of a sandbox settled in `suspended` or `stopped` on the node it uploaded diff --git a/crates/l40_api/internal/api/server/src/lib.rs b/crates/l40_api/internal/api/server/src/lib.rs index 840fd995f5..13c171591a 100644 --- a/crates/l40_api/internal/api/server/src/lib.rs +++ b/crates/l40_api/internal/api/server/src/lib.rs @@ -457,6 +457,12 @@ enum BackupReadiness { LatestReadySuspend(SnapshotId), /// The sandbox is durably restorable from a ready snapshot. DurablyRestorable, + /// The first artifact's ready backup was captured by a later execution + /// than the second's. + CapturedAfter { + later: (LocalArtifactId, ArtifactKind), + earlier: (LocalArtifactId, ArtifactKind), + }, } fn parse_artifact_backup_query( @@ -505,6 +511,19 @@ fn parse_artifact_backup_query( )?) } Some(Artifact::DurablyRestorable(true)) => BackupReadiness::DurablyRestorable, + Some(Artifact::CapturedAfter(order)) => { + let identity = |identity: Option<&crate::proto::LocalArtifactBackupIdentity>, + field: &str| { + let identity = identity.ok_or_else(|| { + Status::invalid_argument(format!("captured_after.{field} is required")) + })?; + parse_artifact(&identity.local_artifact_id, identity.artifact_kind) + }; + BackupReadiness::CapturedAfter { + later: identity(order.later.as_ref(), "later")?, + earlier: identity(order.earlier.as_ref(), "earlier")?, + } + } Some(Artifact::DurablyRestorable(false)) => { return Err(Status::invalid_argument("durably_restorable must be true")); } @@ -997,7 +1016,12 @@ impl ClusterApiService { storage_uri, )? .with_artifact(artifact) - .with_execution_metadata(execution_metadata); + .with_execution_metadata(execution_metadata) + // Orders captures only when the node vouches that the generation + // is the capturing execution's; it fences either way. + .with_generation( + execution_generation.filter(|_| req.execution_generation_names_capture), + ); let mut tx = self .pool .begin() @@ -1200,6 +1224,11 @@ impl ClusterApiService { .await .map_err(|e| readiness_error(&e))? } + BackupReadiness::CapturedAfter { later, earlier } => { + snapshot_store::artifact_captured_after(&self.pool, sandbox_id, later, earlier) + .await + .map_err(|e| readiness_error(&e))? + } }; Ok(Response::new(GetArtifactBackupLocationResponse { ready: Some(ready), diff --git a/crates/l40_api/internal/api/server/src/tests.rs b/crates/l40_api/internal/api/server/src/tests.rs index bfbd698ac8..5b1fbe738e 100644 --- a/crates/l40_api/internal/api/server/src/tests.rs +++ b/crates/l40_api/internal/api/server/src/tests.rs @@ -5033,6 +5033,7 @@ async fn begin_artifact_backup_from_node( execution: Some(test_snapshot_execution()), capture_host_cpu: None, execution_generation: None, + execution_generation_names_capture: false, fence_verdict_only: false, }, node_ip, @@ -5068,6 +5069,7 @@ async fn begin_artifact_backup_with_storage_uri( execution: Some(test_snapshot_execution()), capture_host_cpu: None, execution_generation: None, + execution_generation_names_capture: false, fence_verdict_only: false, }, TEST_WORKER_IP, @@ -5946,6 +5948,7 @@ async fn begin_artifact_backup_rejects_missing_execution_metadata() -> TestResul execution: None, capture_host_cpu: None, execution_generation: None, + execution_generation_names_capture: false, fence_verdict_only: false, })) .await @@ -5986,6 +5989,7 @@ async fn begin_artifact_backup_persists_capture_host_cpu() -> TestResult { execution: Some(test_snapshot_execution()), capture_host_cpu: capture_host_cpu.map(str::to_owned), execution_generation: None, + execution_generation_names_capture: false, fence_verdict_only: false, }, TEST_WORKER_IP, diff --git a/crates/l40_api/internal/api/server/src/tests/backup_generation_fence.rs b/crates/l40_api/internal/api/server/src/tests/backup_generation_fence.rs index eb518e4341..0304ceb082 100644 --- a/crates/l40_api/internal/api/server/src/tests/backup_generation_fence.rs +++ b/crates/l40_api/internal/api/server/src/tests/backup_generation_fence.rs @@ -25,6 +25,7 @@ fn teardown_backup_request( execution: Some(test_snapshot_execution()), capture_host_cpu: None, execution_generation: Some(execution_generation), + execution_generation_names_capture: true, fence_verdict_only: false, } } @@ -926,3 +927,167 @@ async fn a_rolled_back_restore_keeps_its_capture_with_retirement_disabled() -> T ); Ok(()) } + +/// A registration records the generation of the execution that captured it, +/// and the readiness check orders two captures by it, not by when their +/// backups registered: the older capture, backed up again after a newer one, +/// still orders before it. +#[tokio::test(flavor = "current_thread")] +async fn captures_order_by_the_generation_that_took_them() -> TestResult { + use crate::proto::backup_readiness_query::Artifact; + use crate::proto::{ + BackupReadinessQuery, LocalArtifactBackupIdentity, LocalArtifactCapturedAfter, + }; + let Some(fixture) = cluster_fixture().await? else { + return Ok(()); + }; + let pool = &fixture.service.pool; + let (sandbox_id, source_ip, _) = seed_sandbox_on_source_node(pool).await?; + insert_execution(pool, sandbox_id, 4, "running", true).await?; + let (older, newer) = (LocalArtifactId::new(), LocalArtifactId::new()); + let ready = |attempt: SnapshotId| async move { + sqlx::query(sql!("UPDATE snapshots SET state = 'ready' WHERE id = $1")) + .bind(attempt) + .execute(pool) + .await + }; + + let first = SnapshotId::new(); + begin_teardown_backup_of(&fixture.service, source_ip, sandbox_id, older, first, 4).await?; + ready(first).await?; + retire_execution(pool, sandbox_id, 4).await?; + insert_execution(pool, sandbox_id, 5, "running", true).await?; + let later = SnapshotId::new(); + begin_teardown_backup_of(&fixture.service, source_ip, sandbox_id, newer, later, 5).await?; + ready(later).await?; + // The older capture backed up again, after the newer one registered. + let again = SnapshotId::new(); + begin_teardown_backup_of(&fixture.service, source_ip, sandbox_id, older, again, 4).await?; + ready(again).await?; + + for (attempt, generation) in [(first, 4), (later, 5), (again, 4)] { + let recorded: Option = + sqlx::query_scalar(sql!("SELECT generation FROM snapshots WHERE id = $1")) + .bind(attempt) + .fetch_one(pool) + .await?; + assert_eq!(recorded, Some(generation)); + } + let full = pplx_space_sbx_mgr_client::proto::ArtifactKind::Full as i32; + let identity = |id: LocalArtifactId| LocalArtifactBackupIdentity { + local_artifact_id: id.to_string(), + artifact_kind: full, + }; + let captured_after = |later: LocalArtifactId, earlier: LocalArtifactId| { + let service = &fixture.service; + async move { + service + .get_artifact_backup_location(request_from_node( + GetArtifactBackupLocationRequest { + sandbox_id: sandbox_id.to_string(), + readiness: Some(BackupReadinessQuery { + artifact: Some(Artifact::CapturedAfter(LocalArtifactCapturedAfter { + later: Some(identity(later)), + earlier: Some(identity(earlier)), + })), + }), + ..Default::default() + }, + source_ip, + )) + .await + .map(|response| response.into_inner().ready) + } + }; + assert_eq!(captured_after(newer, older).await?, Some(true)); + assert_eq!(captured_after(older, newer).await?, Some(false)); + assert_eq!( + captured_after(LocalArtifactId::new(), older).await?, + Some(false) + ); + Ok(()) +} + +/// A registration whose node doesn't vouch that the generation is the +/// capture's own is still admitted under it, but records no capture +/// generation, so it orders nothing: a capture registered unvouched under a +/// later generation doesn't read as captured after one vouched for under an +/// earlier generation. +#[tokio::test(flavor = "current_thread")] +async fn an_unvouched_generation_fences_but_orders_nothing() -> TestResult { + use crate::proto::backup_readiness_query::Artifact; + use crate::proto::{ + BackupReadinessQuery, LocalArtifactBackupIdentity, LocalArtifactCapturedAfter, + }; + let Some(fixture) = cluster_fixture().await? else { + return Ok(()); + }; + let pool = &fixture.service.pool; + let (sandbox_id, source_ip, _) = seed_sandbox_on_source_node(pool).await?; + insert_execution(pool, sandbox_id, 4, "running", true).await?; + let (older, newer) = (LocalArtifactId::new(), LocalArtifactId::new()); + let register = |local_artifact_id: LocalArtifactId, generation: i64, names_capture: bool| { + let service = &fixture.service; + async move { + let attempt = SnapshotId::new(); + service + .begin_artifact_backup(request_from_node( + BeginArtifactBackupRequest { + execution_generation_names_capture: names_capture, + ..teardown_backup_request( + sandbox_id, + local_artifact_id, + attempt, + generation, + ) + }, + source_ip, + )) + .await?; + sqlx::query(sql!("UPDATE snapshots SET state = 'ready' WHERE id = $1")) + .bind(attempt) + .execute(pool) + .await + .map_err(|error| tonic::Status::internal(error.to_string()))?; + Ok::<_, tonic::Status>(attempt) + } + }; + let vouched = register(older, 4, true).await?; + retire_execution(pool, sandbox_id, 4).await?; + insert_execution(pool, sandbox_id, 5, "running", true).await?; + let unvouched = register(newer, 5, false).await?; + + for (attempt, recorded) in [(vouched, Some(4)), (unvouched, None)] { + let generation: Option = + sqlx::query_scalar(sql!("SELECT generation FROM snapshots WHERE id = $1")) + .bind(attempt) + .fetch_one(pool) + .await?; + assert_eq!(generation, recorded); + } + let full = pplx_space_sbx_mgr_client::proto::ArtifactKind::Full as i32; + let identity = |id: LocalArtifactId| LocalArtifactBackupIdentity { + local_artifact_id: id.to_string(), + artifact_kind: full, + }; + let ready = fixture + .service + .get_artifact_backup_location(request_from_node( + GetArtifactBackupLocationRequest { + sandbox_id: sandbox_id.to_string(), + readiness: Some(BackupReadinessQuery { + artifact: Some(Artifact::CapturedAfter(LocalArtifactCapturedAfter { + later: Some(identity(newer)), + earlier: Some(identity(older)), + })), + }), + ..Default::default() + }, + source_ip, + )) + .await? + .into_inner() + .ready; + assert_eq!(ready, Some(false)); + Ok(()) +} diff --git a/crates/l40_api/node/ctl/server/src/facets/sandbox_desired_state.rs b/crates/l40_api/node/ctl/server/src/facets/sandbox_desired_state.rs index 166e412d9a..2af905ca06 100644 --- a/crates/l40_api/node/ctl/server/src/facets/sandbox_desired_state.rs +++ b/crates/l40_api/node/ctl/server/src/facets/sandbox_desired_state.rs @@ -231,7 +231,9 @@ pub(super) async fn refresh_sandbox_desired_state( tracing::debug!( sandbox_id, status = %status, - "cluster-deleted eviction deferred to the next sync: restore staging in flight" + "cluster-deleted eviction deferred to the next sync: the sandbox is held by \ + a restore staging pull, a runtime upgrade, an artifact upload, or another \ + operation on its artifacts" ); } ClusterDeletedEvictDisposition::Deferred(backoff) => { diff --git a/crates/l40_api/sbx/mgr/server/src/capture_record.rs b/crates/l40_api/sbx/mgr/server/src/capture_record.rs index a9f14246f0..401259c05b 100644 --- a/crates/l40_api/sbx/mgr/server/src/capture_record.rs +++ b/crates/l40_api/sbx/mgr/server/src/capture_record.rs @@ -16,6 +16,14 @@ //! the whole directory, carries it with the capture it describes. Nor is it in //! the node's database, so a sandbox manager rolled back to a release without //! it opens that database unchanged and ignores the file. +//! +//! Only a record the capturing teardown wrote as it published the capture +//! orders captures ([`read_capture_order`]). Startup recovery also writes +//! records, from the teardown it finds active, and the capture it finds can be +//! an earlier execution's still in the canonical directory: its record fences +//! re-requests as before but orders nothing, unless it finds the capturing +//! teardown's own record already naming that generation, which it keeps. +//! Neither orders any record written before writers were told apart. use std::path::Path; @@ -24,22 +32,48 @@ use pplx_space_common_core::types::{ExecutionGeneration, LocalArtifactId}; /// The record's file name in the capture's directory. pub(crate) const CAPTURE_RECORD_FILENAME: &str = "capture.json"; +/// Who wrote a record, which decides whether it may order captures. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum RecordWriter { + /// The teardown suspend that took the capture, as it publishes it. + CapturingTeardown, + /// Startup recovery, naming the generation of the teardown it found + /// active, which needn't be the one that took the capture. + Recovery, +} + #[derive(serde::Serialize, serde::Deserialize)] struct CaptureRecord { local_artifact_id: LocalArtifactId, execution_generation: ExecutionGeneration, + /// Written by the capturing teardown. Absent in every record recovery + /// writes, and in every record written before the field existed. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + written_at_capture: bool, } /// Record `execution_generation` as the execution whose teardown suspend /// captured `local_artifact_id` in `artifact_dir`, replacing the file durably. +/// Recovery leaves the capturing teardown's own record of the same capture and +/// generation as it is: the teardown wrote it before the node went down, and +/// recovery's would say the same thing with less evidence. pub(crate) fn write( artifact_dir: &Path, local_artifact_id: LocalArtifactId, execution_generation: ExecutionGeneration, + writer: RecordWriter, ) -> std::io::Result<()> { + // A record that can't be read keeps nothing, and is replaced as before. + if writer == RecordWriter::Recovery + && read_capture_order(artifact_dir, local_artifact_id) + .is_ok_and(|recorded| recorded == Some(execution_generation)) + { + return Ok(()); + } let bytes = serde_json::to_vec(&CaptureRecord { local_artifact_id, execution_generation, + written_at_capture: writer == RecordWriter::CapturingTeardown, }) .map_err(|error| std::io::Error::new(std::io::ErrorKind::InvalidData, error))?; crate::snapshot_upload::replace_file_durably( @@ -58,6 +92,26 @@ pub(crate) fn read( artifact_dir: &Path, local_artifact_id: LocalArtifactId, ) -> std::io::Result> { + Ok(read_record(artifact_dir, local_artifact_id)?.map(|record| record.execution_generation)) +} + +/// The execution generation that took `local_artifact_id`, as only the +/// capturing teardown's own record shows it, for ordering captures. `None` +/// for a record recovery wrote or one older than the distinction, as for no +/// record at all. +pub(crate) fn read_capture_order( + artifact_dir: &Path, + local_artifact_id: LocalArtifactId, +) -> std::io::Result> { + Ok(read_record(artifact_dir, local_artifact_id)? + .filter(|record| record.written_at_capture) + .map(|record| record.execution_generation)) +} + +fn read_record( + artifact_dir: &Path, + local_artifact_id: LocalArtifactId, +) -> std::io::Result> { let bytes = match std::fs::read(artifact_dir.join(CAPTURE_RECORD_FILENAME)) { Ok(bytes) => bytes, Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None), @@ -65,7 +119,7 @@ pub(crate) fn read( }; let record: CaptureRecord = serde_json::from_slice(&bytes) .map_err(|error| std::io::Error::new(std::io::ErrorKind::InvalidData, error))?; - Ok((record.local_artifact_id == local_artifact_id).then_some(record.execution_generation)) + Ok((record.local_artifact_id == local_artifact_id).then_some(record)) } #[cfg(test)] diff --git a/crates/l40_api/sbx/mgr/server/src/capture_record/tests.rs b/crates/l40_api/sbx/mgr/server/src/capture_record/tests.rs index fa9c0a718d..ec3dd59761 100644 --- a/crates/l40_api/sbx/mgr/server/src/capture_record/tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/capture_record/tests.rs @@ -10,7 +10,13 @@ fn a_capture_record_reads_back_only_for_its_artifact() { assert_eq!(read(dir.path(), artifact).unwrap(), None); let generation = ExecutionGeneration::new(7).unwrap(); - write(dir.path(), artifact, generation).unwrap(); + write( + dir.path(), + artifact, + generation, + RecordWriter::CapturingTeardown, + ) + .unwrap(); assert_eq!(read(dir.path(), artifact).unwrap(), Some(generation)); assert_eq!( read(dir.path(), LocalArtifactId::new()).unwrap(), @@ -25,6 +31,75 @@ fn a_capture_record_reads_back_only_for_its_artifact() { ); } +/// Every record fences, but only the capturing teardown's own orders +/// captures: not recovery's, nor one written before writers were told apart, +/// which reads like recovery's. Recovery keeps the teardown's record when it +/// names the same capture and generation, and replaces anything else. +#[test] +fn only_the_capturing_teardowns_record_orders_captures() { + let dir = tempfile::tempdir().unwrap(); + let artifact = LocalArtifactId::new(); + let generation = ExecutionGeneration::new(7).unwrap(); + + write( + dir.path(), + artifact, + generation, + RecordWriter::CapturingTeardown, + ) + .unwrap(); + assert_eq!(read(dir.path(), artifact).unwrap(), Some(generation)); + assert_eq!( + read_capture_order(dir.path(), artifact).unwrap(), + Some(generation) + ); + assert_eq!( + read_capture_order(dir.path(), LocalArtifactId::new()).unwrap(), + None + ); + + // Recovery naming the same generation keeps the teardown's record. + write(dir.path(), artifact, generation, RecordWriter::Recovery).unwrap(); + assert_eq!( + read_capture_order(dir.path(), artifact).unwrap(), + Some(generation) + ); + // Recovery naming another generation replaces it with one that fences + // under that generation and orders nothing. + let active = ExecutionGeneration::new(8).unwrap(); + write(dir.path(), artifact, active, RecordWriter::Recovery).unwrap(); + assert_eq!(read(dir.path(), artifact).unwrap(), Some(active)); + assert_eq!(read_capture_order(dir.path(), artifact).unwrap(), None); + // Nor does recovery keep the teardown's record of another capture. + let other = LocalArtifactId::new(); + write( + dir.path(), + other, + generation, + RecordWriter::CapturingTeardown, + ) + .unwrap(); + write(dir.path(), artifact, generation, RecordWriter::Recovery).unwrap(); + assert_eq!(read(dir.path(), artifact).unwrap(), Some(generation)); + assert_eq!(read_capture_order(dir.path(), artifact).unwrap(), None); + // A record that can't be read is replaced, as before. + std::fs::write(dir.path().join(CAPTURE_RECORD_FILENAME), b"{").unwrap(); + write(dir.path(), artifact, generation, RecordWriter::Recovery).unwrap(); + assert_eq!(read(dir.path(), artifact).unwrap(), Some(generation)); + + std::fs::write( + dir.path().join(CAPTURE_RECORD_FILENAME), + serde_json::to_vec(&serde_json::json!({ + "local_artifact_id": artifact, + "execution_generation": generation, + })) + .unwrap(), + ) + .unwrap(); + assert_eq!(read(dir.path(), artifact).unwrap(), Some(generation)); + assert_eq!(read_capture_order(dir.path(), artifact).unwrap(), None); +} + /// The record is node-local: no upload, of any artifact kind or rootfs /// codec, plaintext or encrypted, carries it to the object store, so no /// restore or fork elsewhere inherits an execution that isn't its own. diff --git a/crates/l40_api/sbx/mgr/server/src/metrics/sandbox_eviction.rs b/crates/l40_api/sbx/mgr/server/src/metrics/sandbox_eviction.rs index 776058e504..b0a051fce2 100644 --- a/crates/l40_api/sbx/mgr/server/src/metrics/sandbox_eviction.rs +++ b/crates/l40_api/sbx/mgr/server/src/metrics/sandbox_eviction.rs @@ -39,6 +39,9 @@ pub fn tick(result: &str, started_at: Instant, candidates: u64) { /// nothing can restore the bytes. /// - `execution_retired` — the cluster refused a teardown suspend's /// backup because it had retired the execution that captured it. +/// - `stale_upgrade_copy` — a runtime upgrade decided from another +/// checkpoint found this suspended copy, proven backed for its exact +/// pinned artifact, and evicted it to stage the decided checkpoint. pub fn evicted(reason: &str) { counter("sandbox.evict.evicted", 1, &[tag("reason", reason)]); } @@ -105,16 +108,26 @@ pub fn malformed_updated_at(rows: u64) { /// - `not_suspended_at_execute` — the sandbox left `suspended` (or its /// pin changed from the proven identity) between planning and the /// eviction claim (e.g. a resume raced us). +/// - `held_at_execute` — the sandbox was still suspended with the proven +/// pin, but the eviction claim was refused because something held it: a +/// runtime upgrade, an artifact upload in flight, another local artifact +/// without a backup stamp, or another operation holding its artifact +/// lock. Each ends on its own; a later tick reconsiders the row. +/// - `unrecorded_snapshot` — the sandbox's snapshot directory holds a user +/// snapshot no local artifact records (a publication that failed after +/// writing it), which the eviction would delete. It stays until someone +/// backs the snapshot up or deletes it, so the row is backed off. /// - `local_read_error` — the node-local SQLite read of one candidate failed; /// that row is backed off while the scan continues. /// - `rpc_error` — a candidate-specific cluster response failed or was /// malformed; that row is backed off while the scan continues. /// - `rpc_transport_error` — the cluster dependency is unavailable; the scan /// stops and retries from the same cursor next tick. -/// - `local_artifact_awaiting_backup` — a retired execution's teardown -/// release is held because another local artifact of the sandbox still -/// awaits its backup; the pod keeps terminating until it completes. -/// Emitted by the teardown driver once per drive. +/// - `local_artifact_awaiting_backup` — another local artifact of the +/// sandbox still awaits its backup, and the eviction, or a retired +/// execution's teardown release, would delete it too: the disk-pressure +/// loop leaves the sandbox for a later tick, and the teardown driver keeps +/// the pod terminating until it completes, once per drive. /// - `local_artifact_stamp_failed` — a retired execution's teardown release /// is held because the cluster proved another local artifact of the sandbox /// backed, but its node-local backup stamp couldn't be written, and the diff --git a/crates/l40_api/sbx/mgr/server/src/run/tests.rs b/crates/l40_api/sbx/mgr/server/src/run/tests.rs index 8a8faa8e88..0f55814c76 100644 --- a/crates/l40_api/sbx/mgr/server/src/run/tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/run/tests.rs @@ -1396,7 +1396,13 @@ async fn a_database_holding_a_recorded_capture_reopens_at_startup() { ); let artifact_dir = dir.path().join("suspend"); std::fs::create_dir_all(&artifact_dir).unwrap(); - crate::capture_record::write(&artifact_dir, staged, generation).unwrap(); + crate::capture_record::write( + &artifact_dir, + staged, + generation, + crate::capture_record::RecordWriter::CapturingTeardown, + ) + .unwrap(); conn.execute( "UPDATE local_artifacts SET local_path = ?1 WHERE local_artifact_id = ?2", crate::db::fixture::params![artifact_dir.to_str().unwrap(), staged.to_string()], diff --git a/crates/l40_api/sbx/mgr/server/src/sandbox_eviction.rs b/crates/l40_api/sbx/mgr/server/src/sandbox_eviction.rs index ff1113b1a7..600827044e 100644 --- a/crates/l40_api/sbx/mgr/server/src/sandbox_eviction.rs +++ b/crates/l40_api/sbx/mgr/server/src/sandbox_eviction.rs @@ -217,10 +217,9 @@ impl EvictionConfig { } /// Why a candidate is evictable. Doubles as the `reason` tag on -/// `sandbox.evict.evicted` and the sandbox event detail. There is -/// deliberately only one reason: every eviction must carry a proof -/// tied to the exact bytes being deleted ("a newer generation exists -/// elsewhere" is not such a proof and was removed). +/// `sandbox.evict.evicted` and the sandbox event detail. Each reason +/// names a proof tied to the exact bytes being deleted ("a newer +/// generation exists elsewhere" is not such a proof and was removed). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum EvictReason { /// The bytes on disk are provably backed: the pinned artifact's @@ -241,6 +240,11 @@ pub(crate) enum EvictReason { /// restore can adopt, so it's discarded unpublished. Driven by the /// teardown driver, not by this loop. ExecutionRetired, + /// A runtime upgrade decided from another checkpoint found this suspended + /// copy, and the copy's pinned artifact was proven backed for its exact + /// id, so it makes way for the decided checkpoint's staging. Driven by the + /// upgrade, not by this loop. + StaleUpgradeCopy, } impl EvictReason { @@ -249,6 +253,7 @@ impl EvictReason { EvictReason::LatestBackedIdle => "latest_backed_idle", EvictReason::ClusterRowAbsent => "cluster_row_absent", EvictReason::ExecutionRetired => "execution_retired", + EvictReason::StaleUpgradeCopy => "stale_upgrade_copy", } } } @@ -272,6 +277,16 @@ pub(crate) enum SkipReason { GenerationUnproven, RetryBackoff, NotSuspendedAtExecute, + /// The sandbox was still suspended with the proven pin, but its claim was + /// refused because something held it: a runtime upgrade, an artifact + /// upload in flight, another local artifact without a backup stamp, or + /// another operation holding its artifact lock. + HeldAtExecute, + /// The sandbox's snapshot directory holds a user snapshot no local + /// artifact records, which the eviction would delete. Unlike the holds + /// in [`Self::HeldAtExecute`] it doesn't end on its own: someone has to + /// back the snapshot up or delete it, so the row is backed off. + UnrecordedSnapshot, /// The node-local SQLite read of the decision inputs failed. /// Distinct from [`Self::RpcError`] so a flaky cluster-api and a /// node-local DB problem don't share a dashboard signal. @@ -282,9 +297,9 @@ pub(crate) enum SkipReason { /// The cluster dependency is unavailable for the whole scan. Retain the /// cursor and retry the same oldest candidate next tick. RpcTransportError, - /// A retired execution's teardown release is held because another local - /// artifact of the sandbox still awaits its backup; the release would - /// delete those bytes too. Counted by the teardown driver, not this loop. + /// The eviction, or a retired execution's teardown release, is held + /// because another local artifact of the sandbox still awaits its + /// backup; it would delete those bytes too. LocalArtifactAwaitingBackup, /// A retired execution's teardown release is held because the cluster /// proved another local artifact of the sandbox backed, but its @@ -308,6 +323,8 @@ impl SkipReason { SkipReason::GenerationUnproven => "generation_unproven", SkipReason::RetryBackoff => "retry_backoff", SkipReason::NotSuspendedAtExecute => "not_suspended_at_execute", + SkipReason::HeldAtExecute => "held_at_execute", + SkipReason::UnrecordedSnapshot => "unrecorded_snapshot", SkipReason::LocalReadError => "local_read_error", SkipReason::RpcError => "rpc_error", SkipReason::RpcTransportError => "rpc_transport_error", @@ -325,6 +342,7 @@ impl SkipReason { | Self::UnknownLocalGeneration | Self::CurrentArtifactUnproven | Self::GenerationUnproven + | Self::UnrecordedSnapshot | Self::LocalReadError | Self::RpcError ) @@ -756,6 +774,19 @@ fn record_continuing_candidate_skip( debug_assert_eq!(flow, CandidateFlow::Continue); } +/// The skip reason for an eviction the executor refused with +/// `FailedPrecondition` or `NotFound`, `still_proven` when the row is still +/// the suspended copy the decision proved. +pub(crate) fn claim_refusal_skip_reason(status: &tonic::Status, still_proven: bool) -> SkipReason { + if crate::service::is_unrecorded_snapshot_refusal(status) { + SkipReason::UnrecordedSnapshot + } else if still_proven { + SkipReason::HeldAtExecute + } else { + SkipReason::NotSuspendedAtExecute + } +} + fn classify_rpc_failure(status: &tonic::Status) -> SkipReason { match status.code() { // Codes that mean the cluster dependency cannot serve any candidate @@ -927,6 +958,66 @@ async fn prove_pinned_artifact( Ok(true) } +/// A cluster-api client for one sandbox's backup readiness `check`; a +/// failure to connect is [`SkipReason::RpcTransportError`], logged as the +/// check's. +async fn connect_cluster( + sandbox_id: &str, + check: &'static str, +) -> Result { + pplx_space_node_api_client::cluster::connect_from_env() + .await + .map_err(|error| { + warn!(sandbox_id, %error, "{check} could not reach cluster-api"); + SkipReason::RpcTransportError + }) +} + +/// Whether `pinned` is proven backed by the same evidence eviction acts on: +/// its local `backup_completed_at` marker, or else the cluster's exact-artifact +/// readiness check ([`prove_pinned_artifact`]). `Ok(false)` when the cluster +/// has no ready backup of it; a failure to get an answer is its classified +/// reason. +pub(crate) async fn prove_pinned_backup( + service: &SandboxManagerService, + sandbox_id: &str, + pinned: &PinnedArtifact, +) -> Result { + if pinned.backup_marker { + return Ok(true); + } + let mut cluster = connect_cluster(sandbox_id, "pinned artifact backup proof").await?; + prove_pinned_artifact(&mut cluster, service, sandbox_id, pinned).await +} + +/// Whether the cluster shows `later`'s ready backup was captured by a later +/// execution of `sandbox_id` than `earlier`'s: its capture generation is +/// greater. `Ok(false)` when it isn't, or when either backup has no recorded +/// capture generation; a failure to get an answer is its classified reason. +pub(crate) async fn prove_captured_after( + sandbox_id: &str, + later: (LocalArtifactId, ArtifactKind), + earlier: (LocalArtifactId, ArtifactKind), +) -> Result { + use pplx_space_node_api_client::proto::{ + LocalArtifactBackupIdentity, LocalArtifactCapturedAfter, backup_readiness_query::Artifact, + }; + let identity = |(id, kind): (LocalArtifactId, ArtifactKind)| LocalArtifactBackupIdentity { + local_artifact_id: id.to_string(), + artifact_kind: pplx_space_sbx_mgr_client::proto::ArtifactKind::from(kind) as i32, + }; + let mut cluster = connect_cluster(sandbox_id, "checkpoint capture order check").await?; + backup_ready( + &mut cluster, + sandbox_id, + Artifact::CapturedAfter(LocalArtifactCapturedAfter { + later: Some(identity(later)), + earlier: Some(identity(earlier)), + }), + ) + .await +} + /// Stamp `artifact_id`'s `backup_completed_at` marker after the cluster proved /// its backup ready. Returns whether the stamp was written; a failure is /// logged. Eviction needs only the proof and re-proves next tick; the @@ -996,12 +1087,7 @@ pub(crate) async fn prove_local_artifact_backups( if artifacts.is_empty() { return Ok(true); } - let mut cluster = pplx_space_node_api_client::cluster::connect_from_env() - .await - .map_err(|error| { - warn!(sandbox_id, %error, "local artifact backup proof could not reach cluster-api"); - SkipReason::RpcTransportError - })?; + let mut cluster = connect_cluster(sandbox_id, "local artifact backup proof").await?; for artifact in artifacts { if !backup_ready( &mut cluster, @@ -1025,12 +1111,7 @@ pub(crate) async fn prove_local_artifact_backups( /// sandbox that isn't restorable. pub(crate) async fn sandbox_has_backed_copy(sandbox_id: &str) -> Result { use pplx_space_node_api_client::proto::backup_readiness_query::Artifact; - let mut cluster = pplx_space_node_api_client::cluster::connect_from_env() - .await - .map_err(|error| { - warn!(sandbox_id, %error, "backed-copy check could not reach cluster-api"); - SkipReason::RpcTransportError - })?; + let mut cluster = connect_cluster(sandbox_id, "backed-copy check").await?; backup_ready(&mut cluster, sandbox_id, Artifact::DurablyRestorable(true)).await } @@ -1511,6 +1592,56 @@ async fn evict_tick( LocalEvidence::Pinned(pinned) => Some(pinned.artifact_id), _ => None, }; + // The eviction deletes every local artifact of the sandbox, not only + // the one proven: a named snapshot nothing backed up would be lost. + // Each other artifact the cluster shows backed is stamped, and the + // sandbox is left while any stays unproven; the claim refuses while + // one is unstamped. + let awaiting = service.db_conn().and_then(|db| { + crate::db::read::sandbox_view(&db, &sandbox_id) + .artifacts() + .awaiting_backup_besides(proven_pin) + .map_err(|error| tonic::Status::internal(format!("local artifact lookup: {error}"))) + }); + let Ok(awaiting) = awaiting else { + record_continuing_candidate_skip( + &mut report, + scan_state, + &sandbox_id, + cursor, + SkipReason::LocalReadError, + scan_started, + ); + continue; + }; + match prove_local_artifact_backups(service, &sandbox_id, &awaiting).await { + Ok(true) => {} + Ok(false) => { + record_continuing_candidate_skip( + &mut report, + scan_state, + &sandbox_id, + cursor, + SkipReason::LocalArtifactAwaitingBackup, + scan_started, + ); + continue; + } + Err(reason) => { + let flow = record_candidate_skip( + &mut report, + scan_state, + &sandbox_id, + cursor, + reason, + scan_started, + ); + if flow == CandidateFlow::Stop { + break; + } + continue; + } + } // Size estimate before the directory disappears. let cache_dir = sandbox_cache_dir(&sandbox_id); @@ -1589,13 +1720,23 @@ async fn evict_tick( { // A resume/restore claimed the row (or a delete removed // it) between planning and the eviction CAS — exactly - // what the claim exists for. + // what the claim exists for — or the row is still the + // suspended copy proven, and the claim found it held. + let still_proven = status.code() == tonic::Code::FailedPrecondition + && service + .db_conn() + .ok() + .and_then(|db| crate::db::read::sandbox(&db, &sandbox_id).ok().flatten()) + .is_some_and(|row| { + row.state.is_unowned(crate::state::SandboxState::Suspended) + && row.current_local_artifact_id == proven_pin + }); record_continuing_candidate_skip( &mut report, scan_state, &sandbox_id, cursor, - SkipReason::NotSuspendedAtExecute, + claim_refusal_skip_reason(&status, still_proven), scan_started, ); } @@ -1706,5 +1847,29 @@ pub async fn run_loop( } } +/// One disk-pressure tick over `service`'s candidates at 80% disk use, with +/// no idle minimum, for a test outside this module: how many candidates it +/// evicted and skipped. +#[cfg(test)] +pub(crate) async fn evict_tick_for_test(service: &SandboxManagerService) -> (usize, usize) { + let cfg = EvictionConfig { + enabled: true, + engage_fraction: 0.50, + target_fraction: 0.40, + min_idle: Duration::from_secs(0), + interval: Duration::from_secs(60), + max_per_tick: 10, + reupload_max_per_tick: 0, + }; + let disk = crate::disk_usage::DiskUsage { + used_bytes: 80, + total_bytes: 100, + }; + let report = evict_tick(service, cfg, Some(disk), &mut CandidateScanState::default()) + .await + .unwrap(); + (report.evicted, report.skipped) +} + #[cfg(test)] mod tests; diff --git a/crates/l40_api/sbx/mgr/server/src/sandbox_eviction/tests.rs b/crates/l40_api/sbx/mgr/server/src/sandbox_eviction/tests.rs index 2634f197f6..107893b416 100644 --- a/crates/l40_api/sbx/mgr/server/src/sandbox_eviction/tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/sandbox_eviction/tests.rs @@ -720,6 +720,32 @@ fn candidates_filter_state_idle_and_inflight_uploads() { // durability with the cluster. insert_sandbox(&conn, "sbx-pinned", "suspended", 3_000); insert_pinned_artifact(&conn, "sbx-pinned", &v7(3_000).to_string(), None); + // Pinned but still held, which the eviction claim would refuse after a + // proof: an upload still cancelling, and a runtime upgrade that owns the + // row — excluded. + insert_sandbox(&conn, "sbx-cancelling", "suspended", 3_000); + insert_pinned_artifact(&conn, "sbx-cancelling", &v7(3_001).to_string(), None); + insert_upload_job( + &conn, + "j-cancelling", + "sbx-cancelling", + "cancelling", + SnapshotId::from_uuid(v7(3_000)), + "2026-01-01 00:00:00", + ); + insert_sandbox(&conn, "sbx-upgrading", "suspended", 3_000); + insert_pinned_artifact(&conn, "sbx-upgrading", &v7(3_002).to_string(), None); + conn.execute( + "INSERT INTO sandbox_runtime_upgrades ( + operation_id, sandbox_id, target_environment, target_state, + authorization_id, authorization_reason, memory_loss_authorized, + source_state, previous_execution_metadata_json, phase, + created_at, updated_at) + VALUES (?1, 'sbx-upgrading', 'e1', 'started', 'a', 'r', 1, + 'suspended', '{}', 'verified', '1', '1')", + params![uuid::Uuid::now_v7().to_string()], + ) + .unwrap(); let got = candidate_ids(list_candidates(&conn, 5_000, None, 10).unwrap()); assert_eq!(got, vec!["sbx-done".to_string(), "sbx-pinned".to_string()]); diff --git a/crates/l40_api/sbx/mgr/server/src/service/activation_support.rs b/crates/l40_api/sbx/mgr/server/src/service/activation_support.rs index 072d72cbcc..ea9449c5f2 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/activation_support.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/activation_support.rs @@ -449,6 +449,48 @@ impl SandboxManagerService { ) } + /// Refuse an activation identity that names another node. + fn require_identity_on_this_node( + &self, + identity: &SpaceApiRouteIdentity, + ) -> Result<(), Status> { + if let Some(requested_node_id) = identity.current_node_id { + let local_node_id = self.local_node_id.or_else(local_space_node_id).ok_or_else(|| { + Status::failed_precondition( + "runtime activation identity validation requires SPACE_NODE_ID or a populated /space/etc/node-id", + ) + })?; + if requested_node_id != local_node_id { + return Err(Status::failed_precondition(format!( + "runtime activation identity targets node {requested_node_id}, but this sandbox-manager is node {local_node_id}", + ))); + } + } + Ok(()) + } + + /// The identity [`Self::resolve_runtime_activation_identity`] admits for + /// `sandbox_id` once this node holds no row of it, which could supply + /// provenance or a retained generation. A caller about to remove the row + /// checks it first, so an identity the stage that follows would refuse + /// removes nothing. + pub(super) fn resolve_rowless_activation_identity( + &self, + sandbox_id: &str, + requested: SpaceApiRouteIdentity, + ) -> Result { + validate_route_identity_sandbox(&requested, sandbox_id)?; + let identity = + resolved_requested_runtime_activation_identity(requested, false, self.local_node_id) + .ok_or_else(|| { + pplx_space_sbx_mgr_client::authoritative_route_identity_required_status( + "identity-free activation has no local route provenance; supply an authoritative route identity", + ) + })?; + self.require_identity_on_this_node(&identity.identity)?; + Ok(identity.identity) + } + pub(super) fn resolve_runtime_activation_identity_with_authority( &self, sandbox_id: &str, @@ -467,18 +509,7 @@ impl SandboxManagerService { ) { let generation_fence_enabled = pplx_space_api_route::space_api_route_generation_fence_enabled(); - if let Some(requested_node_id) = identity.identity.current_node_id { - let local_node_id = self.local_node_id.or_else(local_space_node_id).ok_or_else(|| { - Status::failed_precondition( - "runtime activation identity validation requires SPACE_NODE_ID or a populated /space/etc/node-id", - ) - })?; - if requested_node_id != local_node_id { - return Err(Status::failed_precondition(format!( - "runtime activation identity targets node {requested_node_id}, but this sandbox-manager is node {local_node_id}", - ))); - } - } + self.require_identity_on_this_node(&identity.identity)?; let provenance = { let db = self.db_conn()?; match crate::db::read::sandbox_view(&db, sandbox_id).activation_route_provenance() { diff --git a/crates/l40_api/sbx/mgr/server/src/service/eviction.rs b/crates/l40_api/sbx/mgr/server/src/service/eviction.rs index 16c3ca6181..d37f5c2960 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/eviction.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/eviction.rs @@ -41,6 +41,19 @@ pub(crate) enum EvictionClaim { /// exact pinned artifact the eviction decision proved backed, or `None` /// asserting the row is still unpinned. SuspendedPinned(Option), + /// [`Self::SuspendedPinned`] for a sandbox the cluster deleted: the + /// sandbox's other local artifacts don't hold it back, since the + /// cluster's snapshots of a sandbox go with its row. + DeletedSuspended(Option), + /// `suspended → deleting` for a copy a runtime upgrade replaces with the + /// checkpoint it was decided from: the proven pin, no upgrade owning the + /// row and no upload in flight, as for [`Self::SuspendedPinned`], and, in + /// place of a row version, the activation generations the upgrade checked + /// its activation against, since removing the row removes their fence. + StaleUpgradeCopy { + pin: LocalArtifactId, + generations: crate::db::ActivationGenerations, + }, /// `restoring → deleting` for a row a failed cross-node restore retained /// "for retry" (`failure_reason` persisted, state left `restoring`). /// Only meaningful once the cluster reports the sandbox deleted — a @@ -144,6 +157,175 @@ pub(crate) enum EvictionClaim { }, } +/// Marks a suspended eviction refused because another operation held the +/// sandbox's artifact lock: the refusal keeps `FailedPrecondition`, which the +/// disk-pressure loop skips quietly, and a caller that retries on its next +/// sync tells it from a claim the row's state refused by this. +const ARTIFACT_LOCK_HELD_METADATA: &str = "x-space-eviction-artifact-lock-held"; + +/// The refusal of a suspended eviction of `sandbox_id` while another +/// operation holds its artifact lock. +fn artifact_lock_held_status(sandbox_id: &str) -> Status { + marked_refusal( + format!( + "another operation holds sandbox {sandbox_id}'s artifacts (a backup being admitted, \ + a transfer, a publication or a cleanup); retry the eviction once it's done" + ), + ARTIFACT_LOCK_HELD_METADATA, + ) +} + +/// A suspended eviction's `FailedPrecondition` refusal carrying `marker`, so +/// a caller can tell this refusal from the others with that code. +fn marked_refusal(message: String, marker: &'static str) -> Status { + let mut status = Status::failed_precondition(message); + status + .metadata_mut() + .insert(marker, tonic::metadata::MetadataValue::from_static("true")); + status +} + +/// Marks a suspended eviction refused because the sandbox's snapshot +/// directory holds a user-named snapshot no local artifact records. It keeps +/// `FailedPrecondition`; the disk-pressure loop counts it under its own skip +/// reason, since unlike the other holds it doesn't end on its own. +const UNRECORDED_SNAPSHOT_METADATA: &str = "x-space-eviction-unrecorded-snapshot"; + +/// Whether `status` is a suspended eviction's refusal for a snapshot no local +/// artifact records. +pub(crate) fn is_unrecorded_snapshot_refusal(status: &Status) -> bool { + status.code() == tonic::Code::FailedPrecondition + && status.metadata().contains_key(UNRECORDED_SNAPSHOT_METADATA) +} + +/// The refusal of a suspended eviction of `sandbox_id`, whose snapshot +/// directory holds `names` that no local artifact records. +fn unrecorded_snapshot_status(sandbox_id: &str, names: &[String]) -> Status { + marked_refusal( + format!( + "sandbox {sandbox_id} holds snapshots no local artifact records ({}); the eviction \ + would delete them, so back them up or delete them first", + names.join(", ") + ), + UNRECORDED_SNAPSHOT_METADATA, + ) +} + +/// The entries of a sandbox's `snapshots_dir` that are user snapshots no +/// local artifact records, sorted: see [`is_unrecorded_user_snapshot`]. A +/// missing directory, or one replaced by a symlink, whose target the cleanup +/// doesn't delete, holds nothing to keep. Neither does an entry whose name +/// isn't UTF-8: snapshot RPCs carry names as strings, so none can address +/// it. Node-manager's snapshot listing skips it, and the full-snapshot scan +/// shows it only under a lossily decoded name, which is another path. +pub(in crate::service) fn unrecorded_snapshots( + snapshots_dir: &std::path::Path, + recorded: &std::collections::HashSet, +) -> std::io::Result> { + match std::fs::symlink_metadata(snapshots_dir) { + Ok(metadata) if metadata.file_type().is_symlink() => return Ok(Vec::new()), + Ok(_) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(Vec::new()), + Err(error) => return Err(error), + } + let mut unrecorded = Vec::new(); + for entry in std::fs::read_dir(snapshots_dir)? { + let Ok(name) = entry?.file_name().into_string() else { + continue; + }; + if is_unrecorded_user_snapshot(snapshots_dir, &name, recorded)? { + unrecorded.push(name); + } + } + unrecorded.sort(); + Ok(unrecorded) +} + +/// Whether `name`, an entry of the sandbox's `snapshots_dir`, is a user +/// snapshot that no name in `recorded` (the snapshot names of the sandbox's +/// named local artifacts) records. +/// +/// A publication writes a snapshot's directory before its row, so one that +/// fails in between, or a crash, leaves the directory without a row. Listing +/// shows it and backup adopts it, so it's the user's data, and the claim's +/// backup clause can't see it. Every name a user snapshot can have starts with +/// `d-` or `f-`: sandbox-manager names manual and scheduled captures itself, +/// and an import's name must carry the prefix of its type; backup adoption +/// accepts only those prefixes too. Of those, two kinds aren't the user's to +/// keep: +/// - internal names (operation captures and their encoded artifacts, a +/// runtime upgrade's preserved disk, stopped-disk artifacts and GC +/// staging), which users can neither list nor back up; +/// - a full snapshot without its VM state, which a failed capture leaves and +/// which restores nothing: the listing reports it `incomplete` by the same +/// test. A disk snapshot has no such test, so a half-written one is kept. +/// +/// A scheduled capture is kept like any other: retention prunes it only while +/// the sandbox runs, and a suspended sandbox's scheduler is stopped. Only a +/// real directory counts, not a symlink or any other file, as only those are +/// what the snapshot listing shows: an entry nobody can see must not hold the +/// sandbox. Everything else under the directory (suspend and +/// encrypted-capture staging, say) has neither prefix, and only the eviction +/// would ever remove it. +/// +/// An entry that's gone, or a VM state file that's missing or empty, is an +/// answer; any other error reading them is returned rather than taken for one, +/// since the eviction deletes whatever this doesn't keep. +pub(in crate::service) fn is_unrecorded_user_snapshot( + snapshots_dir: &std::path::Path, + name: &str, + recorded: &std::collections::HashSet, +) -> std::io::Result { + use super::grpc_spaced_process_snapshots::{full_snapshot_complete, snapshot_entry_metadata}; + let is_disk = name.starts_with("d-"); + if !(is_disk || name.starts_with("f-")) + || super::grpc_spaced_process_snapshots::is_internal_snapshot_name(name) + || recorded.contains(name) + { + return Ok(false); + } + let path = snapshots_dir.join(name); + match snapshot_entry_metadata(&path) { + Ok(metadata) if metadata.is_dir() => {} + Ok(_) => return Ok(false), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(false), + Err(error) => return Err(error), + } + if is_disk { + Ok(true) + } else { + full_snapshot_complete(&path) + } +} + +/// The refusal of a cluster-deleted eviction of `sandbox_id` while `hold` +/// holds it: the hold ends on its own, so the caller retries on its next sync. +fn held_status(sandbox_id: &str, hold: &str) -> Status { + Status::aborted(format!( + "sandbox {sandbox_id} is held by {hold}; retry the cluster-deleted eviction on the next \ + sync" + )) +} + +/// What still holds a suspended row that its eviction claim waits for: a +/// runtime upgrade that still owns it, or an upload of its artifacts that may +/// still read them. `None` when nothing does. +fn suspended_eviction_hold( + db: &crate::db::Connection, + sandbox_id: &str, +) -> Result, Status> { + let read_error = |error: crate::db::Error| Status::internal(format!("db read failed: {error}")); + if let Some(operation) = + crate::db::read::unretired_runtime_upgrade(db, sandbox_id).map_err(read_error)? + { + return Ok(Some(format!("runtime upgrade {operation}"))); + } + if crate::db::read::snapshot_upload_in_flight(db, sandbox_id).map_err(read_error)? { + return Ok(Some("an upload of its artifacts".to_string())); + } + Ok(None) +} + impl SandboxManagerService { /// Remove a sandbox's entire local presence: claim the row with the /// CAS `claim` describes (see [`EvictionClaim`]), then run the standard `DeleteRow` cleanup to @@ -167,7 +349,15 @@ impl SandboxManagerService { /// with a persisted failure or `quarantined` at the version read, or not `suspending` /// with the terminal teardown-backup marker, the same staged artifact /// and the same `state_changed_at` (a concurrent resume / re-suspend / - /// retry won the race); nothing was changed. + /// retry won the race); a `suspended` row a runtime upgrade or an + /// artifact upload still holds, or, for a stale upgrade copy, whose + /// activation generations moved since they were read; or, for a + /// pinned-suspended or stale-upgrade-copy claim, a user snapshot no + /// local artifact records ([`is_unrecorded_snapshot_refusal`]); nothing + /// was changed. A cleanup the claim handed off can fail with this code + /// too, and then the row is `deleting`. + /// - `Unavailable` before the claim — a cleanup is already active, or the + /// snapshot inventory couldn't be read; nothing was changed. /// - other statuses — the claim succeeded but cleanup failed; the /// row stays in `deleting` and the cleanup machinery converges on /// retry (the same contract a failed Delete RPC has). @@ -412,6 +602,46 @@ impl SandboxManagerService { result } + /// Refuse a suspended eviction of `sandbox_id` while its snapshot + /// directory holds a user snapshot no local artifact records (see + /// [`unrecorded_snapshots`]). The caller holds the artifact lock. + /// + /// The listing runs inline, as the claim's SQLite write after it does, so + /// the claim stays in the same poll. Its size isn't fixed: retention aims + /// to keep 22 scheduled captures (the `KEEP_*` counts in + /// `snapshot_scheduler/retention.rs`) and may keep more, and manual + /// snapshots have no count of their own. The named-row read it starts + /// with refuses a sandbox past 4096 rows (`MAX_ITEMS`) or 16 MiB of row + /// data (`MAX_RAW_BYTES`) as `Unavailable`. + fn refuse_unrecorded_snapshots(&self, sandbox_id: &str) -> Result<(), Status> { + let recorded: std::collections::HashSet = { + let db = self.db_conn()?; + crate::db::read::sandbox_view(&db, sandbox_id) + .artifacts() + .named() + .map_err(|error| { + Status::unavailable(format!( + "couldn't read sandbox {sandbox_id}'s named snapshots: {error}" + )) + })? + .into_iter() + .map(|publication| publication.snapshot_name.to_string()) + .collect() + }; + let snapshots_dir = self.cache_root().join(sandbox_id).join("snapshots"); + let unrecorded = unrecorded_snapshots(&snapshots_dir, &recorded).map_err(|error| { + Status::unavailable(format!( + "couldn't list {}: {error}", + snapshots_dir.display() + )) + })?; + if unrecorded.is_empty() { + Ok(()) + } else { + Err(unrecorded_snapshot_status(sandbox_id, &unrecorded)) + } + } + async fn evict_local_sandbox_owned( &self, sandbox_id: &str, @@ -458,9 +688,53 @@ impl SandboxManagerService { })?, ), EvictionClaim::SuspendedPinned(_) + | EvictionClaim::DeletedSuspended(_) + | EvictionClaim::StaleUpgradeCopy { .. } + | EvictionClaim::TeardownSuspendPermanentlyFailed { .. } + | EvictionClaim::TeardownExecutionRetired { .. } => None, + }; + // A suspended row's eviction deletes every local artifact of the + // sandbox, and a backup being admitted (`enqueue_artifact_backup` + // holds the artifact lock from reading the row until its upload job + // is persisted) would lose its source; the claim's upload clause + // sees only a persisted job. So the claim is taken under the artifact + // lock, taken without waiting: a backup holding it refuses the + // eviction, which retries, and an admission that waits for it finds + // the row `deleting` and refuses itself. Never waiting is what keeps + // the lock order free of cycles, and the guard is dropped before the + // cleanup, whose finalization takes the same lock. + let artifact_guard = match claim { + EvictionClaim::SuspendedPinned(_) + | EvictionClaim::DeletedSuspended(_) + | EvictionClaim::StaleUpgradeCopy { .. } => Some( + self.auto_lifecycle + .artifact_lock(sandbox_id) + .try_lock_owned() + .map_err(|_| artifact_lock_held_status(sandbox_id))?, + ), + EvictionClaim::Stopped + | EvictionClaim::FailedRestoring + | EvictionClaim::Quarantined | EvictionClaim::TeardownSuspendPermanentlyFailed { .. } | EvictionClaim::TeardownExecutionRetired { .. } => None, }; + // Under the artifact lock, which every writer of a named snapshot + // holds, a user snapshot without a row can't appear before the claim. + // A cluster-deleted sandbox's snapshots went with its cluster row, so + // that eviction doesn't look. + if matches!( + claim, + EvictionClaim::SuspendedPinned(_) | EvictionClaim::StaleUpgradeCopy { .. } + ) { + self.refuse_unrecorded_snapshots(sandbox_id)?; + } + #[cfg(test)] + if artifact_guard.is_some() { + crate::auto_lifecycle::AutoLifecycleCoordinator::rendezvous( + &self.auto_lifecycle.eviction_claim_barrier, + ) + .await; + } let claimed = { let db = self.db_conn()?; match &claim { @@ -473,6 +747,23 @@ impl SandboxManagerService { expected_updated_at, ) } + EvictionClaim::DeletedSuspended(pin) => { + let pin = pin.map(|id| id.to_string()); + crate::db::claim_deleted_suspended_sandbox_for_eviction( + &db, + sandbox_id, + pin.as_deref(), + expected_updated_at, + ) + } + EvictionClaim::StaleUpgradeCopy { pin, generations } => { + crate::db::claim_stale_upgrade_copy_for_eviction( + &db, + sandbox_id, + &pin.to_string(), + generations, + ) + } EvictionClaim::Stopped => crate::db::claim_stopped_sandbox_for_eviction( &db, sandbox_id, @@ -530,7 +821,19 @@ impl SandboxManagerService { format!("sandbox {sandbox_id} is no longer stopped at the observed row version") } EvictionClaim::SuspendedPinned(_) => format!( - "sandbox {sandbox_id} is no longer suspended with the proven artifact pinned" + "sandbox {sandbox_id} is no longer suspended with the proven artifact \ + pinned, a runtime upgrade or an artifact upload still holds it, or another \ + of its local artifacts isn't backed up" + ), + EvictionClaim::DeletedSuspended(_) => format!( + "sandbox {sandbox_id} is no longer suspended with the artifact it pinned, \ + or a runtime upgrade or an artifact upload still holds it" + ), + EvictionClaim::StaleUpgradeCopy { .. } => format!( + "sandbox {sandbox_id} is no longer suspended with the proven artifact \ + pinned at the activation generations checked, a runtime upgrade or an \ + artifact upload still holds it, or another of its local artifacts isn't \ + backed up" ), EvictionClaim::FailedRestoring => format!( "sandbox {sandbox_id} is no longer a failed cross-node restore at the \ @@ -552,9 +855,10 @@ impl SandboxManagerService { ), })); } - // The row is `deleting` now; release the staging lock before the - // cleanup re-acquires it. + // The row is `deleting` now; release the staging and artifact locks + // before the cleanup re-acquires them. drop(staging_guard); + drop(artifact_guard); self.watchdog.remove_sandbox(sandbox_id).await; self.snapshot_scheduler.remove_sandbox(sandbox_id).await; @@ -619,8 +923,12 @@ impl SandboxManagerService { /// `suspending`, not since the backup was refused) for the terminal /// teardown suspend, whose row is touched by live loops without /// changing (see [`EvictionClaim::TeardownSuspendPermanentlyFailed`]); + /// - a `suspended` row a runtime upgrade still owns, or whose artifacts an + /// upload may still read, is refused as `Aborted`, so the caller retries + /// on its next sync: the hold ends on its own, by the upgrade's + /// recovery or the upload's last attempt; /// - the teardown CAS (`suspended → deleting` with the pinned-artifact - /// identity re-asserted, `restoring/quarantined → deleting` at the exact row + /// identity re-asserted and no such hold, `restoring/quarantined → deleting` at the exact row /// version read, or `suspending → deleting` with the marker, staged /// artifact and `state_changed_at` re-asserted) closes the remaining /// window. @@ -651,7 +959,7 @@ impl SandboxManagerService { // Both reads come off the same connection back to back, and every // claim re-asserts what it decided against, so a row that moves // between them fails its CAS rather than being acted on. - let (row, state_changed_at) = { + let (row, state_changed_at, held) = { let db = self.db_conn()?; let row = crate::db::read::sandbox(&db, &sandbox_id) .map_err(|e| Status::internal(format!("db sandbox lookup failed: {e}")))? @@ -659,13 +967,25 @@ impl SandboxManagerService { let state_changed_at = crate::db::read::state_changed_at_witness(&db, &sandbox_id) .map_err(|e| Status::internal(format!("db sandbox lookup failed: {e}")))? .ok_or_else(|| Status::not_found(format!("sandbox {sandbox_id} not found")))?; - (row, state_changed_at) + // Only a suspended row's claim waits for a hold. + let held = if row.state.is_unowned(crate::state::SandboxState::Suspended) { + suspended_eviction_hold(&db, &sandbox_id)? + } else { + None + }; + (row, state_changed_at, held) }; let claim = match row.state.unowned() { Some(crate::state::SandboxState::Stopped) => EvictionClaim::Stopped, Some(crate::state::SandboxState::Quarantined) => EvictionClaim::Quarantined, + // The claim refuses the row while an upgrade or an upload holds it, + // and such a hold ends on its own, so it's answered as a pull in + // flight is: retry on the next sync, not after a back-off. + Some(crate::state::SandboxState::Suspended) if let Some(hold) = held => { + return Err(held_status(&sandbox_id, &hold)); + } Some(crate::state::SandboxState::Suspended) => { - EvictionClaim::SuspendedPinned(row.current_local_artifact_id) + EvictionClaim::DeletedSuspended(row.current_local_artifact_id) } Some(crate::state::SandboxState::Restoring) if row @@ -750,11 +1070,14 @@ impl SandboxManagerService { } EvictionClaim::Stopped | EvictionClaim::SuspendedPinned(_) + | EvictionClaim::DeletedSuspended(_) | EvictionClaim::FailedRestoring | EvictionClaim::Quarantined => ("updated_at", row.updated_at.as_str()), - EvictionClaim::TeardownExecutionRetired { .. } => { + EvictionClaim::TeardownExecutionRetired { .. } + | EvictionClaim::StaleUpgradeCopy { .. } => { return Err(Status::internal( - "a cluster-deleted eviction never claims a retired teardown execution", + "a cluster-deleted eviction never claims a retired teardown execution or a \ + runtime upgrade's stale copy", )); } }; @@ -813,6 +1136,10 @@ impl SandboxManagerService { } else { tracing::Span::none() }; + let suspended_claim = matches!( + claim, + EvictionClaim::SuspendedPinned(_) | EvictionClaim::DeletedSuspended(_) + ); let teardown = async move { let reason = crate::sandbox_eviction::EvictReason::ClusterRowAbsent; // Size estimate before the directory disappears; log/event @@ -861,6 +1188,28 @@ impl SandboxManagerService { Some(&checked_updated_at), ) => result, }; + // A hold that appeared after the check above refused the claim + // the same way; answer it as the check would have. So is another + // operation holding the sandbox's artifacts, which ends on its + // own too. + let result = match result { + Err(status) if status.metadata().contains_key(ARTIFACT_LOCK_HELD_METADATA) => Err( + held_status(&sandbox_id, "another operation on its artifacts"), + ), + Err(status) + if suspended_claim && status.code() == tonic::Code::FailedPrecondition => + { + // A read that fails names no hold, and the claim's own + // refusal stands. + let held = service + .db_conn() + .and_then(|db| suspended_eviction_hold(&db, &sandbox_id)) + .ok() + .flatten(); + Err(held.map_or(status, |hold| held_status(&sandbox_id, &hold))) + } + result => result, + }; if result.is_ok() { crate::metrics::sandbox_eviction::logical_bytes_freed(reason.as_str(), bytes); tracing::info!( diff --git a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle.rs b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle.rs index 1bff0003cb..c51ec0cd82 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle.rs @@ -322,6 +322,11 @@ struct ArtifactBackupSubmission<'a> { handoff: ArtifactBackupHandoff, /// The teardown suspend's execution, for `BeginArtifactBackup`'s fence. execution_generation: Option, + /// The caller holds evidence that `execution_generation` is the + /// capturing execution's: the capture was just taken by it, or an earlier + /// upload of the same bytes recorded it so. Without that, the capture's + /// own record can still show it (see `submit_artifact_backup`). + capture_vouched: bool, } #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -470,6 +475,8 @@ mod backup_policy_tests; #[cfg(test)] mod retired_teardown_tests; #[cfg(test)] +pub(super) use self::retired_teardown_tests::{answer, with_cluster_api}; +#[cfg(test)] mod tests; #[cfg(test)] diff --git a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/artifact_backup.rs b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/artifact_backup.rs index ec3ae954df..4ed3e7841c 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/artifact_backup.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/artifact_backup.rs @@ -654,6 +654,19 @@ impl SandboxManagerService { .lock_artifact_for_sandbox(sandbox_id) .await .map_err(|status| artifact_backup_lookup_error(status, full_current_instance))?; + // An eviction claims a row under this lock, so a backup that waited + // for it may find the row on its way out, its artifacts about to be + // deleted: nothing can be backed up from it. + if sandbox.state.is_unowned(SandboxState::Deleting) + || sandbox + .cleanup_completion + .is_some_and(crate::db::CleanupCompletion::deletes_row) + { + return Err(Status::failed_precondition(format!( + "sandbox {} is being deleted; its artifacts can't be backed up", + sandbox.id + ))); + } // A runtime upgrade's failed boot leaves its disk in the instance until // the rollback restores the preserved copy; it must not be captured. self.refuse_runtime_upgrade_fence(&sandbox.id)?; @@ -981,6 +994,10 @@ impl SandboxManagerService { format, handoff: ArtifactBackupHandoff::RestoreVisible, execution_generation, + capture_vouched: earlier_upload_vouches( + previous_job.as_ref(), + execution_generation, + ), }, ) .await @@ -1038,6 +1055,16 @@ impl SandboxManagerService { Status, > { let id = self.lookup_sandbox(selector)?.id; + #[cfg(test)] + if let Some(hook) = self + .auto_lifecycle + .artifact_lock_wait_hook + .lock() + .unwrap() + .clone() + { + hook.notify_one(); + } let guard = super::restart_admission::wait_for_activation_admission( &id, self.auto_lifecycle.artifact_lock(&id).lock_owned(), @@ -1209,6 +1236,10 @@ impl SandboxManagerService { format: reupload_format, handoff: ArtifactBackupHandoff::DurableWorker, execution_generation, + capture_vouched: earlier_upload_vouches( + previous_job.as_ref(), + execution_generation, + ), }, ) .await @@ -1299,6 +1330,24 @@ impl SandboxManagerService { } } +/// Whether `previous_job`, the bytes' earlier upload, recorded `generation` +/// as the capturing execution's, so a re-upload under it may order captures. +pub(super) fn earlier_upload_vouches( + previous_job: Option<&pplx_space_common_jobs::Job>, + generation: Option, +) -> bool { + generation.is_some() + && previous_job + .and_then(|job| { + job.payload_as::() + .ok() + }) + .is_some_and(|payload| { + payload.execution_generation_names_capture + && payload.execution_generation == generation + }) +} + /// The execution generation the capture's record holds for the teardown /// suspend that captured `artifact`, if the record in its directory names it. /// A record that can't diff --git a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/backup_policy_tests.rs b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/backup_policy_tests.rs index f845a3a2c5..ddbf958175 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/backup_policy_tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/backup_policy_tests.rs @@ -534,6 +534,7 @@ async fn durable_artifact_backup_persists_the_submitted_artifact_identity() { format: SuspendSnapshotFormat::Full, handoff: ArtifactBackupHandoff::DurableWorker, execution_generation: None, + capture_vouched: false, }, ) .await; @@ -628,6 +629,118 @@ async fn durable_artifact_backup_persists_the_submitted_artifact_identity() { } } +/// A durable backup intent marks its execution generation as the capture's +/// only on evidence: the caller's, or the capturing teardown's own record +/// naming that generation for this artifact. Recovery's record doesn't count. A generation without either fences the registration but +/// mustn't order captures. +#[tokio::test] +async fn an_upload_names_its_capture_generation_only_on_evidence() { + 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 generation = |n| pplx_space_common_core::types::ExecutionGeneration::new(n).unwrap(); + use crate::capture_record::RecordWriter::{CapturingTeardown as Teardown, Recovery}; + // (case, generation sent, caller vouches, record's generation and writer, + // expected) + let cases = [ + ("caller vouches", Some(7), true, None, true), + ( + "record names the generation", + Some(7), + false, + Some((7, Teardown)), + true, + ), + ( + "record names another generation", + Some(7), + false, + Some((6, Teardown)), + false, + ), + ( + "recovery's record names the generation", + Some(7), + false, + Some((7, Recovery)), + false, + ), + ("no evidence", Some(7), false, None, false), + ( + "no generation to name", + None, + true, + Some((7, Teardown)), + false, + ), + ]; + for (name, sent, vouched, recorded, expected) in cases { + let conn = crate::db::test_conn(); + let id = SandboxId::new().to_string(); + insert_test_sandbox(&conn, &id, SandboxState::Suspended); + let mut service = SandboxManagerService::for_test(conn); + service.snapshot_upload_config = Some(Arc::new(upload_config("default-bucket"))); + let sandbox = service.lookup_sandbox(&id).unwrap(); + let root = tempfile::tempdir().unwrap(); + let snapshot_dir = root.path().join("artifact"); + write_artifact(&snapshot_dir, &id, &["vmstate", "memory", "rootfs.ext4"]); + let local_artifact_id = LocalArtifactId::new(); + if let Some((recorded, writer)) = recorded { + crate::capture_record::write( + &snapshot_dir, + local_artifact_id, + generation(recorded), + writer, + ) + .unwrap(); + } + + let outcome = service + .submit_artifact_backup( + &sandbox, + ArtifactBackupSubmission { + local_artifact_id, + artifact_kind: ArtifactKind::Full, + artifact_origin: ArtifactOrigin::Suspend, + snapshot_name: None, + backup_attempt_id: BackupAttemptId::new(), + snapshot_dir: snapshot_dir.to_str().unwrap(), + request_override: None, + format: SuspendSnapshotFormat::Full, + handoff: ArtifactBackupHandoff::DurableWorker, + execution_generation: sent.map(generation), + capture_vouched: vouched, + }, + ) + .await; + assert!( + matches!(outcome, Ok(SubmitSuspendSnapshotOutcome::Enqueued { .. })), + "{name}: {outcome:?}" + ); + let payload = service + .snapshot_upload_worker() + .find_job_by_dedup_key( + crate::snapshot_upload_jobs::SNAPSHOT_UPLOAD_JOB_KIND, + &local_artifact_id.to_string(), + ) + .unwrap() + .expect(name) + .payload_as::() + .unwrap(); + assert_eq!(payload.execution_generation, sent.map(generation), "{name}"); + assert_eq!( + payload.execution_generation_names_capture, expected, + "{name}" + ); + } +} + #[derive(Clone, Copy, Debug)] enum Egress { Allow, diff --git a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/backup_submission.rs b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/backup_submission.rs index 8e13e08220..044d4e288a 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/backup_submission.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/backup_submission.rs @@ -136,6 +136,7 @@ impl SandboxManagerService { ArtifactBackupHandoff::DurableWorker }, execution_generation: None, + capture_vouched: false, }, ) .await @@ -271,6 +272,7 @@ impl SandboxManagerService { format, handoff, execution_generation, + capture_vouched, } = submission; let config: &crate::snapshot_upload::SnapshotUploadConfig = match request_override { Some(c) => c, @@ -346,10 +348,28 @@ impl SandboxManagerService { ) .map_err(|error| format!("prepare snapshot upload intent failed: {error}"))?; jobs.execution_generation = execution_generation; + // Ordered by only on evidence: the caller's, or the record the + // capturing teardown published beside the bytes, naming this + // generation for this artifact. + jobs.execution_generation_names_capture = execution_generation.is_some_and(|generation| { + capture_vouched + || crate::capture_record::read_capture_order( + std::path::Path::new(snapshot_dir), + local_artifact_id, + ) + .ok() + .flatten() + == Some(generation) + }); let mut storage_uri = jobs.storage_uri.clone(); let dedup_key = crate::snapshot_upload_jobs::artifact_backup_dedup_key(local_artifact_id); let local_artifact_id = local_artifact_id.to_string(); + #[cfg(test)] + crate::auto_lifecycle::AutoLifecycleCoordinator::rendezvous( + &self.auto_lifecycle.backup_admission_barrier, + ) + .await; let job_id = match self.snapshot_upload_worker.submit_json( crate::snapshot_upload_jobs::SNAPSHOT_UPLOAD_JOB_KIND, &jobs, @@ -545,6 +565,8 @@ impl SandboxManagerService { snapshot_dir, format, node_local_registration_generation(sandbox), + // The current execution just took this capture. + true, ) .await .map_err(|error| artifact_backup_fence_status(error.to_string())) @@ -557,6 +579,7 @@ impl SandboxManagerService { snapshot_dir: &str, format: pplx_space_sbx_mgr_client::proto::SuspendSnapshotFormat, registration_generation: Option, + capture_vouched: bool, ) -> Result { let request_override = match self.resolve_org_snapshot_upload_config(&sandbox.id).await { OrgSnapshotConfigResolution::Resolved(config) => Some(config), @@ -584,6 +607,7 @@ impl SandboxManagerService { format, handoff: ArtifactBackupHandoff::TeardownFence, execution_generation: registration_generation, + capture_vouched, }, ) .await diff --git a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests.rs b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests.rs index bf31a93e89..83eb157cb6 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests.rs @@ -22,11 +22,12 @@ use crate::service::tests::service_tests::{ use crate::snapshot_upload_jobs::RegistrationProbe; use crate::state::SandboxState; pub(super) use cluster_api_stub_tests::{ - FinalizedBackup, finalized_backups, record_finalized_backup, with_cluster_api, + FinalizedBackup, finalized_backups, record_finalized_backup, }; use cluster_api_stub_tests::{ RETIRING, RetiringClusterApi, admitted_registrations, bound_upload_payload, }; +pub(in crate::service) use cluster_api_stub_tests::{answer, with_cluster_api}; const GENERATION: i64 = 7; diff --git a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/cluster_api_stub_tests.rs b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/cluster_api_stub_tests.rs index 40708ed4e6..377af6a23b 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/cluster_api_stub_tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/cluster_api_stub_tests.rs @@ -310,10 +310,8 @@ where /// Serves `stub` and runs `test` in an isolated child process whose /// cluster-api client wiring points at it, with a synthetic snapshot upload /// destination configured. -pub(in crate::service::grpc_lifecycle) async fn with_cluster_api( - stub: S, - test: impl AsyncFnOnce(), -) where +pub(in crate::service) async fn with_cluster_api(stub: S, test: impl AsyncFnOnce()) +where S: tower::Service< http::Request, Response = http::Response, @@ -395,3 +393,37 @@ pub(super) fn bound_upload_payload( payload.execution_generation = Some(generation(GENERATION)); payload } + +/// A gRPC handler answering one request type, for [`answer`]. +struct Answer(std::sync::Arc, std::marker::PhantomData Resp>); + +impl tonic::server::UnaryService for Answer +where + F: Fn(Req) -> Result, +{ + type Response = Resp; + type Future = std::future::Ready, tonic::Status>>; + + fn call(&mut self, request: tonic::Request) -> Self::Future { + std::future::ready((self.0)(request.into_inner()).map(tonic::Response::new)) + } +} + +/// Answer one gRPC `request` with `handler`, for a stub's `tower::Service`. +pub(in crate::service) async fn answer( + request: http::Request, + handler: impl Fn(Req) -> Result, +) -> http::Response +where + B: http_body::Body + Send + 'static, + B::Error: Into + Send + 'static, + Req: prost::Message + Default + Send + 'static, + Resp: prost::Message + Send + 'static, +{ + tonic::server::Grpc::new(tonic_prost::ProstCodec::::default()) + .unary( + Answer(std::sync::Arc::new(handler), std::marker::PhantomData), + request, + ) + .await +} diff --git a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/registration_tests.rs b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/registration_tests.rs index 453e818458..b9c1c4f2dc 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/registration_tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/registration_tests.rs @@ -78,7 +78,8 @@ fn node_local_suspend_registers_under_its_kubernetes_execution() { } /// The node-local suspend path end to end, up to the runtime it keeps: the -/// upload it persists carries the sandbox's Kubernetes execution generation, +/// upload it persists carries the sandbox's Kubernetes execution generation +/// as the capture's own, /// the cluster's refusal of that generation comes back as a backup-fence /// failure, and that failure rolls the suspend back to `paused` with the /// runtime still resident. @@ -136,6 +137,8 @@ async fn node_local_suspend_of_a_superseded_execution_rolls_back() { let payload: crate::snapshot_upload::SnapshotUploadJobs = serde_json::from_str(&payload).unwrap(); assert_eq!(payload.execution_generation, Some(generation(GENERATION))); + // The running execution took this capture, with no record beside it. + assert!(payload.execution_generation_names_capture); service .require_node_local_backup_handoff(&sandbox_id, local_artifact_id, Err(refused)) @@ -264,13 +267,18 @@ async fn eviction_reupload_of_a_refused_capture_keeps_its_execution_generation() ..RETIRING }, async || { - for (recorded, upload_retained, carried) in [ - (true, false, Some(generation(GENERATION))), - (false, true, Some(generation(GENERATION))), - (false, false, None), + // The re-upload orders captures by its generation only when the + // capture's record names it or the earlier upload did. + for (recorded, upload_retained, upload_names_capture, carried, names_capture) in [ + (true, false, false, Some(generation(GENERATION)), true), + (false, true, false, Some(generation(GENERATION)), false), + (false, true, true, Some(generation(GENERATION)), true), + (false, false, false, None, false), ] { let refused = refused_teardown_with(|staged| { - serde_json::to_string(&bound_upload_payload(staged)).unwrap() + let mut payload = bound_upload_payload(staged); + payload.execution_generation_names_capture = upload_names_capture; + serde_json::to_string(&payload).unwrap() }) .await; let service = &refused.service; @@ -320,6 +328,11 @@ async fn eviction_reupload_of_a_refused_capture_keeps_its_execution_generation() payload.execution_generation, carried, "recorded {recorded}, upload retained {upload_retained}" ); + assert_eq!( + payload.execution_generation_names_capture, names_capture, + "recorded {recorded}, upload retained {upload_retained}, earlier upload \ + names its capture {upload_names_capture}" + ); drain(service).await; } }, @@ -333,17 +346,22 @@ async fn eviction_reupload_of_a_refused_capture_keeps_its_execution_generation() /// earlier upload names that generation: after a retriable failure, and after /// the refused upload is pruned. The control plane, which superseded the /// generation, refuses it each time. A capture with no such evidence, as an -/// earlier node-local suspend's has, still registers unfenced. +/// earlier node-local suspend's has, still registers unfenced. The attempt +/// marks the generation as the capture's own only when the record names it or +/// the earlier upload marked it so. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn a_restart_suspended_teardown_capture_never_registers_unfenced() { with_cluster_api(RETIRING, async || { - for (recorded, upload_retained, fenced) in [ - (true, false, true), - (false, true, true), - (false, false, false), + for (recorded, upload_retained, upload_names_capture, fenced, names_capture) in [ + (true, false, false, true, true), + (false, true, false, true, false), + (false, true, true, true, true), + (false, false, false, false, false), ] { let refused = refused_teardown_with(|staged| { - serde_json::to_string(&bound_upload_payload(staged)).unwrap() + let mut payload = bound_upload_payload(staged); + payload.execution_generation_names_capture = upload_names_capture; + serde_json::to_string(&payload).unwrap() }) .await; let service = &refused.service; @@ -410,12 +428,167 @@ async fn a_restart_suspended_teardown_capture_never_registers_unfenced() { }; result.expect_err("a refused registration can't make the suspend durable"); } + let attempt = service + .snapshot_upload_worker + .find_job_by_dedup_key( + crate::snapshot_upload_jobs::SNAPSHOT_UPLOAD_JOB_KIND, + &staged, + ) + .unwrap() + .expect("the drive persists an attempt") + .payload_as::() + .unwrap(); + assert_ne!( + attempt.snapshot_id, + bound_upload_payload(refused.staged).snapshot_id, + "{case}: the drive must start a new attempt" + ); + assert_eq!( + attempt.execution_generation_names_capture, names_capture, + "{case}, earlier upload names its capture {upload_names_capture}" + ); drain(service).await; } }) .await; } +/// The earlier upload vouches for a generation only when it marked that same +/// generation as the capture's own. +#[test] +fn an_earlier_upload_vouches_only_for_the_generation_it_marked() { + let artifact = LocalArtifactId::new(); + let job = |names_capture| { + let mut payload = bound_upload_payload(artifact); + payload.execution_generation_names_capture = names_capture; + crate::snapshot_upload_jobs::tests::job_fixture( + crate::snapshot_upload_jobs::SNAPSHOT_UPLOAD_JOB_KIND, + serde_json::to_string(&payload).unwrap(), + ) + }; + let vouches = super::super::artifact_backup::earlier_upload_vouches; + let marked = job(true); + assert!(vouches(Some(&marked), Some(generation(GENERATION)))); + assert!(!vouches(Some(&marked), Some(generation(GENERATION + 1)))); + assert!(!vouches(Some(&marked), None)); + assert!(!vouches(Some(&job(false)), Some(generation(GENERATION)))); + assert!(!vouches(None, Some(generation(GENERATION)))); +} + +/// A teardown that crashed before publishing its capture leaves the +/// canonical directory holding the previous capture, A, which an earlier +/// execution took. Startup recovery adopts A as the teardown's capture and +/// records the teardown's generation for it, so its backup stays fenced under +/// that generation, as before. That record doesn't make A the capture of the +/// later execution: the drive's upload doesn't mark the generation as A's +/// own, so cluster-api records no capture generation for it and A can't order +/// as newer than a capture taken between the two executions. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_capture_recovery_adopts_after_a_crash_before_publish_orders_nothing() { + with_cluster_api( + RetiringClusterApi { + supersession_lapsed: true, + ..RETIRING + }, + async || { + use crate::service::tests::service_tests::{insert_test_sandbox, runtime_fields}; + let conn = crate::db::test_conn(); + let sandbox_id = SandboxId::new().to_string(); + insert_test_sandbox(&conn, &sandbox_id, SandboxState::Paused); + let mut fields = runtime_fields(generation(GENERATION), 7); + fields.space_api_route_identity.substrate_kind = + Some(pplx_space_common_core::types::NodeManagedBy::Kubernetes); + crate::db::update_runtime_fields(&conn, sandbox_id.parse().unwrap(), fields).unwrap(); + // A, captured and published by the previous execution's teardown. + let cache_dir = tempfile::tempdir().unwrap(); + let snapshot_dir = + crate::service::suspend_snapshot_publication::canonical_suspend_snapshot_dir( + &cache_dir.path().join(&sandbox_id), + ); + let earlier = LocalArtifactId::new(); + crate::service::suspend_snapshot_publication::write_full_suspend_artifact( + &snapshot_dir, + &sandbox_id, + earlier, + b"earlier-capture", + ); + crate::capture_record::write( + &snapshot_dir, + earlier, + generation(GENERATION - 1), + crate::capture_record::RecordWriter::CapturingTeardown, + ) + .unwrap(); + // This execution's teardown began, then the node went down before + // its capture was published. + crate::db::begin_teardown_suspend(&conn, &sandbox_id, generation(GENERATION)).unwrap(); + let (service, _dispatched) = service_recording_cleanup(conn); + service + .recover_prelaunch_startup_states_in(cache_dir.path(), &Default::default()) + .unwrap(); + + let row = service.lookup_sandbox(&sandbox_id).unwrap(); + assert_eq!(row.current_local_artifact_id, Some(earlier)); + assert_eq!( + crate::capture_record::read(&snapshot_dir, earlier).unwrap(), + Some(generation(GENERATION)), + "recovery's record fences A under the active teardown, as before" + ); + assert_eq!( + crate::capture_record::read_capture_order(&snapshot_dir, earlier).unwrap(), + None, + "recovery's record must not order A" + ); + + let artifact = crate::db::LocalArtifactRow { + local_artifact_id: earlier, + sandbox_id: sandbox_id.clone(), + artifact_kind: ArtifactKind::Full, + artifact_origin: ArtifactOrigin::Suspend, + snapshot_name: None, + local_path: snapshot_dir.to_str().unwrap().to_string(), + }; + let drive = tokio::time::timeout( + Duration::from_secs(3), + answered_or_admitted( + earlier, + service.ensure_teardown_suspend_backup( + &row, + TeardownSuspendBackup { + execution_generation: generation(GENERATION), + artifact: &artifact, + format: SuspendSnapshotFormat::Full, + origin: super::super::TeardownBackupOrigin::Captured, + }, + ), + ), + ) + .await + .expect("the drive registers or answers"); + if let Drive::Answered(result) = &drive { + assert!(result.is_ok(), "the lapsed fence admits A: {result:?}"); + } + let payload = service + .snapshot_upload_worker + .find_job_by_dedup_key( + crate::snapshot_upload_jobs::SNAPSHOT_UPLOAD_JOB_KIND, + &earlier.to_string(), + ) + .unwrap() + .expect("the drive persists A's upload") + .payload_as::() + .unwrap(); + assert_eq!(payload.execution_generation, Some(generation(GENERATION))); + assert!( + !payload.execution_generation_names_capture, + "the later execution's generation must not be recorded as A's capture" + ); + drain(&service).await; + }, + ) + .await; +} + /// Evidence that can't be read can't vouch for a previously-suspended /// capture's generation. A drive that would start a new attempt for it fails /// rather than register it unfenced, and the stub admits nothing. A drive @@ -729,5 +902,11 @@ fn stage_refused_capture( /// Record [`GENERATION`] as the capture's execution generation in the /// capture's record at `snapshot_dir`, as a teardown suspend's capture does. fn record_capture_generation(snapshot_dir: &std::path::Path, artifact: LocalArtifactId) { - crate::capture_record::write(snapshot_dir, artifact, generation(GENERATION)).unwrap(); + crate::capture_record::write( + snapshot_dir, + artifact, + generation(GENERATION), + crate::capture_record::RecordWriter::CapturingTeardown, + ) + .unwrap(); } diff --git a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/release_tests.rs b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/release_tests.rs index db0a22426c..91748b47e6 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/release_tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/release_tests.rs @@ -598,7 +598,7 @@ fn release_claim_waits_for_other_local_artifacts_to_back_up() { "an unbacked named snapshot must keep the row" ); assert_eq!( - crate::db::other_local_artifacts_awaiting_backup(&conn, &sandbox_id_string, staged) + crate::db::other_local_artifacts_awaiting_backup(&conn, &sandbox_id_string, Some(staged)) .unwrap() .iter() .map(|artifact| artifact.local_artifact_id) @@ -616,7 +616,7 @@ fn release_claim_waits_for_other_local_artifacts_to_back_up() { crate::db::mark_local_artifact_backup_completed(&conn, &named.to_string()).unwrap(); assert!( - crate::db::other_local_artifacts_awaiting_backup(&conn, &sandbox_id_string, staged) + crate::db::other_local_artifacts_awaiting_backup(&conn, &sandbox_id_string, Some(staged)) .unwrap() .is_empty() ); diff --git a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/verdict_tests.rs b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/verdict_tests.rs index b8495f24d1..6a0e71780c 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/verdict_tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/retired_teardown_tests/verdict_tests.rs @@ -726,7 +726,12 @@ async fn an_admission_clears_no_refusal_for_an_unfenced_attempt() { b"refused-capture", ); if readable { - crate::capture_record::write(&snapshot_dir, staged, generation(GENERATION - 1)) + crate::capture_record::write( + &snapshot_dir, + staged, + generation(GENERATION - 1), + crate::capture_record::RecordWriter::CapturingTeardown, + ) .unwrap(); } else { std::fs::write( diff --git a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/teardown_backup_tests.rs b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/teardown_backup_tests.rs index e7c646fc16..22be923613 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/teardown_backup_tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/teardown_backup_tests.rs @@ -313,39 +313,6 @@ async fn teardown_retry_seals_publishes_backs_up_and_completes_the_same_capture( server.await.unwrap().unwrap(); } -/// A gRPC handler answering one request type, for the fakes below. -struct Answer(std::sync::Arc, std::marker::PhantomData Resp>); - -impl tonic::server::UnaryService for Answer -where - F: Fn(Req) -> Result, -{ - type Response = Resp; - type Future = std::future::Ready, Status>>; - - fn call(&mut self, request: Request) -> Self::Future { - std::future::ready((self.0)(request.into_inner()).map(Response::new)) - } -} - -async fn answer( - request: http::Request, - handler: impl Fn(Req) -> Result, -) -> http::Response -where - B: http_body::Body + Send + 'static, - B::Error: Into + Send + 'static, - Req: prost::Message + Default + Send + 'static, - Resp: prost::Message + Send + 'static, -{ - tonic::server::Grpc::new(tonic_prost::ProstCodec::::default()) - .unary( - Answer(std::sync::Arc::new(handler), std::marker::PhantomData), - request, - ) - .await -} - /// The sandbox whose artifact the control plane refuses to register. The /// stub serves from the parent test process, so its refusals are keyed by /// identities both processes know up front. diff --git a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/teardown_suspend.rs b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/teardown_suspend.rs index 58630fe8b4..e592abe1a9 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/teardown_suspend.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/grpc_lifecycle/teardown_suspend.rs @@ -1307,6 +1307,14 @@ impl SandboxManagerService { // The verdict a rejoined refusal leaves with, so the settle needn't // ask the cluster again. let mut verdict = None; + // Beyond the capture's record, only an earlier upload of these bytes + // that marked the generation as the capture's own vouches for it; one + // the teardown merely supplies doesn't. Read before the match below + // takes the job. + let earlier_upload_vouches = super::artifact_backup::earlier_upload_vouches( + persisted_job.as_ref(), + backup.registration_generation(), + ); let joined = match persisted_job { Some(job) => match self.teardown_use_of_upload(&backup, &job)? { KeyedUploadUse::Join => { @@ -1363,6 +1371,7 @@ impl SandboxManagerService { &backup.artifact.local_path, backup.format, backup.registration_generation(), + earlier_upload_vouches, ) .await .map_err(|error| { diff --git a/crates/l40_api/sbx/mgr/server/src/service/grpc_spaced_process_snapshots.rs b/crates/l40_api/sbx/mgr/server/src/service/grpc_spaced_process_snapshots.rs index cd260414c6..0adc4caac9 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/grpc_spaced_process_snapshots.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/grpc_spaced_process_snapshots.rs @@ -31,6 +31,65 @@ 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; +/// The VM state files a full snapshot can't restore without. +const FULL_SNAPSHOT_REQUIRED_FILES: &[&str] = &["vmstate", "memory"]; + +/// Whether every one of `required` is a non-empty regular file in `path`. +fn snapshot_files_present(path: &std::path::Path, required: &[&str]) -> bool { + required.iter().all(|file| { + std::fs::symlink_metadata(path.join(file)) + .is_ok_and(|metadata| metadata.is_file() && metadata.len() > 0) + }) +} + +/// Whether the full snapshot directory at `path` holds the VM state it +/// restores from, by the test the snapshot listing reports `incomplete` by: +/// each of [`FULL_SNAPSHOT_REQUIRED_FILES`] is a non-empty regular file. A +/// failed capture can leave the disk without it. Unlike the listing, which +/// shows an unreadable file as incomplete, a caller deciding whether the +/// directory may be deleted gets any error other than `NotFound` back, so +/// that an unreadable snapshot isn't taken for an incomplete one. +pub(super) fn full_snapshot_complete(path: &std::path::Path) -> std::io::Result { + for file in FULL_SNAPSHOT_REQUIRED_FILES { + match snapshot_entry_metadata(&path.join(file)) { + Ok(metadata) if metadata.is_file() && metadata.len() > 0 => {} + Ok(_) => return Ok(false), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(false), + Err(error) => return Err(error), + } + } + Ok(true) +} + +/// `symlink_metadata` of an entry in a sandbox's snapshot directory, which +/// doesn't follow a symlink, as the listing's `DirEntry::file_type` doesn't. +/// A test can make it fail for chosen paths ([`fail_snapshot_metadata`]). +pub(super) fn snapshot_entry_metadata( + path: &std::path::Path, +) -> std::io::Result { + #[cfg(test)] + if SNAPSHOT_METADATA_FAULTS + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .iter() + .any(|fault| fault == path) + { + return Err(std::io::Error::other("injected snapshot metadata error")); + } + std::fs::symlink_metadata(path) +} + +/// The paths whose [`snapshot_entry_metadata`] fails in this test process. +#[cfg(test)] +static SNAPSHOT_METADATA_FAULTS: std::sync::Mutex> = + std::sync::Mutex::new(Vec::new()); + +/// Make [`snapshot_entry_metadata`] fail for `path` in this test process. +#[cfg(test)] +pub(crate) fn fail_snapshot_metadata(path: std::path::PathBuf) { + SNAPSHOT_METADATA_FAULTS.lock().unwrap().push(path); +} + /// 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). @@ -1104,7 +1163,7 @@ fn project_snapshot_inventory( // survive a failed SQLite commit, which an identical retry repairs. // Otherwise skip_if_exists would falsely acknowledge that retry. let required: &[&str] = if full { - &["vmstate", "memory"] + FULL_SNAPSHOT_REQUIRED_FILES } else { &["rootfs.ext4"] }; @@ -1112,13 +1171,7 @@ fn project_snapshot_inventory( Some(state) if state != crate::local_artifacts::ArtifactState::Ready => { state.as_str() } - _ if !required.iter().all(|file| { - std::fs::symlink_metadata(path.join(file)) - .is_ok_and(|metadata| metadata.is_file() && metadata.len() > 0) - }) => - { - "incomplete" - } + _ if !snapshot_files_present(&path, required) => "incomplete", Some(_) => "ready", None => "unknown", } diff --git a/crates/l40_api/sbx/mgr/server/src/service/mod.rs b/crates/l40_api/sbx/mgr/server/src/service/mod.rs index 10ef41cbc9..ac3d79a013 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/mod.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/mod.rs @@ -2089,7 +2089,7 @@ pub(crate) use guest_image::profile_for_sandbox; pub(crate) mod drain; mod eviction; pub(crate) mod snapshot_capture; -pub(crate) use eviction::EvictionClaim; +pub(crate) use eviction::{EvictionClaim, is_unrecorded_snapshot_refusal}; mod grpc_auto_lifecycle_delete_receipts; mod grpc_core; mod grpc_lifecycle; diff --git a/crates/l40_api/sbx/mgr/server/src/service/recovery.rs b/crates/l40_api/sbx/mgr/server/src/service/recovery.rs index 42b47af343..cb13f92d19 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/recovery.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/recovery.rs @@ -958,11 +958,15 @@ impl SandboxManagerService { // A capture published before its record was written gets // one now. Best effort: the capture is already published, // and without the record a re-request falls back to the - // upload job's and the running teardown's generation. + // upload job's and the running teardown's generation. The + // canonical capture can be an earlier execution's, left + // there by a teardown that crashed before publishing its + // own, so the record fences but orders nothing. if let Err(error) = crate::capture_record::write( &artifact.path, artifact.local_artifact_id, execution_generation, + crate::capture_record::RecordWriter::Recovery, ) { tracing::warn!( sandbox_id = %row.id, @@ -993,7 +997,7 @@ impl SandboxManagerService { // live path does; a record that can't be written defers the // publication like a failed publish. let published = - suspend_snapshot_publication::publish_recorded_suspend_snapshot_dir( + suspend_snapshot_publication::publish_recovered_suspend_snapshot_dir( &staged.staging_dir, &snapshot_dir, staged.local_artifact_id, diff --git a/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade.rs b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade.rs index cca469d0f7..bd5e90fa05 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade.rs @@ -42,6 +42,7 @@ use crate::state::{SandboxState, SandboxStatus}; mod node_effects; mod rpc; +mod stale_copy; #[cfg(test)] mod tests; @@ -71,6 +72,22 @@ pub(super) fn refuse_runtime_upgrade_fence_in( } } +/// [`super::SandboxManagerService::refuse_unretired_runtime_upgrade`] on a +/// held connection. +pub(super) fn refuse_unretired_runtime_upgrade_in( + db: &crate::db::Connection, + sandbox_id: &str, +) -> Result<(), Status> { + match crate::db::read::unretired_runtime_upgrade(db, sandbox_id) + .map_err(|error| Status::unavailable(format!("read runtime upgrade: {error}")))? + { + Some(operation_id) => Err(Status::unavailable(format!( + "runtime upgrade {operation_id} is retiring this sandbox's old checkpoint; retry" + ))), + None => Ok(()), + } +} + /// The status an activation returns when it can't claim the sandbox. pub(super) fn begin_activation_status(error: BeginActivationError, context: &str) -> Status { match error { @@ -1172,14 +1189,7 @@ impl super::SandboxManagerService { /// retirement of the one it superseded: until then the canonical /// checkpoint may be the old runtime's, over a disk the new boot wrote. pub(super) fn refuse_unretired_runtime_upgrade(&self, sandbox_id: &str) -> Result<(), Status> { - match crate::db::read::unretired_runtime_upgrade(&*self.db_conn()?, sandbox_id) - .map_err(|error| Status::unavailable(format!("read runtime upgrade: {error}")))? - { - Some(operation_id) => Err(Status::unavailable(format!( - "runtime upgrade {operation_id} is retiring this sandbox's old checkpoint; retry" - ))), - None => Ok(()), - } + refuse_unretired_runtime_upgrade_in(&*self.db_conn()?, sandbox_id) } /// Gracefully stop `sandbox_id` and wait for it, but only while it runs the diff --git a/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/node_effects/tests.rs b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/node_effects/tests.rs index 263c6095e8..bc04753d9e 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/node_effects/tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/node_effects/tests.rs @@ -148,6 +148,15 @@ impl Fixture { pub(in crate::service::runtime_upgrade) fn adopt_checkpoint(&self) -> LocalArtifactId { let artifact = LocalArtifactId::new(); + self.adopt_checkpoint_as(artifact); + artifact + } + + /// Pin `artifact` as the suspended sandbox's checkpoint. + pub(in crate::service::runtime_upgrade) fn adopt_checkpoint_as( + &self, + artifact: LocalArtifactId, + ) { assert!( crate::db::adopt_suspended_local_artifact( &self.service.db.lock().unwrap(), @@ -157,7 +166,6 @@ impl Fixture { ) .unwrap() ); - artifact } fn staged(&self, id: impl std::fmt::Display) -> PathBuf { @@ -976,7 +984,7 @@ fn every_caller_allowed_a_wake_start_context_may_also_upgrade() { /// A suspended Wake upgrade naming another node as the checkpoint's source /// and a backup to stage, as the control plane sends for a pod placed away /// from the checkpoint. -fn remote_upgrade_wake( +pub(in crate::service::runtime_upgrade) fn remote_upgrade_wake( f: &Fixture, checkpoint: Option, ) -> crate::proto::WakeRequest { diff --git a/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/rpc.rs b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/rpc.rs index 2393d8ab79..b7dd1d2cb7 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/rpc.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/rpc.rs @@ -29,7 +29,7 @@ use crate::proto::{ use crate::state::SandboxState; #[cfg(test)] -mod tests; +pub(in crate::service::runtime_upgrade) mod tests; /// The staging `Start` for a suspended sandbox absent from the node: the /// upgrade's own start, as a restore from `stage_from`'s checkpoint. A blank @@ -139,10 +139,21 @@ impl crate::service::SandboxManagerService { // An operation id names one upgrade of one sandbox. A request that // reuses one another sandbox's upgrade recorded stages nothing. upgrader.refuse_another_request(&upgrade)?; + // A backed-up suspended copy of another checkpoint makes way for + // the decided one; an unproven one stays, and the upgrade's + // checkpoint check refuses it. + let staging = staging_start(&upgrade.start, stage_from); + self.evict_stale_checkpoint_copy( + &upgrade.sandbox_id, + upgrade.operation_id, + upgrade.checkpoint_local_artifact_id, + upgrade.authorization.memory_loss, + &staging, + ) + .await?; // Leaves a present sandbox alone, in any state; the upgrade's // checkpoint check decides whether its copy may be upgraded. - self.stage_missing_checkpoint(staging_start(&upgrade.start, stage_from)) - .await?; + self.stage_missing_checkpoint(staging).await?; } let target_state = upgrade.target_state; let operation_id = upgrade.operation_id; diff --git a/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/rpc/tests.rs b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/rpc/tests.rs index 900c5f0fef..6fb1e3efc2 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/rpc/tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/rpc/tests.rs @@ -46,7 +46,7 @@ fn pod_identity(sandbox_id: &str, pod_uid: &str, generation: i64) -> SpaceApiRou /// The `UpgradeRuntime` a runtime upgrade pod's shim sends: the pod's own /// cold start, with its namespace and route identity. -fn pod_upgrade( +pub(in crate::service::runtime_upgrade) fn pod_upgrade( sandbox_id: &str, target: &ExecutionMetadata, operation_id: RuntimeUpgradeOperationId, @@ -70,11 +70,11 @@ fn pod_upgrade( } } -fn operation() -> RuntimeUpgradeOperationId { +pub(in crate::service::runtime_upgrade) fn operation() -> RuntimeUpgradeOperationId { RuntimeUpgradeOperationId::from_uuid(uuid::Uuid::now_v7()) } -fn authorization() -> RuntimeUpgradeAuthorization { +pub(in crate::service::runtime_upgrade) fn authorization() -> RuntimeUpgradeAuthorization { RuntimeUpgradeAuthorization { authorization_id: "environment-block-e0".into(), reason: "e0 runs a blocked runtime".into(), @@ -114,7 +114,7 @@ impl Fixture { } } -async fn upgrade( +pub(in crate::service::runtime_upgrade) async fn upgrade( service: &SandboxManagerService, request: UpgradeRuntimeRequest, ) -> Result { diff --git a/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/stale_copy.rs b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/stale_copy.rs new file mode 100644 index 0000000000..f467eabad7 --- /dev/null +++ b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/stale_copy.rs @@ -0,0 +1,306 @@ +//! Replacing a stale suspended copy before an upgrade stages its checkpoint. +//! +//! A resume-time runtime upgrade boots the checkpoint it was decided from, +//! which the row must pin ([`super::require_decided_checkpoint`]). A restore +//! placed on a node that still holds an older suspended copy of the sandbox +//! finds another pin there, and staging lays a checkpoint down only for a +//! sandbox absent from the node. So the copy is evicted first, through the +//! eviction executor and only on the proof eviction acts on: a ready backup of +//! exactly the artifact it pins. Without that proof the copy may be the only +//! one of something, and the upgrade goes on refusing it. +//! +//! A newer local copy is never replaced by an older decided checkpoint, +//! whatever the minting nodes' clocks say or which backup registered last. +//! Each checkpoint is captured by one execution, and executions' generations +//! only grow: the copy is replaced only when the cluster shows the decided +//! checkpoint's ready backup was captured by a later execution than the +//! copy's. Without a capture generation on both backups nothing orders them, +//! and the copy is refused, as every copy pinning another checkpoint was +//! before replacement existed. An encrypted copy is left too, since an +//! encrypted checkpoint staged here can't be cold-booted. + +use pplx_space_common_core::types::LocalArtifactId; +use tonic::Status; + +use crate::db::RuntimeUpgradeOperationId; +use crate::sandbox_eviction::LocalEvidence; +use crate::state::{CleanupState, SandboxState, SandboxStatus}; + +#[cfg(test)] +mod tests; + +fn unproven(sandbox_id: &str) -> Status { + Status::failed_precondition(format!( + "sandbox {sandbox_id}'s checkpoint on this node isn't the one its runtime upgrade was \ + decided from, and nothing proves it backed up; refusing to replace it" + )) +} + +impl crate::service::SandboxManagerService { + /// Evict this node's suspended copy of `sandbox_id` when it pins an + /// artifact other than `decided`, the checkpoint upgrade `operation_id` + /// was decided from, so the caller's staging lays that checkpoint down in + /// its place. Everything else is left to the upgrade's own checks: an + /// absent sandbox, one in another state, a copy of the decided + /// checkpoint, a request that names no checkpoint, and a sandbox this + /// operation already ran on, whose record decides what a replay means. + /// + /// An upgrade that doesn't authorize the memory loss a suspended + /// sandbox's upgrade means is left to the upgrade too, which refuses it. + /// + /// A copy that isn't replaced is refused before any node work: + /// `FailedPrecondition` when nothing proves it backed, when nothing shows + /// the decided checkpoint was captured after it, when it's encrypted, or + /// when the sandbox holds a user snapshot no local artifact records, + /// and `Unavailable` when the proof couldn't be had or another upgrade + /// still owns the copy. So is a request whose stage would be refused for + /// its form or route identity, and a retry of an activation the row + /// rolled back. A copy that changed under the eviction's claim, or one + /// with an artifact upload pending, running or cancelling, is refused by + /// the claim as `Aborted` until that settles. Each is safe to retry. A + /// cleanup that fails after the claim leaves the row's cleanup failed, its + /// delete intent kept, for the cleanup machinery to finish; once it has, the sandbox is absent and a + /// retry stages it as it would any absent sandbox. + pub(in crate::service) async fn evict_stale_checkpoint_copy( + &self, + sandbox_id: &str, + operation_id: RuntimeUpgradeOperationId, + decided: Option, + memory_loss_authorized: bool, + staging: &crate::proto::StartRequest, + ) -> Result<(), Status> { + let Some(decided) = decided else { + return Ok(()); + }; + if !memory_loss_authorized { + return Ok(()); + } + // Only a Kubernetes execution's captures register the generation that + // orders them; nothing orders a Direct sandbox's, so its copy is left + // to the upgrade, which refuses it as it did before replacement + // existed, without the cluster being asked anything. The request's + // identity names the substrate: a suspended row keeps none. + if crate::service::activation_support::space_api_route_identity_from_start_request(staging)? + .effective_substrate_kind() + != Some(pplx_space_common_core::types::NodeManagedBy::Kubernetes) + { + return Ok(()); + } + // One hold of the node's only database connection, which staging + // writes through too: no stage can repin the row between reading its + // pin and reading the evidence the proof names. After it, the + // eviction's claim re-asserts the proven pin and the activation + // generations read here, so a generation an activation retains after + // the checks below fails the claim. + let (pinned, generations) = { + let db = self.db_conn()?; + let sandbox = crate::db::read::sandbox(&db, sandbox_id) + .map_err(|error| Status::internal(format!("db sandbox lookup failed: {error}")))?; + let Some(sandbox) = sandbox else { + return Ok(()); + }; + if !sandbox.state.is_unowned(SandboxState::Suspended) + || sandbox.current_local_artifact_id == Some(decided) + { + return Ok(()); + } + let recorded = crate::db::read::runtime_upgrade_report(&db, operation_id) + .map_err(|error| Status::internal(format!("read runtime upgrade: {error}")))?; + if recorded.is_some() { + return Ok(()); + } + // An interrupted upgrade's preserved disk, or a checkpoint whose + // retirement is still owed, lives in the copy, and the recovery + // loop finishes it. + super::refuse_runtime_upgrade_fence_in(&db, sandbox_id)?; + super::refuse_unretired_runtime_upgrade_in(&db, sandbox_id)?; + if sandbox.volume_encryption { + return Err(Status::failed_precondition(format!( + "sandbox {sandbox_id}'s checkpoint on this node isn't the one its runtime \ + upgrade was decided from, and an encrypted checkpoint staged in its place \ + can't be cold-booted; refusing to replace it" + ))); + } + let evidence = crate::sandbox_eviction::gather_local_evidence(&db, sandbox_id) + .map_err(|error| { + Status::unavailable(format!( + "read sandbox {sandbox_id}'s pinned artifact: {error}" + )) + })?; + let pinned = match evidence { + Some((LocalEvidence::Pinned(pinned), _)) => pinned, + // Unpinned, or pinning an artifact row it doesn't own: no + // proof names these bytes. + _ => return Err(unproven(sandbox_id)), + }; + let generations = crate::db::read::activation_generations(&db, sandbox_id) + .map_err(|error| { + Status::unavailable(format!( + "read sandbox {sandbox_id}'s activation generations: {error}" + )) + })? + .ok_or_else(|| Status::not_found(format!("sandbox {sandbox_id} not found")))?; + (pinned, generations) + }; + // A stage that would be refused stages nothing in the copy's place, + // and an activation the row would refuse, such as one at a generation + // it already retained, must not get past it by removing the row. + let activation_claim = self.validate_staging_start(staging)?; + // A retry of an activation the row rolled back matches the claim the + // row kept open for it, which is how every later attempt of that + // activation is asked whether its execution was retired. Removing the + // row closes the claim, so a retry after the copy is gone would match + // nothing and go unasked; such a retry keeps the copy, as it did before + // replacement existed, until a new activation replaces it. + if let Some(claim) = &activation_claim { + let db = self.db_conn()?; + let retried = crate::db::read::sandbox_view(&db, sandbox_id) + .route_claim() + .matches(claim) + .map_err(|error| Status::internal(format!("read activation claim: {error}")))?; + if retried { + return Err(Status::failed_precondition(format!( + "sandbox {sandbox_id}'s copy on this node holds the open claim of the \ + activation this request retries, which replacing the copy would close; \ + refusing to replace it for a retry" + ))); + } + } + match crate::sandbox_eviction::prove_pinned_backup(self, sandbox_id, &pinned).await { + Ok(true) => {} + Ok(false) => return Err(unproven(sandbox_id)), + Err(reason) => { + return Err(Status::unavailable(format!( + "couldn't prove sandbox {sandbox_id}'s copy on this node backed up ({}); \ + retry", + reason.as_str() + ))); + } + } + // Asked after the copy's backup is proven ready, so the cluster + // compares two ready backups' capture generations. + match crate::sandbox_eviction::prove_captured_after( + sandbox_id, + (decided, pplx_space_common_core::types::ArtifactKind::Full), + (pinned.artifact_id, pinned.artifact_kind), + ) + .await + { + Ok(true) => {} + Ok(false) => { + return Err(Status::failed_precondition(format!( + "nothing shows the checkpoint sandbox {sandbox_id}'s runtime upgrade was \ + decided from was captured after its copy on this node; refusing to replace \ + the copy" + ))); + } + Err(reason) => { + return Err(Status::unavailable(format!( + "couldn't order sandbox {sandbox_id}'s checkpoints ({}); retry", + reason.as_str() + ))); + } + } + tracing::info!( + sandbox_id, + %operation_id, + stale_local_artifact_id = %pinned.artifact_id, + decided_local_artifact_id = %decided, + "evicting a backed-up stale suspended copy so the runtime upgrade can stage its \ + checkpoint" + ); + // The eviction deletes every local artifact of the sandbox, not only + // the copy: a named snapshot nothing backed up would be lost. Each one + // the cluster shows backed is stamped, and the claim refuses while any + // other stays unstamped. + let awaiting = self.db_conn().and_then(|db| { + crate::db::read::sandbox_view(&db, sandbox_id) + .artifacts() + .awaiting_backup_besides(Some(pinned.artifact_id)) + .map_err(|error| Status::unavailable(format!("local artifact lookup: {error}"))) + })?; + match crate::sandbox_eviction::prove_local_artifact_backups(self, sandbox_id, &awaiting) + .await + { + Ok(true) => {} + Ok(false) => { + return Err(Status::failed_precondition(format!( + "another local artifact of sandbox {sandbox_id} isn't backed up, and \ + replacing its copy would delete it; refusing to replace the copy" + ))); + } + Err(reason) => { + return Err(Status::unavailable(format!( + "couldn't prove sandbox {sandbox_id}'s other local artifacts backed up ({}); \ + retry", + reason.as_str() + ))); + } + } + let result = self + .evict_local_sandbox( + sandbox_id, + crate::service::EvictionClaim::StaleUpgradeCopy { + pin: pinned.artifact_id, + generations, + }, + None, + ) + .await; + let reported = reported_eviction_result( + &result, + self.lookup_sandbox(sandbox_id).ok().map(|row| row.state), + ); + crate::sandbox_eviction::report_eviction_outcome( + sandbox_id, + crate::sandbox_eviction::EvictReason::StaleUpgradeCopy, + pplx_space_events::SandboxEventDetails::default() + .local_artifact_id(pinned.artifact_id.to_string()), + &reported, + "stale suspended copy eviction for a runtime upgrade failed", + ); + // The claim refuses a row that changed or is held as + // `FailedPrecondition`, and a cleanup it handed off can fail with the + // same code; a retry finds which. A row another eviction removed + // during the proof is absent, as staging needs it to be. + // A snapshot no local artifact records stays until someone acts on + // it, so that refusal keeps its code rather than inviting a retry. + result.or_else(|status| match status.code() { + tonic::Code::NotFound => Ok(()), + tonic::Code::FailedPrecondition + if crate::service::is_unrecorded_snapshot_refusal(&status) => + { + Err(status) + } + tonic::Code::FailedPrecondition => Err({ + Status::aborted(format!( + "the runtime upgrade couldn't replace sandbox {sandbox_id}'s copy on this \ + node: {}; retry", + status.message() + )) + }), + _ => Err(status), + }) + } +} + +/// What an eviction attempt reports, given the row's state after it. A +/// `FailedPrecondition` after which the row's cleanup failed came from the +/// cleanup the claim handed off, not from the claim, and is reported as the +/// failure it is. Any other `FailedPrecondition` is the claim's: the row +/// changed, or another eviction claimed it first and the row is `deleting` +/// under that one, a race lost and not a failure. +fn reported_eviction_result( + result: &Result<(), Status>, + row_state: Option, +) -> Result<(), Status> { + match result { + Err(status) + if status.code() == tonic::Code::FailedPrecondition + && row_state.and_then(SandboxStatus::cleanup) == Some(CleanupState::Failed) => + { + Err(Status::internal(status.message().to_string())) + } + other => other.clone(), + } +} diff --git a/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/stale_copy/tests.rs b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/stale_copy/tests.rs new file mode 100644 index 0000000000..9068067198 --- /dev/null +++ b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/stale_copy/tests.rs @@ -0,0 +1,1337 @@ +//! A runtime upgrade meeting a stale suspended copy, through `UpgradeRuntime` +//! (and the Wake, which leaves the copy alone), against a cluster-api stub that +//! answers the backup proof and +//! a test-served node-manager. Each case seeds the copy the node holds: a +//! backed-up one, an unproven one, an unpinned one and an encrypted one. +//! +//! The cluster-api stub serves the parent process and the case runs in a +//! child (`with_cluster_api`), so artifact ids the stub knows are fixed. + +use std::sync::{Arc, Mutex}; + +use pplx_space_common_core::types::{ExecutionMetadata, LocalArtifactId}; +use pplx_space_node_api_client::proto::{ + GetArtifactBackupLocationRequest, GetArtifactBackupLocationResponse, + LocalArtifactBackupIdentity, backup_readiness_query::Artifact, +}; +use tonic::Status; + +use super::super::node_effects::tests::{Fixture, certified_target, fixture, remote_upgrade_wake}; +use super::super::rpc::tests::{authorization, operation, pod_upgrade, upgrade}; +use crate::db::RuntimeUpgradeOperationId; +use crate::node_manager_transport::fake::FakeNodeManager; +use crate::proto::{RuntimeUpgradeStageFrom, UpgradeRuntimeRequest, WakeTargetState}; +use crate::service::activation_support::internal_request; +use crate::service::grpc_lifecycle::{answer, with_cluster_api}; +use crate::state::{CleanupState, SandboxState, SandboxStatus}; + +/// The checkpoint the node's stale copy pins. +const STALE: &str = "0192f0c0-0000-7000-8000-00000000a001"; +/// The checkpoint the upgrade was decided from, whose id sorts after +/// [`STALE`]'s. +const DECIDED: &str = "0192f0c0-0000-7000-8000-00000000a002"; +/// A checkpoint whose id sorts after [`DECIDED`]'s. +const NEWER: &str = "0192f0c0-0000-7000-8000-00000000a003"; +/// A named snapshot the sandbox holds beside its copy. +const NAMED: &str = "0192f0c0-0000-7000-8000-00000000a004"; + +fn artifact(id: &str) -> LocalArtifactId { + id.parse().unwrap() +} + +/// A cluster-api stub answering the backup readiness checks. +#[derive(Clone, Copy)] +enum BackupReadiness { + /// A ready backup of exactly `backed`, and of no other; and, when + /// `captured_after` is `(later, earlier)`, `later`'s ready backup captured + /// by a later execution than `earlier`'s, and no other order. + Backs { + backed: &'static str, + captured_after: Option<(&'static str, &'static str)>, + }, + /// Every check answers `UNAVAILABLE`. + Unavailable, + /// As [`Self::backs`] of [`STALE`], and a ready backup of the named + /// snapshot [`NAMED`] when `named_backed`. + AndNamed { named_backed: bool }, +} + +impl BackupReadiness { + /// Backs `backed`, and shows the decided checkpoint captured after it. + const fn backs(backed: &'static str) -> Self { + Self::Backs { + backed, + captured_after: Some((DECIDED, backed)), + } + } + + fn answer( + self, + request: GetArtifactBackupLocationRequest, + ) -> Result { + if let Self::AndNamed { named_backed } = self { + let asks_named = matches!( + request.readiness.as_ref().and_then(|query| query.artifact.as_ref()), + Some(Artifact::LocalArtifact(identity)) if identity.local_artifact_id == NAMED + ); + if !asks_named { + return Self::backs(STALE).answer(request); + } + return Ok(GetArtifactBackupLocationResponse { + ready: Some(named_backed), + ..Default::default() + }); + } + let Self::Backs { + backed, + captured_after, + } = self + else { + return Err(Status::unavailable("cluster-api is restarting")); + }; + // A location lookup, as a backup's admission makes: the cluster + // knows a location only for an artifact it holds a backup of. + if request.readiness.is_none() && request.local_artifact_id != backed { + return Err(Status::not_found("no backup of that artifact")); + } + let ready = match request.readiness.and_then(|query| query.artifact) { + Some(Artifact::LocalArtifact(identity)) => identity.local_artifact_id == backed, + Some(Artifact::CapturedAfter(order)) => { + let id = |identity: Option| { + identity.map(|identity| identity.local_artifact_id) + }; + captured_after.is_some_and(|(later, earlier)| { + id(order.later).as_deref() == Some(later) + && id(order.earlier).as_deref() == Some(earlier) + }) + } + _ => false, + }; + Ok(GetArtifactBackupLocationResponse { + ready: Some(ready), + ..Default::default() + }) + } +} + +/// Backs the decided checkpoint and not the copy, so a proof of the wrong +/// artifact passes where it must not, and shows the decided checkpoint +/// captured after the copy. +const BACKS_ONLY_DECIDED: BackupReadiness = BackupReadiness::Backs { + backed: DECIDED, + captured_after: Some((DECIDED, STALE)), +}; + +impl tonic::server::NamedService for BackupReadiness { + const NAME: &'static str = "space.node.api.v1.ClusterApi"; +} + +impl tower::Service> for BackupReadiness +where + B: http_body::Body + Send + 'static, + B::Error: Into + Send + 'static, +{ + type Response = http::Response; + type Error = std::convert::Infallible; + type Future = futures_util::future::BoxFuture<'static, Result>; + + fn poll_ready( + &mut self, + _: &mut std::task::Context<'_>, + ) -> std::task::Poll> { + std::task::Poll::Ready(Ok(())) + } + + fn call(&mut self, request: http::Request) -> Self::Future { + let stub = *self; + Box::pin(async move { + Ok( + if request.uri().path().ends_with("/GetArtifactBackupLocation") { + answer(request, move |request| stub.answer(request)).await + } else if request + .uri() + .path() + .ends_with("/GetSandboxSnapshotStorageUri") + { + // No org override: a backup goes to the node's default. + answer( + request, + |_: pplx_space_node_api_client::proto::GetSandboxSnapshotStorageUriRequest| { + Ok( + pplx_space_node_api_client::proto::GetSandboxSnapshotStorageUriResponse { + storage_uri: None, + }, + ) + }, + ) + .await + } else { + Status::unimplemented(request.uri().path().to_owned()).into_http() + }, + ) + }) + } +} + +/// The `UpgradeRuntime` a pod's shim sends for a restore pod placed away from +/// the decided checkpoint: staged from its storage URI. +fn staged_pod_upgrade( + f: &Fixture, + target: &ExecutionMetadata, + operation_id: RuntimeUpgradeOperationId, +) -> UpgradeRuntimeRequest { + let mut request = pod_upgrade( + &f.sandbox_id, + target, + operation_id, + WakeTargetState::Started, + ); + request.stage_from = Some(RuntimeUpgradeStageFrom { + suspend_restore_storage_uri: staging_request(f).suspend_restore_storage_uri.unwrap(), + suspend_restore_source_node: Some("worker-source".into()), + }); + request.checkpoint_local_artifact_id = Some(DECIDED.to_string()); + request +} + +/// The staging `Start` either caller builds for the decided checkpoint, with +/// the pod's route identity. +fn staging_request(f: &Fixture) -> crate::proto::StartRequest { + let start = pod_upgrade( + &f.sandbox_id, + &f.target_environment(), + operation(), + WakeTargetState::Started, + ) + .start + .unwrap(); + crate::proto::StartRequest { + checkpoint: Some(crate::service::create_support::SUSPEND_CHECKPOINT.to_string()), + suspend_restore_storage_uri: Some(format!( + "s3://space-test/suspend/{}/{DECIDED}?region=us-east-1", + f.sandbox_id + )), + ..start + } +} + +/// The Wake the control plane sends a Direct sandbox's restore with a +/// resume-time upgrade decided from the checkpoint. +fn decided_wake(f: &Fixture) -> crate::proto::WakeRequest { + remote_upgrade_wake(f, Some(artifact(DECIDED))) +} + +/// A suspended sandbox whose copy on the node pins `pin`, with the cleanups +/// its eviction hands off recorded and answered as applied. +fn stale_copy(encrypted: bool, pin: Option<&str>) -> (Fixture, Arc>>) { + let f = fixture( + SandboxState::Suspended, + encrypted, + FakeNodeManager::default(), + ); + if let Some(pin) = pin { + f.adopt_checkpoint_as(artifact(pin)); + } + let cleanups = Arc::new(Mutex::new(Vec::new())); + let recorded = cleanups.clone(); + *f.service.cleanup_executor.dispatch.lock().unwrap() = Some(Arc::new(move |sandbox_id, _| { + recorded.lock().unwrap().push(sandbox_id); + Box::pin(async { Ok(()) }) + })); + (f, cleanups) +} + +impl Fixture { + fn target_environment(&self) -> ExecutionMetadata { + certified_target(&self.source) + } + + async fn pod_upgrade_error(&self, operation_id: RuntimeUpgradeOperationId) -> Status { + upgrade( + &self.service, + staged_pod_upgrade(self, &self.target_environment(), operation_id), + ) + .await + .expect_err("this harness has no object store to stage from") + } + + async fn wake_error(&self, request: crate::proto::WakeRequest) -> Status { + self.service + .wake(internal_request(request)) + .await + .expect_err("this harness has no object store to stage from") + } + + fn local_artifact_exists(&self, id: &str) -> bool { + self.service + .db + .lock() + .unwrap() + .query_row( + "SELECT COUNT(*) FROM local_artifacts WHERE local_artifact_id = ?1", + crate::db::fixture::params![id], + |row| row.get::<_, i64>(0), + ) + .unwrap() + == 1 + } + + /// The copy is exactly as seeded: suspended, pinning `pin`, and nothing + /// was asked of node-manager or the cleanup executor. + fn assert_copy_untouched(&self, pin: Option<&str>, cleanups: &Mutex>) { + let row = self.row(); + assert_eq!(row.state, SandboxStatus::Unowned(SandboxState::Suspended)); + assert_eq!(row.current_local_artifact_id, pin.map(artifact)); + assert!(cleanups.lock().unwrap().is_empty()); + assert!(self._node.calls().is_empty(), "{:?}", self._node.calls()); + } + + /// The copy was evicted once: the cleanup ran, and the sandbox is no + /// longer this node's suspended copy of the stale checkpoint. + fn assert_copy_evicted(&self, cleanups: &Mutex>) { + assert_eq!(*cleanups.lock().unwrap(), vec![self.sandbox_id.clone()]); + assert!(!self.local_artifact_exists(STALE)); + if let Ok(row) = self.service.lookup_sandbox(&self.sandbox_id) { + assert_ne!(row.current_local_artifact_id, Some(artifact(STALE))); + } + } +} + +/// A pod's upgrade meeting a backed-up copy of an older checkpoint evicts it +/// and stages the decided checkpoint in its place, +/// where it would otherwise be refused. The harness has no object store, so +/// staging is what fails. A retry finds the sandbox absent and leaves it to +/// staging, evicting nothing more; staging's own retry of an interrupted +/// stage is covered with staging. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_backed_up_stale_copy_makes_way_for_the_decided_checkpoint() { + with_cluster_api(BackupReadiness::backs(STALE), async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + let operation_id = operation(); + + let error = f.pod_upgrade_error(operation_id).await; + + assert!(!error.message().contains("decided from"), "{error}"); + assert!(error.message().contains("stage"), "{error}"); + f.assert_copy_evicted(&cleanups); + + f.service + .evict_stale_checkpoint_copy( + &f.sandbox_id, + operation_id, + Some(artifact(DECIDED)), + true, + &staging_request(&f), + ) + .await + .unwrap(); + f.assert_copy_evicted(&cleanups); + }) + .await; +} + +/// The Wake carries only a Direct sandbox's upgrade, and nothing orders a +/// Direct sandbox's captures, so the Wake never replaces a copy: its stale +/// copy, backed up or not, is refused as it was before replacement existed, +/// and the cluster isn't asked. The stub answers every question +/// `UNAVAILABLE`, so a Wake that asked would be refused as retryable instead. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_direct_wake_upgrade_keeps_refusing_a_stale_copy() { + with_cluster_api(BackupReadiness::Unavailable, async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + + let error = f.wake_error(decided_wake(&f)).await; + + assert_eq!(error.code(), tonic::Code::FailedPrecondition, "{error}"); + assert!(error.message().contains("decided from"), "{error}"); + f.assert_copy_untouched(Some(STALE), &cleanups); + + // Nor does the Wake take the staging route, whose checks a storage + // URI it then never reads would fail. + let mut malformed = decided_wake(&f); + malformed + .start_context + .as_mut() + .unwrap() + .suspend_restore_storage_uri = Some("not a storage uri".into()); + let error = f.wake_error(malformed).await; + assert!(error.message().contains("decided from"), "{error}"); + + // The replacement itself leaves a Direct request's copy alone + // without asking the cluster, whichever caller hands it one. + let mut direct = staging_request(&f); + let identity = direct.route_identity.as_mut().unwrap(); + identity.substrate_kind = crate::proto::SandboxSubstrateKind::Direct as i32; + identity.pod_uid = None; + f.service + .evict_stale_checkpoint_copy( + &f.sandbox_id, + operation(), + Some(artifact(DECIDED)), + true, + &direct, + ) + .await + .unwrap(); + f.assert_copy_untouched(Some(STALE), &cleanups); + }) + .await; +} + +/// A copy nothing proves backed up may be the only copy of something: it is +/// refused before any node work. The stub backs the +/// decided checkpoint, so only a proof of the copy's own pin counts. An +/// unpinned copy has no artifact to prove. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn an_unproven_stale_copy_is_refused_before_any_node_work() { + with_cluster_api(BACKS_ONLY_DECIDED, async || { + for pin in [Some(STALE), None] { + let (f, cleanups) = stale_copy(false, pin); + let refused = f.pod_upgrade_error(operation()).await; + assert_eq!(refused.code(), tonic::Code::FailedPrecondition, "{refused}"); + assert!(refused.message().contains("backed up"), "{refused}"); + f.assert_copy_untouched(pin, &cleanups); + } + }) + .await; +} + +/// A proof the cluster couldn't give is retryable, and leaves the copy. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn an_unanswered_backup_proof_leaves_the_copy_for_a_retry() { + with_cluster_api(BackupReadiness::Unavailable, async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + + let refused = f.pod_upgrade_error(operation()).await; + + assert_eq!(refused.code(), tonic::Code::Unavailable, "{refused}"); + f.assert_copy_untouched(Some(STALE), &cleanups); + }) + .await; +} + +/// The copy's own backup marker, stamped once its upload was confirmed, is +/// proof enough without asking the cluster, as it is for eviction. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_stamped_backup_marker_proves_the_copy() { + with_cluster_api(BACKS_ONLY_DECIDED, async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + crate::db::mark_local_artifact_backup_completed(&f.service.db.lock().unwrap(), STALE) + .unwrap(); + + let error = f.pod_upgrade_error(operation()).await; + + assert!(error.message().contains("stage"), "{error}"); + f.assert_copy_evicted(&cleanups); + }) + .await; +} + +/// A copy another upgrade still owns is left to it, retryably: one that +/// fences it holds that upgrade's preserved disk, which its recovery +/// finishes, and one that verified its boot still owes the old checkpoint's +/// retirement. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_copy_another_upgrade_owns_is_left_to_it() { + with_cluster_api(BackupReadiness::backs(STALE), async || { + // (the other upgrade's phase, what the refusal says) + for (phase, says) in [ + (None, "is moving this sandbox"), + ( + Some("verified"), + "is retiring this sandbox's old checkpoint", + ), + ] { + let (f, cleanups) = stale_copy(false, Some(STALE)); + let record = crate::db::test_fixtures::fence_runtime_upgrade( + &f.service.db.lock().unwrap(), + &f.sandbox_id, + f.target.environment().unwrap(), + None, + ); + if let Some(phase) = phase { + f.service + .db + .lock() + .unwrap() + .execute( + "UPDATE sandbox_runtime_upgrades SET phase = ?1 WHERE operation_id = ?2", + crate::db::fixture::params![phase, record.operation_id.to_string()], + ) + .unwrap(); + } + + let error = f + .service + .evict_stale_checkpoint_copy( + &f.sandbox_id, + operation(), + Some(artifact(DECIDED)), + true, + &staging_request(&f), + ) + .await + .unwrap_err(); + + assert_eq!(error.code(), tonic::Code::Unavailable, "{error}"); + assert!(error.message().contains(says), "{error}"); + f.assert_copy_untouched(Some(STALE), &cleanups); + } + }) + .await; +} + +/// A sandbox this operation already has an outcome for is left to that +/// record: the copy it holds now came after the operation, and the upgrade's +/// own check refuses it. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_copy_after_this_operations_outcome_is_left_to_the_upgrade() { + with_cluster_api(BackupReadiness::backs(STALE), async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + let operation_id = operation(); + assert!( + crate::db::record_runtime_upgrade_already_on_target( + &f.service.db.lock().unwrap(), + &crate::db::RuntimeUpgradeRequestIdentity { + operation_id, + sandbox_id: f.sandbox_id.clone(), + target_environment: f.target_environment().environment().unwrap(), + target_state: crate::db::RuntimeUpgradeTargetState::Started, + authorization_id: authorization().authorization_id, + }, + ) + .unwrap() + ); + + let refused = f.pod_upgrade_error(operation_id).await; + + assert_eq!(refused.code(), tonic::Code::FailedPrecondition, "{refused}"); + assert!(refused.message().contains("decided from"), "{refused}"); + f.assert_copy_untouched(Some(STALE), &cleanups); + }) + .await; +} + +/// A request whose stage would be refused stages nothing in the copy's +/// place, so even a backed-up copy stays: a malformed storage URI, or a route +/// identity the stage couldn't admit once the row is gone (bound to another +/// sandbox, naming another node, or missing), is refused before the copy is +/// read. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_request_that_cant_stage_leaves_the_copy() { + with_cluster_api(BackupReadiness::backs(STALE), async || { + const MALFORMED: &str = "not a storage uri"; + let (f, cleanups) = stale_copy(false, Some(STALE)); + let mut pod = staged_pod_upgrade(&f, &f.target_environment(), operation()); + pod.stage_from.as_mut().unwrap().suspend_restore_storage_uri = MALFORMED.into(); + + let with_identity = |change: &dyn Fn(&mut crate::proto::SpaceApiRouteIdentity)| { + let mut request = staged_pod_upgrade(&f, &f.target_environment(), operation()); + change( + request + .start + .as_mut() + .unwrap() + .route_identity + .as_mut() + .unwrap(), + ); + request + }; + let misbound = with_identity(&|identity| { + identity.sandbox_id = Some(pplx_space_common_core::types::SandboxId::new().to_string()); + }); + let elsewhere = with_identity(&|identity| { + identity.current_node_id = Some(uuid::Uuid::now_v7().to_string()); + }); + let mut identity_free = staged_pod_upgrade(&f, &f.target_environment(), operation()); + identity_free.start.as_mut().unwrap().route_identity = None; + + for (refused, code) in [ + ( + upgrade(&f.service, pod).await.unwrap_err(), + tonic::Code::InvalidArgument, + ), + ( + upgrade(&f.service, misbound).await.unwrap_err(), + tonic::Code::InvalidArgument, + ), + ( + upgrade(&f.service, elsewhere).await.unwrap_err(), + tonic::Code::FailedPrecondition, + ), + ( + upgrade(&f.service, identity_free).await.unwrap_err(), + tonic::Code::FailedPrecondition, + ), + ] { + assert_eq!(refused.code(), code, "{refused}"); + f.assert_copy_untouched(Some(STALE), &cleanups); + } + }) + .await; +} + +/// The eviction deletes every local artifact of the sandbox, so a copy with +/// an upload still pending is left for a retry once it finishes, even when +/// its pin is backed up. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_copy_with_a_pending_upload_is_left_for_a_retry() { + with_cluster_api(BackupReadiness::backs(STALE), async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + f.service + .db + .lock() + .unwrap() + .execute( + "INSERT INTO jobs (job_id, kind, status, payload, subject_id, created_at) + VALUES (?1, 'snapshot_upload', 'pending', '{}', ?2, '1')", + crate::db::fixture::params![uuid::Uuid::now_v7().to_string(), f.sandbox_id], + ) + .unwrap(); + + let refused = f.pod_upgrade_error(operation()).await; + + assert_eq!(refused.code(), tonic::Code::Aborted, "{refused}"); + assert!(refused.message().contains("upload"), "{refused}"); + f.assert_copy_untouched(Some(STALE), &cleanups); + }) + .await; +} + +/// The upgrade refuses to replace the copy pinning `pin`, backed up as it +/// is, saying `says`, before any node work. +async fn assert_left_in_place(encrypted: bool, pin: &'static str, says: &str) { + let (f, cleanups) = stale_copy(encrypted, Some(pin)); + let refused = f.pod_upgrade_error(operation()).await; + assert_eq!(refused.code(), tonic::Code::FailedPrecondition, "{refused}"); + assert!(refused.message().contains(says), "{refused}"); + f.assert_copy_untouched(Some(pin), &cleanups); +} + +/// An encrypted copy stays, even backed up: an encrypted checkpoint staged +/// in its place can't be cold-booted, so the upgrade would only fail later. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn an_encrypted_stale_copy_is_left_in_place() { + with_cluster_api(BackupReadiness::backs(STALE), async || { + assert_left_in_place(true, STALE, "encrypted checkpoint").await; + }) + .await; +} + +/// A newer local copy is never replaced by an older decided checkpoint, +/// whatever the minting nodes' clocks say or which backup registered last. +/// The copy's id sorts before the decided checkpoint's, as a slow clock on +/// the node that suspended it would mint it, but the cluster shows the copy +/// captured by the later execution, so it stays. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_newer_local_copy_is_never_replaced_whatever_the_clocks_say() { + let copy_captured_later = BackupReadiness::Backs { + backed: STALE, + captured_after: Some((STALE, DECIDED)), + }; + with_cluster_api(copy_captured_later, async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + assert!(artifact(STALE).as_uuid() < artifact(DECIDED).as_uuid()); + + let refused = f.pod_upgrade_error(operation()).await; + assert_eq!(refused.code(), tonic::Code::FailedPrecondition, "{refused}"); + assert!(refused.message().contains("captured after"), "{refused}"); + f.assert_copy_untouched(Some(STALE), &cleanups); + }) + .await; +} + +/// The other order: a copy whose id sorts after the decided checkpoint's, as +/// a fast clock would mint it, captured by an earlier execution, is replaced +/// like any stale copy. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_stale_copy_is_replaced_whatever_the_clocks_say() { + with_cluster_api(BackupReadiness::backs(NEWER), async || { + let (f, cleanups) = stale_copy(false, Some(NEWER)); + assert!(artifact(NEWER).as_uuid() > artifact(DECIDED).as_uuid()); + + let error = f.pod_upgrade_error(operation()).await; + + assert!(error.message().contains("stage"), "{error}"); + assert_eq!(*cleanups.lock().unwrap(), vec![f.sandbox_id.clone()]); + }) + .await; +} + +/// Without a capture order nothing shows which checkpoint is newer, so the +/// copy is refused before any node work, as every copy pinning another +/// checkpoint was before replacement existed: backups that record no capture +/// generation. So is an upgrade whose authorization doesn't accept losing the +/// sandbox's memory, which the upgrade refuses anyway. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn an_unordered_or_unauthorized_upgrade_leaves_the_copy() { + let unordered = BackupReadiness::Backs { + backed: STALE, + captured_after: None, + }; + with_cluster_api(unordered, async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + let refused = f.pod_upgrade_error(operation()).await; + assert_eq!(refused.code(), tonic::Code::FailedPrecondition, "{refused}"); + assert!(refused.message().contains("captured after"), "{refused}"); + f.assert_copy_untouched(Some(STALE), &cleanups); + + let (f, cleanups) = stale_copy(false, Some(STALE)); + let mut request = staged_pod_upgrade(&f, &f.target_environment(), operation()); + request.authorization.as_mut().unwrap().memory_loss = false; + let refused = upgrade(&f.service, request).await.unwrap_err(); + assert_eq!(refused.code(), tonic::Code::FailedPrecondition, "{refused}"); + f.assert_copy_untouched(Some(STALE), &cleanups); + }) + .await; +} + +/// An activation the row would refuse must not get past it by removing the +/// row: a request at a generation the copy's row already retained is refused +/// as stale, and the copy stays. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_request_the_row_would_fence_leaves_the_copy() { + with_cluster_api(BackupReadiness::backs(STALE), async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + // The pod's identity is at generation 4. + f.service + .db + .lock() + .unwrap() + .execute( + "UPDATE sandboxes + SET last_observed_route_generation = 9, + last_observed_execution_generation = 9 + WHERE id = ?1", + crate::db::fixture::params![f.sandbox_id], + ) + .unwrap(); + + let refused = f.pod_upgrade_error(operation()).await; + + assert_eq!(refused.code(), tonic::Code::Aborted, "{refused}"); + assert!( + refused.message().contains("retained generation"), + "{refused}" + ); + f.assert_copy_untouched(Some(STALE), &cleanups); + }) + .await; +} + +/// A cluster-api stub answering the backup checks as `backups` does and +/// `GetExecutionCurrency` with `currency` for every execution. +#[derive(Clone, Copy)] +struct BackupsAndCurrency { + backups: BackupReadiness, + currency: pplx_space_node_api_client::proto::ExecutionCurrency, +} + +impl tonic::server::NamedService for BackupsAndCurrency { + const NAME: &'static str = "space.node.api.v1.ClusterApi"; +} + +impl tower::Service> for BackupsAndCurrency +where + B: http_body::Body + Send + 'static, + B::Error: Into + Send + 'static, +{ + type Response = http::Response; + type Error = std::convert::Infallible; + type Future = futures_util::future::BoxFuture<'static, Result>; + + fn poll_ready( + &mut self, + _: &mut std::task::Context<'_>, + ) -> std::task::Poll> { + std::task::Poll::Ready(Ok(())) + } + + fn call(&mut self, request: http::Request) -> Self::Future { + let stub = *self; + Box::pin(async move { + let path = request.uri().path().to_owned(); + Ok(if path.ends_with("/GetExecutionCurrency") { + answer( + request, + move |_: pplx_space_node_api_client::proto::GetExecutionCurrencyRequest| { + Ok( + pplx_space_node_api_client::proto::GetExecutionCurrencyResponse { + currency: stub.currency.into(), + }, + ) + }, + ) + .await + } else if path.ends_with("/GetArtifactBackupLocation") { + answer(request, move |request| stub.backups.answer(request)).await + } else { + Status::unimplemented(path).into_http() + }) + }) + } +} + +impl Fixture { + /// Leave the copy as a restore of it whose prepare failed leaves it: the + /// cleanup rolled the row back to `suspended` with its pin, recorded the + /// rollback receipt, and kept the activation's claim open, so kubelet's + /// retry of the same pod matches it. Returns that pod's route identity. + fn roll_back_a_restore(&self) -> pplx_space_common_core::types::SpaceApiRouteIdentity { + use pplx_space_common_core::types::{ + ExecutionGeneration, NodeManagedBy, RouteGeneration, SandboxActivationAttemptId, + }; + let generation = ExecutionGeneration::new(4).unwrap(); + let attempt = SandboxActivationAttemptId::new(); + let route = pplx_space_common_core::types::SpaceApiRouteIdentity { + sandbox_id: Some(self.sandbox_id.parse().unwrap()), + route_generation: Some(RouteGeneration::new(4).unwrap()), + execution_generation: Some(generation), + current_node_id: Some(crate::service::test_options::test_node_id()), + substrate_kind: Some(NodeManagedBy::Kubernetes), + activation_attempt_id: Some(attempt), + pod_uid: Some("retried-pod".parse().unwrap()), + }; + let provenance = + crate::auto_lifecycle_delete_receipts::RouteIdentityProvenance::ClusterFenced; + let claim = + crate::db::route_activation_claims::OpenRouteActivationClaim::for_cluster_fenced_identity( + &route, provenance, + ) + .unwrap(); + let db = self.service.db.lock().unwrap(); + let episode = crate::db::begin_activation_with_route_evidence( + &db, + &self.sandbox_id, + SandboxState::Suspended, + crate::db::ActivationKind::Restore, + Some(generation), + Some(attempt), + crate::db::ActivationRouteAdmission { + evidence: crate::auto_lifecycle_delete_receipts::RouteEvidence::from_identity( + &route, provenance, + ), + open_claim: Some(&claim), + ..Default::default() + }, + ) + .unwrap() + .expect("the restore claims the suspended copy"); + crate::db::request_cleanup_episode( + &db, + &self.sandbox_id, + episode.id, + pplx_space_common_core::types::SandboxQuarantineReasonCode::RestorePrepareFailed, + "restore prepare failed", + ) + .unwrap(); + crate::db::test_fixtures::complete_cleanup_episode(&db, &self.sandbox_id, episode.id) + .unwrap() + .expect("the rollback completes"); + let view = crate::db::read::sandbox_view(&db, &self.sandbox_id); + assert!(view.route_claim().matches(&claim).unwrap()); + assert!(view.route_claim().rolled_back(&claim).unwrap()); + route + } + + /// The pod's upgrade, retried under `route`. + async fn retried_pod_upgrade( + &self, + route: &pplx_space_common_core::types::SpaceApiRouteIdentity, + ) -> Status { + let mut request = staged_pod_upgrade(self, &self.target_environment(), operation()); + request.start.as_mut().unwrap().route_identity = + crate::service::activation_support::space_api_route_identity_to_wire(route); + upgrade(&self.service, request) + .await + .expect_err("this harness has no object store to stage from") + } +} + +/// Kubelet's retry of an activation the copy's row rolled back matches the +/// claim the row kept open for it, which is how each later attempt of that +/// activation is asked whether its execution was retired. Replacing the copy +/// would close the claim and leave a retry after it unasked, so a retry never +/// replaces the copy, even when the control plane would answer that its +/// execution is current: the copy, its pin and its claim stay. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_retry_of_a_rolled_back_activation_never_replaces_the_copy() { + with_cluster_api( + BackupsAndCurrency { + backups: BackupReadiness::backs(STALE), + currency: pplx_space_node_api_client::proto::ExecutionCurrency::Current, + }, + async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + let route = f.roll_back_a_restore(); + + let error = f.retried_pod_upgrade(&route).await; + + assert_eq!(error.code(), tonic::Code::FailedPrecondition, "{error}"); + assert!(error.message().contains("retries"), "{error}"); + f.assert_copy_untouched(Some(STALE), &cleanups); + let db = f.service.db.lock().unwrap(); + assert!( + crate::db::route_activation_claims::claim_exists(&db, &f.sandbox_id).unwrap(), + "the claim a later retry is asked about stays" + ); + }, + ) + .await; +} + +/// The control for [`a_retry_of_a_rolled_back_activation_never_replaces_the_copy`]: +/// a new activation of the same row, with an attempt and a generation of its +/// own, matches no claim and replaces the copy, so the refusal there isn't +/// vacuous. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_new_activation_after_a_rollback_replaces_the_copy() { + with_cluster_api(BackupReadiness::backs(STALE), async || { + use pplx_space_common_core::types::{ + ExecutionGeneration, RouteGeneration, SandboxActivationAttemptId, + }; + let (f, cleanups) = stale_copy(false, Some(STALE)); + let rolled_back = f.roll_back_a_restore(); + let route = pplx_space_common_core::types::SpaceApiRouteIdentity { + route_generation: Some(RouteGeneration::new(5).unwrap()), + execution_generation: Some(ExecutionGeneration::new(5).unwrap()), + activation_attempt_id: Some(SandboxActivationAttemptId::new()), + pod_uid: Some("replacement-pod".parse().unwrap()), + ..rolled_back + }; + + let error = f.retried_pod_upgrade(&route).await; + + assert!(error.message().contains("stage"), "{error}"); + f.assert_copy_evicted(&cleanups); + }) + .await; +} + +/// A cleanup that fails after the eviction's claim leaves the row's cleanup +/// failed, which is how the replacement tells it from a claim another +/// eviction won; the upgrade is answered as retryable either way. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_cleanup_that_fails_after_the_claim_leaves_the_row_cleanup_failed() { + with_cluster_api(BackupReadiness::backs(STALE), async || { + let (f, _) = stale_copy(false, Some(STALE)); + *f.service.cleanup_executor.dispatch.lock().unwrap() = Some(Arc::new(|_, _| { + Box::pin(async { Err(Status::failed_precondition("node-manager refused")) }) + })); + + let error = f.pod_upgrade_error(operation()).await; + + assert_eq!(error.code(), tonic::Code::Aborted, "{error}"); + assert_eq!(f.row().state.cleanup(), Some(CleanupState::Failed)); + }) + .await; +} + +/// The report names a cleanup failure as one, and leaves the claim's own +/// refusals, including a claim another eviction won, unreported. +#[test] +fn only_a_failed_cleanup_is_reported_as_a_failure() { + let refused = || Err(Status::failed_precondition("refused")); + for (row_state, reported) in [ + (Some(SandboxStatus::CLEANUP_FAILED), tonic::Code::Internal), + ( + Some(SandboxStatus::Unowned(SandboxState::Deleting)), + tonic::Code::FailedPrecondition, + ), + ( + Some(SandboxStatus::Unowned(SandboxState::Suspended)), + tonic::Code::FailedPrecondition, + ), + (None, tonic::Code::FailedPrecondition), + ] { + assert_eq!( + super::reported_eviction_result(&refused(), row_state) + .unwrap_err() + .code(), + reported, + "{row_state:?}" + ); + } + super::reported_eviction_result(&Ok(()), Some(SandboxStatus::CLEANUP_FAILED)).unwrap(); + assert_eq!( + super::reported_eviction_result( + &Err(Status::unavailable("later")), + Some(SandboxStatus::CLEANUP_FAILED) + ) + .unwrap_err() + .code(), + tonic::Code::Unavailable + ); +} + +/// A backup's admission (`enqueue_artifact_backup`) holds the sandbox's +/// artifact lock from reading the row until its upload job is persisted, and +/// the eviction's claim sees only a persisted job. Paused just before it +/// persists the job, the admission still holds the lock, and the eviction is +/// refused with the copy untouched; the job is persisted only after that. +/// The stub backs only the decided checkpoint, so the admission finds no +/// backup of the copy ready and goes on to persist one. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_backup_being_admitted_holds_off_the_eviction() { + with_cluster_api(BACKS_ONLY_DECIDED, async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + // The copy's bytes, which the admission checks before it persists. + let local_path: String = f + .service + .db + .lock() + .unwrap() + .query_row( + "SELECT local_path FROM local_artifacts WHERE local_artifact_id = ?1", + crate::db::fixture::params![STALE], + |row| row.get(0), + ) + .unwrap(); + crate::service::suspend_snapshot_publication::write_full_suspend_artifact( + std::path::Path::new(&local_path), + &f.sandbox_id, + artifact(STALE), + b"stale-copy", + ); + let mut service = f.service.clone(); + service.snapshot_upload_config = + Some(Arc::new(crate::snapshot_upload::SnapshotUploadConfig { + bucket: "space-test".to_string(), + region: "us-east-1".to_string(), + key_prefix: String::new(), + endpoint: None, + path_style: false, + })); + let service = Arc::new(service); + let barrier = Arc::new(tokio::sync::Barrier::new(2)); + *service + .auto_lifecycle + .backup_admission_barrier + .lock() + .unwrap() = Some(Arc::clone(&barrier)); + let admission = tokio::spawn(admit_backup(Arc::clone(&service), f.sandbox_id.clone())); + let upload = || { + service + .snapshot_upload_worker + .find_job_by_dedup_key(crate::snapshot_upload_jobs::SNAPSHOT_UPLOAD_JOB_KIND, STALE) + .unwrap() + }; + + // The admission is about to persist its upload job. + if tokio::time::timeout(std::time::Duration::from_secs(10), barrier.wait()) + .await + .is_err() + { + panic!( + "the admission never reached its upload job: {:?}", + admission.await.unwrap() + ); + } + let refused = service + .evict_local_sandbox( + &f.sandbox_id, + crate::service::EvictionClaim::SuspendedPinned(Some(artifact(STALE))), + None, + ) + .await + .unwrap_err(); + assert_eq!(refused.code(), tonic::Code::FailedPrecondition, "{refused}"); + assert!( + refused.message().contains("another operation holds"), + "{refused}" + ); + assert!(upload().is_none(), "the job isn't persisted yet"); + let row = f.row(); + assert_eq!(row.state, SandboxStatus::Unowned(SandboxState::Suspended)); + assert_eq!(row.current_local_artifact_id, Some(artifact(STALE))); + assert!(cleanups.lock().unwrap().is_empty()); + + barrier.wait().await; + // The admission may fail after persisting, registering against a stub + // that serves no registration; the job is what it leaves either way. + let _ = admission.await.unwrap(); + assert!(upload().is_some(), "the admission persisted its upload job"); + }) + .await; +} + +/// Admit a backup of [`STALE`], the copy's pinned artifact, as +/// `EnqueueArtifactBackup` does. +async fn admit_backup( + service: Arc, + sandbox_id: String, +) -> Result, Status> { + service + .enqueue_artifact_backup(internal_request( + crate::proto::EnqueueArtifactBackupRequest { + sandbox_id, + expected_artifact_kind: crate::proto::ArtifactKind::Full as i32, + source: Some( + crate::proto::enqueue_artifact_backup_request::Source::LocalArtifactId( + STALE.to_string(), + ), + ), + }, + )) + .await +} + +/// A backup admitted while an eviction claims the row waits for the artifact +/// lock the claim holds, then finds the row `deleting`: it refuses, and +/// persists no upload whose source the eviction's cleanup deletes. The +/// eviction is paused holding the lock before its claim, the admission is +/// seen to reach the lock and stay blocked on it, and the eviction's cleanup +/// is held open, so the row is still `deleting` when the admission gets it. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_backup_admitted_mid_eviction_backs_up_nothing() { + with_cluster_api(BACKS_ONLY_DECIDED, async || { + let (f, _) = stale_copy(false, Some(STALE)); + let service = Arc::new(f.service.clone()); + let cleanup_released = Arc::new(tokio::sync::Notify::new()); + *service.cleanup_executor.dispatch.lock().unwrap() = Some(Arc::new({ + let cleanup_released = Arc::clone(&cleanup_released); + move |_, _| { + let cleanup_released = Arc::clone(&cleanup_released); + Box::pin(async move { + cleanup_released.notified().await; + Ok(()) + }) + } + })); + let barrier = Arc::new(tokio::sync::Barrier::new(2)); + *service + .auto_lifecycle + .eviction_claim_barrier + .lock() + .unwrap() = Some(Arc::clone(&barrier)); + let eviction = tokio::spawn({ + let service = Arc::clone(&service); + let sandbox_id = f.sandbox_id.clone(); + async move { + service + .evict_local_sandbox( + &sandbox_id, + crate::service::EvictionClaim::SuspendedPinned(Some(artifact(STALE))), + None, + ) + .await + } + }); + // The eviction holds the artifact lock and hasn't claimed the row. + tokio::time::timeout(std::time::Duration::from_secs(10), barrier.wait()) + .await + .expect("the eviction reaches its claim holding the lock"); + let at_lock = Arc::new(tokio::sync::Notify::new()); + *service + .auto_lifecycle + .artifact_lock_wait_hook + .lock() + .unwrap() = Some(Arc::clone(&at_lock)); + let mut admission = tokio::spawn(admit_backup(Arc::clone(&service), f.sandbox_id.clone())); + tokio::time::timeout(std::time::Duration::from_secs(10), at_lock.notified()) + .await + .expect("the admission reaches the artifact lock"); + // The admission waits for the lock the eviction holds: it can't + // answer, however long it's given, until the eviction lets the lock go. + // Without the eviction's lock it would answer at once. + assert!( + tokio::time::timeout(std::time::Duration::from_millis(500), &mut admission) + .await + .is_err(), + "the admission must wait for the eviction's artifact lock" + ); + tokio::time::timeout(std::time::Duration::from_secs(10), barrier.wait()) + .await + .expect("the eviction resumes"); + + let refused = tokio::time::timeout(std::time::Duration::from_secs(10), admission) + .await + .expect("the admission answers") + .unwrap() + .unwrap_err(); + + assert_eq!(refused.code(), tonic::Code::FailedPrecondition, "{refused}"); + assert!(refused.message().contains("being deleted"), "{refused}"); + assert!( + service + .snapshot_upload_worker + .find_job_by_dedup_key(crate::snapshot_upload_jobs::SNAPSHOT_UPLOAD_JOB_KIND, STALE) + .unwrap() + .is_none(), + "no upload of the evicted copy is persisted" + ); + cleanup_released.notify_one(); + tokio::time::timeout(std::time::Duration::from_secs(10), eviction) + .await + .expect("the eviction finishes") + .unwrap() + .unwrap(); + + // Nor once the eviction's cleanup owns the row, which then reads as + // the cleanup it's under rather than `deleting`. + let (g, _) = stale_copy(false, Some(STALE)); + { + let db = g.service.db.lock().unwrap(); + assert!( + crate::db::claim_suspended_sandbox_for_eviction( + &db, + &g.sandbox_id, + Some(STALE), + None + ) + .unwrap() + ); + db.execute( + "UPDATE sandboxes + SET cleanup_state = 'required', cleanup_completion = 'delete_row', + cleanup_keep_filesystem = 0 + WHERE id = ?1", + crate::db::fixture::params![g.sandbox_id], + ) + .unwrap(); + } + assert_eq!(g.row().state.cleanup(), Some(CleanupState::Required)); + let refused = g + .service + .enqueue_artifact_backup(internal_request( + crate::proto::EnqueueArtifactBackupRequest { + sandbox_id: g.sandbox_id.clone(), + expected_artifact_kind: crate::proto::ArtifactKind::Full as i32, + source: Some( + crate::proto::enqueue_artifact_backup_request::Source::LocalArtifactId( + STALE.to_string(), + ), + ), + }, + )) + .await + .unwrap_err(); + assert_eq!(refused.code(), tonic::Code::FailedPrecondition, "{refused}"); + assert!(refused.message().contains("being deleted"), "{refused}"); + }) + .await; +} + +impl Fixture { + /// Give the sandbox a named snapshot beside its copy, with no backup + /// stamp: the eviction would delete it with the copy. + fn add_unstamped_named_snapshot(&self) { + self.service + .db + .lock() + .unwrap() + .execute( + "INSERT INTO local_artifacts ( + local_artifact_id, sandbox_id, artifact_kind, artifact_origin, + snapshot_name, local_path, created_at + ) VALUES (?1, ?2, 'disk', 'snapshot', 'd-named', '/space/cache/named', '0')", + crate::db::fixture::params![NAMED, self.sandbox_id], + ) + .unwrap(); + } + + fn named_snapshot_stamped(&self) -> Option { + self.service + .db + .lock() + .unwrap() + .query_row( + "SELECT backup_completed_at IS NOT NULL FROM local_artifacts + WHERE local_artifact_id = ?1", + crate::db::fixture::params![NAMED], + |row| row.get(0), + ) + .ok() + } +} + +/// Replacing the copy deletes every local artifact of the sandbox, so a named +/// snapshot beside it that nothing backed up holds the replacement off: it's +/// refused before any node work, and the copy and the snapshot stay. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn an_unproven_named_snapshot_holds_off_the_replacement() { + with_cluster_api( + BackupReadiness::AndNamed { + named_backed: false, + }, + async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + f.add_unstamped_named_snapshot(); + + let refused = f.pod_upgrade_error(operation()).await; + + assert_eq!(refused.code(), tonic::Code::FailedPrecondition, "{refused}"); + assert!( + refused.message().contains("another local artifact"), + "{refused}" + ); + f.assert_copy_untouched(Some(STALE), &cleanups); + assert_eq!(f.named_snapshot_stamped(), Some(false)); + }, + ) + .await; +} + +/// The control: once the cluster shows the named snapshot backed, it's +/// stamped and the copy is replaced. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_proven_named_snapshot_lets_the_replacement_go() { + with_cluster_api( + BackupReadiness::AndNamed { named_backed: true }, + async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + f.add_unstamped_named_snapshot(); + + let error = f.pod_upgrade_error(operation()).await; + + assert!(error.message().contains("stage"), "{error}"); + f.assert_copy_evicted(&cleanups); + }, + ) + .await; +} + +/// The disk-pressure eviction deletes every local artifact of the sandbox +/// too: with a named snapshot nothing backed up beside a proven copy, the +/// tick skips the sandbox and leaves both. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn an_unproven_named_snapshot_holds_off_the_disk_pressure_eviction() { + with_cluster_api( + BackupReadiness::AndNamed { + named_backed: false, + }, + async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + f.add_unstamped_named_snapshot(); + + let (evicted, skipped) = crate::sandbox_eviction::evict_tick_for_test(&f.service).await; + + assert_eq!((evicted, skipped), (0, 1)); + f.assert_copy_untouched(Some(STALE), &cleanups); + assert_eq!(f.named_snapshot_stamped(), Some(false)); + }, + ) + .await; +} + +/// The control: a named snapshot the cluster shows backed is stamped, and the +/// tick evicts the sandbox. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_proven_named_snapshot_lets_the_disk_pressure_eviction_go() { + with_cluster_api( + BackupReadiness::AndNamed { named_backed: true }, + async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + f.add_unstamped_named_snapshot(); + + let (evicted, skipped) = crate::sandbox_eviction::evict_tick_for_test(&f.service).await; + + assert_eq!((evicted, skipped), (1, 0)); + f.assert_copy_evicted(&cleanups); + }, + ) + .await; +} + +mod unrecorded; diff --git a/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/stale_copy/tests/unrecorded.rs b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/stale_copy/tests/unrecorded.rs new file mode 100644 index 0000000000..7c64f8e486 --- /dev/null +++ b/crates/l40_api/sbx/mgr/server/src/service/runtime_upgrade/stale_copy/tests/unrecorded.rs @@ -0,0 +1,474 @@ +//! A suspended eviction deletes the sandbox's whole cache directory, so a +//! user snapshot directory no local artifact records (a publication that +//! failed after writing it) holds off both suspended evictions, the +//! replacement and the disk-pressure tick. Row-less leftovers that aren't a +//! user's to keep don't, and a cluster-deleted sandbox's eviction doesn't +//! look. + +use super::*; + +/// A manual disk snapshot no row records. +const UNRECORDED: &str = "d-manual-20261107T120000Z"; + +/// A manual full snapshot no row records. +const UNRECORDED_FULL: &str = "f-manual-20261107T120000Z"; + +/// What a snapshot directory holds. +#[derive(Clone, Copy, Debug)] +enum Contents { + /// A disk, and for a full snapshot its VM state too. + Complete, + /// A full snapshot's disk without its VM state file `missing`, and with + /// its other one empty: what a failed capture leaves. + MissingVmState { missing: &'static str }, + /// A full snapshot's disk and VM state, with its VM state file `empty` + /// written but empty. + EmptyVmState { empty: &'static str }, +} + +/// Create `name` holding `contents` under `snapshots_dir`. +fn write_snapshot_dir(snapshots_dir: &std::path::Path, name: &str, contents: Contents) { + let path = snapshots_dir.join(name); + std::fs::create_dir_all(&path).unwrap(); + std::fs::write(path.join("rootfs.ext4"), b"snapshot").unwrap(); + match contents { + Contents::Complete if name.starts_with("f-") => { + std::fs::write(path.join("vmstate"), b"vmstate").unwrap(); + std::fs::write(path.join("memory"), b"memory").unwrap(); + } + Contents::Complete => {} + Contents::MissingVmState { missing } => { + for file in ["vmstate", "memory"] { + if file != missing { + std::fs::write(path.join(file), b"").unwrap(); + } + } + } + Contents::EmptyVmState { empty } => { + for file in ["vmstate", "memory"] { + let bytes: &[u8] = if file == empty { b"" } else { b"state" }; + std::fs::write(path.join(file), bytes).unwrap(); + } + } + } +} + +/// Row-less leftovers that aren't a user's to keep, by kind: internal names, +/// names without a user snapshot's prefix, and full snapshots without their +/// VM state. +fn unprotected_leftovers() -> [Vec<(String, Contents)>; 3] { + let capture = format!("d-operation-{}", uuid::Uuid::now_v7()); + let complete = |name: String| (name, Contents::Complete); + [ + vec![ + complete(capture.clone()), + complete(format!("{capture}-artifact")), + complete(format!("d-operation-runtime-upgrade-{}", operation())), + complete(format!("d-stop-{}", LocalArtifactId::new())), + complete(format!("d-gone.gc-pending-{}", LocalArtifactId::new())), + ], + vec![ + complete(format!(".encrypted-capture-{}", uuid::Uuid::now_v7())), + complete(format!(".suspend-publish-{}", LocalArtifactId::new())), + ], + vec![ + ( + UNRECORDED_FULL.to_string(), + Contents::MissingVmState { missing: "memory" }, + ), + ( + "f-10m-20261107T120000Z".to_string(), + Contents::MissingVmState { missing: "vmstate" }, + ), + ( + "f-1h-20261107T120000Z".to_string(), + Contents::EmptyVmState { empty: "memory" }, + ), + ( + "f-6h-20261107T120000Z".to_string(), + Contents::EmptyVmState { empty: "vmstate" }, + ), + ], + ] +} + +/// Row-less snapshots a user keeps: either type, manual, scheduled or +/// imported. A disk snapshot is kept whatever it holds. +const KEPT: [&str; 8] = [ + UNRECORDED, + UNRECORDED_FULL, + "d-imported-x", + "f-imported-x", + "d-1m-20261107T120000Z", + "d-5m-20261107T120000Z", + "f-10m-20261107T120000Z", + "f-6h-20261107T120000Z", +]; + +/// Each kind of unprotected leftover is left out on its own; every row-less +/// user snapshot, scheduled captures included, is kept unless a row records +/// its name; and a disk snapshot without a disk is kept too. +#[test] +fn only_row_less_user_snapshots_are_kept() { + let kept = |dir: &std::path::Path, name: &str, recorded: &std::collections::HashSet| { + crate::service::eviction::is_unrecorded_user_snapshot(dir, name, recorded).unwrap() + }; + let none = std::collections::HashSet::new(); + for (kind, leftovers) in ["internal", "unprefixed", "incomplete full"] + .into_iter() + .zip(unprotected_leftovers()) + { + for (name, contents) in leftovers { + let dir = tempfile::tempdir().unwrap(); + write_snapshot_dir(dir.path(), &name, contents); + assert!(!kept(dir.path(), &name, &none), "{kind}: {name}"); + } + } + for name in KEPT { + let dir = tempfile::tempdir().unwrap(); + write_snapshot_dir(dir.path(), name, Contents::Complete); + assert!(kept(dir.path(), name, &none), "{name}"); + let recorded = std::collections::HashSet::from([name.to_string()]); + assert!(!kept(dir.path(), name, &recorded), "recorded {name}"); + } + let dir = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(dir.path().join(UNRECORDED)).unwrap(); + assert!( + kept(dir.path(), UNRECORDED, &none), + "an empty disk snapshot" + ); + let root = tempfile::tempdir().unwrap(); + let snapshots = root.path().join("snapshots"); + for name in add_non_directories(&snapshots) { + assert!(!kept(&snapshots, &name, &none), "not a directory: {name}"); + } +} + +/// Entries under `snapshots_dir` with a user snapshot's name that the listing +/// doesn't show, as they aren't directories: a disk symlink and a full one, +/// each to a complete snapshot outside `snapshots_dir`, and a disk regular +/// file. Returns their names. +fn add_non_directories(snapshots_dir: &std::path::Path) -> [String; 3] { + let targets = snapshots_dir.parent().unwrap().join("link-targets"); + write_snapshot_dir(&targets, UNRECORDED, Contents::Complete); + write_snapshot_dir(&targets, UNRECORDED_FULL, Contents::Complete); + std::fs::create_dir_all(snapshots_dir).unwrap(); + let names = [ + "d-manual-link".to_string(), + "f-manual-link".to_string(), + "d-manual-file".to_string(), + ]; + std::os::unix::fs::symlink(targets.join(UNRECORDED), snapshots_dir.join(&names[0])).unwrap(); + std::os::unix::fs::symlink(targets.join(UNRECORDED_FULL), snapshots_dir.join(&names[1])) + .unwrap(); + std::fs::write(snapshots_dir.join(&names[2]), b"snapshot").unwrap(); + names +} + +/// A directory whose name isn't UTF-8 is left out of the inventory: the +/// listing can't show it and no snapshot RPC can name it. Decoded lossily, it +/// would pass for a real snapshot named with the replacement character, which +/// is listed once, beside it. +#[test] +fn a_name_that_isnt_utf8_is_left_out() { + use std::os::unix::ffi::OsStrExt as _; + let root = tempfile::tempdir().unwrap(); + let snapshots = root.path().join("snapshots"); + let undecodable = std::ffi::OsStr::from_bytes(b"d-manual-\xff"); + std::fs::create_dir_all(snapshots.join(undecodable)).unwrap(); + let replaced = "d-manual-\u{FFFD}"; + write_snapshot_dir(&snapshots, replaced, Contents::Complete); + write_snapshot_dir(&snapshots, UNRECORDED, Contents::Complete); + + let found = crate::service::eviction::unrecorded_snapshots( + &snapshots, + &std::collections::HashSet::new(), + ) + .unwrap(); + + assert_eq!(found, vec![UNRECORDED.to_string(), replaced.to_string()]); +} + +impl Fixture { + fn snapshots_dir(&self) -> std::path::PathBuf { + self.checkpoint().parent().unwrap().join("snapshots") + } + + /// Create a complete `name` under the sandbox's snapshot directory, + /// recording no row for it. + fn add_snapshot_dir(&self, name: &str) -> std::path::PathBuf { + write_snapshot_dir(&self.snapshots_dir(), name, Contents::Complete); + self.snapshots_dir().join(name) + } + + /// Leave a row-less leftover of every kind that isn't a user's to keep. + fn add_unprotected_leftovers(&self) { + for (name, contents) in unprotected_leftovers().into_iter().flatten() { + write_snapshot_dir(&self.snapshots_dir(), &name, contents); + } + add_non_directories(&self.snapshots_dir()); + } + + /// Give the sandbox the user snapshot [`NAMED`], with its row and a + /// backup stamp. + fn add_stamped_named_snapshot(&self) { + let path = self.add_snapshot_dir("d-named"); + self.service + .db + .lock() + .unwrap() + .execute( + "INSERT INTO local_artifacts ( + local_artifact_id, sandbox_id, artifact_kind, artifact_origin, + snapshot_name, local_path, created_at, backup_completed_at + ) VALUES (?1, ?2, 'disk', 'snapshot', 'd-named', ?3, '0', '0')", + crate::db::fixture::params![NAMED, self.sandbox_id, path.to_string_lossy()], + ) + .unwrap(); + } +} + +/// A row-less complete `name` beside a backed-up stale copy holds off the +/// replacement before any node work, naming it, and both stay. +async fn assert_holds_off_the_replacement(name: &'static str) { + with_cluster_api(BackupReadiness::backs(STALE), async move || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + let unrecorded = f.add_snapshot_dir(name); + + let refused = f.pod_upgrade_error(operation()).await; + + assert_eq!(refused.code(), tonic::Code::FailedPrecondition, "{refused}"); + assert!(refused.message().contains(name), "{refused}"); + f.assert_copy_untouched(Some(STALE), &cleanups); + assert!(unrecorded.exists()); + }) + .await; +} + +/// A row-less complete `name` beside a backed-up stale copy makes a real +/// disk-pressure tick skip the sandbox, and both stay. +async fn assert_holds_off_the_disk_pressure_eviction(name: &'static str) { + with_cluster_api(BackupReadiness::backs(STALE), async move || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + let unrecorded = f.add_snapshot_dir(name); + + let (evicted, skipped) = crate::sandbox_eviction::evict_tick_for_test(&f.service).await; + + assert_eq!((evicted, skipped), (0, 1), "{name}"); + f.assert_copy_untouched(Some(STALE), &cleanups); + assert!(unrecorded.exists()); + }) + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn an_unrecorded_snapshot_holds_off_the_replacement() { + assert_holds_off_the_replacement(UNRECORDED).await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn an_unrecorded_snapshot_holds_off_the_disk_pressure_eviction() { + assert_holds_off_the_disk_pressure_eviction(UNRECORDED).await; +} + +/// A full snapshot with its VM state is a restorable checkpoint, kept row or +/// no row; the same name without it is an unprotected leftover (see +/// [`unprotected_leftovers`]). +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_complete_unrecorded_full_snapshot_holds_off_both_evictions() { + assert_holds_off_the_replacement(UNRECORDED_FULL).await; + assert_holds_off_the_disk_pressure_eviction(UNRECORDED_FULL).await; +} + +/// A suspended sandbox's scheduler is stopped, so retention never prunes a +/// row-less scheduled capture; it's kept like a manual one. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn an_unrecorded_scheduled_capture_holds_off_both_evictions() { + assert_holds_off_the_replacement("f-10m-20261107T120000Z").await; + assert_holds_off_the_disk_pressure_eviction("d-1m-20261107T120000Z").await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn unprotected_leftovers_without_a_row_let_the_replacement_go() { + with_cluster_api(BackupReadiness::backs(STALE), async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + f.add_unprotected_leftovers(); + + let error = f.pod_upgrade_error(operation()).await; + + assert!(error.message().contains("stage"), "{error}"); + f.assert_copy_evicted(&cleanups); + }) + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn unprotected_leftovers_without_a_row_let_the_disk_pressure_eviction_go() { + with_cluster_api(BackupReadiness::backs(STALE), async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + f.add_unprotected_leftovers(); + + let (evicted, skipped) = crate::sandbox_eviction::evict_tick_for_test(&f.service).await; + + assert_eq!((evicted, skipped), (1, 0)); + f.assert_copy_evicted(&cleanups); + }) + .await; +} + +/// The control for the name filter's other half: a user snapshot with its +/// row and a backup stamp is recorded, and neither eviction holds off. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_recorded_backed_up_snapshot_lets_the_replacement_go() { + with_cluster_api(BackupReadiness::backs(STALE), async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + f.add_stamped_named_snapshot(); + + let error = f.pod_upgrade_error(operation()).await; + + assert!(error.message().contains("stage"), "{error}"); + f.assert_copy_evicted(&cleanups); + }) + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_recorded_backed_up_snapshot_lets_the_disk_pressure_eviction_go() { + with_cluster_api(BackupReadiness::backs(STALE), async || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + f.add_stamped_named_snapshot(); + + let (evicted, skipped) = crate::sandbox_eviction::evict_tick_for_test(&f.service).await; + + assert_eq!((evicted, skipped), (1, 0)); + f.assert_copy_evicted(&cleanups); + }) + .await; +} + +/// A cluster-deleted sandbox's snapshots went with its cluster row, so its +/// eviction goes ahead over an unrecorded snapshot that holds off the same +/// copy's pinned-suspended eviction. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_cluster_deleted_eviction_ignores_an_unrecorded_snapshot() { + let (f, cleanups) = stale_copy(false, Some(STALE)); + f.add_snapshot_dir(UNRECORDED); + + let refused = f + .service + .evict_local_sandbox( + &f.sandbox_id, + crate::service::EvictionClaim::SuspendedPinned(Some(artifact(STALE))), + None, + ) + .await + .unwrap_err(); + assert!( + crate::service::is_unrecorded_snapshot_refusal(&refused), + "{refused}" + ); + // The disk-pressure loop counts it under its own reason, not as a hold + // that ends on its own. + use crate::sandbox_eviction::{SkipReason, claim_refusal_skip_reason}; + assert_eq!( + claim_refusal_skip_reason(&refused, true), + SkipReason::UnrecordedSnapshot + ); + let held = tonic::Status::failed_precondition(refused.message()); + assert_eq!( + claim_refusal_skip_reason(&held, true), + SkipReason::HeldAtExecute + ); + f.assert_copy_untouched(Some(STALE), &cleanups); + + f.service + .evict_local_sandbox( + &f.sandbox_id, + crate::service::EvictionClaim::DeletedSuspended(Some(artifact(STALE))), + None, + ) + .await + .unwrap(); + f.assert_copy_evicted(&cleanups); +} + +/// Where a metadata read fails while the inventory looks at a row-less user +/// snapshot: its entry, or one of a full snapshot's VM state files. +#[derive(Clone, Copy, Debug)] +enum MetadataFault { + Entry, + RequiredFile, +} + +impl MetadataFault { + /// The snapshot the fault hides and the path whose metadata read fails. + fn target(self, snapshots_dir: &std::path::Path) -> (&'static str, std::path::PathBuf) { + match self { + Self::Entry => (UNRECORDED, snapshots_dir.join(UNRECORDED)), + Self::RequiredFile => ( + UNRECORDED_FULL, + snapshots_dir.join(UNRECORDED_FULL).join("vmstate"), + ), + } + } +} + +/// A metadata error other than `NotFound` is returned, not taken for "not a +/// user snapshot", at either read. +#[test] +fn a_metadata_error_is_returned_not_taken_for_an_answer() { + for fault in [MetadataFault::Entry, MetadataFault::RequiredFile] { + let root = tempfile::tempdir().unwrap(); + let snapshots = root.path().join("snapshots"); + let (name, failing) = fault.target(&snapshots); + write_snapshot_dir(&snapshots, name, Contents::Complete); + crate::service::grpc_spaced_process_snapshots::fail_snapshot_metadata(failing); + + let read = crate::service::eviction::is_unrecorded_user_snapshot( + &snapshots, + name, + &std::collections::HashSet::new(), + ); + + assert!(read.is_err(), "{fault:?}: {read:?}"); + } +} + +/// An unreadable row-less user snapshot refuses both suspended evictions as +/// `Unavailable` before the claim: nothing is claimed or cleaned up, and the +/// row, its pin and the snapshot stay. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn an_unreadable_unrecorded_snapshot_holds_off_both_evictions() { + for fault in [MetadataFault::Entry, MetadataFault::RequiredFile] { + with_cluster_api(BackupReadiness::backs(STALE), async move || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + let (name, failing) = fault.target(&f.snapshots_dir()); + let snapshot = f.add_snapshot_dir(name); + crate::service::grpc_spaced_process_snapshots::fail_snapshot_metadata(failing); + + let refused = f.pod_upgrade_error(operation()).await; + + assert_eq!( + refused.code(), + tonic::Code::Unavailable, + "{fault:?}: {refused}" + ); + f.assert_copy_untouched(Some(STALE), &cleanups); + assert!(snapshot.exists(), "{fault:?}"); + }) + .await; + with_cluster_api(BackupReadiness::backs(STALE), async move || { + let (f, cleanups) = stale_copy(false, Some(STALE)); + let (name, failing) = fault.target(&f.snapshots_dir()); + let snapshot = f.add_snapshot_dir(name); + crate::service::grpc_spaced_process_snapshots::fail_snapshot_metadata(failing); + + let (evicted, skipped) = crate::sandbox_eviction::evict_tick_for_test(&f.service).await; + + assert_eq!((evicted, skipped), (0, 0), "{fault:?}"); + f.assert_copy_untouched(Some(STALE), &cleanups); + assert!(snapshot.exists(), "{fault:?}"); + }) + .await; + } +} diff --git a/crates/l40_api/sbx/mgr/server/src/service/start.rs b/crates/l40_api/sbx/mgr/server/src/service/start.rs index c065faea8c..6825b28ba3 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/start.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/start.rs @@ -1264,6 +1264,40 @@ impl SandboxManagerService { } } + /// Refuse a staging `Start` that [`Self::stage_missing_checkpoint`] would + /// refuse for a sandbox absent from this node: a malformed request, such + /// as one with a malformed storage URI, or a route identity the stage + /// couldn't admit without a row, bound to another sandbox, naming another + /// node or missing. While the row is still here, the identity must also + /// pass it, including its retained generation. A caller about to remove + /// the sandbox's row checks it first, so a stage that can't follow, or an + /// activation the row would refuse, removes nothing. + /// + /// Returns the open activation claim the request names against the row, + /// as an activation of it would build it: the evidence + /// [`Self::refuse_retired_activation_retry`] needs to tell a retried + /// activation from a first one, which removing the row erases. + pub(super) fn validate_staging_start( + &self, + req: &StartRequest, + ) -> Result, Status> { + let ctx = StartSandboxContext::from_request(req)?; + let requested = ctx.space_api_route_identity; + let against_row = + self.resolve_runtime_activation_identity(&req.sandbox_id, requested.clone(), false)?; + let identity = self.resolve_rowless_activation_identity(&req.sandbox_id, requested)?; + let sandbox_id = req + .sandbox_id + .parse::() + .map_err(|error| Status::invalid_argument(format!("invalid sandbox id: {error}")))?; + resolve_missing_checkpoint_restore_route_identity(sandbox_id, identity)?; + Ok(super::activation_support::open_route_activation_claim( + &against_row.identity, + ctx.route_identity_provenance, + against_row.open_activation_claim_eligible, + )) + } + /// Stage a sandbox absent from this node from its suspend checkpoint, and /// leave its row suspended without launching anything. A Wake that runs a /// runtime upgrade boots the staged disk by cold start instead of loading diff --git a/crates/l40_api/sbx/mgr/server/src/service/suspend_snapshot_publication.rs b/crates/l40_api/sbx/mgr/server/src/service/suspend_snapshot_publication.rs index 52c20bf560..ece54d507c 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/suspend_snapshot_publication.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/suspend_snapshot_publication.rs @@ -948,15 +948,56 @@ pub(super) enum RecordedPublicationError { /// records the execution that took it ([`crate::capture_record`]) in its /// staging directory, so the record is published with the bytes and lasts as /// long as they do. A capture whose record can't be written isn't published. +/// This is the capturing teardown's own publish, so the record orders +/// captures; recovery publishes through +/// [`publish_recovered_suspend_snapshot_dir`]. pub(super) fn publish_recorded_suspend_snapshot_dir( staging_dir: &Path, snapshot_dir: &Path, local_artifact_id: LocalArtifactId, execution_generation: Option, verdicts: &RetainedSourceVerdicts, +) -> Result, RecordedPublicationError> { + publish_with_capture_record( + staging_dir, + snapshot_dir, + local_artifact_id, + execution_generation, + crate::capture_record::RecordWriter::CapturingTeardown, + verdicts, + ) +} + +/// [`publish_recorded_suspend_snapshot_dir`] for startup recovery, naming the +/// teardown it found active. That teardown needn't be the one that staged the +/// capture, so the record fences re-requests but orders nothing. +pub(super) fn publish_recovered_suspend_snapshot_dir( + staging_dir: &Path, + snapshot_dir: &Path, + local_artifact_id: LocalArtifactId, + execution_generation: Option, + verdicts: &RetainedSourceVerdicts, +) -> Result, RecordedPublicationError> { + publish_with_capture_record( + staging_dir, + snapshot_dir, + local_artifact_id, + execution_generation, + crate::capture_record::RecordWriter::Recovery, + verdicts, + ) +} + +fn publish_with_capture_record( + staging_dir: &Path, + snapshot_dir: &Path, + local_artifact_id: LocalArtifactId, + execution_generation: Option, + writer: crate::capture_record::RecordWriter, + verdicts: &RetainedSourceVerdicts, ) -> Result, RecordedPublicationError> { if let Some(execution_generation) = execution_generation { - crate::capture_record::write(staging_dir, local_artifact_id, execution_generation) + crate::capture_record::write(staging_dir, local_artifact_id, execution_generation, writer) .map_err(RecordedPublicationError::Record)?; } Ok(publish_suspend_snapshot_dir( diff --git a/crates/l40_api/sbx/mgr/server/src/service/suspend_snapshot_publication/tests.rs b/crates/l40_api/sbx/mgr/server/src/service/suspend_snapshot_publication/tests.rs index e91dd11756..5ea25688e2 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/suspend_snapshot_publication/tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/suspend_snapshot_publication/tests.rs @@ -623,6 +623,37 @@ fn a_retained_artifact_stays_complete_across_stream_reclaim() { assert!(error.is_pending()); } +/// Recovery publishes a staged capture with a record naming the teardown it +/// found active, which fences re-requests of it but doesn't order it: that +/// teardown needn't be the one that staged the capture. +#[test] +fn a_recovered_publication_records_a_generation_that_fences_but_orders_nothing() { + let generation = ExecutionGeneration::new(7).unwrap(); + let tmp = tempfile::tempdir().expect("create tempdir"); + let snapshot_dir = canonical_suspend_snapshot_dir(tmp.path()); + let local_artifact_id = LocalArtifactId::new(); + let staging_dir = suspend_snapshot_staging_dir(tmp.path(), local_artifact_id); + write_full_suspend_artifact(&staging_dir, TEST_SANDBOX_ID, local_artifact_id, b"capture"); + + publish_recovered_suspend_snapshot_dir( + &staging_dir, + &snapshot_dir, + local_artifact_id, + Some(generation), + &Default::default(), + ) + .unwrap(); + + assert_eq!( + crate::capture_record::read(&snapshot_dir, local_artifact_id).unwrap(), + Some(generation) + ); + assert_eq!( + crate::capture_record::read_capture_order(&snapshot_dir, local_artifact_id).unwrap(), + None + ); +} + /// A teardown capture's execution record is published with its bytes, where a /// backup requested again by artifact id reads it; a capture with no execution /// publishes without one. A record that can't be written publishes nothing and @@ -679,6 +710,12 @@ fn a_teardown_capture_publishes_only_with_its_execution_record() { execution_generation, "{case}" ); + assert_eq!( + crate::capture_record::read_capture_order(&snapshot_dir, local_artifact_id) + .unwrap(), + execution_generation, + "{case}: the capturing teardown's own record orders captures" + ); } } } diff --git a/crates/l40_api/sbx/mgr/server/src/service/tests.rs b/crates/l40_api/sbx/mgr/server/src/service/tests.rs index ffd60be887..11b2b88254 100644 --- a/crates/l40_api/sbx/mgr/server/src/service/tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/service/tests.rs @@ -7676,6 +7676,242 @@ pub(crate) mod service_tests { ); } + /// A suspended row the eviction claim would refuse because an upload of + /// its artifacts or a runtime upgrade still holds it, or that another + /// operation on its artifacts holds the artifact lock of, is answered + /// `Aborted`, which node-ctl retries on its next sync rather than after + /// its back-off, and is left as it is. Once nothing holds it, the same + /// eviction reclaims it. + #[tokio::test] + async fn evict_cluster_deleted_suspended_sandbox_is_retried_while_held() { + for hold in ["upload", "upgrade", "artifact lock"] { + let conn = crate::db::test_conn(); + let sandbox_id = SandboxId::new().to_string(); + insert_platform_owned_sandbox(&conn, &sandbox_id, SandboxState::Suspended, None); + conn.execute( + "UPDATE sandboxes SET updated_at = '0' WHERE id = ?1", + crate::db::fixture::params![sandbox_id], + ) + .unwrap(); + let release = match hold { + "upload" => { + conn.execute( + "INSERT INTO jobs (job_id, kind, status, payload, subject_id, created_at) + VALUES ('held-upload', 'snapshot_upload', 'running', '{}', ?1, '1')", + crate::db::fixture::params![sandbox_id], + ) + .unwrap(); + Some("UPDATE jobs SET status = 'completed' WHERE job_id = 'held-upload'") + } + // Another operation on its artifacts: a backup being + // admitted, a transfer, a publication. + "artifact lock" => None, + _ => { + let compiled = + pplx_space_node_runtime_core::catalog::CompiledEnvironment::for_architecture( + pplx_space_common_core::types::Arch::X86_64, + ) + .unwrap(); + crate::db::upsert_sandbox_execution_metadata( + &conn, + &sandbox_id, + &compiled[0].definition().execution_requirements().unwrap(), + ) + .unwrap(); + crate::db::test_fixtures::fence_runtime_upgrade( + &conn, + &sandbox_id, + compiled[1].definition().reference(), + None, + ); + Some("UPDATE sandbox_runtime_upgrades SET phase = 'rolled_back'") + } + }; + conn.execute( + "UPDATE sandboxes SET updated_at = '0' WHERE id = ?1", + crate::db::fixture::params![sandbox_id], + ) + .unwrap(); + let service = service_with_runtime_probe(conn, false); + let dispatched = Arc::new(std::sync::Mutex::new(Vec::new())); + let observed = Arc::clone(&dispatched); + *service.cleanup_executor.dispatch.lock().unwrap() = Some(Arc::new(move |id, kill| { + observed.lock().unwrap().push((id, kill)); + Box::pin(async { Ok(()) }) + })); + let artifact_guard = (hold == "artifact lock").then(|| { + service + .auto_lifecycle + .artifact_lock(&sandbox_id) + .try_lock_owned() + .unwrap() + }); + + let error = service + .evict_cluster_deleted_sandbox(root_peer_request(&sandbox_id)) + .await + .expect_err("a held row must not be evicted"); + assert_eq!(error.code(), tonic::Code::Aborted, "{hold}: {error}"); + assert!(error.message().contains("next sync"), "{hold}: {error}"); + { + let db = service.db.lock().unwrap(); + let row = crate::db::get_sandbox(&db, &sandbox_id).unwrap().unwrap(); + assert_eq!( + row.state, + SandboxStatus::from(SandboxState::Suspended), + "{hold}" + ); + } + assert!(dispatched.lock().unwrap().is_empty(), "{hold}"); + + if let Some(release) = release { + service.db.lock().unwrap().execute(release, []).unwrap(); + } + drop(artifact_guard); + let result = tokio::time::timeout( + Duration::from_secs(5), + service.evict_cluster_deleted_sandbox(root_peer_request(&sandbox_id)), + ) + .await; + service.shutdown_token.cancel(); + service.task_tracker.close(); + tokio::time::timeout(Duration::from_secs(5), service.task_tracker.wait()) + .await + .expect("eviction and cleanup tasks must drain"); + result + .expect("eviction should complete") + .unwrap_or_else(|error| panic!("{hold}: an unheld row is reclaimed: {error}")); + assert_eq!( + *dispatched.lock().unwrap(), + vec![(sandbox_id.clone(), false)], + "{hold}" + ); + } + } + + /// A sandbox the cluster deleted is evicted with a named snapshot nothing + /// backed up: the cluster's snapshots of a sandbox go with its row, so + /// the snapshot no longer exists anywhere but this node. + #[tokio::test] + async fn evict_cluster_deleted_suspended_sandbox_takes_its_unbacked_named_snapshots() { + let conn = crate::db::test_conn(); + let sandbox_id = SandboxId::new().to_string(); + insert_platform_owned_sandbox(&conn, &sandbox_id, SandboxState::Suspended, None); + conn.execute( + "INSERT INTO local_artifacts ( + local_artifact_id, sandbox_id, artifact_kind, artifact_origin, + snapshot_name, local_path, created_at + ) VALUES (?1, ?2, 'disk', 'snapshot', 'd-named', '/space/cache/named', '0')", + crate::db::fixture::params![LocalArtifactId::new().to_string(), sandbox_id], + ) + .unwrap(); + conn.execute( + "UPDATE sandboxes SET updated_at = '0' WHERE id = ?1", + crate::db::fixture::params![sandbox_id], + ) + .unwrap(); + let service = service_with_runtime_probe(conn, false); + let dispatched = Arc::new(std::sync::Mutex::new(Vec::new())); + let observed = Arc::clone(&dispatched); + *service.cleanup_executor.dispatch.lock().unwrap() = Some(Arc::new(move |id, kill| { + observed.lock().unwrap().push((id, kill)); + Box::pin(async { Ok(()) }) + })); + + let result = tokio::time::timeout( + Duration::from_secs(5), + service.evict_cluster_deleted_sandbox(root_peer_request(&sandbox_id)), + ) + .await + .expect("eviction should complete"); + service.shutdown_token.cancel(); + service.task_tracker.close(); + tokio::time::timeout(Duration::from_secs(5), service.task_tracker.wait()) + .await + .expect("tasks drain"); + + result.unwrap(); + assert_eq!(*dispatched.lock().unwrap(), vec![(sandbox_id, false)]); + } + + /// A hold that appears after the cluster-deleted eviction checked for one, + /// before its claim, is refused by the claim the same way: the claim's + /// `FailedPrecondition` is answered `Aborted`, named by the hold, so + /// node-ctl retries on its next sync, and the row and the cleanup are + /// untouched. The eviction is paused at its claim by a test barrier, with + /// the check behind it, while an upload of the sandbox's artifacts starts. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn evict_cluster_deleted_suspended_sandbox_held_after_its_check_is_retried() { + let conn = crate::db::test_conn(); + let sandbox_id = SandboxId::new().to_string(); + insert_platform_owned_sandbox(&conn, &sandbox_id, SandboxState::Suspended, None); + conn.execute( + "UPDATE sandboxes SET updated_at = '0' WHERE id = ?1", + crate::db::fixture::params![sandbox_id], + ) + .unwrap(); + let service = Arc::new(service_with_runtime_probe(conn, false)); + let dispatched = Arc::new(std::sync::Mutex::new(Vec::new())); + let observed = Arc::clone(&dispatched); + *service.cleanup_executor.dispatch.lock().unwrap() = Some(Arc::new(move |id, kill| { + observed.lock().unwrap().push((id, kill)); + Box::pin(async { Ok(()) }) + })); + let barrier = Arc::new(tokio::sync::Barrier::new(2)); + *service + .auto_lifecycle + .eviction_claim_barrier + .lock() + .unwrap() = Some(Arc::clone(&barrier)); + let eviction = tokio::spawn({ + let service = Arc::clone(&service); + let sandbox_id = sandbox_id.clone(); + async move { + service + .evict_cluster_deleted_sandbox(root_peer_request(&sandbox_id)) + .await + } + }); + + // Past the check, not yet claimed. + tokio::time::timeout(Duration::from_secs(10), barrier.wait()) + .await + .expect("the eviction reaches its claim"); + service + .db + .lock() + .unwrap() + .execute( + "INSERT INTO jobs (job_id, kind, status, payload, subject_id, created_at) + VALUES ('late-upload', 'snapshot_upload', 'running', '{}', ?1, '1')", + crate::db::fixture::params![sandbox_id], + ) + .unwrap(); + tokio::time::timeout(Duration::from_secs(10), barrier.wait()) + .await + .expect("the eviction resumes"); + let error = tokio::time::timeout(Duration::from_secs(10), eviction) + .await + .expect("the eviction answers") + .unwrap() + .expect_err("a row held before its claim must not be evicted"); + + assert_eq!(error.code(), tonic::Code::Aborted, "{error}"); + assert!(error.message().contains("held by"), "{error}"); + assert!(error.message().contains("next sync"), "{error}"); + { + let db = service.db.lock().unwrap(); + let row = crate::db::get_sandbox(&db, &sandbox_id).unwrap().unwrap(); + assert_eq!(row.state, SandboxStatus::from(SandboxState::Suspended)); + } + assert!(dispatched.lock().unwrap().is_empty()); + service.shutdown_token.cancel(); + service.task_tracker.close(); + tokio::time::timeout(Duration::from_secs(5), service.task_tracker.wait()) + .await + .expect("tasks drain"); + } + async fn assert_evict_refused_and_row_untouched( service: &SandboxManagerService, sandbox_id: &str, @@ -16069,10 +16305,13 @@ pub(crate) mod service_tests { /// Recovery adopts a teardown capture it finds staged or already /// published, pins it for backup without cleaning up, and records its - /// execution generation in the published capture. + /// execution generation in the published capture, keeping the teardown's + /// own record of that generation when the teardown wrote one. #[tokio::test] async fn startup_recovery_adopts_but_does_not_cleanup_an_unbacked_teardown_artifact() { - for staged in [true, false] { + for (staged, teardown_recorded) in + [(true, false), (false, false), (true, true), (false, true)] + { let conn = crate::db::test_conn(); let sandbox_id = SandboxId::new().to_string(); insert_test_sandbox(&conn, &sandbox_id, SandboxState::Paused); @@ -16093,6 +16332,16 @@ pub(crate) mod service_tests { local_artifact_id, b"published-before-db-pin", ); + if teardown_recorded { + // The teardown recorded its capture before the node went down. + crate::capture_record::write( + &written_dir, + local_artifact_id, + teardown_generation(), + crate::capture_record::RecordWriter::CapturingTeardown, + ) + .unwrap(); + } let service = service_with_runtime_probe(conn, false); let records = service @@ -16125,6 +16374,16 @@ pub(crate) mod service_tests { Some(teardown_generation()), "staged: {staged}" ); + // Recovery can't tell the active teardown took this capture, so + // its own record fences but orders nothing; the teardown's record + // of the same generation, written before the node went down, is + // kept and still orders. + assert_eq!( + crate::capture_record::read_capture_order(&snapshot_dir, local_artifact_id) + .unwrap(), + teardown_recorded.then(teardown_generation), + "staged: {staged}, teardown recorded: {teardown_recorded}" + ); } } diff --git a/crates/l40_api/sbx/mgr/server/src/snapshot_scheduler.rs b/crates/l40_api/sbx/mgr/server/src/snapshot_scheduler.rs index 1727d4d86a..1d99e667b6 100644 --- a/crates/l40_api/sbx/mgr/server/src/snapshot_scheduler.rs +++ b/crates/l40_api/sbx/mgr/server/src/snapshot_scheduler.rs @@ -710,6 +710,63 @@ fn finish_completed_checkpoint_after_resume_error( lifecycle_result } +/// How many times [`discard_incomplete_checkpoint`] asks node-manager to +/// delete the directory, and how long it waits between asks. Node-manager +/// refuses a delete with `Unavailable` while another operation on the +/// sandbox, such as the guest-network reconciler, holds its mutation claim, +/// which it does only briefly. +const INCOMPLETE_CHECKPOINT_DELETE_ATTEMPTS: usize = 3; +const INCOMPLETE_CHECKPOINT_DELETE_BACKOFF: Duration = Duration::from_millis(250); + +/// Delete the snapshot directory a checkpoint created before its VM state +/// capture failed, returning `error` with any failure to delete appended. The +/// directory holds the disk but no VM state, so it restores nothing, yet the +/// listing still shows it, as incomplete, and with no row nothing removes it +/// while the sandbox runs: retention spares a manual checkpoint's name. Only +/// a directory this checkpoint created is deleted: a failed create may have +/// met an existing snapshot of the same name. A delete refused as +/// `Unavailable` is asked again, up to +/// [`INCOMPLETE_CHECKPOINT_DELETE_ATTEMPTS`] times; the caller has already +/// rolled the sandbox back, so the outcome changes nothing else. +async fn discard_incomplete_checkpoint( + node_client: &mut pplx_space_node_mgr_client::NodeManagerGrpcClient, + sandbox_id: &str, + snapshot_name: &str, + error: String, +) -> String { + let mut attempt = 1; + let status = loop { + match node_client + .delete_btrfs_snapshot( + pplx_space_node_mgr_client::proto::DeleteBtrfsSnapshotRequest { + sandbox_id: sandbox_id.to_string(), + snapshot_name: snapshot_name.to_string(), + }, + ) + .await + { + Ok(_) => return error, + Err(status) if status.code() == tonic::Code::NotFound => return error, + Err(status) + if status.code() == tonic::Code::Unavailable + && attempt < INCOMPLETE_CHECKPOINT_DELETE_ATTEMPTS => + { + attempt += 1; + tokio::time::sleep(INCOMPLETE_CHECKPOINT_DELETE_BACKOFF).await; + } + Err(status) => break status, + } + }; + tracing::warn!( + sandbox_id, + snapshot_name, + attempt, + error = %status, + "failed to delete an incomplete checkpoint's snapshot directory" + ); + format!("{error}; delete the incomplete checkpoint {snapshot_name}: {status}") +} + async fn rollback_checkpoint_failure( ctx: &CheckpointContext, node_client: &mut pplx_space_node_mgr_client::NodeManagerGrpcClient, @@ -1014,7 +1071,7 @@ pub async fn do_checkpoint( )) .await { - return Err(rollback_checkpoint_failure( + let error = rollback_checkpoint_failure( ctx, &mut node_client, sandbox_id, @@ -1022,6 +1079,13 @@ pub async fn do_checkpoint( previous_state, format!("checkpoint snapshot failed: {e}"), ) + .await; + return Err(discard_incomplete_checkpoint( + &mut node_client, + sandbox_id, + snapshot_name, + error, + ) .await); } diff --git a/crates/l40_api/sbx/mgr/server/src/snapshot_scheduler/tests.rs b/crates/l40_api/sbx/mgr/server/src/snapshot_scheduler/tests.rs index b6ece0c4a5..3950c255ac 100644 --- a/crates/l40_api/sbx/mgr/server/src/snapshot_scheduler/tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/snapshot_scheduler/tests.rs @@ -1652,3 +1652,5 @@ fn gc_staging_ids_are_canonical() { ); } } + +mod incomplete_checkpoint; diff --git a/crates/l40_api/sbx/mgr/server/src/snapshot_scheduler/tests/incomplete_checkpoint.rs b/crates/l40_api/sbx/mgr/server/src/snapshot_scheduler/tests/incomplete_checkpoint.rs new file mode 100644 index 0000000000..ee92135f98 --- /dev/null +++ b/crates/l40_api/sbx/mgr/server/src/snapshot_scheduler/tests/incomplete_checkpoint.rs @@ -0,0 +1,136 @@ +//! A checkpoint whose VM state capture fails after it created its snapshot +//! directory deletes that directory: with no VM state it restores nothing, +//! and with no row nothing else would remove it. A delete refused as +//! `Unavailable` is asked again a bounded number of times. One whose create +//! fails deletes nothing, since the name may belong to an existing snapshot. + +use std::sync::{Arc, Mutex}; + +use pplx_space_node_mgr_client::proto::{ + CaptureVmStateResponse, CreateBtrfsSnapshotRequest, CreateBtrfsSnapshotResponse, + DeleteBtrfsSnapshotRequest, DeleteBtrfsSnapshotResponse, SyncGuestResponse, +}; + +use super::*; + +const NAME: &str = "f-manual-20261107T120000Z"; + +/// Checkpoint a paused sandbox against a node-manager whose create answers +/// `create`, whose capture fails and whose deletes answer `deletes` in turn, +/// then succeed. Returns the error and the names node-manager was asked to +/// delete. +async fn checkpoint_failing(create: Result<(), tonic::Status>) -> (String, Vec) { + checkpoint_failing_with_deletes(create, Vec::new()).await +} + +async fn checkpoint_failing_with_deletes( + create: Result<(), tonic::Status>, + deletes: Vec, +) -> (String, Vec) { + let deleted = Arc::new(Mutex::new(Vec::new())); + let deletes = Arc::new(Mutex::new(std::collections::VecDeque::from(deletes))); + let node = crate::node_manager_transport::fake::FakeNodeManager::default() + .unary("SyncGuest", |_: SyncGuestRequest| Ok(SyncGuestResponse {})) + .unary( + "CreateBtrfsSnapshot", + move |_: CreateBtrfsSnapshotRequest| { + create.clone().map(|()| CreateBtrfsSnapshotResponse {}) + }, + ) + .unary("CaptureVmState", |_: CaptureVmStateRequest| { + Err::(tonic::Status::internal("injected capture failure")) + }) + .unary("DeleteBtrfsSnapshot", { + let deleted = Arc::clone(&deleted); + move |request: DeleteBtrfsSnapshotRequest| { + deleted.lock().unwrap().push(request.snapshot_name); + match deletes.lock().unwrap().pop_front() { + Some(status) => Err(status), + None => Ok(DeleteBtrfsSnapshotResponse {}), + } + } + }) + .serve(); + let (mut ctx, sandbox_id) = checkpoint_context(SandboxState::Paused); + ctx.node_manager = node.connector(); + + let error = do_checkpoint(&ctx, &sandbox_id, NAME).await.unwrap_err(); + + let row = crate::db::get_sandbox(&ctx.db.lock().unwrap(), &sandbox_id) + .unwrap() + .unwrap(); + assert_eq!( + row.state, + SandboxStatus::Unowned(SandboxState::Paused), + "{error}" + ); + let deleted = deleted.lock().unwrap().clone(); + (error, deleted) +} + +#[tokio::test] +async fn a_failed_capture_deletes_the_directory_it_created() { + let (error, deleted) = checkpoint_failing(Ok(())).await; + + assert!(error.contains("injected capture failure"), "{error}"); + assert_eq!(deleted, vec![NAME.to_string()]); +} + +#[tokio::test] +async fn a_failed_create_deletes_nothing() { + let (error, deleted) = + checkpoint_failing(Err(tonic::Status::already_exists("snapshot exists"))).await; + + assert!(error.contains("snapshot exists"), "{error}"); + assert!(deleted.is_empty(), "{deleted:?}"); +} + +/// Node-manager refuses a delete as `Unavailable` while another operation +/// holds the sandbox's mutation claim; the next ask deletes the directory. +#[tokio::test] +async fn a_delete_refused_as_unavailable_is_asked_again() { + let (error, deleted) = checkpoint_failing_with_deletes( + Ok(()), + vec![tonic::Status::unavailable("mutation in progress")], + ) + .await; + + assert_eq!(deleted, vec![NAME.to_string(), NAME.to_string()]); + assert!( + !error.contains("delete the incomplete checkpoint"), + "{error}" + ); +} + +/// A delete still refused after every attempt gives up and says so. +#[tokio::test] +async fn a_delete_refused_on_every_attempt_gives_up() { + let (error, deleted) = checkpoint_failing_with_deletes( + Ok(()), + vec![ + tonic::Status::unavailable("mutation in progress"); + INCOMPLETE_CHECKPOINT_DELETE_ATTEMPTS + ], + ) + .await; + + assert_eq!(deleted.len(), INCOMPLETE_CHECKPOINT_DELETE_ATTEMPTS); + assert!( + error.contains("delete the incomplete checkpoint"), + "{error}" + ); +} + +/// Only `Unavailable` is asked again: another error is reported at once. +#[tokio::test] +async fn another_delete_error_is_not_asked_again() { + let (error, deleted) = + checkpoint_failing_with_deletes(Ok(()), vec![tonic::Status::internal("delete failed")]) + .await; + + assert_eq!(deleted, vec![NAME.to_string()]); + assert!( + error.contains("delete the incomplete checkpoint"), + "{error}" + ); +} diff --git a/crates/l40_api/sbx/mgr/server/src/snapshot_upload.rs b/crates/l40_api/sbx/mgr/server/src/snapshot_upload.rs index e5d3b2c1a9..b15c15a1b1 100644 --- a/crates/l40_api/sbx/mgr/server/src/snapshot_upload.rs +++ b/crates/l40_api/sbx/mgr/server/src/snapshot_upload.rs @@ -481,6 +481,12 @@ pub struct SnapshotUploadJobs { /// to one execution, and in payloads persisted before the fence existed. #[serde(default, skip_serializing_if = "Option::is_none")] pub execution_generation: Option, + /// `execution_generation` is the capturing execution's by evidence, not + /// inferred, so the control plane records it to order captures (see + /// `BeginArtifactBackupRequest.execution_generation_names_capture`). A + /// payload persisted before this field existed reads as `false`. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub execution_generation_names_capture: bool, /// What this upload's permanent failure, if it has one, failed. The /// worker records it beside the job's own failure, which says only whether /// the job retries. A payload persisted before this field existed reads as @@ -1014,6 +1020,7 @@ pub(crate) fn prepare_suspend_snapshot( includes_metadata_job: true, deferred_metadata, execution_generation: None, + execution_generation_names_capture: false, failure_scope: UploadFailureScope::Attempt, }) } diff --git a/crates/l40_api/sbx/mgr/server/src/snapshot_upload/tests.rs b/crates/l40_api/sbx/mgr/server/src/snapshot_upload/tests.rs index 460a44878b..ba7871274d 100644 --- a/crates/l40_api/sbx/mgr/server/src/snapshot_upload/tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/snapshot_upload/tests.rs @@ -552,6 +552,7 @@ fn new_payload_round_trips_metadata_job_flag() { includes_metadata_job: true, deferred_metadata: false, execution_generation: None, + execution_generation_names_capture: false, failure_scope: Default::default(), }; let round: SnapshotUploadJobs = @@ -576,6 +577,7 @@ fn payload_round_trips_execution_generation() { includes_metadata_job: true, deferred_metadata: false, execution_generation: Some(generation), + execution_generation_names_capture: false, failure_scope: Default::default(), }; let encoded = serde_json::to_value(&jobs).unwrap(); @@ -611,6 +613,7 @@ fn cleanup_metadata_staging_dir_removes_only_owned_staging_dir() { includes_metadata_job: true, deferred_metadata: false, execution_generation: None, + execution_generation_names_capture: false, failure_scope: Default::default(), }; diff --git a/crates/l40_api/sbx/mgr/server/src/snapshot_upload_jobs.rs b/crates/l40_api/sbx/mgr/server/src/snapshot_upload_jobs.rs index ebbb79f664..f786d03103 100644 --- a/crates/l40_api/sbx/mgr/server/src/snapshot_upload_jobs.rs +++ b/crates/l40_api/sbx/mgr/server/src/snapshot_upload_jobs.rs @@ -761,9 +761,28 @@ async fn send_prepared_begin( >, JobError, > { - let local_artifact_id = payload.meta.local_artifact_id.ok_or_else(|| { - JobError::permanent("prepared snapshot upload is missing local_artifact_id") - })?; + let local_artifact_id = payload + .meta + .local_artifact_id + .ok_or_else(|| { + JobError::permanent("prepared snapshot upload is missing local_artifact_id") + })? + .to_string(); + Ok(begin_with_cluster_api(prepared_begin( + payload, + &local_artifact_id, + fence_verdict_only, + )?) + .await) +} + +/// The registration `payload` asks for, with `local_artifact_id` its +/// artifact's id as text. +fn prepared_begin<'a>( + payload: &'a SnapshotUploadJobs, + local_artifact_id: &'a str, + fence_verdict_only: bool, +) -> Result, JobError> { let artifact_origin = payload.meta.artifact_origin.ok_or_else(|| { JobError::permanent("prepared snapshot upload is missing artifact_origin") })?; @@ -775,11 +794,9 @@ async fn send_prepared_begin( .artifact_snapshot_name .as_ref() .map(|name| name.as_str()); - let local_artifact_id = local_artifact_id.to_string(); - - Ok(begin_with_cluster_api(BeginArtifactBackup { + Ok(BeginArtifactBackup { sandbox_id: &payload.meta.sandbox_id, - local_artifact_id: &local_artifact_id, + local_artifact_id, artifact_kind: artifact_kind_from_meta(&payload.meta), artifact_origin, snapshot_name, @@ -787,9 +804,9 @@ async fn send_prepared_begin( storage_uri: &payload.storage_uri, execution, execution_generation: payload.execution_generation, + execution_generation_names_capture: payload.execution_generation_names_capture, fence_verdict_only, }) - .await) } fn classify_begin_error(error: BeginArtifactBackupError) -> JobError { @@ -1294,6 +1311,8 @@ pub(crate) struct BeginArtifactBackup<'a> { pub(crate) storage_uri: &'a str, pub(crate) execution: &'a pplx_space_common_core::types::SnapshotExecutionMetadata, pub(crate) execution_generation: Option, + /// `execution_generation` is the capturing execution's by evidence. + pub(crate) execution_generation_names_capture: bool, /// Ask for the registration's verdict without persisting the attempt. pub(crate) fence_verdict_only: bool, } @@ -1372,6 +1391,7 @@ fn begin_artifact_backup_request( execution_generation: request .execution_generation .map(pplx_space_common_core::types::ExecutionGeneration::get), + execution_generation_names_capture: request.execution_generation_names_capture, fence_verdict_only: request.fence_verdict_only, } } diff --git a/crates/l40_api/sbx/mgr/server/src/snapshot_upload_jobs/tests.rs b/crates/l40_api/sbx/mgr/server/src/snapshot_upload_jobs/tests.rs index 7cc7ffc400..3395147ffd 100644 --- a/crates/l40_api/sbx/mgr/server/src/snapshot_upload_jobs/tests.rs +++ b/crates/l40_api/sbx/mgr/server/src/snapshot_upload_jobs/tests.rs @@ -3,7 +3,7 @@ use pplx_space_common_core::types::LocalArtifactId; use super::*; -fn job_fixture(kind: &str, payload: String) -> Job { +pub(crate) fn job_fixture(kind: &str, payload: String) -> Job { Job { job_id: JobId::new(), kind: kind.to_string(), @@ -782,6 +782,7 @@ pub(crate) fn reclaim_fixture_payload( includes_metadata_job: true, deferred_metadata: false, execution_generation: None, + execution_generation_names_capture: false, failure_scope: Default::default(), } } @@ -1211,12 +1212,43 @@ fn begin_request_carries_the_recorded_capture_host_cpu() { storage_uri: "s3://space-test/suspend-snapshots/attempt?region=us-east-1", execution: &execution, execution_generation: None, + execution_generation_names_capture: false, fence_verdict_only: false, }); assert_eq!(request.capture_host_cpu.as_deref(), host_cpu); } } +/// A persisted upload's mark that its execution generation is the capture's +/// own reaches the registration, so cluster-api records the generation only +/// for a marked capture. +#[test] +fn begin_request_carries_the_capture_generation_mark() { + use pplx_space_common_core::types::{ + Arch, ExecutionGeneration, ExecutionMetadata, SnapshotExecutionMetadata, + }; + + let artifact_id = "01939e3e-eeee-7000-8000-000000000051"; + for names_capture in [false, true] { + let mut payload = + reclaim_fixture_payload(artifact_id, std::path::PathBuf::from("/space/cache/s")); + payload.meta.execution = Some( + SnapshotExecutionMetadata::new(ExecutionMetadata::initial_firecracker( + Arch::X86_64, + vec![], + )) + .unwrap(), + ); + payload.execution_generation = Some(ExecutionGeneration::new(7).unwrap()); + payload.execution_generation_names_capture = names_capture; + + let request = + begin_artifact_backup_request(&prepared_begin(&payload, artifact_id, false).unwrap()); + assert_eq!(request.execution_generation, Some(7)); + assert_eq!(request.execution_generation_names_capture, names_capture); + } +} + /// The teardown probe asks for the fence's verdict without registering, and /// only a reply that doesn't echo the request, from a server that predates /// verdict-only requests, left a row for the probe to close. @@ -1242,6 +1274,7 @@ fn verdict_only_begin_is_requested_and_its_echo_read() { storage_uri: "s3://space-test/suspend-snapshots/attempt?region=us-east-1", execution: &execution, execution_generation: None, + execution_generation_names_capture: false, fence_verdict_only, }); assert_eq!(request.fence_verdict_only, fence_verdict_only); diff --git a/docs/execution-environments.md b/docs/execution-environments.md index e17fe060e2..c6f784eaa0 100644 --- a/docs/execution-environments.md +++ b/docs/execution-environments.md @@ -1407,7 +1407,9 @@ doesn't render it yet. - **`UpgradeRuntime`.** Under the sandbox's upgrade lock, a suspended sandbox absent from the node is staged from `stage_from` and left suspended, never launched; a stage interrupted after it adopted the row is resumed the same - way. A suspended sandbox is then upgraded only from the checkpoint + way. A suspended copy pinning another checkpoint is replaced first (see + "Dispatch" under "Upgrading at resume and start"). A suspended + sandbox is then upgraded only from the checkpoint `checkpoint_local_artifact_id` names, which the upgrade checks for the RPC and the Wake alike. For a started target the response always carries the running execution. A sandbox already on the target is activated in place: a @@ -1559,11 +1561,75 @@ is stranded. one the upgrade was decided from: the Wake names that checkpoint's local artifact, and the row must pin it, as it does on the checkpoint's source node and after staging, so the check holds across retries. An upgrade of a - suspended sandbox that names no checkpoint is refused. A stale copy is - refused with `FailedPrecondition` and left alone, where a plain restore - would have staged the chosen backup over it. Staging an encrypted - checkpoint writes no plaintext rootfs, so an encrypted sandbox restored - away from its checkpoint is refused before the fence. + suspended sandbox that names no checkpoint is refused. A stale copy, a + suspended row pinning another checkpoint, is refused with + `FailedPrecondition` on the Wake, which carries only a Direct sandbox's + upgrade, and left alone. For a Kubernetes sandbox's `UpgradeRuntime` with + `stage_from`, it's replaced: if the pinned artifact is proven backed up, by + its local backup marker or the cluster's exact-artifact readiness check as + eviction proves it, sandbox-manager evicts the copy through the eviction + executor, stages the named checkpoint in its place and upgrades that. + An unproven copy may be the only one of something, so it's refused with + `FailedPrecondition` before any node work, as is an unpinned one, one the + named checkpoint wasn't captured after (the decision is stale, and staging + it would roll the sandbox back) and an encrypted one. Which checkpoint is + newer isn't read from the checkpoint ids, whose order depends on the clocks + of the nodes that minted them, nor from which backup registered last, since + a re-upload registers an old checkpoint again. A backup registration + records the generation of the execution that captured the bytes + (`snapshots.generation`) when the node has evidence it's the capture's + own: the running execution just took the capture, the record the + capturing teardown published with it names it, or an earlier upload of + the same bytes recorded it so + (`BeginArtifactBackupRequest.execution_generation_names_capture`). A + generation the node only infers fences the registration but records + nothing: one from a teardown still running when it re-uploads an + unrecorded capture, or one startup recovery recorded. A teardown that + crashed before publishing leaves the previous capture in the canonical + directory, and recovery adopts it under the active teardown's generation, + so its record fences without ordering, as do records written before the + two were told apart. Recovery keeps a record the capturing teardown wrote + for the same capture and generation before the node went down. After proving the copy's backup ready the + node asks cluster-api whether the named checkpoint's ready backup was + captured by a later execution than the copy's (`BackupReadinessQuery`'s + `captured_after`). Only a yes replaces the copy. A backup that records no + capture generation answers no, as do backups registered before + generations were recorded: those copies are refused, as every copy pinning + another checkpoint was before replacement existed. A Direct sandbox's + captures run under no execution generation, so its copy is refused that + way without the cluster being asked. The eviction's claim is taken under + the sandbox's artifact lock, without waiting for it: a backup being + admitted holds it off, and one admitted behind it finds the row on its + way out and backs up nothing. The eviction deletes every local artifact + of the sandbox, so each other one without a backup stamp, a named + snapshot say, is first proven backed through the cluster and stamped, and + the copy stays while any isn't; the claim re-checks that none is left + unstamped. A request whose + stage would be refused for its form or its route identity, whose + generation the row already retained, or whose authorization doesn't + accept memory loss, deletes nothing; the upgrade's + edge check reads the staged checkpoint, so it runs after the stage. A + row that rolled back an activation keeps that activation's claim open, and + the claim is how each later attempt of it is asked whether its execution + was retired; removing the row closes it, and a retry after that would go + unasked. So a request retrying that activation deletes nothing, and the + copy is refused as it was before replacement existed, until a new + activation replaces it. A copy + another upgrade still fences or owes a retirement is left to that upgrade + with a retryable 503, one with an artifact upload pending, running or cancelling is + refused as retryable until the upload settles (when that upload is the + pin's own backup, the pin isn't proven yet, and it's refused as unproven + until the backup lands), and a sandbox the + operation already recorded an outcome for is left to that record. Once the + copy is gone the sandbox is absent, so a retry stages it as it would any + absent sandbox; the eviction's claim re-asserts the row's activation + generations rather than its `updated_at`, which is kept in whole seconds; + a cleanup that fails after the eviction's claim leaves the + row's cleanup failed, with its delete intent kept, until the cleanup + machinery finishes it. Staging an + encrypted checkpoint writes no plaintext rootfs, so an encrypted sandbox + restored away from its checkpoint is refused before the fence, and an + encrypted stale copy is left in place. While an upgrade holds the sandbox, other activations get the retryable 503 the restore and activation leases already give, and a Wake that meets the diff --git a/proto/node/api/v1/cluster.proto b/proto/node/api/v1/cluster.proto index 01845739da..9408747867 100644 --- a/proto/node/api/v1/cluster.proto +++ b/proto/node/api/v1/cluster.proto @@ -1133,6 +1133,15 @@ message BeginArtifactBackupRequest { // snapshot. A server that predates the field registers the attempt and // leaves `fence_verdict_only` unset in its response. bool fence_verdict_only = 11; + // `execution_generation` is the generation of the execution that captured + // these bytes, by evidence the node holds: the registration of a capture the + // current execution just took, the capture's own record, or an earlier + // upload of the same bytes that carried this mark. Only then does + // the server record it as the backup's capture generation, which orders + // captures (`BackupReadinessQuery.captured_after`). A generation the node + // infers from a running teardown, or one a node that predates the field + // sends, fences the registration but orders nothing. + bool execution_generation_names_capture = 12; } message BeginArtifactBackupResponse { @@ -1223,9 +1232,23 @@ message BackupReadinessQuery { // other two, it names no identity the caller holds, so any registered node // learns whether a sandbox it names is restorable. Must be true. bool durably_restorable = 3; + // Ready when both artifacts have a ready, non-orphaned backup recording + // the execution generation that captured it, and the smallest such + // generation among `later`'s backups exceeds the largest among + // `earlier`'s: every capture of `later` was taken by a later execution + // than every capture of `earlier`. Executions' generations only grow, so + // this orders captures whatever the nodes' clocks or the order their + // backups registered in. Not ready when either has no recorded + // generation. + LocalArtifactCapturedAfter captured_after = 4; } } +message LocalArtifactCapturedAfter { + LocalArtifactBackupIdentity later = 1; + LocalArtifactBackupIdentity earlier = 2; +} + message LocalArtifactBackupIdentity { string local_artifact_id = 1; space.sandbox.manager.v1.ArtifactKind artifact_kind = 2;